Reject a rule output that would overwrite a source file (fixes #194)

src/cli/cmd_build.cpp

@@ -1425,6 +1425,79 @@ struct NewCommands {
     pup::Vec<pup::NodeId> forced_cmds;
 };
 
+/// A rule may not produce a file that already exists as a source. Upstream tup rejects it
+/// ("Attempting to insert '<f>' as a generated node when it already exists as a different
+/// type"); putup used to let the generated node win, and in-tree that overwrote the tracked
+/// file on disk. The previous index is what tells a source file apart from our own output
+/// sitting in the source tree after an in-tree build -- exactly the distinction tup's node
+/// types carry.
+auto reject_shadowed_sources(
+    pup::graph::BuildGraph const& state,
+    pup::index::Index const* old_index,
+    std::string_view source_root
+) -> pup::Result<void>
+{
+    auto const& g = state.graph;
+    auto& pool = pup::global_pool();
+    auto build_root_name = pup::graph::get_build_root_name(g);
+
+    // Two questions putup can answer for certain. With no previous index nothing we
+    // produced can be on disk yet, so anything sitting at an output's path is a source.
+    // With one, a path it recorded as a source File is a source whatever is on disk now.
+    // Everything else -- notably a generated file the previous index does not mention,
+    // which is what a scoped build leaves behind for out-of-scope outputs -- is not
+    // decidable from here and is left alone.
+    auto recorded_sources = pup::Vec<StringId> {};
+    if (old_index != nullptr) {
+        for (auto const& file : old_index->files()) {
+            if (file.type == pup::NodeType::File && !pup::is_empty(file.path)) {
+                recorded_sources.push_back(file.path);
+            }
+        }
+        std::sort(recorded_sources.begin(), recorded_sources.end(), pup::handle_less);
+    }
+
+    for (auto id : pup::graph::all_nodes(g)) {
+        if (pup::node_id::is_command(id) || pup::graph::get<pup::NodeType>(g, id) != pup::NodeType::Generated) {
+            continue;
+        }
+        auto full_path_sv = pup::graph::get_full_path(g, id, state.path_cache);
+        if (full_path_sv.empty()) {
+            continue;
+        }
+        auto rel_sv = pool.get(pup::strip_path_prefix(full_path_sv, build_root_name));
+        // The two-stage configure design has a rule produce the tup.config that the same
+        // build then reads as configuration, so this one path is generated and read on
+        // purpose rather than shadowing anything.
+        if (rel_sv.ends_with("tup.config")) {
+            continue;
+        }
+        auto rel_id = pool.intern(rel_sv);
+        if (old_index != nullptr) {
+            if (!std::binary_search(recorded_sources.begin(), recorded_sources.end(), rel_id, pup::handle_less)) {
+                continue;
+            }
+        } else {
+            auto abs_sv = pool.get(pup::path::join(source_root, rel_sv));
+            if (!pup::platform::exists(abs_sv)) {
+                continue;
+            }
+        }
+
+        auto err = pup::Buf {};
+        err.fmt(
+            "Attempting to create '{}' as a generated file when it already exists as a source "
+            "file. You can do one of two things to fix this:\n"
+            "  1) If this file is really supposed to be created from the command, delete the "
+            "file from the filesystem and try again.\n"
+            "  2) Change your rule in the Tupfile so you aren't trying to overwrite the file.",
+            rel_sv
+        );
+        return pup::make_error<void>(pup::ErrorCode::DuplicateNode, err.view());
+    }
+    return {};
+}
+
 /// Commands that must run because of what they are rather than because a file changed:
 /// no counterpart in the previous build, or a counterpart whose signature differs. Their
 /// outputs join the changed-file set; a command that contributes no output paths cannot
@@ -1817,6 +1890,11 @@ auto build_single_variant(
 
     auto index_path = pup::global_pool().get(ctx.layout().index_path());
     auto const* old_idx_ptr = ctx.old_index();
+
+    if (auto shadowed = reject_shadowed_sources(bs, old_idx_ptr, source_root_str); !shadowed) {
+        veprint(variant_name, "Error: {}\n", shadowed.error().msg());
+        return EXIT_FAILURE;
+    }
     auto use_incremental = false;
     auto changed_files = pup::Vec<StringId> {};
     auto forced_cmds = pup::Vec<pup::NodeId> {};

test/unit/test_e2e.cpp

@@ -4522,6 +4522,54 @@ SCENARIO("Editing a rule's recipe does not make it a different rule", "[e2e][ide
     }
 }
 
+SCENARIO("A rule may not overwrite a checked-in source file", "[e2e][shadow]")
+{
+    GIVEN("a rule whose output path is also a source file in the tree")
+    {
+        auto f = E2EFixture { "glob_mixed_space" };
+        f.write_file("Tupfile", ": gen.src |> cp %f %o |> x.dat\n");
+        f.write_file("gen.src", "FROMRULE\n");
+        f.write_file("x.dat", "FROMSRC\n");
+
+        WHEN("the project is built")
+        {
+            auto result = f.build();
+
+            THEN("it is rejected and the source file is untouched")
+            {
+                INFO("stdout: " << result.stdout_output);
+                INFO("stderr: " << result.stderr_output);
+                REQUIRE_FALSE(result.success());
+                REQUIRE(result.stderr_output.find("x.dat") != std::string::npos);
+                REQUIRE(f.read_file("x.dat") == "FROMSRC\n");
+            }
+        }
+    }
+}
+
+SCENARIO("An in-tree rebuild does not mistake its own output for a source file", "[e2e][shadow]")
+{
+    GIVEN("an ordinary in-tree project built once, so its output is on disk beside the sources")
+    {
+        auto f = E2EFixture { "glob_mixed_space" };
+        f.write_file("Tupfile", ": src.txt |> cp %f %o |> out.txt\n");
+        f.write_file("src.txt", "ORIGINAL\n");
+        REQUIRE(f.build().success());
+        REQUIRE(f.exists("out.txt"));
+
+        WHEN("it is built again")
+        {
+            auto again = f.build();
+
+            THEN("the rebuild succeeds")
+            {
+                INFO("stderr: " << again.stderr_output);
+                REQUIRE(again.success());
+            }
+        }
+    }
+}
+
 SCENARIO("A glob's %f order does not depend on the build directory's name", "[e2e][glob][pathspace]")
 {
     GIVEN("a glob matching one generated and one source file, built out-of-tree")
@@ -4557,12 +4605,11 @@ SCENARIO("A glob's %f order does not depend on the build directory's name", "[e2
     }
 }
 
-SCENARIO("A generated file shadowing a source file is one glob match", "[e2e][glob][pathspace][deviation]")
+SCENARIO("A glob over a generated file that shadows a source is rejected", "[e2e][glob][pathspace]")
 {
-    // Upstream tup rejects this project outright ("Attempting to insert 'x.dat' as a generated
-    // node when it already exists as a different type"). putup accepts it and lets the generated
-    // node win; issue #194 tracks that deviation. This asserts only the dedup #191 is about --
-    // one match rather than two -- and is not a claim that shadowing should be legal.
+    // Tagged [deviation] when filed: putup let the generated node win where tup rejects the
+    // project. Now that #194 rejects it too, the dedup #191 added for this shape is
+    // unreachable through a legal project and remains only as a defence.
     GIVEN("a generated file whose name also exists as a checked-in source")
     {
         auto f = E2EFixture { "glob_mixed_space" };
@@ -4575,13 +4622,14 @@ SCENARIO("A generated file shadowing a source file is one glob match", "[e2e][gl
 
         WHEN("built out-of-tree")
         {
-            REQUIRE(f.build({ "-B", "build" }).success());
+            auto result = f.build({ "-B", "build" });
 
-            THEN("it is consumed once, not twice")
+            THEN("the build is rejected and the source file survives")
             {
-                auto content = f.read_file("build/a/all.txt");
-                INFO("all.txt: " << content);
-                REQUIRE(content == "FROMRULE\n");
+                INFO("stderr: " << result.stderr_output);
+                REQUIRE_FALSE(result.success());
+                REQUIRE(result.stderr_output.find("x.dat") != std::string::npos);
+                REQUIRE(f.read_file("a/x.dat") == "FROMSRC\n");
             }
         }
     }