graph: keep nodes that name no path out of the path namespace

include/pup/core/types.hpp

@@ -186,6 +186,32 @@ constexpr auto names_node_type(std::uint8_t value) -> bool
     return false;
 }
 
+/// Whether a node of this type names a path, and so may be reached by resolving one. A node that
+/// stands for a value rather than for something on disk must answer false: a rule input, a glob or
+/// a discovered dependency that resolved to one would bind to it and never be stat'd, which is a
+/// build that goes stale or fails against a path that does not exist (#486). Exhaustive so that a
+/// new node type is a build error here rather than a silent entry in the path namespace.
+[[nodiscard]]
+constexpr auto is_path_addressable(NodeType type) -> bool
+{
+    switch (type) {
+    case NodeType::File:
+    case NodeType::Command:
+    case NodeType::Directory:
+    case NodeType::Generated:
+    case NodeType::Ghost:
+    case NodeType::Group:
+    case NodeType::GeneratedDir:
+    case NodeType::Root:
+        return true;
+    case NodeType::Variable:
+    case NodeType::Condition:
+    case NodeType::Phi:
+        return false;
+    }
+    return false;
+}
+
 [[nodiscard]]
 constexpr auto names_link_type(std::uint8_t value) -> bool
 {

include/pup/index/format.hpp

@@ -92,7 +92,7 @@ inline constexpr auto INDEX_MAGIC = std::array<char, 4> { 'P', 'U', 'P', 'I' };
 /// `RawFileEntry::name_offset` is semantics-bearing on the same terms: `read_prior_paths` composes
 /// it into the paths `clean`/`distclean` delete and `reject_shadowed_sources` refuses a build over,
 /// so a name this reader cannot reproduce fails the record rather than reading as empty (#381).
-inline constexpr auto INDEX_VERSION = std::uint32_t { 27 };
+inline constexpr auto INDEX_VERSION = std::uint32_t { 28 };
 
 /// The oldest version whose `RawHeader` and `RawFileEntry` bytes mean what today's mean, so a
 /// record that old still says which paths it recorded as sources and which as generated even

spec/requirements/variant-builds.ears.md

@@ -33,6 +33,18 @@ See `README.md` for the format and the rules that apply to every area.
 
 Which node a path spelled in a Tupfile names, when more than one spelling reaches the same file.
 
+### REQ-VARIANT-NAME-IS-A-FILE
+
+- conformance: tup-conformant
+- reference: upstream parents a configuration variable's node under the `tup.config` node itself rather than under its directory - `tup_db_get_tup_config_tent` finds or creates that node, `tup_db_read_vars` passes it down, and `add_var` creates the variable under it - so under tup's `unique(dir, name)` key a configuration variable and a source file can never collide; putup reaches the same disjointness by refusing a path to any node type that names none (`is_path_addressable`), because putup's path namespace is lexical and a parent alone would leave the collision spellable
+- discharge: test "Scenario: A config variable does not hide a source file of the same name"
+- discharge: test "Scenario: A config variable does not capture a rule's input in an out-of-tree build"
+- discharge: test "Scenario: A config variable does not hide a discovered header of the same name"
+- discharge: test "Scenario: A config variable does not hide a source file a glob matches"
+
+When a rule names an input, a glob matches a name, or a dep scan discovers one, putup shall
+resolve that name to the file on disk rather than to a configuration variable that shares it.
+
 ### REQ-VARIANT-OUTPUT-NODE
 
 - conformance: tup-conformant

src/graph/builder.cpp

@@ -2487,11 +2487,6 @@ auto add_tupfile(
 
             auto var_name_id = to_underlying(intern(var_name));
 
-            if (auto existing = find_by_dir_name(build_state.graph, config_dir_id, var_name)) {
-                state.config_var_nodes.insert(var_name_id, *existing);
-                continue;
-            }
-
             auto value = eval.config_vars->get(var_name);
             auto node = FileNode {
                 .type = NodeType::Variable,

src/graph/dag.cpp

@@ -100,7 +100,9 @@ auto add_file_node(Graph& graph, FileNode node) -> Result<NodeId>
             }
         }
         node.path_id = graph.paths.intern(parent_path, node.name);
-        graph.path_to_node.insert(to_underlying(node.path_id), id);
+        if (is_path_addressable(node.type)) {
+            graph.path_to_node.insert(to_underlying(node.path_id), id);
+        }
     }
 
     auto const idx = node_id::index(id);
@@ -110,7 +112,7 @@ auto add_file_node(Graph& graph, FileNode node) -> Result<NodeId>
     }
     graph.files[idx] = node;
 
