From 31d4b1200837e469e1324a6f3df78fd78b87bbb8 Mon Sep 17 00:00:00 2001 From: Mura Li <2606021+typeless@users.noreply.github.com> Date: Mon, 21 Sep 2026 20:53:16 +0800 Subject: [PATCH] graph: refuse a second node at a path another node already owns add_file_node discarded the bool from both of its registrations. Neither is an insert-or-fail: SortedPairVec::insert overwrites the value on an existing key and returns false. A second node at an occupied key therefore won silently, and the node that was there became unreachable by path while still sitting in the graph - the shape behind #484 (putup's own tool fingerprints in the Tupfile export/import namespace) and #486 (a config variable hiding a same-named source file), both of which were found by accident days after the stale build they caused. The check consults both maps before minting the id, because the two key on different functions: path_to_node on intern(parent node's path_id, name), dir_children on (parent node index, name). Guarding only path_to_node would leave dir_children as a second, independently keyed resolution leg in ensure_file_node, and the losing node would still bind under a different PathId - one file resolving to two nodes depending on spelling, which is worse than today's consistent-but-wrong. set_build_root_name is the state where the two genuinely disagree, and the new test reaches the second leg through it; deleting the leg fails that section. Checking before graph.next_file_id++ leaves the graph untouched on the error path. SortedPairVec::insert keeps its upsert contract: VarDb::set and VarDb::append depend on last-wins, which is what Tupfile VAR = and VAR += mean, and test_sorted_id_vec pins it. Upstream tup makes the same uniqueness structural rather than conventional - unique(dir, name) on the node table, every insert through tup_db_node_insert_tent_display with a checked step, and tupid_tree_insert / string_tree_insert failing on a duplicate while a separate tupid_tree_add_dup opts into tolerance. The error is expected to be dead code on every project that builds today. Nine adversarial fixtures - a file literally named against a group of that name, a source directory named $ against the env pseudo-directory, an in-source -B . build, a source subdirectory named like the build root, and a config variable against a generated output - failed to reach the branch from the CLI, and the four blast-radius shapes behave exactly as they did before the change. That is a failure to reach, not a proof of unreachability, which is the argument for the error over a diagnostic nobody would ever see. Verified red-then-green: the new test case fails on the pre-change binary (add_file_node returns the second id) and passes after. make check rc=0, 182526 assertions across 956 test cases. Not taken here: folding path_to_node and dir_children into one binding table. It removes the disagreement structurally rather than checking for it, but find_by_dir_name would then read a table that deliberately holds alias entries, and that equivalence is undischarged in exactly the variant identity paths that account for most of the incremental-correctness bugs. Left as its own issue with the surface-compare ledger attached. Ref: https://github.com/typeless/putup/issues/487 Co-Authored-By: Claude Opus 5 (1M context) --- include/pup/core/types.hpp | 34 ++++++++++++++++ src/graph/dag.cpp | 32 +++++++++++---- test/unit/test_graph.cpp | 79 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 138 insertions(+), 7 deletions(-) diff --git a/include/pup/core/types.hpp b/include/pup/core/types.hpp index 9d27d281..8ad9f516 100644 --- a/include/pup/core/types.hpp +++ b/include/pup/core/types.hpp @@ -5,6 +5,7 @@ #include #include +#include namespace pup { @@ -212,6 +213,39 @@ constexpr auto is_path_addressable(NodeType type) -> bool return false; } +/// What a node of this type is, for a message that has to tell a user which of two things already +/// occupies a path. Exhaustive so that a new node type is a build error here rather than a +/// diagnostic that names a kind it cannot describe. +[[nodiscard]] +constexpr auto node_type_name(NodeType type) -> std::string_view +{ + switch (type) { + case NodeType::File: + return "file"; + case NodeType::Command: + return "command"; + case NodeType::Directory: + return "directory"; + case NodeType::Variable: + return "variable"; + case NodeType::Generated: + return "generated file"; + case NodeType::Ghost: + return "unresolved reference"; + case NodeType::Group: + return "group"; + case NodeType::GeneratedDir: + return "generated directory"; + case NodeType::Root: + return "root"; + case NodeType::Condition: + return "condition"; + case NodeType::Phi: + return "branch merge"; + } + return "node"; +} + [[nodiscard]] constexpr auto names_link_type(std::uint8_t value) -> bool { diff --git a/src/graph/dag.cpp b/src/graph/dag.cpp index b0f0e9b4..197ea98b 100644 --- a/src/graph/dag.cpp +++ b/src/graph/dag.cpp @@ -88,9 +88,7 @@ auto validate_node_id(Graph const& graph, NodeId id) -> bool auto add_file_node(Graph& graph, FileNode node) -> Result { - auto const id = graph.next_file_id++; - node.id = id; - + auto const parent_idx = node_id::index(node.parent_dir); if (!is_empty(node.name)) { auto parent_path = PathId::SourceRoot; if (node.parent_dir != 0) { @@ -100,9 +98,30 @@ auto add_file_node(Graph& graph, FileNode node) -> Result } } node.path_id = graph.paths.intern(parent_path, node.name); - if (is_path_addressable(node.type)) { - graph.path_to_node.insert(to_underlying(node.path_id), id); + } + + auto const names_a_path = !is_empty(node.name) && is_path_addressable(node.type); + if (names_a_path) { + auto const* occupant = graph.path_to_node.find(to_underlying(node.path_id)); + if (!occupant && parent_idx < graph.dir_children.size()) { + occupant = graph.dir_children[parent_idx].find(to_underlying(node.name)); } + if (occupant) { + auto const* existing = get_file_node(std::as_const(graph), *occupant); + auto err = Buf {}; + err.fmt( + "Unable to create '{}' because a {} already occupies that path", + global_pool().get(materialize_path(graph, node.path_id)), + node_type_name(existing ? existing->type : node.type) + ); + return make_error(ErrorCode::DuplicateNode, err.view()); + } + } + + auto const id = graph.next_file_id++; + node.id = id; + if (names_a_path) { + graph.path_to_node.insert(to_underlying(node.path_id), id); } auto const idx = node_id::index(id); @@ -112,8 +131,7 @@ auto add_file_node(Graph& graph, FileNode node) -> Result } graph.files[idx] = node; - 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); + if (names_a_path) { graph.dir_children[parent_idx].insert(to_underlying(graph.files[idx].name), id); } diff --git a/test/unit/test_graph.cpp b/test/unit/test_graph.cpp index de86f574..b9d90361 100644 --- a/test/unit/test_graph.cpp +++ b/test/unit/test_graph.cpp @@ -1603,6 +1603,85 @@ TEST_CASE("A recorded discovery routes the consumer no edge points at", "[graph] } } +TEST_CASE("add_file_node refuses a second node at an occupied path", "[graph][path_pool]") +{ + auto bs = make_build_graph(); + auto& g = bs.graph; + + auto src_dir = add_file_node(g, FileNode { .type = NodeType::Directory, .name = intern("src") }); + REQUIRE(src_dir.has_value()); + auto first = add_file_node(g, FileNode { .name = intern("foo.c"), .parent_dir = *src_dir }); + REQUIRE(first.has_value()); + + SECTION("a second addressable node with the same parent and name is an error") + { + auto second = add_file_node(g, FileNode { .name = intern("foo.c"), .parent_dir = *src_dir }); + REQUIRE_FALSE(second.has_value()); + REQUIRE(second.error().code == pup::ErrorCode::DuplicateNode); + } + + SECTION("the node already at the path stays reachable by both lookups") + { + auto const node_count = g.files.size(); + auto second = add_file_node(g, FileNode { .name = intern("foo.c"), .parent_dir = *src_dir }); + REQUIRE_FALSE(second.has_value()); + + auto const* first_node = get_file_node(g, *first); + REQUIRE(first_node != nullptr); + auto const* resolved = g.path_to_node.find(pup::to_underlying(first_node->path_id)); + REQUIRE(resolved != nullptr); + REQUIRE(static_cast(*resolved) == *first); + REQUIRE(find_by_dir_name(g, *src_dir, "foo.c") == *first); + REQUIRE(g.files.size() == node_count); + } + + SECTION("a node of a different type at the same path is refused just the same") + { + auto second = add_file_node(g, FileNode { + .type = NodeType::Generated, + .name = intern("foo.c"), + .parent_dir = *src_dir, + }); + REQUIRE_FALSE(second.has_value()); + REQUIRE(second.error().code == pup::ErrorCode::DuplicateNode); + } + + SECTION("the build root's name is occupied even though no path resolves to it") + { + set_build_root_name(bs, "out"); + auto clash = add_file_node(g, FileNode { .type = NodeType::Directory, .name = intern("out") }); + REQUIRE_FALSE(clash.has_value()); + REQUIRE(clash.error().code == pup::ErrorCode::DuplicateNode); + REQUIRE(pup::global_pool().get(clash.error().message).find("directory") != std::string_view::npos); + } + + SECTION("the message names the kind of node already at the path") + { + auto second = add_file_node(g, FileNode { .name = intern("foo.c"), .parent_dir = *src_dir }); + REQUIRE_FALSE(second.has_value()); + auto const message = pup::global_pool().get(second.error().message); + REQUIRE(message.find("src/foo.c") != std::string_view::npos); + REQUIRE(message.find("a file already occupies") != std::string_view::npos); + } + + SECTION("a node that names no path may share a name with one that does") + { + auto var = add_file_node(g, FileNode { + .type = NodeType::Variable, + .name = intern("foo.c"), + .parent_dir = *src_dir, + }); + REQUIRE(var.has_value()); + auto twin = add_file_node(g, FileNode { + .type = NodeType::Variable, + .name = intern("foo.c"), + .parent_dir = *src_dir, + }); + REQUIRE(twin.has_value()); + REQUIRE(find_by_dir_name(g, *src_dir, "foo.c") == *first); + } +} + TEST_CASE("FileNode path_id populated by add_file_node", "[graph][path_pool]") { auto bs = make_build_graph();