Move Const from rustc_middle to rustc_type_ir

[Jamesbarford]

View all comments

Split by commit;

• Firstly move the type and methods
• From I::Const -> Const<I>
• Import ConstExt in all places that require the extension trait methods in compiler
• Import ConstExt in all places that require the extension trait methods in clippy

r? @lcnr

[rustbot]

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in compiler/rustc_sanitizers

cc @rcvalle

Some changes occurred in match lowering

cc @Nadrieril

Some changes occurred in match checking

cc @Nadrieril

clippy is developed in its own repository. If possible, consider making this change to rust-lang/rust-clippy instead.

cc @rust-lang/clippy

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

Some changes occurred in rustc_ty_utils::consts.rs

cc @BoxyUwU

Some changes occurred in exhaustiveness checking

cc @Nadrieril

changes to the core type system

cc @lcnr

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

HIR ty lowering was modified

cc @fmease

[lcnr]

why do we need manual impls instead of the macro here again?

View changes since the review

[Jamesbarford]

nop_lift does;

 assert!(tcx.interners.$set.contains_pointer_to(&InternedInSet(&*self.0.0)));

Whereas we need;

assert!(tcx.interners.const_.contains_pointer_to(&InternedInSet(&*self.0)));

[lcnr]

hmm, why does moving Const change this access pattern from .0.0 to just .0 🤔 that's not immediately obvious to me.

Please add that as a comment if it can't be avoided

[Jamesbarford]

I'm not sure why;

nop_lift! { const_; Const<'a> => Const<'tcx> }

Given the following definitions

// rustc_type_ir/src/sty/consts.rs
pub struct Const<I: Interner>(pub I::InternedConstKind);

// rustc_middle/rustc_middle/src/ty/context/impl_interner.rs
type InternedConstKind = Interned<'tcx, WithCachedTypeInfo<ty::ConstKind<'tcx>>>;

// rustc_middle/src/ty/consts.rs
pub type Const<'tcx> = ir::Const<TyCtxt<'tcx>>;

Walking through how I think the above would work, which could be wrong, I'd have thought the following code snippet would be true;

pub type Const<'tcx> = struct Const<TyCtxt<'tcx>>(pub TyCtxt<'tcx>::InternedConst);

// which in turn becomes
pub type Const<'tcx> = struct Const<TyCtxt<'tcx>>(pub  Interned<'tcx, WithCachedTypeInfo<ty::ConstKind<'tcx>>>);

Which is the same as what we have before all be it the definition is composed from different modules and associated types. The rust-analyser LSP I have setup agrees with with me that my intuition is correct.

However I get a bunch of cascading errors. Of which this one seems the most likely culprit. So I did what the compiler error told me to do; implement Lift for WithCachedTypeInfo<...>.

error[E0277]: the trait bound `Interned<'tcx, _>: Lift<TyCtxt<'tcx>>` is not satisfied
    --> compiler/rustc_middle/src/ty/context.rs:1894:54
     |
1894 | struct InternedInSet<'tcx, T: ?Sized + PointeeSized>(&'tcx T);
     |                                                      ^^^^^^^ unsatisfied trait bound
     |
help: the trait `Lift<TyCtxt<'tcx>>` is not implemented for `Interned<'tcx, rustc_type_ir::WithCachedTypeInfo<rustc_type_ir::ConstKind<context::TyCtxt<'tcx>>>>`
      but trait `Lift<TyCtxt<'_>>` is implemented for `Interned<'_, rustc_type_ir::RegionKind<context::TyCtxt<'_>>>`
    --> compiler/rustc_middle/src/ty/context.rs:1721:1

[Jamesbarford]

3b8480e adds the comment;

// `rustc_type_ir::Const<I>` is only the generic wrapper; lifting it delegates
// to `I::InternedConstKind`, so the concrete interned const representation
// must itself implement `Lift`.

It's interesting that when I expanded the macro rust-analyser was able to pick up self.0.0. This confused me probably more than it should have done.

[lcnr]

hmm, confusing. Can you change this to a "FIXME: unclear why exactly the macro doesn't work"?

[lcnr]

instead mark with type_visitable(ignored) 🤔

View changes since the review

[Jamesbarford]

I was able to completely get rid of it

[lcnr]

why does this bound then exist

[Jamesbarford]

I think I was wrong, I've detailed it here; #162628 (comment), but will also comment in the code

[lcnr]

nits, otherwise this is looking good

View changes since this review

[rustbot]

rustc_codegen_cranelift is developed in its own repository. If possible, consider making this change to rust-lang/rustc_codegen_cranelift instead.

cc @bjorn3

rustc_codegen_gcc is developed in its own repository. If possible, consider making this change to rust-lang/rustc_codegen_gcc instead.

cc @antoyo, @GuillaumeGomez

[lcnr]

alright, r=me after rebase + final nit then

View changes since this review

[lcnr]

final nits, then r=me

View changes since this review

[lcnr]

@bors r+ rollup=iffy

[rust-bors]

📌 Commit 0a7563b has been approved by lcnr

It is now in the queue for this repository.

[lcnr]

@bors delegate+

[rust-bors]

✌️ @Jamesbarford, you can now approve this pull request!

If @lcnr told you to "r=me" after making some further change, then please make that change and post @bors r=lcnr.

View changes since this delegation.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[Jamesbarford]

@bors r=lcnr

[rust-bors]

📌 Commit 5e56705 has been approved by lcnr

It is now in the queue for this repository.