Move Region from rustc_middle to rustc_type_ir#154989
Move Region from rustc_middle to rustc_type_ir#154989Jamesbarford wants to merge 19 commits intorust-lang:mainfrom
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| // Use a pre-interned one when possible. | ||
| if let BoundRegion { var, kind: BoundRegionKind::Anon } = bound_region | ||
| // This | ||
| && let Some(inner) = interner.get_anon_re_bounds_lifetime(debruijn.as_usize()) |
There was a problem hiding this comment.
i feel like these interning opts should not be in rustc_type_ir, so maybe add Interner::intern_bound_region and call that directly?
| #[inline] | ||
| pub fn new_canonical_bound(interner: I, var: BoundVar) -> Self { | ||
| // Use a pre-interned one when possible. | ||
| if let Some(re) = interner.get_anon_re_canonical_bounds_lifetime(var.as_usize()) { |
There was a problem hiding this comment.
same here
There was a problem hiding this comment.
I don't fully get the changes to InternerDecoder
we should keep Region<I>: Display around by implementing it via IrPrint 🤔 having RegionDisplay seems unnecessary, we already have this IrPrint handling for other types, don't we?
IntoDiagArg is defined outside of rustc_middle? Maybe we should make it generic over I as well?
compiler/rustc_type_ir/src/binder.rs
Outdated
|
|
||
| fn lift_to_interner(self, interner: U) -> Option<Self::Lifted> { | ||
| Some(BoundRegion { var: self.var, kind: self.kind.lift_to_interner(interner)? }) | ||
| } |
|
|
||
| impl<I: Interner> Eq for RegionKind<I> {} | ||
|
|
||
| impl<I: Interner, U: Interner> Lift<U> for RegionKind<I> |
| // fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
| // write!(f, "{:?}", self.kind()) | ||
| // } | ||
| // } |
| rustc_hir::Safety, | ||
| rustc_middle::mir::ConstValue, | ||
| rustc_span::ErrorGuaranteed, | ||
| rustc_span::Symbol, |
There was a problem hiding this comment.
the alternative would be to restrict the region lift impl to not actually change these types
do we have some |
fc9ac0c to
39c966a
Compare
|
Some changes occurred in src/tools/clippy cc @rust-lang/clippy |
39c966a to
7c825ed
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
6de045f to
89ecd54
Compare
| use std::{debug_assert_matches, fmt}; | ||
|
|
||
| use rustc_abi::ExternAbi; | ||
| use rustc_data_structures::intern::Interned; |
There was a problem hiding this comment.
we should not use rustc_data_structures::intern, but instead, an associated type type Interned<T: ...>; on the trait Interner as r-a uses a different interning strategy
There was a problem hiding this comment.
I've tried the below;
// compiler/rustc_type_ir/src/interner.rs
pub trait Interner:
Sized
+ Copy
+ /* etc... */
{
type Interned<T>;
}
// compiler/rustc_middle/src/ty/context/impl_interner.rs
impl<'tcx> Interner for TyCtxt<'tcx> {
// Attempt 1:
type Interned<T> = rustc_data_structures::intern::Interned<'tcx, T>;
// Attempt 2:
type Interned<T: 'tcx> = rustc_data_structures::intern::Interned<'tcx, T>;
}- Attempt 1 fails with;
error[E0309]: the parameter type `T` may not live long enough
--> compiler/rustc_middle/src/ty/context/impl_interner.rs:35:5
|
30 | impl<'tcx> Interner for TyCtxt<'tcx> {
| ---- the parameter type `T` must be valid for the lifetime `'tcx` as defined here...
...
35 | type Interned<T> = rustc_data_structures::intern::Interned<'tcx, T>;
| ^^^^^^^^^^^^^^^^ ...so that the type `T` will meet its required lifetime bounds...
|
note: ...that is required by this bound
--> compiler/rustc_data_structures/src/intern.rs:26:28
|
26 | pub struct Interned<'a, T>(pub &'a T, pub private::PrivateZst);
| ^^^^^^^^^
help: consider adding an explicit lifetime bound
|
35 | type Interned<T: 'tcx> = rustc_data_structures::intern::Interned<'tcx, T>;
| ++++++
- Which leads to Attempt 2 which fails with;
error[E0276]: impl has stricter requirements than trait
--> compiler/rustc_middle/src/ty/context/impl_interner.rs:35:22
|
35 | type Interned<T: 'tcx> = rustc_data_structures::intern::Interned<'tcx, T>;
| ^^^^ impl has extra requirement `T: 'tcx`
I'm not sure if I'm missing something? It looks like my problem where I tried to use Interned from rustc_data_structures needing a lifetime added to Interner?
There was a problem hiding this comment.
ah yeah 😅 can you add type InternedRegionKind instead 😭
This is annoying limitation of Rust and I don't quite know whether to avoid this for now.
This comment has been minimized.
This comment has been minimized.
`rustc_type_ir` methods for the struct.
…e already been ported to ir
89ecd54 to
706e15e
Compare
|
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. |
View all comments
General notes
Probably best reviewed commit by commit. I've tried where to make the changes as isolated as possible.
pub struct Regionif we useInterned<...>in the definition then we'd need to add a lifetime to theInternertrait. As such I opted for a typeInternedRegionKindwhich isInterned<'tcx, RegionKind<'tcx>>in the implementation. To allow calling thekind()method I implementedinherent::IntoKindforInterned<'tcx, T>.Regiontrait ininherenthave been moved tocompiler/rustc_type_ir/src/sty/mod.rs.Liftmanually.kind()is correct or even should be implemented forInterned<...>?pretty.rsI've used an intermediary struct;RegionDisplayso we can still accessty::tls::withRegionDiagArgRegioncompiler/rustc_middle/src/ty/region.rsGenerally anything that needed to access fields on
TyCtxthave been made methods on the interner.interner.lifetimes.anon_re_bounds.get(debruijn.as_usize())for getting an interned lifetime, is now a trait method onInterner;fn get_anon_re_bounds_lifetime(...)fn intern_region(...)now a trait method onInternerinterner.lifetimes.anon_re_canonical_bounds.get(debruijn.as_usize())requiredfor
fn new_canonical_bound(...). Is nowfn get_anon_re_canonical_bounds_lifetime(...)interner.lifetimes.re_statichas becomefn get_re_static_lifetime()for methodfn new_static((..)r? @lcnr