Add rigid alias marker#156742
Open
adwinwhite wants to merge 10 commits into
Open
Conversation
rustbot
added
A-LLVM
Contributor
Author
|
Unfinished. Let's have a look at the perf impact nonetheless. @bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
This comment has been minimized.
This comment has been minimized.
rust-bors Bot
pushed a commit
that referenced
this pull request
May 19, 2026
[Experiment] Add rigid alias marker
This comment has been minimized.
This comment has been minimized.
Contributor
|
💔 Test for 73eccb5 failed: CI. Failed job:
|
This comment has been minimized.
This comment has been minimized.
Contributor
Author
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
rust-bors Bot
pushed a commit
that referenced
this pull request
May 20, 2026
[Experiment] Add rigid alias marker
This comment has been minimized.
This comment has been minimized.
Contributor
This comment has been minimized.
This comment has been minimized.
Collaborator
|
Finished benchmarking commit (42e2939): comparison URL. Overall result: ❌ regressions - please read:Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf. Next, please: If you can, justify the regressions found in this try perf run in writing along with @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 1.6%, secondary -1.7%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 2.5%, secondary 17.8%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary 0.0%, secondary 0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 510.83s -> 514.725s (0.76%) |
rustbot
added
perf-regression
Contributor
Author
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
This comment has been minimized.
This comment has been minimized.
Collaborator
|
Some changes occurred in cc @BoxyUwU Some changes occurred in match checking cc @Nadrieril changes to the core type system cc @lcnr Some changes occurred to constck cc @fee1-dead This PR changes rustc_public cc @oli-obk, @celinval, @ouz-a, @makai410 Some changes occurred in exhaustiveness checking cc @Nadrieril Some changes occurred in cc @BoxyUwU Some changes occurred in need_type_info.rs cc @lcnr |
Collaborator
|
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. |
Contributor
Author
|
There's an assumptions-on-binders ICE I don't know how to fix yet. That shouldn't affect perf much. Let's see. |
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
This comment has been minimized.
This comment has been minimized.
adwinwhite
commented
Jun 17, 2026
| }; | ||
|
|
||
| if let Ok(_) = relation.relate(alias.to_ty(infcx.cx()), alias2) { | ||
| if let Ok(_) = relation.relate(alias.to_ty(infcx.cx(), IsRigid::No), alias2) { |
Contributor
Author
There was a problem hiding this comment.
This relating causes ICE. Unsure why it didn't surface before. And I'm having trouble grasping the rigidness invariants here so don't have a clue about where to insert normalization or set_aliases_to_rigid.
Collaborator
|
The job Click to see the possible cause of the failure (guessed by this bot)For more information how to resolve CI failures of this job, visit this link. |
Contributor
This comment has been minimized.
This comment has been minimized.
Collaborator
|
Finished benchmarking commit (d885f34): comparison URL. Overall result: ❌✅ regressions and improvements - please read:Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf. Next, please: If you can, justify the regressions found in this try perf run in writing along with @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 2.4%, secondary 1.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 3.4%, secondary -4.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary 0.0%, secondary 0.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 523.739s -> 531.127s (1.41%) |
rustbot
removed
the
S-waiting-on-perf
lcnr
reviewed
Jun 18, 2026
| } | ||
|
|
||
| ty::Alias(data) => match self.structurally_relate_aliases { | ||
| // We shouldn't perform lazy norm for rigid aliases. |
Contributor
There was a problem hiding this comment.
that sentence is slightly confusing, both times u use it.
I think "performing lazy norm" doesn't immediately make sense to me.
Would probably do sth like this?
Suggested change
| // We shouldn't perform lazy norm for rigid aliases. | |
| // We only need to be careful with potentially normalizeable | |
| // aliases here. See `generalize_alias_term` for more information. |
and then change the comment of generalize_alias_term to talk about "potentially normalizeable aliases" instead of just alias :>
lcnr
reviewed
Jun 18, 2026
| // `assemble_candidates_after_normalizing_self_ty`. | ||
| ty::Alias(_) | ty::Placeholder(..) | ty::Error(_) => (), | ||
| ty::Alias(ty::IsRigid::Yes, _) | ty::Placeholder(..) | ty::Error(_) => (), | ||
| // Need to support aliases not marked as rigid for the old solver. |
Contributor
There was a problem hiding this comment.
Suggested change
| // Need to support aliases not marked as rigid for the old solver. | |
| // FIXME(-Znext-solver=no): Need to support aliases not marked as rigid for the old solver. |
lcnr
reviewed
Jun 18, 2026
lcnr
reviewed
Jun 18, 2026
Comment on lines
+238
to
+241
| // FIXME(rigid_aliases_marker): ideally we set aliases to non_rigid right after | ||
| // the typing mode is set to `PostAnalysis`. | ||
| // But modifying local decls in MIR body is inconvenient. And we can't fold | ||
| // `PlaceRef` in `checked_places`. |
| } | ||
| fn fold_ty(&mut self, ty: I::Ty) -> I::Ty { | ||
| if let ty::Alias(alias_ty) = ty.kind() { | ||
| if let ty::Alias(_is_rigid, alias_ty) = ty.kind() { |
Contributor
There was a problem hiding this comment.
hmm, does an assert that is_rigid, ty::IsRigid::No ever trigger? I think we should only ever have non-rigid opaques here
| // FIXME: Strictly speaking this may be incomplete if the normalized-to | ||
| // type contains an ambiguous alias referencing bound regions. We should | ||
| // consider changing this to only use "shallow structural equality". | ||
| self.eq_structurally_relating_aliases(goal.param_env, term, unconstrained_term)?; |
Contributor
There was a problem hiding this comment.
we can rip out eq_structurally_relating_aliases entirely now i think as the relevant aliases are all marked as Rigid
Comment on lines
+97
to
+98
| let actual = | ||
| self.normalize(GoalSource::Misc, goal.param_env, actual)?; |
Contributor
There was a problem hiding this comment.
oh, we shouldn't normalize while there are still erased regions 😅, i'd keep the existing code here
| .insert_placeholder_assumptions(self.infcx.universe(), Some(Assumptions::empty())); | ||
|
|
||
| let (normalized, ambig_goal) = (self.normalize)(alias_term)?; | ||
| let (normalized, normalization_was_ambiguous) = (self.normalize)(alias_term)?; |
Contributor
There was a problem hiding this comment.
oh, what is happening here? why are we creating a new universe here?
Contributor
There was a problem hiding this comment.
ah, this check is wrong 😅 we have to use the universe from before we create the new universes for placeholders
rn the check we do here doesn't really detect anything afaict 😅
Contributor
Author
There was a problem hiding this comment.
We need to detect that the normalized term doesn't contain new infer vars. We do that by increasing the universe index at the start so that newly created infer vars will be in a unnameable universe.
Contributor
There was a problem hiding this comment.
it's more limiting, it should not reference any infer vars from a universe of any of the placeholders we created as part of normalization.
so somewhat more strict than what is currently implemented
lcnr
reviewed
Jun 18, 2026
| let infcx = self.infcx; | ||
| *self.var_map.entry(ty).or_insert_with(|| infcx.next_ty_var(DUMMY_SP)) | ||
| } | ||
| &ty::Alias(is_rigid, alias) |
Contributor
There was a problem hiding this comment.
Suggested change
| &ty::Alias(is_rigid, alias) | |
| // FIXME(rigid_aliases_marker): This should automatically | |
| // handled by type folders instead of needing to do it | |
| // manually here | |
| &ty::Alias(is_rigid, alias) |
lcnr
reviewed
Jun 18, 2026
| obligation.param_env, | ||
| ty::Unnormalized::new_wip(obligation.predicate), | ||
| ); | ||
| self.register_predicate_obligations(infcx, ocx.into_pending_obligations()); |
Contributor
There was a problem hiding this comment.
lcnr
reviewed
Jun 18, 2026
Comment on lines
+239
to
+240
| Const::new_unevaluated(cx, ty::IsRigid::Yes, a), | ||
| Const::new_unevaluated(cx, ty::IsRigid::Yes, b), |
Contributor
There was a problem hiding this comment.
that can actually introduce IsRigid::Yes in the old solver, which is not ideal xx
doesn't rly matter, but could we add a IsRigid::yes_if_next_solver(relation.infcx) or sth for this
lcnr
reviewed
Jun 18, 2026
|
|
||
| if ty != unnormalized_ty { | ||
| if normalized_ty != unnormalized_ty { | ||
| // FIXME: our rigidness folding is redundant in this case. |
Contributor
There was a problem hiding this comment.
we can instead only do this conversion if we actually change the typing_env. If it is already PostAnalysis when entering, we're all good :>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 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.
View all comments
This PR adds a rigidness marker to
AliasTy/UnevaluatedConst.It's used to skip renormalization of rigid aliases.
The difficulty is that rigid aliases are only valid in their own
TypingEnvso we need to treat them as non-rigid when entering anotherTypingEnv.This PR also adds a flag
-Zrenormalize-rigid-aliasesto test whether we properly handle typing mode change.Currently only
tests/ui/traits/next-solver/assembly/ambiguity-due-to-uniquification-4.rsfails this check.r? lcnr