Order discovered variants by name, not by interning order (fixes #175) - #176
Conversation
discover_variants ended with std::sort(result.begin(), result.end()) on a Vec<StringId>. StringId is an enum class over the interning handle, and the names are interned in read_directory order, so the sort ordered by raw readdir order. Same defect as #171, at the second and last site that reads a directory into StringIds and treats handle order as name order. Its one consumer is the build-directory hint, so the damage is cosmetic -- the order never reaches %f, command identity, or anything persisted -- but the same tree listed its candidates differently on different machines: before: found: build-yankee, build-mike, build-charlie, build-bravo, build-alpha, build-zeta after: found: build-alpha, build-bravo, build-charlie, build-mike, build-yankee, build-zeta (readdir order was yankee mike charlie bravo alpha zeta.) The test fixes interning order rather than trusting the filesystem to enumerate out of order, as the #171 test does. The remaining std::sort calls over StringId -- cmd_build.cpp, context.cpp, scheduler.cpp, glob_expand_all's exclusion list -- are sort+unique or binary_search set structures, where handle order is order-agnostic by construction. cmd_show.cpp and multi_variant.cpp already sort through pool.get. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TLob4Ef3qyMd3Dnp9q3XmA
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 · 14986/17313 lines covered Deltas vs main@1738aa715. Updated for 02533ff |
|
Adversarial review of this PR found that its claim 2 — that the remaining
Neither is introduced by this PR — both are pre-existing — but the audit in the PR body cleared them, and claim 1 ("the second and LAST site") is wrong as a result. The code change here is still correct; the reasoning in the description was not. The general fix is #185. |
…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 -- #171, #175, and the two this commit fixes -- and the two live ones were audited 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
…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
…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
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
discover_variantsended withstd::sort(result.begin(), result.end())on aVec<StringId>.StringIdis an enum class over the interning handle and the names are interned inread_directoryorder, so the sort ordered by raw readdir order — the same defect as #171 (fixed in #174), at the second and last site that reads a directory intoStringIds and treats handle order as name order.Its one consumer is the build-directory hint, so the damage is cosmetic — this order never reaches
%f, command identity, or anything persisted — but the same tree listed its candidates differently on different machines.Measured
Six build dirs enumerating (
ls -U) asyankee mike charlie bravo alpha zeta:Test
Same shape as the #171 regression test: it interns the names in reverse-lexicographic order before scanning, so handle order is deterministically the reverse of name order rather than whatever the filesystem happens to return. Observed failing first:
Class closed
The remaining
std::sortcalls overStringId— incmd_build.cpp,context.cpp,scheduler.cpp, andglob_expand_all's exclusion list — are sort+unique or binary_search set structures, where handle order is order-agnostic by construction.cmd_show.cpp:173andmulti_variant.cpp:76already sort throughpool.get. No index version bump: nothing here is persisted.make testgreen (142310 assertions, 617 cases, 32 e2e shards);make tidyandmake iwyuclean.🤖 Generated with Claude Code
https://claude.ai/code/session_01TLob4Ef3qyMd3Dnp9q3XmA