Refactor StableMIR #140643

makai410 · 2025-05-04T18:14:23Z

This PR refactors stable-mir according to the guidance in this doc. It reverses the dependency between rustc_smir and stable_mir, making rustc_smir completely agnostic of stable_mir.

Under the new architecture, the rustc_smir crate would retain direct access to rustc queries, while stable_mir should proxy all such requests through rustc_smir instead of accessing rustc's internals directly. stable_mir would only be responsible for the conversion between internal and stable constructs.

This PR mainly introduces these changes:

Bridge / Tables<'tcx, B: Bridge>

/// A trait defining types that are used to emulate StableMIR components, which is really
/// useful when programming in stable_mir-agnostic settings.
pub trait Bridge {
    type DefId: Copy + Debug + PartialEq + IndexedVal;
    type AllocId: Copy + Debug + PartialEq + IndexedVal;
    type Span: Copy + Debug + PartialEq + IndexedVal;
    type Ty: Copy + Debug + PartialEq + IndexedVal;
    type InstanceDef: Copy + Debug + PartialEq + IndexedVal;
    type TyConstId: Copy + Debug + PartialEq + IndexedVal;
    type MirConstId: Copy + Debug + PartialEq + IndexedVal;
    type Layout: Copy + Debug + PartialEq + IndexedVal;
    type Error: SmirError;
}

pub struct Tables<'tcx, B: Bridge> {
    tcx: TyCtxt<'tcx>,
    pub(crate) def_ids: IndexMap<DefId, B::DefId>,
    pub(crate) alloc_ids: IndexMap<AllocId, B::AllocId>,
    pub(crate) spans: IndexMap<rustc_span::Span, B::Span>,
    pub(crate) types: IndexMap<Ty<'tcx>, B::Ty>,
    pub(crate) instances: IndexMap<ty::Instance<'tcx>, B::InstanceDef>,
    pub(crate) ty_consts: IndexMap<ty::Const<'tcx>, B::TyConstId>,
    pub(crate) mir_consts: IndexMap<mir::Const<'tcx>, B::MirConstId>,
    pub(crate) layouts: IndexMap<rustc_abi::Layout<'tcx>, B::Layout>,
}

Since rustc_smir needs these stable types somewhere, using associated types is a good approach.

SmirContainer / SmirInterface

/// A container which is used for TLS.
pub struct SmirContainer<'tcx, B: Bridge> {
    pub tables: RefCell<Tables<'tcx, B>>,
    pub cx: RefCell<SmirCtxt<'tcx, B>>,
}

impl<'tcx> SmirInterface for SmirContainer<'tcx, BridgeTys> {
    // ...
}

/// Provides direct access to rustc's internal queries.
///
/// The [`crate::stable_mir::compiler_interface::SmirInterface`] must go through
/// this context to obtain rustc-level information.
pub struct SmirCtxt<'tcx, B: Bridge> {
    tcx: TyCtxt<'tcx>,
    _marker: PhantomData<B>,
}

This PR moves Tables from SmirCtxt to a new SmirContainer struct, since mutable borrows of tables should only be managed by SmirInterface. This change prevents SmirCtxt from holding separate borrows and requires passing tables explicitly when needed:

impl<'tcx, B: Bridge> SmirCtxt<'tcx, B> {
    // ...
    /// Get the body of an Instance which is already monomorphized.
    pub fn instance_body(
        &self,
        instance: ty::Instance<'tcx>,
        tables: &mut Tables<'tcx, B>,
    ) -> Option<Body<'tcx>> {
        tables
            .instance_has_body(instance)
            .then(|| BodyBuilder::new(self.tcx, instance).build(tables))
    }
    // ...
}

This PR introduces SmirContainer as a separate struct rather than bundling it into a SmirInterface struct. This separation makes the architecture more modular and easier to reason about.

context/traits.rs

We use this file to define traits that are used for encapsulating the associated functions in the rustc's internals. This is much easier to use and maintain than directly cramming everything into SmirCtxt. Here is a real-world use case:

