Add adt_def to struct and enum metadata - #64
Merged
Merged
Conversation
jberthold
marked this pull request as ready for review
April 4, 2025 05:02
dkcumming
approved these changes
Apr 4, 2025
3 tasks
dkcumming
pushed a commit
that referenced
this pull request
Mar 4, 2026
This PR replaces all index-based sort keys with content-derived ones, making the ordering of output vectors deterministic across runs. The one remaining source of cross-run non-determinism is `adt_def` (see below for why that can't be fixed here). ## Context For those unfamiliar: rustc stores types, allocations, and other values in central tables and refers to them by integer index. These indices are assigned in the order the compiler happens to intern items, which is not guaranteed to be the same between invocations. There are two independent reasons for this: 1. **Hash-map iteration order**: Rustc uses hash maps heavily (often [`FxHashMap`](https://github.com/rust-lang/rustc-hash) for speed rather than the standard library's [`RandomState`](https://doc.rust-lang.org/std/hash/struct.RandomState.html)-backed `HashMap`). [Hash-map iteration order is not specified](https://doc.rust-lang.org/std/collections/struct.HashMap.html#method.iter) and depends on hash/table layout and insertion history. If the order of insertions varies (because upstream work completed in a different order, or because of platform or compiler version differences), then iterating the map and interning items in iteration order produces different interned indices. 2. **Parallel query evaluation**: Rustc uses rayon for parallel compilation. Query evaluation, monomorphization collection, and codegen unit partitioning can all run work items concurrently. The order in which parallel tasks complete depends on OS thread scheduling, which can affect insertion order into those hash maps. So if type A gets interned before type B in one run because its query finished first, their indices may swap in the next run. Either source alone is sufficient to produce different indices across runs. The output vectors in `collect_smir()` were sorted like this: ```rust allocs.sort_by(|a, b| a.alloc_id.to_index().cmp(&b.alloc_id.to_index())); functions.sort_by(|a, b| a.0 .0.to_index().cmp(&b.0 .0.to_index())); types.sort_by(|a, b| a.0.to_index().cmp(&b.0.to_index())); spans.sort(); ``` These `.to_index()` calls return those interned IDs. The original comment said "stabilise output (a bit)", which is honest about the fact that this was only partially working. And `uneval_consts` (coming from `HashMap::into_iter()`) wasn't sorted at all. The integration test harness worked around all of this with a jq normalization filter (`normalise-filter.jq`) that strips unstable IDs and re-sorts by content before diffing. But the raw JSON output itself was not reproducible. ## What changed All changes are in `src/printer.rs`, in the sorting section of `collect_smir()`. Here's the new sort strategy for each vector: | Vector | Old sort key | New sort key | Tiebreaker | |--------|-------------|--------------|------------| | `functions` | `Ty.to_index()` | `Ty` display string (via `ty_pretty`) | interned index | | `types` | `Ty.to_index()` | `Ty` display string (via `ty_pretty`) | interned index | | `allocs` | `AllocId.to_index()` | content-derived string (see below) | none needed | | `spans` | span index (opaque) | location tuple: `(filename, lo_line, lo_col, hi_line, hi_col)` | none needed | | `uneval_consts` | unsorted | item name string | none needed | | `items` | unchanged | already uses a content-based `Ord` impl | N/A | **Allocation sort keys** deserve some detail. The new `alloc_sort_key()` helper produces a content-derived string from each `AllocInfo` by matching on the `GlobalAlloc` variant: | Variant | Sort key format | Example | |---------|----------------|---------| | `Memory` | `0_Memory_` + zero-padded byte length | `"0_Memory_00000000000000000032"` | | `Static` | `1_Static_` + def name | `"1_Static_MY_STATIC"` | | `VTable` | `2_VTable_` + type string | `"2_VTable_dyn Trait"` | | `Function` | `3_Function_` + instance name | `"3_Function_foo::bar"` | The numeric prefix groups entries by variant kind. Byte length is zero-padded to 20 digits so that `32` sorts before `128` lexicographically. (Span locations are `usize` values that compare numerically, so no padding is needed there.) Three golden test files were regenerated because their normalized output changed due to the new ordering. The jq normalization filter doesn't fully sort `TupleType` and `FunType` entries, so those were sensitive to input order. ## Remaining non-determinism: `adt_def` We looked into stabilizing the `adt_def` field on `EnumType`/`StructType`/`UnionType` as well; it's another interned index that the jq filter strips for test normalization. Turns out it can't be dropped or replaced. The reason: downstream consumers need `adt_def` as a cross-reference key to match `AggregateKind::Adt(adt_def, ...)` in MIR bodies with the corresponding type metadata entry. `AggregateKind` serialization comes from stable_mir (we don't control it), so both sides of the join have to use the same key format. The index is consistent within a single JSON file; it's just not stable across runs. So `adt_def` remains the one known source of cross-run non-determinism in the output, and the jq filter still needs to strip it for golden test comparison. A comment was added on the field explaining this constraint. If we ever get the ability to customize `AggregateKind` serialization upstream, we could replace these indices with names, but that's a stable_mir change, not something we can do on our end. (See PR #64 for the full discussion.) ## On the `Ty` display string tiebreaker `Ty`'s display impl goes through `ty_pretty()`, which calls `to_string()` on rustc's internal `Ty`. For monomorphized types (all generics resolved, no lifetime parameters), this should be injective: distinct types produce distinct strings. But there's a theoretical concern: could two distinct `Ty` values display identically? Perhaps some obscure lifetime or where-clause difference that gets elided in the display output. We weren't confident enough to rule this out entirely, so the interned index is used as a tiebreaker: content-based ordering for the common case, with the index preserving a consistent (within-run) ordering for any hypothetical ties. If a tie does occur, the ordering at the tie point would be non-deterministic across runs, but we haven't observed this in practice. ## What this means in practice If you run stable-mir-json twice on the same input file and diff the raw JSON (no jq filter), the only differences will be the interned index *values* themselves (`adt_def`, `alloc_id`, `Ty` keys, etc.). The structural ordering of every output vector is now identical across runs. Before this change, even the order of types, functions, allocs, and spans could shuffle around. The jq normalization filter is still needed for golden tests (to strip those interned IDs), but the sort operations in it are now redundant; they just confirm what the source already guarantees. ## Performance The `format!("{}", ty)` calls for sorting `functions` and `types` allocate strings. This is fine; the vectors are small (one entry per unique type or function in the program), and this code runs once at the end after all collection is done. ## Test plan - [x] `cargo build` compiles cleanly - [x] `make integration-test` passes (all 28 tests) - [x] Ran stable-mir-json twice on `assert_eq.rs` and diffed the raw JSON: identical modulo interned index values (`adt_def` only); all vector ordering matched exactly
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
2 participants
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.
These
adt_defofstructandenummetadata are required for a reverse mappingadt_def -> Tyin the semantics.The
AggregateKindonly carries theadt_defbut we have to look up the discriminant information (and potentially other metadata later) from aTykey.Note that the
adt_defis unstable, it changes with every compiler run, so we delete the field in the golden tests.