-    if (!is_empty(graph.files[idx].name)) {
+    if (!is_empty(graph.files[idx].name) && is_path_addressable(graph.files[idx].type)) {
         auto const parent_idx = node_id::index(graph.files[idx].parent_dir);
         graph.dir_children[parent_idx].insert(to_underlying(graph.files[idx].name), id);
     }

src/index/entry.cpp

@@ -478,7 +478,7 @@ auto files_by_path(Index const& index) -> FilesByPath
     result.entries.reserve(index.files().size());
 
     for (auto const& file : index.files()) {
-        if (!is_empty(file.path)) {
+        if (!is_empty(file.path) && is_path_addressable(file.type)) {
             result.entries.emplace_back(file.path, &file);
         }
     }

test/e2e/fixtures/config_var_shadow/Tupfile.fixture

@@ -0,0 +1 @@
+: LICENSE |> cp %f %o |> out.txt

test/unit/test_e2e.cpp

@@ -9589,6 +9589,119 @@ SCENARIO("A variable named after a tool is still an ordinary exported variable",
     }
 }
 
+SCENARIO("A config variable does not hide a source file of the same name", "[e2e][incremental][config]")
+{
+    GIVEN("an in-source project whose config key matches a rule's input file")
+    {
+        auto f = E2EFixture { "config_var_shadow" };
+        REQUIRE(f.init().success());
+        f.write_file("tup.config", "CONFIG_LICENSE=y\n");
+        f.write_file("LICENSE", "MIT\n");
+        REQUIRE(f.build().success());
+        REQUIRE(f.read_file("out.txt") == "MIT\n");
+        REQUIRE(f.build().is_noop());
+
+        WHEN("the shadowed source file changes")
+        {
+            f.write_file("LICENSE", "BSD\n");
+            auto result = f.build();
+
+            THEN("the command re-runs against the new contents")
+            {
+                INFO("stdout: " << result.stdout_output);
+                INFO("stderr: " << result.stderr_output);
+                REQUIRE(result.success());
+                REQUIRE_FALSE(result.is_noop());
+                REQUIRE(f.read_file("out.txt") == "BSD\n");
+            }
+        }
+    }
+}
+
+SCENARIO("A config variable does not capture a rule's input in an out-of-tree build", "[e2e][incremental][config]")
+{
+    GIVEN("an out-of-tree project whose config key matches a rule's input file")
+    {
+        auto f = E2EFixture { "config_var_shadow" };
+        f.mkdir("build");
+        f.write_file("LICENSE", "MIT\n");
+        REQUIRE(f.pup({ "configure", "-B", "build" }).success());
+        f.write_file("build/tup.config", "CONFIG_LICENSE=y\n");
+
+        WHEN("the project is built")
+        {
+            auto result = f.build({ "-B", "build" });
+
+            THEN("the input resolves to the source file rather than the config variable")
+            {
+                INFO("stdout: " << result.stdout_output);
+                INFO("stderr: " << result.stderr_output);
+                REQUIRE(result.success());
+                REQUIRE(f.read_file("build/out.txt") == "MIT\n");
+            }
+        }
+    }
+}
+
+SCENARIO("A config variable does not hide a discovered header of the same name", "[e2e][incremental][config]")
+{
+    GIVEN("a project whose config key matches a header the compiler discovers")
+    {
+        auto f = E2EFixture { "config_var_shadow" };
+        REQUIRE(f.init().success());
+        f.write_file("tup.config", "CONFIG_dep.h=y\n");
+        f.write_file("dep.h", "#define V 1\n");
+        f.write_file("main.c", "#include \"dep.h\"\nint main() { return V; }\n");
+        f.write_file("Tupfile", ": main.c |> gcc -MD -c %f -o %o |> main.o\n");
+        REQUIRE(f.build().success());
+        REQUIRE(f.build().is_noop());
+
+        WHEN("the shadowed header changes")
+        {
+            f.write_file("dep.h", "#define V 2\n");
+            auto result = f.build();
+
+            THEN("the command re-runs")
+            {
+                INFO("stdout: " << result.stdout_output);
+                INFO("stderr: " << result.stderr_output);
+                REQUIRE(result.success());
+                REQUIRE_FALSE(result.is_noop());
+            }
+        }
+    }
+}
+
+SCENARIO("A config variable does not hide a source file a glob matches", "[e2e][incremental][config]")
+{
+    GIVEN("a project whose config key matches a file a glob brings into the graph")
+    {
+        auto f = E2EFixture { "config_var_shadow" };
+        f.write_file("LICENSE", "MIT\n");
+        f.write_file("Tupfile", ": foreach *E |> cp %f %o |> %b.out\n");
+        REQUIRE(f.init().success());
+        f.write_file("tup.config", "CONFIG_LICENSE=y\n");
+        REQUIRE(f.build().success());
+        REQUIRE(f.read_file("LICENSE.out") == "MIT\n");
+        REQUIRE(f.build().is_noop());
+
+        WHEN("the shadowed source file changes")
+        {
+            f.write_file("LICENSE", "BSD\n");
+            auto result = f.build();
+
+            THEN("the command re-runs against the new contents")
+            {
+                INFO("stdout: " << result.stdout_output);
+                INFO("stderr: " << result.stderr_output);
+                REQUIRE(result.success());
+                REQUIRE_FALSE(result.is_noop());
+                REQUIRE(f.read_file("LICENSE.out") == "BSD\n");
+            }
+        }
+    }
+}
+
 SCENARIO("Content change with preserved size and mtime", "[e2e][incremental]")
 {
     GIVEN("a built project whose input has an aged mtime")

test/unit/test_index.cpp

@@ -2267,9 +2267,15 @@ TEST_CASE("Keying a record's file table by path keeps every addressable entry an
         auto const by_path = files_by_path(index);
 
         auto expected = std::vector<FileEntry const*> {};
+        auto unaddressable = std::vector<FileEntry const*> {};
         for (auto const& file : index.files()) {
-            if (!pup::is_empty(file.path)) {
+            if (pup::is_empty(file.path)) {
+                continue;
+            }
+            if (pup::is_path_addressable(file.type)) {
                 expected.push_back(&file);
+            } else {
+                unaddressable.push_back(&file);
             }
         }
 
@@ -2293,6 +2299,11 @@ TEST_CASE("Keying a record's file table by path keeps every addressable entry an
             REQUIRE(found->path == file->path);
         }
 
+        for (auto const* file : unaddressable) {
+            auto const* found = by_path.find(file->path);
+            REQUIRE((found == nullptr || pup::is_path_addressable(found->type)));
+        }
+
         REQUIRE(by_path.find(intern("no entry is ever recorded at this path " + std::to_string(seed))) == nullptr);
         REQUIRE(by_path.find(StringId::Empty) == nullptr);
     }