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]

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

[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]

So from one of the rebases I've done, I was getting a lot of failing UI tests (narrowed to one error I've pasted in below). Tracing the error to the changes, the only implementation changes were in the implementation of TypeVisitable for Const.

ErrorGuaranteed was no longer being visited and instead returned V::Result::output(). The same is true for ValueConst, although that doesn't seem to require visiting.

Putting the implementation back for ErrorGuaranteed has made the tests pass again (locally, 🤞 for the pipeline). I suspect this is because the flags and the structural visitor had become inconsistent: the flags still indicated that an error was present, but the visitor could no longer reach the corresponding ErrorGuaranteed.

thread 'rustc' (2161986) panicked at compiler/rustc_type_ir/src/visit.rs:402:17:
type flags said there was an error, but now there is not
stack backtrace:
   0: __rustc::rust_begin_unwind
             at /rustc/cbae9b4cae2b108f6a3d18cfe6075714bb739463/library/std/src/panicking.rs:679:5
   1: core::panicking::panic_fmt
             at /rustc/cbae9b4cae2b108f6a3d18cfe6075714bb739463/library/core/src/panicking.rs:80:14
   2: <T as rustc_type_ir::visit::TypeVisitableExt<I>>::error_reported
   3: rustc_infer::infer::InferCtxt::deeply_resolve_ignoring_regions
   4: rustc_hir_typeck::expr::<impl rustc_hir_typeck::fn_ctxt::FnCtxt>::check_expr_with_expectation_and_args
   5: rustc_hir_typeck::expr::<impl rustc_hir_typeck::fn_ctxt::FnCtxt>::check_expr_with_expectation
   6: rustc_hir_typeck::fn_ctxt::_impl::<impl rustc_hir_typeck::fn_ctxt::FnCtxt>::with_breakable_ctxt
   7: rustc_hir_typeck::fn_ctxt::checks::<impl rustc_hir_typeck::fn_ctxt::FnCtxt>::check_expr_block
   8: rustc_hir_typeck::expr::<impl rustc_hir_typeck::fn_ctxt::FnCtxt>::check_expr_with_expectation_and_args
   9: rustc_hir_typeck::expr::<impl rustc_hir_typeck::fn_ctxt::FnCtxt>::check_return_or_body_tail
  10: rustc_hir_typeck::check::check_fn
  11: rustc_hir_typeck::typeck_with_inspect
      [... omitted 2 frames ...]
  12: rustc_middle::query::calls::TyCtxtEnsureOk::typeck
  13: rustc_middle::hir::map::<impl rustc_middle::ty::context::TyCtxt>::par_hir_body_owners::{{closure}}
  14: rustc_data_structures::sync::parallel::par_for_each_in
  15: rustc_hir_analysis::check_crate
  16: rustc_interface::passes::analysis
      [... omitted 2 frames ...]
  17: std::thread::local::LocalKey<T>::with
  18: rustc_middle::ty::context::TyCtxt::create_global_ctxt
  19: rustc_interface::passes::create_and_enter_global_ctxt
  20: scoped_tls::ScopedKey<T>::set
  21: rustc_span::create_session_globals_then
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.