graph: refuse a second node at a path another node already owns

include/pup/core/types.hpp

@@ -5,6 +5,7 @@
 
 #include <array>
 #include <cstdint>
+#include <string_view>
 
 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
 {

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<NodeId>
 {
-    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<NodeId>
             }
         }
         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<NodeId>(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<NodeId>
     }
     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);
     }
 

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<pup::NodeId>(*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();