graph: refuse an output under a file or onto a directory at parse time

spec/requirements/output-paths.ears.md

@@ -57,12 +57,24 @@ putup shall reject the Tupfile and name that path, whether or not the rule's gua
 ### 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
+- 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 stats each component of the output's or group's directory that no node holds yet, so in an in-tree build the refusal does not depend on whether a rule reads the file before or after; in a variant build upstream places both in the variant tree and accepts them, and so does putup; putup does not yet refuse a group under an earlier rule's generated output, because the group's directory is a source path and the output is a build path, and in-tree putup aliases a build path to its source twin but not the reverse
 - 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.
+If a rule names an output under a path that the graph holds as a file, a generated file or a group,
+or a group under a path that the graph holds as a file or a group, or, in an in-tree build, either
+under a path that is a file in the source tree, then putup shall reject the Tupfile and name that
+path and what it is.
+
+### REQ-OUTPUT-NOT-A-DIRECTORY
+
+- conformance: deliberate-deviation
+- reference: upstream refuses the same class in `validate_output` (`src/tup/parser.c`) with "Attempting to insert '<name>' as a generated node when it already exists as a different type"; putup differs three times: it names the type "directory" where upstream says "generated directory", because putup never creates a generated-directory node; it refuses a rule under an unsatisfied guard, which upstream never evaluates, so that whether the Tupfile is accepted does not depend on the configuration, as REQ-OUTPUT-INSIDE-HIERARCHY does; and it does not refuse at parse time an output onto a directory in the source tree or onto a group's directory, which upstream refuses (in-tree putup refuses the source directory later, as a file the build does not own)
+- discharge: test "Scenario: A rule cannot output to a path another rule uses as a directory"
+
+If a rule declares an output at a path under which another rule has declared an output, then putup
+shall reject the Tupfile and name that path and what it holds, whether or not the rule's guards are
+satisfied.
 
 ## Group: canonicality
 

src/graph/builder.cpp

@@ -408,6 +408,37 @@ auto resolve_input_node(
     return ensure_file_node(ctx.state->graph, path_id, NodeType::Ghost);
 }
 
+auto bind_source_files_along(BuilderContext& ctx, PathId dir) -> Result<void>
+{
+    auto& graph = ctx.state->graph;
+    if (!get_build_root_name(graph).empty() || is_root(dir) || graph.path_to_node.find(to_underlying(dir))) {
+        return {};
+    }
+    auto& pool = global_pool();
+    auto rel = pool.get(graph.paths.to_string(dir, pool));
+    if (auto source_id = graph.paths.find_path(rel, pool, PathId::SourceRoot);
+        source_id && graph.path_to_node.find(to_underlying(*source_id))) {
+        return {};
+    }
+    if (auto outer = bind_source_files_along(ctx, graph.paths.parent(dir)); !outer) {
+        return outer;
+    }
+    auto source_path = pool.get(pup::path::join(str(ctx.options.source_root), rel));
+    auto is_source_file = pup::platform::is_file(source_path);
+    if (!is_source_file && !pup::platform::exists(source_path) && !is_empty(ctx.options.config_root)
+        && str(ctx.options.config_root) != str(ctx.options.source_root)) {
+        is_source_file = pup::platform::is_file(pool.get(pup::path::join(str(ctx.options.config_root), rel)));
+    }
+    if (!is_source_file) {
+        return {};
+    }
+    auto file_id = ensure_file_node(graph, graph.paths.intern_path(rel, pool, PathId::SourceRoot), NodeType::File);
+    if (!file_id) {
+        return pup::unexpected<Error>(file_id.error());
+    }
+    return {};
+}
+
 /// Get all files that are members of a group (via file → group edges)
 /// Returns file NodeIds by finding all input edges to the group node
 auto get_group_members(Graph const& graph, NodeId group_id) -> Vec<NodeId>
@@ -429,6 +460,9 @@ auto get_or_create_group_node(
     }
 
     auto dir_path_id = ctx.state->graph.paths.intern_path(directory, global_pool(), PathId::SourceRoot);
