Delete StringId's relational operators so handle order and name order stop being the same character (fixes #185, #183, #184)

include/pup/core/string_id.hpp

@@ -32,4 +32,19 @@ constexpr auto make_string_id(std::uint32_t value) -> StringId
     return static_cast<StringId>(value);
 }
 
+/// Relative order of two StringIds is deleted, so each site states which order it
+/// means: `handle_less` for a set, or a pool projection for anything observable.
+/// The built-in enum comparison is interning order, and the two spellings were
+/// indistinguishable — issues #171, #175, #183 and #184 were all that confusion.
+auto operator<(StringId, StringId) -> bool = delete;
+auto operator>(StringId, StringId) -> bool = delete;
+auto operator<=(StringId, StringId) -> bool = delete;
+auto operator>=(StringId, StringId) -> bool = delete;
+
+/// Order by interning handle. Valid only where the order is unobservable —
+/// membership, dedup, binary search against a handle-keyed range.
+inline constexpr auto handle_less = [](StringId a, StringId b) {
+    return to_underlying(a) < to_underlying(b);
+};
+
 } // namespace pup

src/cli/cmd_build.cpp

@@ -210,7 +210,7 @@ auto collect_implicit_dep_files(
         }
     }
 
-    std::sort(result.begin(), result.end());
+    std::sort(result.begin(), result.end(), pup::handle_less);
     result.erase(std::unique(result.begin(), result.end()), result.end());
     return result;
 }
@@ -247,7 +247,7 @@ auto collect_inactive_output_paths(pup::graph::BuildGraph const& state) -> Vec<S
         }
     }
 
-    std::sort(result.begin(), result.end());
+    std::sort(result.begin(), result.end(), pup::handle_less);
     result.erase(std::unique(result.begin(), result.end()), result.end());
     return result;
 }
@@ -282,7 +282,7 @@ auto find_changed_files_with_implicit(
         }
 
         // No active rule produces this, so its absence is not a change.
