Delete StringId's relational operators so handle order and name order stop being the same character (fixes #185, #183, #184) - #186
Merged
Conversation
PR metricsPerformance (gcc example, Linux)
Deterministic signals: instructions (cachegrind-simulated instruction reads — exact across runs, no PMU needed), page faults, peak RSS, and the cachegrind D1/LL miss rates. CPU time is user+sys from time(1). Internal statistics (gcc example, up-to-date dry run)
Counters from Binary size (Linux)
Test coverage (lines)
103 files · 15008/17335 lines covered Deltas vs main@59ff72962. Updated for eecf318 |
…184) StringId is enum class : uint32_t, so < on it was the built-in enum comparison: interning order. Whether a given comparison meant "handle order, because this is a set" or "name order, because this order is observable" was untyped, unstated, and spelled with the same character. Four bugs came out of that confusion -- and wrongly cleared in PR #176's own description. The relational operators are now deleted. A user-declared deleted operator wins overload resolution over the built-in candidate for a scoped enum, so every comparison site had to be visited and state which order it means: handle_less for a set, or a pool projection for anything a person or another build can observe. Equality is untouched. The compiler found 21 sites. Eighteen were genuine set structures and now say so. Three were ordering bugs: src/exec/scheduler.cpp:62 (fixes #183) -- the env cache was sorted by handle and binary-searched by name at :44, so lower_bound's precondition was violated and lookups missed variables that were present. prepare_job_launch then omitted them and, since base_child_env contributes only PATH=, they were absent from the child entirely. Three exports declared in reverse-lexicographic order lost all three, silently, exit 0. src/cli/context.cpp:677 (fixes #184) -- cached_env_vars was sorted by handle and binary-searched by name at builder.cpp:1305. A missed lookup falls through to an empty value, so an imported variable lost its cached value on the next build, changing rendered command text and therefore command identity. src/parser/var_tracking.cpp:21 -- group_by_name ordered by handle despite its name, and the show-var command prints the result. Now ordered by name, which changes that command's output order. src/core/layout.cpp:221 is fixed here too because it will not compile otherwise; that is the same one-line change as PR #176, which can land either before or after this. No INDEX_VERSION bump: no persisted format changes and no deliberate identity semantics change. Commands whose identity was computed from a wrongly-empty env value will re-run once, which is the intended repair. Both bugs were reproduced before the fix and re-checked after: export ZVAR/MVAR/AVAR before: 0 of 3 in child env after: 3 of 3 import Z/M/A, rebuild before: A= M= Z= after: A=aaa M=mmm Z=zzz Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TLob4Ef3qyMd3Dnp9q3XmA
typeless
force-pushed
the
fix/185-stringid-ordering
branch
from
July 27, 2026 08:55
d7c1b5c to
eecf318
Compare
typeless
added a commit
that referenced
this pull request
Jul 27, 2026
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TLob4Ef3qyMd3Dnp9q3XmA
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
StringIdisenum class : uint32_t, so<on it was the built-in enum comparison — interning order. Whether a site meant "handle order, because this is a set" or "name order, because this order is observable" was untyped, unstated, and spelled with the same character. Four bugs came out of that confusion: #171, #175, and the two live ones this fixes — and the two live ones were audited and wrongly cleared in PR #176's own description.The change
A user-declared deleted operator wins overload resolution over the built-in candidate for a scoped enum, so this does not require making
StringIda class type. Equality, casts, andto_underlyingare untouched.What the compiler found
21 sites. Eighteen were genuine set structures — dedup, membership, binary search against a handle-keyed range — and now say
handle_lessout loud. Every sort/search pair was checked to agree.Three were ordering bugs:
src/exec/scheduler.cpp:62(#183)lower_bound-searched by name at:44. Precondition violated, lookups miss present entries,prepare_job_launchomits the variable, andbase_child_envcontributes onlyPATH=— so it is absent from the child entirely.src/cli/context.cpp:677(#184)cached_env_varssorted by handle, searched by name atbuilder.cpp:1305. A missed lookup falls through to an empty value, changing rendered command text and therefore command identity.src/parser/var_tracking.cpp:21group_by_nameordered by handle despite its name, andshow varprints it. Now name-ordered — this changes that command's output order.Reproduced before and after
Both are pinned by new tests, each observed failing first: a three-variable section in
test/unit/test_exec.cpp(the existing test used one variable, andlower_boundover a one-element range cannot be wrong), and animport_ordere2e fixture that builds with the variables set and rebuilds with them unset.Notes
src/core/layout.cpp:221is fixed here too, because it will not compile otherwise. That is the same one-line change as Order discovered variants by name, not by interning order (fixes #175) #176, which can land before or after this — expect a trivial conflict if Order discovered variants by name, not by interning order (fixes #175) #176 goes first.INDEX_VERSIONbump. No persisted format change and no deliberate identity-semantics change; commands whose identity was computed from a wrongly-empty env value re-run once, which is the repair. Flagging it explicitly since Order glob matches by path, not by interning order (fixes #171) #174 and Merge filesystem and generated glob matches into one path-ordered list (fixes #177, fixes #178) #180 both bumped.std::map/std::setkeyed onStringId, andSortedIdVec/SortedPairVectake rawuint32_t, so they were unaffected.make testgreen (142317 assertions, 617 cases, 32 e2e shards);make tidyandmake iwyuclean; bootstrap scripts unchanged.🤖 Generated with Claude Code
https://claude.ai/code/session_01TLob4Ef3qyMd3Dnp9q3XmA