fix(why-miss): join the cascade walk by compilation unit, not crate name (#627) - #642
Merged
Conversation
…ame (#627) The `extern:` cascade walk paired events by `crate_name`, which is not a compilation-unit identity. One build tree routinely holds several units sharing a name: two versions of a package, a host and a target build of the same crate, the same crate under different feature sets. Pairing by name lets the walk diff a compile against an unrelated unit's history and descend into a branch that has nothing to do with the miss. Cargo's dependency renaming breaks it from the other side: with `foo_old = { package = "foo" }` the consumer's key records `foo_old` while the producer's own events say `foo`, so the walk finds no event and dead-ends on the alias. Join by cargo's `-C extra-filename` hash instead. The producing compile records it as `unit_id`; the consumer recovers the same value per extern from the `--extern` artifact filename (`lib<name>-<hash>.rlib`) into `extern_units`. That hash is what cargo uses to keep those units' artifacts from colliding in one deps/ directory, so it identifies the unit exactly, and it is visible from both sides, so the join holds whatever name the consumer used. Not `-C metadata`: cargo sets the two to different hashes (observed `-C metadata=04bad873faff484a -C extra-filename=-843f02d6a46ebef1`), and only extra-filename appears in the filename the consumer sees. The issue proposed recording the dependency artifact's content hash on the producing event. This is cheaper (no hashing at all, both values are already parsed) and fixes more: a content hash changes on every compile, so it cannot pair a compile with its own previous compile, which is what baseline selection needs. Both fields are diagnostic only. Neither is folded into a cache key — keying on cargo's unit hashing would break cross-machine sharing — and both ride the existing `[cache] explain_miss` gate, so the default path is unchanged. Events without them (pre-#627, non-cargo rustc, sysroot crates) still match by name, and an event carrying a DIFFERENT unit id is never accepted on a name match, so the fallback cannot mispair two known units. Endpoints now report the producing crate's own name rather than the consumer's alias, and the walk tracks each branch by the identity of the event it actually selected, so `reached`, `seen`, the cycle check and every root agree on what counts as one node. Verified on a real build with a renamed path dependency: `app` records extern_units={foo_old: 0532daf0ee3516f0} and `foo` records unit_id=0532daf0ee3516f0, and `why-miss app` now resolves to "root: foo -- own inputs changed: sources" where it used to dead-end at "unresolved: foo_old". The rename and duplicate-version tests re-run their events with the unit ids stripped and assert the old, wrong result, so what the fix buys is pinned rather than assumed.
CI's staged mutation gate flagged nine survivors, all in the new code: the node cap and both reached-counter increments, the two halves of the cycle test, the three conditions of the legacy baseline fallback, and the unit-identity stash accessors. Each now has a test that fails when the operator is flipped: a cycle that excludes the starting crate (so only the path half of loops_back fires), a graph wide enough to trip MAX_NODES, convergence counted on an unresolved endpoint, a nearer legacy event of a different crate that the baseline fallback must refuse, and a key computation whose stashed unit identity is taken exactly once.
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.
Fixes #627.
What was wrong
The
extern:cascade walk paired events bycrate_name, which is not a compilation-unit identity. One build tree routinely holds several units sharing a name:foo v1andfoo v2)Pairing by name lets the walk diff a compile against an unrelated unit's history and descend into a branch that has nothing to do with the miss. Cargo's dependency renaming breaks it from the other side: with
foo_old = { package = "foo" }the consumer's key recordsfoo_oldwhile the producing crate's own events sayfoo, so the walk finds no event and dead-ends on the alias.The join
Cargo's
-C extra-filenamehash. The producing compile records it asunit_id; the consumer recovers the same value per extern from the--externartifact filename (lib<name>-<hash>.rlib) intoextern_units.That hash is exactly what cargo uses to keep same-named units' artifacts from colliding in one
deps/directory, so within a build tree it identifies the unit precisely, and it is visible from both sides, so the join holds whatever name the consumer used.Not
-C metadata: cargo sets the two to different hashes (observed-C metadata=04bad873faff484a -C extra-filename=-843f02d6a46ebef1), and only extra-filename appears in the filename a consumer sees on its--externpath.Why not the approach the issue proposed
The issue recommended recording the dependency artifact's content hash on the producing event (option 2). This is cheaper and fixes more:
Diagnostic only
Neither field is folded into a cache key. Keying on cargo's unit hashing would tie kache's keys to it and break cross-machine sharing. Both ride the existing
[cache] explain_missgate, so the default path is unchanged, and both are#[serde(default)]and skipped when empty.Events without them (pre-#627 wrappers, non-cargo rustc invocations, sysroot crates) still match by name. An event carrying a DIFFERENT unit id is never accepted on a name match, so the fallback cannot mispair two known units, and an exact unit match anywhere in the window is preferred over a nearer name-only one.
Also
Endpoints now report the producing crate's own name rather than the consumer's alias, since the alias names nothing the user can go look at. The walk tracks each branch by the identity of the event it actually selected, so
reached,seen, the cycle check and every root agree on what counts as one node.Verification
A real build with a renamed path dependency:
apprecordsextern_units={foo_old: 0532daf0ee3516f0}andfoorecordsunit_id=0532daf0ee3516f0.why-miss appresolves towhere it previously dead-ended at
unresolved: foo_old -- artifact differs, but it has no compile recorded in this event window, the exact output quoted in the issue.The rename and duplicate-version tests re-run their events with the unit ids stripped and assert the OLD, wrong result, so what the fix buys is pinned rather than assumed. One of those assertions corrected me: I expected name matching to blame the wrong version's
argsgroup, but it actually descends into that version's unrelated dependencies.1418 unit tests pass (20 in
miss_chain), clippy clean. The e2e gate suite passed locally before the final restructure; CI covers it here on all three platforms.Review note
A blind 2-leg cross-model review changed this patch. Both legs independently found the same identity inconsistency I had hit while they ran, but argued for the opposite normalization: track a branch by the identity of the event actually selected, not by what the consumer asked for. That is better, and the reason is concrete: when two different requested units both fall back by name to one legacy producer, my direction explores that event twice and emits two roots for it, while theirs collapses them into one node with the right branch count. It also makes cycle detection compare like with like, since hops carry their own event's identity. Adopted, with a test for that case.
Adopting it broke an existing cycle test, which was worth knowing: moving the check after producer resolution meant a dependency pointing back at the crate being explained, with no event of its own, stopped registering as a loop. The check now runs on both identities, requested before resolution and resolved after.
Not addressed
passthroughs_forstill attributes uncached compiles by package directory rather than by unit. Both review legs flagged it. It is pre-existing, already documented as a deliberately loose heuristic that over-reports rather than misdirects, and orthogonal to the join fixed here.