impl RustcInternal for ExistentialTraitRef {
    type T<'tcx> = rustc_ty::ExistentialTraitRef<'tcx>;

    fn internal<'tcx>(
        &self,
        tables: &mut Tables<'_, BridgeTys>,
        cx: &SmirCtxt<'tcx, BridgeTys>,
    ) -> Self::T<'tcx> {
        use rustc_smir::context::SmirExistentialTraitRef;
        cx.new_from_args(self.def_id.0.internal(tables, cx), self.generic_args.internal(tables, cx))
    }
}

Separation of rustc_smir::alloc

The previous rustc_smir::alloc had many direct calls to rustc queries. This PR splits it into two parts: rustc_smir::alloc and stable_mir::alloc. Following the same pattern as SmirCtxt and SmirInterface, the rustc_smir::alloc handles all direct interactions with rustc queries and performs the actual memory allocations, while the stable_mir::alloc is responsible for constructing stable components.

Removal of convert/error.rs

We use SmirError::from_internal instead, since implementing Stable for these internal errors would be redundant—tables is not actually used. If we later need to add something like LayoutError to stable_mir, we could implement it as follows:

impl SmirError for stable_mir::LayoutError {
    fn from_internal<T: Debug>(err: T) -> Self {
        // ...
    }
}

Unresolved questions:

There are still a few direct calls to rustc's internals scattered across impl Stables, but most of them appear to be relatively stable, e.g., mir::interpret::ConstAllocation::inner(self) and mir::syntax::SwitchTargets::otherwise(self).

r? @celinval

compiler/rustc_smir/src/rustc_smir/mod.rs

compiler/rustc_smir/src/rustc_smir/context/mod.rs

compiler/rustc_smir/src/rustc_smir/mod.rs

celinval · 2025-05-07T00:54:19Z

Sweet! I'll start the review tomorrow. Thanks

compiler/rustc_smir/src/stable_mir/compiler_interface.rs

celinval

I really like the new name schema. One high level comment, I would prefer keeping StableMIR definitions and logic separate from any sort of conversion and usage of internal rustc code. We could restrict the usage of internal items to be inside specific modules, maybe rustc_internal and convert modules. What do you think?

compiler/rustc_smir/src/stable_mir/mod.rs

celinval · 2025-05-12T19:02:57Z

compiler/rustc_smir/src/stable_mir/mod.rs

+use rustc_hir::def::DefKind;
+use rustc_smir::IndexedVal;


We could create a stable version of IndexedVal as well to avoid spreading this everywhere. We could just add an internal trait impl for all types that implement the stable one.

I wonder if we can just add a

pub(crate) use rustc_smir::IndexedVal;

🤔

Yeah, that would do it for now

makai410 · 2025-05-13T11:49:15Z

I really like the new name schema. One high level comment, I would prefer keeping StableMIR definitions and logic separate from any sort of conversion and usage of internal rustc code. We could restrict the usage of internal items to be inside specific modules, maybe rustc_internal and convert modules. What do you think?

Ahh I agree. The only problem is stable_mir/alloc.rs, which uses some internal items, but I think we can just move all the logic to the rustc_smir side^^

celinval

Some more comments. Let me know when you get to address them! Thanks

@rustbot author

compiler/rustc_smir/src/rustc_smir/bridge.rs

compiler/rustc_smir/src/stable_mir/convert/mod.rs

celinval · 2025-05-21T22:26:51Z

compiler/rustc_smir/src/rustc_internal/mod.rs

@@ -54,139 +43,11 @@ pub fn stable<'tcx, S: Stable<'tcx>>(item: S) -> S::T {
 /// # Panics
 ///
 /// This function will panic if StableMIR has not been properly initialized.
-pub fn internal<'tcx, S>(tcx: TyCtxt<'tcx>, item: S) -> S::T<'tcx>
+pub fn internal<'tcx, S>(item: S) -> S::T<'tcx>


Actually, this isn't safe. This allows users to create objects with any lifetime 'tcx, including 'static. See this commit and this PR for more context.