+    if (auto bound = bind_source_files_along(ctx, dir_path_id); !bound) {
+        return pup::unexpected<Error>(bound.error());
+    }
     auto parent_id_result = ensure_file_node(ctx.state->graph, dir_path_id, NodeType::Directory);
     if (!parent_id_result) {
         return parent_id_result;
@@ -2194,13 +2228,36 @@ auto expand_rule(
     for (auto i = std::size_t { 0 }; i < all_declared.size(); ++i) {
         auto output_path = all_declared[i];
         auto const is_extra = i >= primary_count;
+        if (auto bound = bind_source_files_along(ctx, ctx.state->graph.paths.parent(output_path)); !bound) {
+            return pup::unexpected<Error>(bound.error());
+        }
         auto output_id = ensure_file_node(
             ctx.state->graph, output_path, is_context_active(ctx) ? NodeType::Generated : NodeType::Ghost
         );
         if (!output_id) {
             return pup::unexpected<Error>(output_id.error());
         }
 
+        auto const bound_type = get_file_node(ctx.state->graph, *output_id)->type;
+        switch (bound_type) {
+        case NodeType::File:
+        case NodeType::Generated:
+        case NodeType::Ghost:
+            break;
+        case NodeType::Command:
+        case NodeType::Directory:
+        case NodeType::Variable:
+        case NodeType::Group:
+        case NodeType::GeneratedDir:
+        case NodeType::Root:
+        case NodeType::Condition:
+        case NodeType::Phi: {
+            auto err = Buf {};
+            err.fmt("Attempting to insert '{}' as a generated node when it already exists as a different type ({})", get_full_path(ctx.state->graph, *output_id, ctx.state->path_cache), node_type_name(bound_type));
+            return make_error<void>(ErrorCode::DuplicateNode, err.view());
+        }
+        }
+
         auto output_inputs = get_inputs(ctx.state->graph, *output_id);
         if (!output_inputs.empty()) {
             for (auto existing_id : output_inputs) {
@@ -2251,9 +2308,10 @@ auto expand_rule(
             }
 
             auto group_id_result = get_or_create_group_node(ctx, state, str(dir), str(*eff_output_oo_group));
-            if (group_id_result) {
-                (void)add_edge(ctx.state->graph, *output_id, *group_id_result, LinkType::Group);
+            if (!group_id_result) {
+                return pup::unexpected<Error>(group_id_result.error());
             }
+            (void)add_edge(ctx.state->graph, *output_id, *group_id_result, LinkType::Group);
         }
     }
 

src/graph/dag.cpp

@@ -173,13 +173,14 @@ auto ensure_file_node(Graph& graph, PathId path_id, NodeType type) -> Result<Nod
         auto rel = pool.get(graph.paths.to_string(path_id, pool));
         if (auto src_pid = graph.paths.find_path(rel, pool, PathId::SourceRoot)) {
             if (auto const* hit = graph.path_to_node.find(to_underlying(*src_pid))) {
-                auto* node = get_file_node(graph, *hit);
+                auto const hit_id = NodeId { *hit };
+                auto* node = get_file_node(graph, hit_id);
                 if (node && (node->type == NodeType::Ghost || node->type == NodeType::File)) {
-                    graph.path_to_node.insert(to_underlying(path_id), *hit);
+                    graph.path_to_node.insert(to_underlying(path_id), hit_id);
                     if (type == NodeType::Generated) {
                         node->type = NodeType::Generated;
                     }
-                    return *hit;
+                    return hit_id;
                 }
             }
         }
@@ -188,26 +189,28 @@ auto ensure_file_node(Graph& graph, PathId path_id, NodeType type) -> Result<Nod
     if (!graph.paths.is_grounded(path_id)) {
         auto build_id = graph.paths.ground(path_id, PathId::BuildRoot);
         if (auto const* hit = graph.path_to_node.find(to_underlying(build_id))) {
-            graph.path_to_node.insert(to_underlying(path_id), *hit);
+            auto const hit_id = NodeId { *hit };
+            graph.path_to_node.insert(to_underlying(path_id), hit_id);
             if (type == NodeType::Generated) {
-                auto* node = get_file_node(graph, *hit);
+                auto* node = get_file_node(graph, hit_id);
                 if (node && (node->type == NodeType::Ghost || node->type == NodeType::File)) {
                     node->type = NodeType::Generated;
                 }
             }
-            return *hit;
+            return hit_id;
         }
 
         auto source_id = graph.paths.ground(path_id, PathId::SourceRoot);
         if (auto const* hit = graph.path_to_node.find(to_underlying(source_id))) {
-            graph.path_to_node.insert(to_underlying(path_id), *hit);
+            auto const hit_id = NodeId { *hit };
+            graph.path_to_node.insert(to_underlying(path_id), hit_id);
             if (type == NodeType::Generated) {
-                auto* node = get_file_node(graph, *hit);
+                auto* node = get_file_node(graph, hit_id);
                 if (node && (node->type == NodeType::Ghost || node->type == NodeType::File)) {
                     node->type = NodeType::Generated;
                 }
             }
-            return *hit;
+            return hit_id;
         }
 
         auto root = (type == NodeType::File || type == NodeType::Directory)

test/unit/test_e2e.cpp

@@ -15929,6 +15929,85 @@ SCENARIO("A rule cannot put an output or a group under a path that is not a dire
             }
         }
 
+        WHEN("a rule outputs into the source file before another rule reads it")
+        {
+            f.write_file("Tupfile", ": |> echo hi > %o |> sub/x.o\n: sub |> cat %f > %o |> a.txt\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 two levels under the source file and no rule names the file")
+        {
+            f.write_file("Tupfile", ": |> echo hi > %o |> sub/deep/x.o\n");
+            auto result = f.build();
+
+            THEN("the Tupfile is refused and names the file")
+            {
+                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 a source file inside a source directory no rule names")
+        {
+            f.mkdir("d");
+            f.write_file("d/f", "x\n");
+            f.write_file("Tupfile", ": |> echo hi > %o |> d/f/x.o\n");
+            auto result = f.build();
+
+            THEN("the Tupfile is refused, names the file, and nothing is written in its directory")
+            {
+                INFO("stdout: " << result.stdout_output);
+                INFO("stderr: " << result.stderr_output);
+                REQUIRE_FALSE(result.success());
+                REQUIRE(result.stderr_output.find("the file 'd/f' is not a directory") != std::string::npos);
+                REQUIRE_FALSE(f.exists("d/x.o"));
+            }
+        }
+
+        WHEN("a rule outputs into directories that do not exist yet")
+        {
+            f.write_file("Tupfile", ": |> echo hi > %o |> newdir/deep/x.o\n");
+            auto result = f.build();
+
+            THEN("the build succeeds and writes the output")
+            {
+                INFO("stdout: " << result.stdout_output);
+                INFO("stderr: " << result.stderr_output);
+                REQUIRE(result.success());
+                REQUIRE(f.read_file("newdir/deep/x.o") == "hi\n");
+            }
+        }
+
+        WHEN("a refused output into the source file is retried after the file becomes a directory")
+        {
+            f.write_file("Tupfile", ": |> echo hi > %o |> sub/x.o\n");
+            auto refused = f.build();
+            f.remove_file("sub");
+            f.mkdir("sub");
+            auto result = f.build();
+
+            THEN("the first build is refused and the second writes the output")
+            {
+                INFO("stdout: " << result.stdout_output);
+                INFO("stderr: " << result.stderr_output);
+                REQUIRE_FALSE(refused.success());
+                REQUIRE(result.success());
+                REQUIRE(f.read_file("sub/x.o") == "hi\n");
+            }
+        }
+
         WHEN("a rule outputs under another rule's output")
         {
             f.write_file("Tupfile", ": |> echo a > %o |> gen\n: |> echo b > %o |> gen/x\n");
@@ -15956,5 +16035,88 @@ SCENARIO("A rule cannot put an output or a group under a path that is not a dire
                 REQUIRE(result.stderr_output.find("the file 'sub' is not a directory") != std::string::npos);
             }
         }
+
+        WHEN("a variant build puts an output and a group under the source file")
+        {
+            f.mkdir("build");
+            f.write_file("build/tup.config", "");
+            f.write_file("Tupfile", ": |> echo hi > %o |> sub/x.o sub/<g>\n");
+            auto result = f.build({ "-B", "build" });
+
+            THEN("the build succeeds, because the variant tree holds no file named sub")
+            {
+                INFO("stdout: " << result.stdout_output);
+                INFO("stderr: " << result.stderr_output);
+                REQUIRE(result.success());
+                REQUIRE(f.read_file("build/sub/x.o") == "hi\n");
+            }
+        }
+
+        WHEN("a rule puts a group under the source file and no other rule names the file")
+        {
+            f.write_file("Tupfile", ": |> echo a > %o |> a.txt sub/<g>\n");
+            auto result = f.build();
+
+            THEN("the Tupfile is refused and names the file")
+            {
+                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);
+            }
+        }
+    }
+}
+
+SCENARIO("A rule cannot output to a path another rule uses as 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 outputs to a path an earlier rule wrote under")
+        {
+            f.write_file("Tupfile", ": |> echo b > %o |> gen/x\n: |> echo a > %o |> gen\n");
+            auto result = f.build();
+
+            THEN("the Tupfile is refused before any command runs and names the directory")
+            {
+                INFO("stdout: " << result.stdout_output);
+                INFO("stderr: " << result.stderr_output);
+                REQUIRE_FALSE(result.success());
+                REQUIRE(result.stderr_output.find("Attempting to insert 'gen' as a generated node when it already exists as a different type (directory)") != std::string::npos);
+                REQUIRE(result.stdout_output.find("echo b") == std::string::npos);
+            }
+        }
+
+        WHEN("the same two rules build as a variant")
+        {
+            f.mkdir("build");
+            f.write_file("build/tup.config", "");
+            f.write_file("Tupfile", ": |> echo b > %o |> gen/x\n: |> echo a > %o |> gen\n");
+            auto result = f.build({ "-B", "build" });
+
+            THEN("the Tupfile is refused and names the directory")
+            {
+                INFO("stdout: " << result.stdout_output);
+                INFO("stderr: " << result.stderr_output);
+                REQUIRE_FALSE(result.success());
+                REQUIRE(result.stderr_output.find("as a different type (directory)") != std::string::npos);
+            }
+        }
+
+        WHEN("the rule outputting to the directory's path is under an unsatisfied guard")
+        {
+            f.write_file("Tupfile", ": |> echo b > %o |> gen/x\nifeq (@(FOO),y)\n: |> echo a > %o |> gen\nendif\n");
+            auto result = f.build();
+
+            THEN("the Tupfile is still refused")
+            {
+                INFO("stdout: " << result.stdout_output);
+                INFO("stderr: " << result.stderr_output);
+                REQUIRE_FALSE(result.success());
+                REQUIRE(result.stderr_output.find("as a different type (directory)") != std::string::npos);
+            }
+        }
     }
 }

test/unit/test_graph.cpp

@@ -1793,6 +1793,24 @@ TEST_CASE("FileNode path_id populated by add_file_node", "[graph][path_pool]")
     }
 }
 
+TEST_CASE("ensure_file_node returns the source node an in-tree build path aliases", "[graph]")
+{
+    auto bs = make_build_graph();
+    auto& graph = bs.graph;
+    auto& pool = pup::global_pool();
+
+    auto build_twin = graph.paths.intern_path("d/f", pool, pup::PathId::BuildRoot);
+    auto source_file = graph.paths.intern_path("d/f", pool, pup::PathId::SourceRoot);
+    auto file_id = ensure_file_node(graph, source_file, NodeType::File);
+    REQUIRE(file_id.has_value());
+
+    auto aliased = ensure_file_node(graph, build_twin, NodeType::Directory);
+
+    REQUIRE(aliased.has_value());
+    REQUIRE(*aliased == *file_id);
+    REQUIRE(*graph.path_to_node.find(pup::to_underlying(build_twin)) == *file_id);
+}
+
 TEST_CASE("ensure_file_node creates nodes from PathId", "[graph]")
 {
     auto bs = make_build_graph();