Don't ICE on pending obligations from deep normalization in a loop #140021
+163
−14
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
|
This PR changes a file inside |
lcnr
reviewed
Apr 19, 2025
Comment on lines
78
to
82
| let mut errors = fulfill_cx.select_all_or_error(self.infcx); | ||
| // Drain pending obligations too, since deep normalization may happen | ||
| // in a loop and we don't want to trigger the assertion on the next | ||
| // iteration due to pending obligations we've left over. | ||
| errors.extend(fulfill_cx.collect_remaining_errors(self.infcx)); |
There was a problem hiding this comment.
hmm, fn select_all_or_error is currently implemented as
fn select_all_or_error(&mut self, infcx: &InferCtxt<'tcx>) -> Vec<E> {
let errors = self.select_where_possible(infcx);
if !errors.is_empty() {
return errors;
}
self.collect_remaining_errors(infcx)
}it feels a bit footgunny to not add the overflowing obligations if there are other errors. I think changing the if errors.is_empty() branch to also add the remaining errors would be better 🤔
There was a problem hiding this comment.
or no, in case select_where_possible returns an error, we want to ignore overflow and ambiguity errors. I think we also want to ignore these ambiguity errors here, so maybe change this to
let errors = fulfill_cx.select_all_or_error(self.infcx);
if errors.is_empty() {
Ok(self.infcx.resolve_vars_if_possible(value))
} else {
// In case normalization resulted in an error, we want to
// drain any still ambiguous goal to avoid triggering the
// assertion on the next iteration.
let _ = fulfill_cx.collect_remaining_errors(self.infcx));
Err(errors)
}There was a problem hiding this comment.
alternatively we could drop the remaining obligations in the !errors.is_empty() branch in select_all_or_error itself? 🤔
There was a problem hiding this comment.
alternatively we could drop the remaining obligations in the
!errors.is_empty()branch inselect_all_or_erroritself? 🤔
Hm, feels like a much larger and more subtle change. I'd rather keep it located to deep normalization.
There was a problem hiding this comment.
or no, in case select_where_possible returns an error, we want to ignore overflow and ambiguity errors. I think we also want to ignore these ambiguity errors here, so maybe change this to [...]
Functionally equivalent to what I have right now, but I guess alright.
There was a problem hiding this comment.
Functionally equivalent to what I have right now, but I guess alright.
Correction -- not functionally equivalent, but doesn't matter for UI tests. I originally had let _ = fulfill_cx.collect_remaining_errors(self.infcx)) on the good path of deep norm too, but it felt spooky. I guess it's fine though.
compiler-errors
force-pushed
the
no-deep-norm-ice
branch
from
12269e9 to
47911eb
Compare
|
@bors r+ rollup |
bors
added
S-waiting-on-bors
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Apr 20, 2025
…lcnr Don't ICE on pending obligations from deep normalization in a loop See the comment I left inline in `compiler/rustc_trait_selection/src/traits/normalize.rs`. Fixes rust-lang#133868 r? lcnr
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
spurious @bors retry |
bors
added
S-waiting-on-bors
jhpratt
added a commit
to jhpratt/rust
that referenced
this pull request
Apr 21, 2025
…r=lcnr Don't ICE on pending obligations from deep normalization in a loop See the comment I left inline in `compiler/rustc_trait_selection/src/traits/normalize.rs`. Fixes rust-lang#133868 r? lcnr
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Apr 21, 2025
Rollup of 11 pull requests Successful merges: - rust-lang#139795 (Clarify why SGX code specifies linkage/symbol names for certain statics) - rust-lang#139946 (fix missing word in comment) - rust-lang#139982 (SystemTime doc tweaks) - rust-lang#140009 (docs(LocalKey<T>): clarify that T's Drop shouldn't panic) - rust-lang#140021 (Don't ICE on pending obligations from deep normalization in a loop) - rust-lang#140036 (Advent of `tests/ui` (misc cleanups and improvements) [4/N]) - rust-lang#140047 (remove a couple clones) - rust-lang#140052 (Fix error when an intra doc link is trying to resolve an empty associated item) - rust-lang#140074 (rustdoc-json: Improve test for auto-trait impls) - rust-lang#140076 (jsondocck: Require command is at start of line) - rust-lang#140081 (Update `libc` to 0.2.172) r? `@ghost` `@rustbot` modify labels: rollup
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Apr 21, 2025
Rollup of 11 pull requests Successful merges: - rust-lang#139795 (Clarify why SGX code specifies linkage/symbol names for certain statics) - rust-lang#139946 (fix missing word in comment) - rust-lang#139982 (SystemTime doc tweaks) - rust-lang#140009 (docs(LocalKey<T>): clarify that T's Drop shouldn't panic) - rust-lang#140021 (Don't ICE on pending obligations from deep normalization in a loop) - rust-lang#140036 (Advent of `tests/ui` (misc cleanups and improvements) [4/N]) - rust-lang#140047 (remove a couple clones) - rust-lang#140052 (Fix error when an intra doc link is trying to resolve an empty associated item) - rust-lang#140074 (rustdoc-json: Improve test for auto-trait impls) - rust-lang#140076 (jsondocck: Require command is at start of line) - rust-lang#140081 (Update `libc` to 0.2.172) r? `@ghost` `@rustbot` modify labels: rollup
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Apr 21, 2025
Rollup of 11 pull requests Successful merges: - rust-lang#139795 (Clarify why SGX code specifies linkage/symbol names for certain statics) - rust-lang#139946 (fix missing word in comment) - rust-lang#139982 (SystemTime doc tweaks) - rust-lang#140009 (docs(LocalKey<T>): clarify that T's Drop shouldn't panic) - rust-lang#140021 (Don't ICE on pending obligations from deep normalization in a loop) - rust-lang#140036 (Advent of `tests/ui` (misc cleanups and improvements) [4/N]) - rust-lang#140047 (remove a couple clones) - rust-lang#140052 (Fix error when an intra doc link is trying to resolve an empty associated item) - rust-lang#140074 (rustdoc-json: Improve test for auto-trait impls) - rust-lang#140076 (jsondocck: Require command is at start of line) - rust-lang#140081 (Update `libc` to 0.2.172) r? `@ghost` `@rustbot` modify labels: rollup
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Apr 21, 2025
…enton Rollup of 8 pull requests Successful merges: - rust-lang#139946 (fix missing word in comment) - rust-lang#139982 (SystemTime doc tweaks) - rust-lang#140009 (docs(LocalKey<T>): clarify that T's Drop shouldn't panic) - rust-lang#140021 (Don't ICE on pending obligations from deep normalization in a loop) - rust-lang#140029 (Relocate tests in `tests/ui`) - rust-lang#140030 (Fix autodiff debug builds) - rust-lang#140120 (Use `output_base_dir` for `mir_dump_dir`) - rust-lang#140121 (Document why CodeStats::type_sizes is public) r? `@ghost` `@rustbot` modify labels: rollup
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Apr 21, 2025
Rollup merge of rust-lang#140021 - compiler-errors:no-deep-norm-ice, r=lcnr Don't ICE on pending obligations from deep normalization in a loop See the comment I left inline in `compiler/rustc_trait_selection/src/traits/normalize.rs`. Fixes rust-lang#133868 r? lcnr
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.
See the comment I left inline in
compiler/rustc_trait_selection/src/traits/normalize.rs.Fixes #133868
r? lcnr