move TypeOutlives normalization out of region checking#152270
Open
JohnTitor wants to merge 7 commits intorust-lang:mainfrom
Open
move TypeOutlives normalization out of region checking#152270JohnTitor wants to merge 7 commits intorust-lang:mainfrom
TypeOutlives normalization out of region checking#152270JohnTitor wants to merge 7 commits intorust-lang:mainfrom
Conversation
rustbot
added
S-waiting-on-review
lcnr
reviewed
Feb 11, 2026
Comment on lines
90
to
94
| for obligation in infcx.take_registered_region_obligations() { | ||
| if seen_outputs.insert((obligation.sup_type, obligation.sub_region)) { | ||
| normalized_obligations.push(obligation); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
we can't guarantee that the newly registered obligations are normalized.
What I would like to do instead is to normalize in fn compute_type_outlives_goal, rn we register the goal right away, but we could deeply normalize the type first. This means that the trait solver handles overflow for us then.
We don't have a folder to deeply normalize in the solver, so either duplicate the existing impl used by deeply_normalize (which can only be used outside of the solver) ,and change it to use an ecx, or make it generic over the normalization procedure
Contributor
There was a problem hiding this comment.
we should entirely remove the fn resolve_regions_with_normalize and instead expect the obligations to already be normalized
Member
Author
There was a problem hiding this comment.
I see removed the normalization steps (I couldn't remove function entirely as it's used in rustc_trait_selection) in region checking and tried to implement it on next solver: 3f9571c
I don't think I've implemented normalizer on next solver completely but would like to receive your review.
(Seems ICE'd but I'd like to confirm I'm going to the right way)
JohnTitor
force-pushed
the
normalize-before-region-obl
branch
from
af4e816 to
3f9571c
Compare
Collaborator
|
Some changes occurred to the core trait solver cc @rust-lang/initiative-trait-system-refactor |
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. |
This comment has been minimized.
This comment has been minimized.
lcnr
reviewed
Feb 14, 2026
| assert!(!self.in_snapshot(), "cannot process registered region obligations in a snapshot"); | ||
|
|
||
| // Must loop since the process of normalizing may itself register region obligations. | ||
| // We loop to handle the case that processing one obligation ends up registering another. |
Contributor
There was a problem hiding this comment.
this should now never happen. Afaik we only added the loop in the PR doing normalization. Probably useful to look at the git blame here to revert pretty much all the unnecessary changes here
lcnr
reviewed
Feb 14, 2026
Comment on lines
120
to
122
| /// With the next solver, `TypeOutlives` goals are normalized while they are | ||
| /// evaluated (and before being registered as region obligations), so region | ||
| /// resolution does not need to normalize them. |
Contributor
There was a problem hiding this comment.
I think the framing of this comment only makes sense while doing the change in this PR and is otherwise confusing.
I think if we want to talk about normalization at all here, we should mention that type outlives obligations are expected to already be normalized at this point.
Member
Author
There was a problem hiding this comment.
It was almost for myself, removed: 794d33d (this PR)
| @@ -0,0 +1,15 @@ | |||
| //@ compile-flags: -Znext-solver=globally | |||
|
|
|||
Contributor
There was a problem hiding this comment.
please add comment explaining what we're testing here and why it ICE'd
| &ObligationCause::dummy_with_span(span), | ||
| ); | ||
| Some(Certainty::Yes) | ||
| } |
Contributor
There was a problem hiding this comment.
try removing the fast path. I think it's not necessary for performance and easy to get wrong. We currently incorrectly register things if they have inference variables in them. These inference variables could then get instantiated with something that references an (higher-ranked) alias we have to normalize
| // obligations so that later processing does not have to structurally process aliases. | ||
| let ty = self.resolve_vars_if_possible(ty); | ||
| let ty = if ty.has_aliases() { | ||
| self.deeply_normalize_for_outlives(goal.param_env, ty) |
Contributor
There was a problem hiding this comment.
do the has_aliases check in the function instead of before calling it 🤔 while moving these sorts of checks out functions is generally good, here this check is an optimization of this function, so it should stay inside of it
| return Ok(ct); | ||
| } | ||
|
|
||
| let ty::ConstKind::Unevaluated(..) = ct.kind() else { |
Contributor
There was a problem hiding this comment.
style: please use an if let instead of an early return
| return Ok(ty); | ||
| } | ||
|
|
||
| let ty::Alias(..) = ty.kind() else { return ty.try_super_fold_with(self) }; |
| ), | ||
| ); | ||
|
|
||
| let result = match self.ecx.try_evaluate_goal(GoalSource::TypeRelating, goal) { |
Contributor
There was a problem hiding this comment.
Why try_evaluate_goal?
Thinking about how to implement this, I'd take a look at the FindParamInClause visitor
I expect this function to do:
- call
structurally_normalize_terminstead of manually evaluating - if you encounter an infer var, return
Certainty::Maybe(change the error ty of the type folder to beResult<Certainty, NoSolution>) - if it errors, return an error
- if you hit the recursion limit, return overflow (i think
FindParamInClausealso needs to check the recursion limit and can currently cause stack overflows)
With this the returned type should be fully normalized
when encountering binders, you need to map the bound vars to placeholders, and then map placeholders back to bound vars on success (same as the deeply_normalize folder
Member
Author
There was a problem hiding this comment.
Addressed in 471f298 but I'm still not sure if it follows your intention completely. I had to tweak some checks in compute_type_outlives_goal otherwise it'd emit spammy recursion limit errors for may test cases on next solver.
lcnr
reviewed
Feb 15, 2026
| self.probe(|_| inspect::ProbeKind::NormalizedSelfTyAssembly) | ||
| .enter(|ecx| ecx.deeply_normalize_for_outlives(goal.param_env, ty)) | ||
| .ok() | ||
| .unwrap_or(ty) |
Contributor
There was a problem hiding this comment.
what do you mean with "this avoids spurious overflows"?
I would expect us to always deeply_normalize and if we returb Err(Ok(certainty)) we don't register a type outlives but instead call evaluate_added_goals_and_make_canonical_response with that certainty. Actually, please change deeply_normalize_for_outlvies to return a MaybeCause instead as we should never return Certainty::Yes
Collaborator
|
The job Click to see the possible cause of the failure (guessed by this bot) |
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
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.
This adds two more normalization steps for outlives before region checking, as mentioned in the issue.
r? @lcnr
Fixes #151461 and fixes rust-lang/trait-system-refactor-initiative#260