Move struct placeholder pt2 #150271
Draft
Move struct placeholder pt2 #150271
+597
−432
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
S-waiting-on-author
This comment has been minimized.
This comment has been minimized.
lcnr
reviewed
Dec 22, 2025
Jamesbarford
force-pushed
the
chore/refactor-struct-placeholder-pt2
branch
from
753b92c to
2077c6b
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
lcnr
reviewed
Dec 30, 2025
This comment has been minimized.
This comment has been minimized.
lcnr
reviewed
Jan 5, 2026
lcnr
reviewed
Jan 5, 2026
lcnr
reviewed
Jan 5, 2026
Comment on lines
19
to
20
| mapped_regions: IndexMap<I::PlaceholderRegion, ty::BoundRegion>, | ||
| mapped_types: IndexMap<I::PlaceholderTy, ty::BoundTy>, |
Contributor
There was a problem hiding this comment.
why does PlaceholderRegion and PlaceholderTy still exist? 🤔
lcnr
reviewed
Jan 5, 2026
lcnr
reviewed
Jan 5, 2026
|
|
||
| pub type PlaceholderConst<I: Interner> = ty::Placeholder<I, BoundConst>; | ||
|
|
||
| impl<I: Interner> PlaceholderLike<I> for PlaceholderConst<I> { |
Contributor
There was a problem hiding this comment.
no longer need the trait, can just use inherent methods now
lcnr
reviewed
Jan 5, 2026
Contributor
|
The reason
Try to replace the manual impl with a derive and see if anything breaks. If it does break, add |
Jamesbarford
force-pushed
the
chore/refactor-struct-placeholder-pt2
branch
from
d262f5c to
d36238d
Compare
This comment has been minimized.
This comment has been minimized.
Jamesbarford
force-pushed
the
chore/refactor-struct-placeholder-pt2
branch
from
d36238d to
a03a907
Compare
This comment has been minimized.
This comment has been minimized.
Jamesbarford
force-pushed
the
chore/refactor-struct-placeholder-pt2
branch
from
a03a907 to
b4de573
Compare
This comment has been minimized.
This comment has been minimized.
lcnr
reviewed
Jan 15, 2026
lcnr
reviewed
Jan 15, 2026
This comment has been minimized.
This comment has been minimized.
Jamesbarford
force-pushed
the
chore/refactor-struct-placeholder-pt2
branch
from
044bbd5 to
cdff0b9
Compare
This comment has been minimized.
This comment has been minimized.
lcnr
reviewed
Jan 19, 2026
This comment has been minimized.
This comment has been minimized.
implementation of `HashStable`
…es I moved over to `binder.rs`
…ment `HashStable` for required emums and structs
Jamesbarford
force-pushed
the
chore/refactor-struct-placeholder-pt2
branch
from
e30f48d to
9dec2e9
Compare
This comment has been minimized.
This comment has been minimized.
Collaborator
|
The job Click to see the possible cause of the failure (guessed by this bot) |
lcnr
reviewed
Jan 21, 2026
Comment on lines
+371
to
+375
| let mut full_binders: Vec<ty::BoundVariableKind<'tcx>> = | ||
| self.rbv.late_bound_vars.get_mut_or_insert_default(hir_id.local_id).clone(); | ||
|
|
||
| full_binders.extend(supertrait_bound_vars); | ||
|
|
lcnr
reviewed
Jan 21, 2026
| assert!(matches!(index_kind, ty::BoundVarIndexKind::Canonical)); | ||
| opt_values[bv.var()] = Some(*original_value); | ||
| opt_values | ||
| [<ty::BoundConst as rustc_type_ir::inherent::BoundVarLike<I>>::var(bv)] = |
Contributor
There was a problem hiding this comment.
should just be bv.var() still. We shouldn't need the BoundVarLike trait anymore, do we?
lcnr
reviewed
Jan 21, 2026
Comment on lines
+961
to
963
| #[derive_where(Clone, Ord, Eq, PartialEq, Hash; I: Interner, T)] | ||
| #[derive_where(PartialOrd; I: Interner, T: Ord)] | ||
| #[derive_where(Copy; I: Interner, T: Copy, T)] |
Contributor
There was a problem hiding this comment.
existing: what are these derive_where 😅 they are an inconsistent mess xd
lcnr
reviewed
Jan 21, 2026
| T: HashStable<CTX>, | ||
| { | ||
| fn hash_stable(&self, hcx: &mut CTX, hasher: &mut StableHasher) { | ||
| self.universe.hash_stable(hcx, hasher); |
Contributor
There was a problem hiding this comment.
Suggested change
| self.universe.hash_stable(hcx, hasher); | |
| let Placeholder { universe, bound, _tcx } = self; | |
| universe.hash_stable(hcx, hasher); | |
| bound.hash_stable(hcx, hasher); |
while unlikely, this makes sure that we won't forget to update this function when adding a field to placeholder
lcnr
reviewed
Jan 21, 2026
Comment on lines
+1016
to
+1017
| #[derive_where(Clone, PartialEq, Eq, Hash; I: Interner)] | ||
| #[derive_where(Copy; I: Interner)] |
Contributor
There was a problem hiding this comment.
why is the Copy separate here
lcnr
reviewed
Jan 21, 2026
Comment on lines
+1058
to
+1059
| I::Symbol: Hash, | ||
| I::DefId: Hash, |
Contributor
There was a problem hiding this comment.
that's wrong. these things need to be HashStable. Directly hashing DefIds has different results in different compilation sessions, causing your ICE
lcnr
reviewed
Jan 21, 2026
Comment on lines
+1117
to
+1118
| #[derive_where(Clone, PartialEq, Eq, Debug, Hash; I: Interner)] | ||
| #[derive_where(Copy; I: Interner)] |
lcnr
reviewed
Jan 21, 2026
Comment on lines
+1142
to
+1143
| #[derive_where(Clone, PartialEq, Eq, Debug, Hash; I: Interner)] | ||
| #[derive_where(Copy; I: Interner)] |
lcnr
reviewed
Jan 21, 2026
Comment on lines
+1197
to
+1198
| #[derive_where(Clone, PartialEq, Eq, Hash; I: Interner)] | ||
| #[derive_where(Copy; I: Interner)] |
lcnr
reviewed
Jan 21, 2026
|
|
||
| pub type PlaceholderRegion<I> = ty::Placeholder<I, BoundRegion<I>>; | ||
|
|
||
| impl<I: Interner> PlaceholderLike<I> for PlaceholderRegion<I> { |
Contributor
There was a problem hiding this comment.
remove PlaceholderLike, can be inherent methods
lcnr
reviewed
Jan 21, 2026
| } | ||
| } | ||
|
|
||
| impl<I: Interner> BoundVarLike<I> for BoundRegion<I> { |
lcnr
reviewed
Jan 21, 2026
Comment on lines
+1268
to
+1269
| #[derive_where(Clone, PartialEq, Eq, Hash; I: Interner)] | ||
| #[derive_where(Copy; I: Interner)] |
lcnr
reviewed
Jan 21, 2026
| } | ||
| ty::GenericArgKind::Const(ct) => { | ||
| matches!(ct.kind(), ty::ConstKind::Bound(ty::BoundVarIndexKind::Canonical, bc) if bc.var().as_usize() == bv) | ||
| matches!(ct.kind(), ty::ConstKind::Bound(ty::BoundVarIndexKind::Canonical, bc) if <ty::BoundConst as BoundVarLike<I>>::var(bc).as_usize() == bv) |
lcnr
reviewed
Jan 21, 2026
| } | ||
| ty::GenericArgKind::Const(ct) => { | ||
| if matches!(ct.kind(), ty::ConstKind::Bound(ty::BoundVarIndexKind::Canonical, bc) if var == bc.var()) | ||
| if matches!(ct.kind(), ty::ConstKind::Bound(ty::BoundVarIndexKind::Canonical, bc) if var == <ty::BoundConst as BoundVarLike<I>>::var(bc)) |
lcnr
reviewed
Jan 21, 2026
| } | ||
|
|
||
| type DefId: DefId<Self>; | ||
| type DefId: DefId<Self> + Hash; |
Contributor
There was a problem hiding this comment.
add HashStable as a requirement to DefId behind a cfg(nightly)
edit: actually, looking at other HashStable<CTX> impls, this just shouldn't be necessary 🤔 we can just add I::DefId: HashStable<CTX> as a where-bound in the impl?
lcnr
reviewed
Jan 21, 2026
| let name = self.opt_item_name(id).unwrap_or_else(|| { | ||
| bug!("item_name: no name for {:?}", self.def_path(id)); | ||
| }); | ||
| if name != kw::UnderscoreLifetime { Some(name) } else { None } |
Contributor
There was a problem hiding this comment.
why not add opt_item_name to the interner instead 🤔
I guess we'd need to import kw::UnderscopeLifetime to rustc_middle?
maybe instead add a fn is_kw_underscore_lifetime to I::Symbol/trait Symbol?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
r? ghost