graph: refuse unresolvable and non-directory parents

DESIGN.md

@@ -712,7 +712,6 @@ auto get_phi_node(Graph&, id) -> PhiNode*;             // nullptr for non-phi ID
 // Phi-node model helpers
 auto add_condition_node(Graph&, ConditionNode) -> Result<NodeId>;
 auto add_phi_node(Graph&, PhiNode) -> Result<NodeId>;
-auto resolve_phi_node(Graph const&, phi_id) -> NodeId; // Returns active output
 auto is_guard_satisfied(Graph const&, NodeId) -> bool;
 
 // String access helpers (resolve StringId -> string_view)

spec/requirements/glob-expansion.ears.md

@@ -90,6 +90,14 @@ Which paths a pattern names.
 putup shall expand a glob to both the source files on disk and the files produced by the
 project's rules.
 
+### REQ-GLOB-FILES-ONLY
+
+- conformance: tup-conformant
+- reference: upstream `nl_add_path` expands a rule's glob through `tup_db_select_node_dir_glob` with directories excluded, so only normal and generated files match
+- discharge: test "Scenario: glob expansion matches files and never directories"
+
+putup shall expand a glob to the files it matches and never to a directory whose name it matches.
+
 ### REQ-GLOB-DEDUP
 
 - conformance: unclassified

spec/requirements/output-paths.ears.md

@@ -54,6 +54,16 @@ How far out of the tree a rule may write.
 If a rule declares an output whose path does not resolve to a location inside the build root, then
 putup shall reject the Tupfile and name that path, whether or not the rule's guards are satisfied.
 
+### REQ-OUTPUT-UNDER-DIRECTORY
+
+- conformance: tup-conformant
+- reference: upstream refuses the same class in `find_dir_tupid_dt_pg` (`src/tup/create_name_file.c`), which accepts only a directory or a generated directory as a path component of an output or a group and reports "Unable to output to a different directory"; upstream knows a source path's type from its scan, where putup learns it only once a rule names the path, so an output named before any rule reads the file is not refused
+- discharge: test "add_file_node refuses a parent that is not a directory node"
+- discharge: test "Scenario: A rule cannot put an output or a group under a path that is not a directory"
+
+If a rule names an output or a group under a path that the graph holds as a file, a generated file
+or a group, then putup shall reject the Tupfile and name that path and what it is.
+
 ## Group: canonicality
 
 Which of a path's spellings the record carries.

src/graph/builder.cpp

@@ -387,14 +387,14 @@ auto resolve_input_node(
     auto source_path = pool.get(pup::path::join(str(ctx.options.source_root), normalized_path));
     if (pup::platform::exists(source_path)) {
         auto path_id = ctx.state->graph.paths.intern_path(normalized_path, pool, PathId::SourceRoot);
-        return ensure_file_node(ctx.state->graph, path_id, NodeType::File);
+        return ensure_file_node(ctx.state->graph, path_id, pup::platform::is_directory(source_path) ? NodeType::Directory : NodeType::File);
     }
 
     if (!is_empty(ctx.options.config_root) && str(ctx.options.config_root) != str(ctx.options.source_root)) {
         auto config_path_sv = pool.get(pup::path::join(str(ctx.options.config_root), normalized_path));
         if (pup::platform::exists(config_path_sv)) {
             auto path_id = ctx.state->graph.paths.intern_path(normalized_path, pool, PathId::SourceRoot);
-            return ensure_file_node(ctx.state->graph, path_id, NodeType::File);
+            return ensure_file_node(ctx.state->graph, path_id, pup::platform::is_directory(config_path_sv) ? NodeType::Directory : NodeType::File);
         }
     }
 

src/graph/dag.cpp

@@ -88,15 +88,28 @@ auto validate_node_id(Graph const& graph, NodeId id) -> bool
 
 auto add_file_node(Graph& graph, FileNode node) -> Result<NodeId>
 {
+    auto parent_path = PathId::SourceRoot;
+    if (node.parent_dir != 0) {
+        auto const* parent = get_file_node(std::as_const(graph), node.parent_dir);
+        if (!parent) {
+            return make_error<NodeId>(ErrorCode::InvalidNodeId, "Invalid parent directory node ID");
+        }
+        if (parent->type != NodeType::Directory && parent->type != NodeType::GeneratedDir) {
+            auto const parent_spelling = global_pool().get(materialize_path(graph, parent->path_id));
+            auto err = Buf {};
+            err.fmt(
+                "Unable to create '{}/{}' because the {} '{}' is not a directory",
+                parent_spelling,
+                global_pool().get(node.name),
+                node_type_name(parent->type),
+                parent_spelling
+            );
+            return make_error<NodeId>(ErrorCode::InvalidNodeId, err.view());
+        }
+        parent_path = parent->path_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) {
-            auto const* parent = get_file_node(std::as_const(graph), node.parent_dir);
-            if (parent) {
-                parent_path = parent->path_id;
-            }
-        }
         node.path_id = graph.paths.intern(parent_path, node.name);
     }
 
@@ -494,21 +507,6 @@ auto get_phi_node(Graph& graph, NodeId id) -> PhiNode*
     return const_cast<PhiNode*>(get_phi_node(std::as_const(graph), id));
 }
 
