Move Region from rustc_middle to rustc_type_ir#154989
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?
| Canonical, | ||
| } | ||
|
|
||
| impl<U: Interner> Lift<U> for BoundVarIndexKind { |
There was a problem hiding this comment.
surprising that this isn't/can't be derived or ignored in the derive
There was a problem hiding this comment.
39c966a, by adding back nop_lift! { region; Region<'a> => Region<'tcx> } in compiler/rustc_middle/src/ty/context.rs it takes care of all of the comments about Lift
|
|
||
| impl<I: Interner> Eq for RegionKind<I> {} | ||
|
|
||
| impl<I: Interner, U: Interner> Lift<U> for RegionKind<I> |
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
This comment has been minimized.
This comment has been minimized.
89ecd54 to
706e15e
Compare
This comment has been minimized.
This comment has been minimized.
|
rust-analyzer is developed in its own repository. If possible, consider making this change to rust-lang/rust-analyzer instead. cc @rust-lang/rust-analyzer |
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.
| } | ||
| impl<'tcx, T: std::fmt::Debug + Clone + Copy> rustc_type_ir::intern::Interned<TyCtxt<'tcx>> | ||
| for Interned<'tcx, T> | ||
| { |
There was a problem hiding this comment.
should not be in region as it applies to all interned things
| Canonical, | ||
| } | ||
|
|
||
| impl<U: Interner> Lift<U> for BoundVarIndexKind { |
|
|
||
| fn lift_region_raw<I: Interner>(self, region: I::InternedRegionKind) -> Self::InternedRegionKind | ||
| where | ||
| I::InternedRegionKind: Lift<Self, Lifted = Self::InternedRegionKind>, |
There was a problem hiding this comment.
why do we need this instead of deriving Lift for Region (so that it just recursively lifts its field)
|
|
||
| pub fn type_flags(self) -> TypeFlags { | ||
| fn type_flags(self) -> TypeFlags { | ||
| let mut flags = TypeFlags::empty(); |
There was a problem hiding this comment.
that method should be unused 🤔 more generally, rustc_type_ir duplicated some of the methods we also implement here, can you go through the methods on the extension trait and check whether we have the same method as an inherent method?
5222b87 to
2254fbc
Compare
|
@bors r+ rollup=never |
|
@bors p=6 |
This comment has been minimized.
This comment has been minimized.
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing 79a4b00 (parent) -> 0e29c21 (this PR) Test differencesShow 129 test diffs129 doctest diffs were found. These are ignored, as they are noisy. Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard 0e29c21d93abc9bdc330182be1fbd33cfd5fee68 --output-dir test-dashboardAnd then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
|
Finished benchmarking commit (0e29c21): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis perf run didn't have relevant results for this metric. Max RSS (memory usage)Results (secondary -0.7%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesThis perf run didn't have relevant results for this metric. Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 489.717s -> 495.463s (1.17%) |
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 forInterned<RegionKind>(I::InternedRegionKind) which is a no-op.kind()is correct or even should be implemented forInterned<...>?Regionand moved this along with otherencode/decodeimplementations into aserializefile.compiler/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