-        if (std::binary_search(inactive_outputs.begin(), inactive_outputs.end(), file.path)) {
+        if (std::binary_search(inactive_outputs.begin(), inactive_outputs.end(), file.path, pup::handle_less)) {
             continue;
         }
 
@@ -299,7 +299,7 @@ auto find_changed_files_with_implicit(
         if (!scopes.empty() && !is_tupfile(file_path_sv)
             && !pup::is_path_in_any_scope(scope_path_sv, scopes)
             && !std::binary_search(upstream_files.begin(), upstream_files.end(), file_path_sv)
-            && !std::binary_search(implicit_dep_files.begin(), implicit_dep_files.end(), file.path)) {
+            && !std::binary_search(implicit_dep_files.begin(), implicit_dep_files.end(), file.path, pup::handle_less)) {
             continue;
         }
 
@@ -1187,7 +1187,7 @@ auto expand_implicit_deps(
     for (auto const& s : changed) {
         added.push_back(s);
     }
-    std::sort(added.begin(), added.end());
+    std::sort(added.begin(), added.end(), pup::handle_less);
 
     // Build sorted path -> file pointer map
     auto path_to_file = Vec<std::pair<StringId, pup::index::FileEntry const*>> {};
@@ -1197,7 +1197,7 @@ auto expand_implicit_deps(
             path_to_file.emplace_back(file.path, &file);
         }
     }
-    std::sort(path_to_file.begin(), path_to_file.end());
+    std::sort(path_to_file.begin(), path_to_file.end(), [](auto const& a, auto const& b) { return pup::handle_less(a.first, b.first); });
 
     // Build edge index using NodeIdArenaIndex pattern (from_id -> edge pointers)
     // Use a sorted vector of (from_id, edge*) for grouped lookup
@@ -1211,7 +1211,7 @@ auto expand_implicit_deps(
     std::sort(edges_by_from.begin(), edges_by_from.end(), [](auto const& a, auto const& b) { return a.first < b.first; });
 
     for (auto const& path : changed) {
-        auto it = std::lower_bound(path_to_file.begin(), path_to_file.end(), path, [](auto const& p, auto const& k) { return p.first < k; });
+        auto it = std::lower_bound(path_to_file.begin(), path_to_file.end(), path, [](auto const& p, auto const& k) { return pup::handle_less(p.first, k); });
         if (it == path_to_file.end() || it->first != path) {
             continue;
         }
@@ -1236,8 +1236,8 @@ auto expand_implicit_deps(
                 auto output_path_sv = pup::graph::get_full_path(state.graph, output_id, state.path_cache);
                 if (!output_path_sv.empty()) {
                     auto output_path_id = pup::global_pool().intern(output_path_sv);
-                    if (!std::binary_search(added.begin(), added.end(), output_path_id)) {
-                        auto pos = std::lower_bound(added.begin(), added.end(), output_path_id);
+                    if (!std::binary_search(added.begin(), added.end(), output_path_id, pup::handle_less)) {
+                        auto pos = std::lower_bound(added.begin(), added.end(), output_path_id, pup::handle_less);
                         added.insert(pos, output_path_id);
                         result.push_back(output_path_id);
                     }
@@ -1433,7 +1433,7 @@ auto reconcile_input_set(
             new_sources.push_back(path_id);
         }
     }
-    std::sort(graph_paths.begin(), graph_paths.end());
+    std::sort(graph_paths.begin(), graph_paths.end(), pup::handle_less);
 
     auto index_files = pup::Vec<std::pair<StringId, pup::NodeId>> {};
     index_files.reserve(idx.files().size());
@@ -1442,11 +1442,11 @@ auto reconcile_input_set(
             index_files.emplace_back(file.path, file.id);
         }
     }
-    std::sort(index_files.begin(), index_files.end());
+    std::sort(index_files.begin(), index_files.end(), [](auto const& a, auto const& b) { return pup::handle_less(a.first, b.first); });
 
     auto find_indexed = [&](StringId path_id) -> pup::index::FileEntry const* {
         auto it = std::lower_bound(
-            index_files.begin(), index_files.end(), path_id, [](auto const& entry, StringId key) { return entry.first < key; }
+            index_files.begin(), index_files.end(), path_id, [](auto const& entry, StringId key) { return pup::handle_less(entry.first, key); }
         );
         return (it != index_files.end() && it->first == path_id) ? idx.find_file_by_id(it->second) : nullptr;
     };
@@ -1463,7 +1463,7 @@ auto reconcile_input_set(
 
     auto orphaned = pup::Vec<pup::NodeId> {};
     for (auto path_id : changed) {
-        if (std::binary_search(graph_paths.begin(), graph_paths.end(), path_id)) {
+        if (std::binary_search(graph_paths.begin(), graph_paths.end(), path_id, pup::handle_less)) {
             continue;
         }
         auto const* file = find_indexed(path_id);

src/cli/cmd_show.cpp

@@ -574,7 +574,7 @@ auto cmd_export_instructions(Options const& opts, std::string_view variant_name)
 
     for (auto const& cmd : graph.commands) {
         if (!is_empty(cmd.instruction_id)) {
-            auto pos = std::lower_bound(instruction_usage.begin(), instruction_usage.end(), cmd.instruction_id, [](auto const& p, auto const& k) { return p.first < k; });
+            auto pos = std::lower_bound(instruction_usage.begin(), instruction_usage.end(), cmd.instruction_id, [](auto const& p, auto const& k) { return pup::handle_less(p.first, k); });
             if (pos != instruction_usage.end() && pos->first == cmd.instruction_id) {
                 pos->second.push_back(cmd.id);
             } else {

src/cli/context.cpp

@@ -257,7 +257,7 @@ auto discover_tupfile_dirs(
         return true;
     });
 
-    std::sort(dirs.begin(), dirs.end());
+    std::sort(dirs.begin(), dirs.end(), pup::handle_less);
     dirs.erase(std::unique(dirs.begin(), dirs.end()), dirs.end());
     return dirs;
 }
@@ -460,14 +460,14 @@ auto compose_nested_project_subtree(std::string_view dir, ParseContext& ctx) ->
     for (auto sub_id : sub_dirs) {
         auto sub_sv = pool.get(sub_id);
         auto rel_id = (sub_sv == ".") ? pool.intern(marker_prefix) : pup::path::join(marker_prefix, sub_sv);
-        if (!std::binary_search(available.begin(), available.end(), rel_id)) {
+        if (!std::binary_search(available.begin(), available.end(), rel_id, pup::handle_less)) {
             available.push_back(rel_id);
         }
     }
     if (available.size() == before) {
         return false;
     }
-    std::sort(available.begin(), available.end());
+    std::sort(available.begin(), available.end(), pup::handle_less);
     return true;
 }
 
@@ -674,7 +674,7 @@ auto load_old_index(std::string_view output_root, bool verbose) -> IndexLoadResu
         }
     }
 
-    std::sort(result.cached_env_vars.begin(), result.cached_env_vars.end());
+    std::ranges::sort(result.cached_env_vars, {}, [&pool](auto const& p) { return pool.get(p.first); });
 
     if (verbose && !result.cached_env_vars.empty()) {
         print("Loaded {} cached env vars from index\n", result.cached_env_vars.size());

src/cli/output.cpp

@@ -36,7 +36,7 @@ auto remove_empty_directories(
     for (auto id : output_dir_ids) {
         dirs.push_back(id);
     }
-    std::ranges::sort(dirs);
+    std::ranges::sort(dirs, handle_less);
     dirs.erase(std::unique(dirs.begin(), dirs.end()), dirs.end());
     std::ranges::sort(dirs, std::greater {}, [](auto id) {
         return global_pool().get(id).size();

src/exec/scheduler.cpp

@@ -59,7 +59,7 @@ auto build_env_cache(Vec<BuildJob> const& jobs) -> EnvCache
             names.push_back(var_id);
         }
     }
-    std::sort(names.begin(), names.end());
+    std::ranges::sort(names, {}, [&pool](StringId id) { return pool.get(id); });
     names.erase(std::unique(names.begin(), names.end()), names.end());
 
     // Look up each name once

src/graph/builder.cpp

@@ -187,7 +187,7 @@ auto request_demand_driven_parse(
     auto in_available = [&] {
         auto dir_id = global_pool().find(dir_path);
         return dir_id != StringId::Empty
-            && std::binary_search(eval.available_tupfile_dirs->begin(), eval.available_tupfile_dirs->end(), dir_id);
+            && std::binary_search(eval.available_tupfile_dirs->begin(), eval.available_tupfile_dirs->end(), dir_id, pup::handle_less);
     };
     if (!in_available() && eval.compose_nested_project) {
         (void)eval.compose_nested_project(dir_path);

src/parser/glob.cpp

@@ -296,10 +296,10 @@ auto glob_expand_all(
 
     if (!result.exclusions.empty()) {
         auto excl_sorted = Vec<StringId> { result.exclusions };
-        std::sort(excl_sorted.begin(), excl_sorted.end());
+        std::sort(excl_sorted.begin(), excl_sorted.end(), pup::handle_less);
         auto& m = result.matches;
         for (auto it = m.begin(); it != m.end();) {
-            if (std::binary_search(excl_sorted.begin(), excl_sorted.end(), *it)) {
+            if (std::binary_search(excl_sorted.begin(), excl_sorted.end(), *it, pup::handle_less)) {
                 it = m.erase(it);
             } else {
                 ++it;

src/parser/var_tracking.cpp

@@ -18,7 +18,10 @@ auto group_by_name(AssignmentLog const& log)
     auto result = Vec<VarHistory> {};
 
     for (auto const& assign : log) {
-        auto it = std::lower_bound(result.begin(), result.end(), assign.name, [](auto const& h, auto const& n) { return h.name < n; });
+        // Ordered by name, not handle: `show var` prints this list.
+        auto it = std::lower_bound(result.begin(), result.end(), assign.name, [&](auto const& h, auto const& n) {
+            return global_pool().get(h.name) < global_pool().get(n);
+        });
         if (it == result.end() || it->name != assign.name) {
             it = result.insert(it, VarHistory { .name = assign.name, .assignments = {}, .final_value = {} });
         }

test/e2e/fixtures/import_order/Tupfile.fixture

@@ -0,0 +1,4 @@
+import PUP_T_ZVAR
+import PUP_T_MVAR
+import PUP_T_AVAR
+: |> echo "A=$(PUP_T_AVAR) M=$(PUP_T_MVAR) Z=$(PUP_T_ZVAR)" > %o |> out.txt

test/unit/test_e2e.cpp

@@ -8267,3 +8267,31 @@ SCENARIO("A glob reaching into a sibling directory counts each match once", "[e2
         }
     }
 }
+
+SCENARIO("Imported values survive in the cache whatever order the imports are declared in", "[e2e][incremental]")
+{
+    GIVEN("a project importing three variables, declared in reverse-lexicographic order")
+    {
+        auto f = E2EFixture { "import_order" };
+        {
+            auto z = EnvGuard { "PUP_T_ZVAR", "zzz" };
+            auto m = EnvGuard { "PUP_T_MVAR", "mmm" };
+            auto a = EnvGuard { "PUP_T_AVAR", "aaa" };
+            REQUIRE(f.init().success());
+            REQUIRE(f.build().success());
+            REQUIRE(f.read_file("out.txt") == "A=aaa M=mmm Z=zzz\n");
+        }
+
+        WHEN("the variables are unset and the project is rebuilt")
+        {
+            auto second = f.build();
+
+            THEN("the values cached in the index are used")
+            {
+                INFO("stdout: " << second.stdout_output);
+                REQUIRE(second.success());
+                REQUIRE(f.read_file("out.txt") == "A=aaa M=mmm Z=zzz\n");
+            }
+        }
+    }
+}

test/unit/test_exec.cpp

@@ -479,6 +479,57 @@ TEST_CASE("Scheduler exported_vars", "[exec]")
         pup::platform::unset_env("PUP_TEST_EXPORT_VAR");
     }
 
+    SECTION("every exported var reaches the environment whatever order it was interned in")
+    {
+        // Interned reverse-lexicographically, so handle order is the reverse of
+        // name order. Names are unique to this test so this decides the handles.
+        auto z_id = intern("PUP_TEST_ORDER_Z");
+        auto m_id = intern("PUP_TEST_ORDER_M");
+        auto a_id = intern("PUP_TEST_ORDER_A");
+        pup::platform::set_env("PUP_TEST_ORDER_Z", "zzz");
+        pup::platform::set_env("PUP_TEST_ORDER_M", "mmm");
+        pup::platform::set_env("PUP_TEST_ORDER_A", "aaa");
+
+        auto bs = graph::make_build_graph();
+
+        auto input_id = graph::add_file_node(bs.graph, graph::FileNode {
+            .name = intern("/dev/null"),
+        });
+
+        auto cmd_node = graph::CommandNode {
+            .instruction_id = intern("echo \"[$PUP_TEST_ORDER_A|$PUP_TEST_ORDER_M|$PUP_TEST_ORDER_Z]\""),
+        };
+        cmd_node.exported_vars.insert(to_underlying(z_id));
+        cmd_node.exported_vars.insert(to_underlying(m_id));
+        cmd_node.exported_vars.insert(to_underlying(a_id));
+        auto cmd_id = graph::add_command_node(bs.graph, std::move(cmd_node));
+
+        auto output_id = graph::add_file_node(bs.graph, graph::FileNode {
+            .type = NodeType::Generated,
+            .name = intern("/tmp/test_export_order.txt"),
+        });
+
+        (void)graph::add_edge(bs.graph, *input_id, *cmd_id);
+        (void)graph::add_edge(bs.graph, *cmd_id, *output_id);
+
+        auto captured_output = std::string {};
+        auto opts = SchedulerOptions { .jobs = 1 };
+        auto scheduler = Scheduler { opts };
+        scheduler.on_job_complete([&](BuildJob const&, JobResult const& result) {
+            captured_output = std::string { sv(result.output) };
+        });
+
+        auto result = scheduler.build(bs);
+
+        REQUIRE(result.has_value());
+        REQUIRE(result->completed_jobs == 1);
+        REQUIRE(captured_output.find("[aaa|mmm|zzz]") != std::string_view::npos);
+
+        pup::platform::unset_env("PUP_TEST_ORDER_Z");
+        pup::platform::unset_env("PUP_TEST_ORDER_M");
+        pup::platform::unset_env("PUP_TEST_ORDER_A");
+    }
+
     SECTION("unexported vars not passed")
     {
         pup::platform::set_env("PUP_TEST_HIDDEN_VAR", "hidden_value");