-auto resolve_phi_node(Graph const& graph, NodeId phi_id) -> NodeId
-{
-    auto const* phi = get_phi_node(graph, phi_id);
-    if (!phi) {
-        return INVALID_NODE_ID;
-    }
-
-    auto const* cond = get_condition_node(graph, phi->condition);
-    if (!cond) {
-        return INVALID_NODE_ID;
-    }
-
-    return cond->current_value ? phi->then_output : phi->else_output;
-}
-
 auto is_guard_satisfied(Graph const& graph, NodeId id) -> bool
 {
     auto const* cmd = get_command_node(graph, id);

src/parser/glob.cpp

@@ -232,7 +232,7 @@ auto glob_expand(
             if (!options.include_hidden && !name_sv.empty() && name_sv[0] == '.') {
                 return false;
             }
-            if (glob.matches(rel_path)) {
+            if (!entry.is_dir && glob.matches(rel_path)) {
                 auto result_id = dir_part.empty() ? pool.intern(rel_path) : pup::path::join(dir_part, rel_path);
                 results.push_back(result_id);
             }
@@ -246,7 +246,7 @@ auto glob_expand(
                 if (!options.include_hidden && !name_sv.empty() && name_sv[0] == '.') {
                     continue;
                 }
-                if (glob.matches(name_sv)) {
+                if (!entry.is_dir && glob.matches(name_sv)) {
                     auto result_id = dir_part.empty() ? pool.intern(name_sv) : pup::path::join(dir_part, name_sv);
                     results.push_back(result_id);
                 }

test/e2e/fixtures/output_under_non_directory/Tupfile.fixture

@@ -0,0 +1 @@
+: |> echo hi > %o |> ok.txt

test/e2e/fixtures/output_under_non_directory/srcdir/keep.c

@@ -0,0 +1 @@
+int keep;

test/e2e/fixtures/output_under_non_directory/sub

@@ -0,0 +1 @@
+not a directory

test/unit/test_e2e.cpp

@@ -15893,3 +15893,68 @@ SCENARIO("Numbered input flags name the basename and the basename without extens
         }
     }
 }
+
+SCENARIO("A rule cannot put an output or a group under a path that is not a directory", "[e2e][parse]")
+{
+    GIVEN("a project whose source tree holds a file named sub and a directory named srcdir")
+    {
+        auto f = E2EFixture { "output_under_non_directory" };
+
+        WHEN("a rule reads the source directory and others read and write paths inside it")
+        {
+            f.write_file("Tupfile", ": srcdir |> ls %f > %o |> list.txt\n: srcdir/keep.c |> cp %f %o |> keep.o\n: |> echo x > %o |> srcdir/x.o\n");
+            auto result = f.build();
+
+            THEN("the build succeeds")
+            {
+                INFO("stdout: " << result.stdout_output);
+                INFO("stderr: " << result.stderr_output);
+                REQUIRE(result.success());
+                REQUIRE(f.read_file("srcdir/x.o") == "x\n");
+            }
+        }
+
+        WHEN("a rule reads the source file and another outputs into it as if it were a directory")
+        {
+            f.write_file("Tupfile", ": sub |> cat %f > %o |> a.txt\n: |> echo hi > %o |> sub/x.o\n");
+            auto result = f.build();
+
+            THEN("the Tupfile is refused before any command runs")
+            {
+                INFO("stdout: " << result.stdout_output);
+                INFO("stderr: " << result.stderr_output);
+                REQUIRE_FALSE(result.success());
+                REQUIRE(result.stderr_output.find("the file 'sub' is not a directory") != std::string::npos);
+                REQUIRE(result.stdout_output.find("echo hi") == std::string::npos);
+            }
+        }
+
+        WHEN("a rule outputs under another rule's output")
+        {
+            f.write_file("Tupfile", ": |> echo a > %o |> gen\n: |> echo b > %o |> gen/x\n");
+            auto result = f.build();
+
+            THEN("the Tupfile is refused and names the generated file")
+            {
+                INFO("stdout: " << result.stdout_output);
+                INFO("stderr: " << result.stderr_output);
+                REQUIRE_FALSE(result.success());
+                REQUIRE(result.stderr_output.find("the generated file 'gen' is not a directory") != std::string::npos);
+            }
+        }
+
+        WHEN("a rule puts a group under the source file")
+        {
+            f.write_file("Tupfile", ": sub |> cat %f > %o |> a.txt sub/<g>\n: sub/<g> |> echo used > %o |> b.txt\n");
+            auto result = f.build();
+
+            THEN("the Tupfile is refused where the build used to succeed")
+            {
+                INFO("stdout: " << result.stdout_output);
+                INFO("stderr: " << result.stderr_output);
+                REQUIRE_FALSE(result.success());
+                REQUIRE(result.stderr_output.find("the file 'sub' is not a directory") != std::string::npos);
+            }
+        }
+    }
+}

test/unit/test_glob.cpp

@@ -313,3 +313,40 @@ SCENARIO("glob expansion orders matches by path, not by interning order", "[glob
         }
     }
 }
+
+SCENARIO("glob expansion matches files and never directories", "[glob]")
+{
+    GIVEN("a directory holding a file and a subdirectory that itself holds a file")
+    {
+        auto tmp = TempDir {};
+        fs::create_directory(tmp.path() / "gdir_sub");
+        tmp.create_file("gdir_top.txt");
+        tmp.create_file("gdir_sub/gdir_leaf.txt");
+
+        auto expanded = [&](std::string_view pattern) {
+            auto matches = glob_expand(pattern, tmp.path().string());
+            REQUIRE(matches.has_value());
+            auto result = std::vector<std::string_view> {};
+            for (auto id : *matches) {
+                result.push_back(sv(id));
+            }
+            return result;
+        };
+
+        WHEN("a pattern that the subdirectory's name also matches is expanded")
+        {
+            THEN("only the file is a match")
+            {
+                REQUIRE(expanded("gdir_*") == std::vector<std::string_view> { "gdir_top.txt" });
+            }
+        }
+
+        WHEN("a recursive pattern is expanded")
+        {
+            THEN("the files at every depth match and the subdirectory does not")
+            {
+                REQUIRE(expanded("**") == std::vector<std::string_view> { "gdir_sub/gdir_leaf.txt", "gdir_top.txt" });
+            }
+        }
+    }
+}

test/unit/test_graph.cpp

@@ -1682,6 +1682,68 @@ TEST_CASE("add_file_node refuses a second node at an occupied path", "[graph][pa
     }
 }
 
