Properly stall coroutine witnesses in new solver #138845
Open
+389
−137
Conversation
This file contains 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-review
|
Some changes occurred to the core trait solver cc @rust-lang/initiative-trait-system-refactor changes to |
compiler-errors
commented
Mar 23, 2025
| @@ -65,13 +65,13 @@ pub enum TypingMode<I: Interner> { | |||
| /// let x: <() as Assoc>::Output = true; | |||
| /// } | |||
| /// ``` | |||
| Analysis { defining_opaque_types: I::DefiningOpaqueTypes }, | |||
| Analysis { defining_opaque_types: I::LocalDefIds, stalled_generators: I::LocalDefIds }, | |||
There was a problem hiding this comment.
Probably could squash this into one list.
compiler-errors
commented
Mar 23, 2025
| @@ -162,7 +162,7 @@ where | |||
| self.select(selcx) | |||
| } | |||
|
|
|||
| fn drain_unstalled_obligations( | |||
| fn drain_stall_obligations_for_coroutines( | |||
There was a problem hiding this comment.
Suggested change
| fn drain_stall_obligations_for_coroutines( | |
| fn drain_stalled_obligations_for_coroutines( |
| ScrubbedTraitError<'tcx>, | ||
| >(self.at, ct, vec![None; ct.outer_exclusive_binder().as_usize()]) | ||
| { | ||
| Ok((value, _)) => value, |
There was a problem hiding this comment.
We throw away ambiguous preds here b/c we may be using this folder on types that really are just not fully normalizable.
There was a problem hiding this comment.
The preds list should just contain stalled coroutine obligations, after all.
| /// entered before passing `value` to the function. This is currently needed for | ||
| /// `normalize_erasing_regions`, which skips binders as it walks through a type. | ||
| /// | ||
| /// TODO: doc |
There was a problem hiding this comment.
I need to explain that this doesn't return all ambiguous preds, just the ones that are stalled on coroutines.
| struct StalledOnCoroutines<'tcx> { | ||
| stalled_generators: &'tcx ty::List<LocalDefId>, | ||
| span: Span, | ||
| // TODO: Cache |
There was a problem hiding this comment.
Cache would be nice since visiting everything 128318913 times to look for coroutines is probably expensive.
| @@ -189,6 +189,20 @@ where | |||
| debug_assert!(ecx.opaque_type_is_rigid(opaque_ty.def_id)); | |||
| } | |||
|
|
|||
| if let ty::CoroutineWitness(def_id, _) = goal.predicate.self_ty().kind() { | |||
There was a problem hiding this comment.
I should pull this out into a helper, b/c I think I need to also apply this hack to copy/clone. I think those are it tho.
| // Increase this limit if necessary, but do try to keep the size low if possible | ||
| #[cfg(target_pointer_width = "64")] | ||
| const _: () = { | ||
| if size_of::<Key<'static>>() > 88 { | ||
| if size_of::<Key<'static>>() > 96 { |
| predicate | ||
| assert!( | ||
| !self.should_normalize, | ||
| "normalizing predicats in writeback is not generally sound" |
There was a problem hiding this comment.
Suggested change
| "normalizing predicats in writeback is not generally sound" | |
| "normalizing predicates in writeback is not generally sound" |
There was a problem hiding this comment.
I guess this could be a bug!() so that we don't need to fold here.
|
The job Click to see the possible cause of the failure (guessed by this bot) |
jhpratt
added a commit
to jhpratt/rust
that referenced
this pull request
Mar 24, 2025
Tweaks to writeback and `Obligation -> Goal` conversion Each of these commits are self-contained, but are prerequisites that I'd like to land before rust-lang#138845, which still needs some cleaning. The ""most controversial"" one is probably [Explicitly don't fold coroutine obligations in writeback](rust-lang@e7d27ba), which I prefer because I think using `fold_predicate` to control against not normalizing predicates seems... easy to mess up 🤔, and we could have *other things* that we don't want to normalize. Explicitly noting whether we want `resolve` to normalize is a lot clearer (and currently in writeback is limited to resolving stalled coroutine obligations), since we can attach it to a comment that explains *why*.
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Mar 24, 2025
Rollup merge of rust-lang#138846 - compiler-errors:stall-prereqs, r=lcnr Tweaks to writeback and `Obligation -> Goal` conversion Each of these commits are self-contained, but are prerequisites that I'd like to land before rust-lang#138845, which still needs some cleaning. The ""most controversial"" one is probably [Explicitly don't fold coroutine obligations in writeback](rust-lang@e7d27ba), which I prefer because I think using `fold_predicate` to control against not normalizing predicates seems... easy to mess up 🤔, and we could have *other things* that we don't want to normalize. Explicitly noting whether we want `resolve` to normalize is a lot clearer (and currently in writeback is limited to resolving stalled coroutine obligations), since we can attach it to a comment that explains *why*.
|
☔ The latest upstream changes (presumably #138873) made this pull request unmergeable. Please resolve the merge conflicts. |
lcnr
reviewed
Mar 24, 2025
| } | ||
|
|
||
| fn pending_obligations(&self) -> PredicateObligations<'tcx> { | ||
| self.obligations.clone_pending() | ||
| } | ||
|
|
||
| fn drain_unstalled_obligations(&mut self, _: &InferCtxt<'tcx>) -> PredicateObligations<'tcx> { | ||
| self.obligations.take_pending() | ||
| fn drain_stall_obligations_for_coroutines( |
There was a problem hiding this comment.
Suggested change
| fn drain_stall_obligations_for_coroutines( | |
| fn drain_stalled_obligations_for_coroutines( |
lcnr
reviewed
Mar 24, 2025
| fn visit_goal(&mut self, inspect_goal: &super::inspect::InspectGoal<'_, 'tcx>) -> Self::Result { | ||
| inspect_goal.goal().predicate.visit_with(self)?; | ||
|
|
||
| if let Some(candidate) = inspect_goal.unique_applicable_candidate() { |
There was a problem hiding this comment.
this type visitor feels somewhat fragile and I expect unique_applicable_candidate and the limited recursion depth to cause us to fail to stall obligations in very rare cases. otoh I don't think this is a problem though
so my understanding here is:
- for correctness it doesn't matter how many obligations we stall
- for diagnostics (and perf) we want to stall as few obligations as possible
- failing to stall causes unexpected ambiguity errors
Please add this as a comment somewhere, prolly the stalled_coroutine_obligations field of the typeck results
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
TODO: write description
r? lcnr