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();