compiler/rustc_smir/src/stable_mir/compiler_interface.rs

compiler/rustc_smir/src/stable_mir/convert/mod.rs

rustbot · 2025-05-21T22:48:10Z

Reminder, once the PR becomes ready for a review, use @rustbot ready.

makai410 · 2025-05-22T03:43:30Z

One high level comment, I would prefer keeping StableMIR definitions and logic separate from any sort of conversion and usage of internal rustc code. We could restrict the usage of internal items to be inside specific modules, maybe rustc_internal and convert modules.

iiuc, we shouldn't use any internal items even like let did = tables[def_id]? Did that say, we should extract the conversion logic from compiler_interface.rs, then move it to somewhere like the convert module?

Or we just don't want to see something like use rustc_hir::def::DefKind; in compiler_interface.rs?

celinval · 2025-05-22T18:44:13Z

I can live with tables[def_id] since that is part of rustc_smir public interface. I think we should minimize manipulation of internal items inside stable_mir, so it won't get inadvertently broken.

I think we should avoid calling internal methods in stable_mir, e.g.: Const::new(), and restrict where we inspect the internals of a type.

bors · 2025-05-27T00:30:11Z

☔ The latest upstream changes (presumably #141605) made this pull request unmergeable. Please resolve the merge conflicts.

makai410

What if we wrap TyCtxt<'tcx> in a new SmirCtxt? I'm not sure if this approach is correct or not.

compiler/rustc_smir/src/rustc_internal/mod.rs

makai410 · 2025-05-27T13:35:12Z

compiler/rustc_smir/src/rustc_internal/mod.rs

-    pub(crate) fn layout_id(&mut self, layout: rustc_abi::Layout<'tcx>) -> Layout {
-        self.layouts.create_or_fetch(layout)
-    }
+    with_container(|tables, cx| item.internal(tables, cx))


Suggested change

with_container(|tables, cx| item.internal(tables, cx))

with_container(Some(tcx), |tables, cx| item.internal(tables, cx))

The idea is for internal calls, we want the tcx that the user provided.

Suggested change

with_container(|tables, cx| item.internal(tables, cx))

with_container(|tables, _| item.internal(tables, SmirCtxt::new(tcx)))

If we implement the TyLift trait, this would become:

Suggested change

with_container(|tables, cx| item.internal(tables, cx))

with_container(|tables, _| item.internal(tables, tcx))

My concern is that tcx.lift() is not the only method being used in impl RustcInternals. There are other calls to rustc queries e.g. tcx.mk_args_from_iter(), tcx.mk_pat(), tcx.mk_poly_existential_predicates(), which are not defined in the ty::Lift trait but live in inherent impl of TyCtxt. I was wondering should we allow these calls? If not, then the approach of passing impl TyLift would fail.

IIUC, the unsoundness comes from the lifetime. To solve this issue requires passing a TyCtxt<'tcx> to rustc_internal::internal(). Wrapping the provided TyCtxt<'tcx> in a new SmirCtxt would keep the lifetime, which would also make RustcInternal insulated from direct calls to rustc queries.

By TyLift, I mean that's a private trait that we define inside stable_mir::internal that allow us to control which methods are OK to call from internal methods. mk_args_from_iter(), mk_pat() and other mk_* functions like lift are about ensuring things are in the arena for that TyCtxt, so we should include those. Maybe TyLift is not the best name. We could also name it after Arena

Ah then it makes sense. In that case TyLift would be only for RustcInternal, any other attempts to rustc queries should be proxied to SmirCtxt.

What if we call this trait InternalCx?

- rewrite all `SmirInterface` apis. - add `BridgeTys` to impl those associated types in `Bridge`. - move `**_def()` stuffs living in `impl Tables` from `rustc_internal` to `stable_mir`.

The previous `rustc_smir::alloc` had many direct calls to rustc queries. This commit splits it into two parts: `rustc_smir::alloc` and `stable_mir::alloc`. Following the same pattern as `SmirCtxt` and `SmirInterface`, the `rustc_smir::alloc` handles all direct interactions with rustc queries and performs the actual memory allocations, while the `stable_mir::alloc` is responsible for constructing stable components.

This commit removes the `Tables` field from `SmirCtxt`, since borrows of `tables` should only be managed by `SmirInterface`. This change prevents `SmirCtxt` from holding separate borrows and requires passing `tables` explicitly when needed. We use the `traits.rs` file to define traits that are used for encapsulating the associated functions in the rustc's internals. This is much easier to use and maintain than directly cramming everything into `SmirCtxt`.

note that this commit delete `convert/error.rs`, we would use `SmirError::from_internal` instead. **Unresolved questions:** - There are still a few direct calls to rustc's internals scattered across `impl Stable`s, but most of them appear to be relatively stable, e.g., `mir::interpret::ConstAllocation::inner(self)` and `mir::syntax::SwitchTargets::otherwise(self)`.

the only functionality of `Tables` is caching results. this commit moves calls to rustc queries from `Tables` to `SmirCtxt`.

define bridge types for `***Def`s. consolidate scattered `Tables` implementations into single inherent impl.

we should no longer keep `IndexMap` in `rustc_internal`, as we've decided to migrate `rustc_internal` to `stable_mir` under a feature (rust-lang#140532).

add a new trait `InternalCx`, which defines the methods that are fine to call from `RustcInternal`. `RustcInternal::internal()` then takes a `impl InternalCx<'tcx>` instead of `TyCtxt<'tcx>`. make `tcx` in `SmirCtxt` public, since we need to pass it to `RustcInternal::internal()` in `SmirInterface`.

We want to keep StableMIR definitions and logic separate from any sort of conversion and usage of internal rustc code. So we bundle all unstable items that have no stability guarantees into `stable_mir::unstable`.

…explicit_predicates_of()`

rustbot · 2025-06-09T08:29:48Z

⚠️ Warning ⚠️

There are issue links (such as #123) in the commit messages of the following commits.
Please move them to the PR description, to avoid spamming the issues with references to the commit, and so this bot can automatically canonicalize them to avoid issues with subtree.
- 109e74a

makai410 · 2025-06-09T08:36:17Z

compiler/rustc_smir/src/rustc_smir/context/impls.rs

-    pub fn predicates_of(&self, def_id: DefId) -> GenericPredicates<'tcx> {
-        self.tcx.predicates_of(def_id)
+    pub fn predicates_of(
+        &self,
+        def_id: DefId,
+    ) -> (Option<DefId>, Vec<(ty::PredicateKind<'tcx>, Span)>) {
+        let ty::GenericPredicates { parent, predicates } = self.tcx.predicates_of(def_id);
+        (
+            parent,
+            predicates
+                .iter()
+                .map(|(clause, span)| (clause.as_predicate().kind().skip_binder(), *span))
+                .collect(),
+        )


Pretty unsure if this is a good approach but I can't figure out a perfect way🤷‍♂️

makai410 · 2025-06-09T08:44:13Z

ummm pretty sure that I had something to say earlier but can’t quite recall it atm😅, so please let me know if you get any questions when reviewing, thanks!
@rustbot ready

bors · 2025-06-11T12:28:42Z

☔ The latest upstream changes (presumably #141942) made this pull request unmergeable. Please resolve the merge conflicts.

rustbot assigned celinval May 4, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 4, 2025

makai410 changed the title ~~Smir refactor migrate~~ StableMIR Refactor May 4, 2025

makai410 changed the title ~~StableMIR Refactor~~ Refactor StableMIR May 4, 2025

This comment has been minimized.

Sign in to view

makai410 force-pushed the smir-refactor-migrate branch from 28f5ec1 to a169e52 Compare May 5, 2025 08:00

makai410 commented May 5, 2025

View reviewed changes

compiler/rustc_smir/src/rustc_smir/mod.rs Outdated Show resolved Hide resolved

makai410 commented May 5, 2025

View reviewed changes

compiler/rustc_smir/src/rustc_smir/context/mod.rs Outdated Show resolved Hide resolved

makai410 commented May 5, 2025

View reviewed changes

compiler/rustc_smir/src/rustc_smir/mod.rs Outdated Show resolved Hide resolved

makai410 commented May 8, 2025

View reviewed changes

compiler/rustc_smir/src/stable_mir/compiler_interface.rs Outdated Show resolved Hide resolved

celinval reviewed May 12, 2025

View reviewed changes

celinval reviewed May 21, 2025

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 21, 2025

makai410 commented May 27, 2025

View reviewed changes

makai410 added 8 commits June 7, 2025 15:57

refactor: add Tables<'tcx, B: Bridge> and SmirContainer

0575cd0

refactor: make SmirInterface a trait and impl it for SmirContainer

0e7a4a5

- rewrite all `SmirInterface` apis. - add `BridgeTys` to impl those associated types in `Bridge`. - move `**_def()` stuffs living in `impl Tables` from `rustc_internal` to `stable_mir`.

refactor: move IndexedVal from stable_mir to rustc_smir

be05aa4

refactor: impl SmirError for stable_mir::Error

2d4e300

fix: rewrite some parts of rustc_smir::builder.rs

e769aef

makai410 added 11 commits June 7, 2025 16:06

test: fix the usage of rustc_internal::internal

1862f20

cleanup: delete useless methods in SmirCtxt

6d81378

refactor: remove the tcx field in Tables

9e8b1bf

the only functionality of `Tables` is caching results. this commit moves calls to rustc queries from `Tables` to `SmirCtxt`.

refactor: unify Tables implementation with bridge types

3c4c145

define bridge types for `***Def`s. consolidate scattered `Tables` implementations into single inherent impl.

refactor: move IndexMap to rustc_smir

109e74a

we should no longer keep `IndexMap` in `rustc_internal`, as we've decided to migrate `rustc_internal` to `stable_mir` under a feature (rust-lang#140532).

refactor: move convert to unstable

1eed3f8

We want to keep StableMIR definitions and logic separate from any sort of conversion and usage of internal rustc code. So we bundle all unstable items that have no stability guarantees into `stable_mir::unstable`.

cleanup: re-export IndexedVal and fix some nits

aa6b7df

test: fix the usage of rustc_internal::internal()

09829e1

refactor: avoid calling internal functions in predicates_of() and `…

e2992c7

…explicit_predicates_of()`

cleanup: fmt

37370d5

rust-cloud-vms bot force-pushed the smir-refactor-migrate branch from c8f00a1 to 37370d5 Compare June 9, 2025 08:29

makai410 commented Jun 9, 2025

View reviewed changes

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 9, 2025

	with_container(\|tables, cx\| item.internal(tables, cx))
	with_container(Some(tcx), \|tables, cx\| item.internal(tables, cx))

	with_container(\|tables, cx\| item.internal(tables, cx))
	with_container(\|tables, _\| item.internal(tables, SmirCtxt::new(tcx)))

Refactor StableMIR #140643

Are you sure you want to change the base?

Refactor StableMIR #140643

Conversation

makai410 commented May 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

Uh oh!

celinval commented May 7, 2025

Uh oh!

Uh oh!

celinval left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

makai410 commented May 13, 2025

Uh oh!

celinval left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rustbot commented May 21, 2025

Uh oh!

makai410 commented May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

celinval commented May 22, 2025

Uh oh!

bors commented May 27, 2025

Uh oh!

makai410 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

makai410 Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rustbot commented Jun 9, 2025

Uh oh!

makai410 Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

makai410 commented Jun 9, 2025

Uh oh!

bors commented Jun 11, 2025

Uh oh!

Uh oh!

makai410 commented May 4, 2025 •

edited

Loading

makai410 commented May 22, 2025 •

edited

Loading

makai410 Jun 5, 2025 •

edited

Loading

makai410 Jun 9, 2025 •

edited

Loading