Open
Conversation
Split the monolithic src/printer.rs (1700+ lines) into focused submodules: - mod.rs: entry points (collect_smir, emit_smir) - schema.rs: serialization types (SmirJson, Item, AllocMap, etc.) - collect.rs: collect_items, collect_and_analyze_items, assemble_smir - items.rs: mk_item, get_item_details, MonoItemKind construction - mir_visitor.rs: BodyAnalyzer (MirVisitor impl), collect_alloc, get_prov_ty - ty_visitor.rs: TyCollector (Visitor impl) - link_map.rs: LinkMap types, update_link_map, fn_inst_sym - types.rs: type helpers (fn_inst_for_ty, has_unresolved_ty, etc.) - util.rs: debug_enabled, debug_log_println, take_any Pure mechanical split; no behavioral changes.
Document the purpose and responsibilities of each submodule in the printer directory: mod.rs (overview table), collect.rs (three-phase pipeline), items.rs (Item construction), link_map.rs (function resolution), mir_visitor.rs (body traversal), schema.rs (data model), ty_visitor.rs (type collection), types.rs (TypeMetadata), and util.rs (helpers).
d0c859f to
0cdfeb6
Compare
Replace if-let/else with unwrap_or_else, is_none/unwrap with let-else, remove unnecessary .clone() calls, pass offset by value in collect_alloc, extract opaque_placeholder_ty helper, and add doc comments to public types and key internal structures in schema.rs.
There was a problem hiding this comment.
Pull request overview
This PR refactors the Stable MIR JSON “printer” by splitting the former monolithic src/printer.rs into a src/printer/ module directory with focused submodules, keeping the public API via re-exports and adding module-level documentation.
Changes:
- Split
src/printer.rsintosrc/printer/{mod.rs, schema.rs, mir_visitor.rs, collect.rs, items.rs, types.rs, ty_visitor.rs, link_map.rs, util.rs}. - Added module-level doc comments across the new submodules.
- Small idiomatic cleanups during extraction (option handling, helper extraction, signature tweaks like
offset: usize).
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/printer/mod.rs | New module entrypoint: env-var helpers/macros, submodule wiring, public re-exports, emit_smir. |
| src/printer/schema.rs | Extracted data model/types for *.smir.json plus serialization helpers and debug-only alloc coherence verification. |
| src/printer/collect.rs | Extracted 3-phase collection pipeline and final JSON assembly/sorting. |
| src/printer/items.rs | Extracted Item/MonoItemKind construction and optional debug details/foreign module enumeration. |
| src/printer/mir_visitor.rs | Extracted MIR traversal (BodyAnalyzer) and allocation/type/span/link-map collection logic. |
| src/printer/ty_visitor.rs | Extracted recursive type visitor for reachable types + layout capture. |
| src/printer/types.rs | Extracted TypeMetadata construction from TyKind + layout. |
| src/printer/link_map.rs | Extracted link-time function resolution map update + symbol classification logic. |
| src/printer/util.rs | Extracted small helpers (hashing, map queueing, name resolution, attribute queries). |
| src/printer.rs | Removed old monolithic implementation after split. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Use eprintln! instead of println! on error paths to avoid corrupting
JSON output when emit_smir targets stdout
- Fix uninterpolated {path} in expect message; include actual path and
IO error via unwrap_or_else + panic!
- Remove redundant .as_str() in LinkMapKey serialization
- Replace alloc+sort with single linear scan in field_containing_offset
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Several module-level doc comments overclaimed or described functionality that doesn't match the implementation: - collect.rs: phase boundary was described as "enforced by types" but CollectedCrate contains Item which retains a MonoItem field; reworded to "enforced by interface and convention" with an explicit caveat - schema.rs: SmirJson doc said all fields are sorted, but uneval_consts and debug fields are not; narrowed to name the specific sorted fields - types.rs: doc claimed the module provides "resolving function instances from types, checking for unresolved generics" but those live in util.rs; corrected to describe only mk_type_metadata - ty_visitor.rs: doc said every encountered type is recorded, but closures, FnDef/FnPtr, and coroutine witnesses are traversed without being stored in the type map; added a note about these exceptions - mir_visitor.rs: summary line still said "collecting interned values", a holdover from the deleted InternedValueCollector; updated to match the actual collection targets - mod.rs: updated the module table to match corrected submodule docs
The three-phase collection pipeline has an invariant: phase 3 (assemble_smir) must not call inst.body() or otherwise re-enter rustc. Previously this was enforced by convention only, because Item carried a pub(super) mono_item: MonoItem field that gave any code with an &Item full access to Instance::body(). The doc comment said "enforced by interface and convention," which is a polite way of saying "enforced by hoping nobody makes a mistake." Turns out we can do better: remove mono_item from Item entirely, so the invariant is structural. Phase 3 code literally cannot call inst.body() because it never has a MonoItem to call it on. The type system enforces what discipline previously had to. Concretely: mk_item now returns (MonoItem, Item) instead of Item, and the phase 1+2 maps carry (MonoItem, Item) tuples. The MonoItem lives alongside the Item during collection and analysis, gets passed directly to maybe_add_to_link_map and warn_missing_body (now a free function in collect.rs), and is naturally dropped when only the Item is pushed into all_items. By the time assemble_smir runs, no MonoItem values exist. As a bonus, this fixes a latent PartialEq/Ord inconsistency: PartialEq was comparing mono_item while Ord compared symbol_name + item-kind name. PartialEq now delegates to Ord, so the two are consistent.
The previous commit removed MonoItem from Item, but several doc comments still described the old world: hedging about "convention," speaking of Item where we now have (MonoItem, Item) pairs, and so on. This brings all the prose in line with the actual types. The key point worth calling out: CollectedCrate's doc comment previously said "no more inst.body() calls should be needed," which is the kind of phrasing that quietly hopes nobody violates the rule. Now that Item doesn't carry a MonoItem, the comment can say "structurally cannot" instead of "should not," because the type system makes the wrong thing unrepresentable rather than merely discouraged.
…ntimeverification#124, runtimeverification#126, runtimeverification#127 Several merged PRs were missing from the changelog or lacked PR links: - runtimeverification#127: mutability field on PtrType/RefType in TypeMetadata - runtimeverification#124: ADR-001 (index-first graph architecture) - runtimeverification#121: existing entries for 3-phase pipeline, AllocMap coherence, and dead fixup removal now link to the PR - runtimeverification#126: existing entries for UI test runner rewrite and provenance resolution fixes now link to the PR
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.
This PR splits
src/printer.rs(~1700 lines) into asrc/printer/directory module with focused submodules, then follows up with doc comments, idiomatic Rust cleanup, and a structural improvement to the phase boundary enforcement.All integration and UI tests pass unchanged.
Structure
The split creates nine submodules, each with a single responsibility:
schema.rsSmirJson,Item,AllocInfo,TypeMetadata,LinkMapKey, etc.)mir_visitor.rsBodyAnalyzer: single-pass MIR body traversal collecting calls, allocs, types, spanscollect.rsitems.rsItemconstruction fromMonoItemwith optional debug-level detailstypes.rsTypeMetadataconstruction fromTyKind+ layoutty_visitor.rslink_map.rsmod.rsemit_smirutil.rsChanges by commit
Split printer.rs into a directory module: mechanical extraction; no code changes beyond moving
use/extern cratedeclarations into each submodule and adjusting visibility.Add module-level doc comments to all printer submodules:
//!doc headers describing each module's purpose and key types.Idiomatic Rust cleanup across printer submodules:
if let Some(x) = y { x } else { z }toy.unwrap_or_else(|| z)if opt.is_none() { return; } let val = opt.unwrap()tolet Some(val) = opt else { return; }.clone()calls onkeyinget_mut/insertcollect_allocto takeoffset: usizeby value (was&usize)opaque_placeholder_ty()helper for theTy::to_val(0)sentinelFnSymType,LinkMapKey,AllocInfo,TypeMetadata,SourceData,SmirJson,SmirJsonDebugInfoAddress review feedback and fix doc comment inaccuracies.
Remove
MonoItemfromItemfor structural phase enforcement:The three-phase collection pipeline has an invariant: phase 3 (
assemble_smir) must not callinst.body()or otherwise re-enter rustc. Previously this was enforced by convention only, becauseItemcarried apub(super) mono_item: MonoItemfield that gave any code with an&Itemfull access toInstance::body().This commit removes
mono_itemfromItementirely, so the invariant is structural: phase 3 code literally cannot callinst.body()because it never has aMonoItemto call it on. The type system enforces what discipline previously had to.Concretely:
mk_itemnow returns(MonoItem, Item)instead ofItem, and the phase 1+2 maps carry(MonoItem, Item)tuples. TheMonoItemlives alongside theItemduring collection and analysis, gets passed directly tomaybe_add_to_link_mapandwarn_missing_body(now a free function incollect.rs), and is naturally dropped when only theItemis pushed intoall_items. By the timeassemble_smirruns, noMonoItemvalues exist.As a bonus, this fixes a latent
PartialEq/Ordinconsistency:PartialEqwas comparingmono_itemwhileOrdcomparedsymbol_name+ item-kind name.PartialEqnow delegates toOrd, so the two are consistent.Test plan
cargo buildsucceedscargo clippycleanmake integration-testpasses (28/28)make test-uipasses (2875/2875, 0 failures)