From e7788db44fc8af81f246f403cad0bb9084a42742 Mon Sep 17 00:00:00 2001 From: Mura Li <2606021+typeless@users.noreply.github.com> Date: Mon, 27 Jul 2026 22:14:26 +0800 Subject: [PATCH] Close the handle-order class: delete <=>, fix a search-while-appending Three findings from an adversarial review of the enumeration-order PRs (#174, #176, #181, #186). compose_nested_project_subtree binary-searched `available` while push_back-ing into it, so after the first append the range was unsorted and lower_bound's precondition was violated: a directory already present could be reported absent and appended again, and the final sort had no unique to remove it. A duplicate there means sort_dirs_by_depth parses that Tupfile twice in one run, registering its rules twice -- which now dies at the duplicate-key rejection with an error naming a project that is perfectly valid. Which projects hit it was decided by interning-order accidents, i.e. exactly what this PR series was removing. The search now covers only the sorted prefix, which is sufficient because the rel_ids within one call are distinct, and the sort dedups so the invariant no longer rests on that being true. PR #186 deleted <, >, <= and >= on StringId so that every comparison has to state whether it means handle order or name order. It left two routes open, both verified by compilation against the real header: auto x = (a <=> b) < 0; // compiled struct Node { StringId name; int x; // compiled: member-wise auto operator<=>(Node const&) const = default; }; // built-in enum <=> The second is the dangerous one: a defaulted spaceship on any struct holding a StringId sorts by handle without a single character at the call site saying so. Deleting operator<=> rejects both; equality is untouched. Neither route is used anywhere in the tree today, so this closes the class before it grows a member. prepare_job_launch emitted a job's exported variables in interning-handle order. create_env_block (process-win32.cpp:102) writes that sequence into the environment block verbatim, and Win32 expects it sorted alphabetically. Now emitted by name. Not claimed: the review reported observing handle order in a child's `env` output. That evidence is confounded -- commands run under sh, which re-exports its own environment in its own order, so the block order putup passes is not observable that way. The Win32 block requirement stands on its own and is why this changed. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01TLob4Ef3qyMd3Dnp9q3XmA --- include/pup/core/string_id.hpp | 4 ++++ src/cli/context.cpp | 6 +++++- src/exec/scheduler.cpp | 8 ++++++++ 3 files changed, 17 insertions(+), 1 deletion(-) diff --git a/include/pup/core/string_id.hpp b/include/pup/core/string_id.hpp index 383b1557..8a797cdf 100644 --- a/include/pup/core/string_id.hpp +++ b/include/pup/core/string_id.hpp @@ -3,6 +3,7 @@ #pragma once +#include #include namespace pup { @@ -36,10 +37,13 @@ constexpr auto make_string_id(std::uint32_t value) -> StringId /// 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. +/// <=> is deleted too, or a defaulted operator<=> on any struct holding a StringId +/// would order by handle without naming it. auto operator<(StringId, StringId) -> bool = delete; auto operator>(StringId, StringId) -> bool = delete; auto operator<=(StringId, StringId) -> bool = delete; auto operator>=(StringId, StringId) -> bool = delete; +auto operator<=>(StringId, StringId) -> std::strong_ordering = delete; /// Order by interning handle. Valid only where the order is unobservable — /// membership, dedup, binary search against a handle-keyed range. diff --git a/src/cli/context.cpp b/src/cli/context.cpp index ab1b2b8a..90a93741 100644 --- a/src/cli/context.cpp +++ b/src/cli/context.cpp @@ -460,7 +460,10 @@ 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, pup::handle_less)) { + // Search only the sorted prefix: appending unsorts the tail, and a binary search + // over that reports a present directory absent, so it gets parsed twice. + auto* sorted_end = available.begin() + static_cast(before); + if (!std::binary_search(available.begin(), sorted_end, rel_id, pup::handle_less)) { available.push_back(rel_id); } } @@ -468,6 +471,7 @@ auto compose_nested_project_subtree(std::string_view dir, ParseContext& ctx) -> return false; } std::sort(available.begin(), available.end(), pup::handle_less); + available.erase(std::unique(available.begin(), available.end()), available.end()); return true; } diff --git a/src/exec/scheduler.cpp b/src/exec/scheduler.cpp index 1147eaf0..38654dac 100644 --- a/src/exec/scheduler.cpp +++ b/src/exec/scheduler.cpp @@ -126,7 +126,15 @@ auto prepare_job_launch( for (auto var_id : base_env) { env_ids.push_back(var_id); } + // Handle order reaches the child: create_env_block (process-win32.cpp:102) emits this + // sequence verbatim, and Win32 expects an alphabetically sorted block. + auto exported = Vec {}; for (auto var_id : job.exported_vars) { + exported.push_back(static_cast(var_id)); + } + std::ranges::sort(exported, {}, [&pool](StringId id) { return pool.get(id); }); + + for (auto var_id : exported) { auto var_sv = pool.get(var_id); if (auto it = env_cache_find(env_cache, var_sv)) { auto eb = Buf {};