+TEST_CASE("add_file_node refuses a parent that is not a directory node", "[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 root_foo = add_file_node(g, FileNode { .name = intern("foo.c") });
+    REQUIRE(root_foo.has_value());
+    auto const next_id = g.next_file_id;
+
+    SECTION("an id no node was minted under is an error")
+    {
+        auto orphan = add_file_node(g, FileNode { .name = intern("bar.c"), .parent_dir = pup::NodeId { 999 } });
+        REQUIRE_FALSE(orphan.has_value());
+        REQUIRE(orphan.error().code == pup::ErrorCode::InvalidNodeId);
+        REQUIRE(g.next_file_id == next_id);
+        REQUIRE(find_by_dir_name(g, pup::NodeId { 0 }, "bar.c") == std::nullopt);
+    }
+
+    SECTION("a command id is an error even when its index matches a directory")
+    {
+        auto const cmd = pup::node_id::make_command(pup::node_id::index(*src_dir));
+        auto orphan = add_file_node(g, FileNode { .name = intern("bar.c"), .parent_dir = cmd });
+        REQUIRE_FALSE(orphan.has_value());
+        REQUIRE(orphan.error().code == pup::ErrorCode::InvalidNodeId);
+        REQUIRE(find_by_dir_name(g, *src_dir, "bar.c") == std::nullopt);
+    }
+
+    SECTION("a parent that is a file rather than a directory is an error naming what it is")
+    {
+        auto child = add_file_node(g, FileNode { .name = intern("x.o"), .parent_dir = *root_foo });
+        REQUIRE_FALSE(child.has_value());
+        REQUIRE(child.error().code == pup::ErrorCode::InvalidNodeId);
+        REQUIRE(pup::global_pool().get(child.error().message).find("the file 'foo.c' is not a directory") != std::string_view::npos);
+        REQUIRE(g.next_file_id == next_id);
+    }
+
+    SECTION("a parent that is a group is an error")
+    {
+        auto group = add_file_node(g, FileNode { .type = NodeType::Group, .name = intern("<g>"), .parent_dir = *src_dir });
+        REQUIRE(group.has_value());
+        auto child = add_file_node(g, FileNode { .name = intern("x.o"), .parent_dir = *group });
+        REQUIRE_FALSE(child.has_value());
+        REQUIRE(child.error().code == pup::ErrorCode::InvalidNodeId);
+    }
+
+    SECTION("the build root is a directory a node may sit under")
+    {
+        auto child = add_file_node(g, FileNode { .type = NodeType::Generated, .name = intern("x.o"), .parent_dir = pup::BUILD_ROOT_ID });
+        REQUIRE(child.has_value());
+    }
+
+    SECTION("the refusal leaves the source root's entry of the same name alone")
+    {
+        auto orphan = add_file_node(g, FileNode { .name = intern("foo.c"), .parent_dir = pup::NodeId { 999 } });
+        REQUIRE_FALSE(orphan.has_value());
+        REQUIRE(orphan.error().code == pup::ErrorCode::InvalidNodeId);
+        REQUIRE(find_by_dir_name(g, pup::NodeId { 0 }, "foo.c") == *root_foo);
+    }
+}
+
 TEST_CASE("FileNode path_id populated by add_file_node", "[graph][path_pool]")
 {
     auto bs = make_build_graph();