Fix ICE in normalization during closure capture analysis (#149746)#149800
Fix ICE in normalization during closure capture analysis (#149746)#149800bors merged 1 commit intorust-lang:mainfrom
Conversation
|
r? @fee1-dead rustbot has assigned @fee1-dead. Use |
This comment has been minimized.
This comment has been minimized.
|
r? compiler |
|
|
|
|
This comment has been minimized.
This comment has been minimized.
| let input = self.typing_env.as_query_input(arg); | ||
|
|
||
| // 2. Use unwrap_or(arg) to return the original arg if it fails | ||
| self.tcx.try_normalize_generic_arg_after_erasing_regions(input).unwrap_or(arg) |
There was a problem hiding this comment.
we're intentionally using a bug! here as we're in normalize_generic_arg_after_erasing_regions.
The issue happens in the caller:
- we either have to change the caller to use
try_normalize...to avoid this ICE if the error is unavoidable - or prevent this error entirely by not calling
normalize_...for types which aren#t well-formed
There was a problem hiding this comment.
Thanks for the feedback @lcnr ! I have reverted the global change in rustc_middle and moved the fix to compiler/rustc_hir_typeck/src/upvar.rs as requested. specifically, I updated the needs_drop closure within has_significant_drop_outside_of_captures to use try_normalize_erasing_regions; if normalization fails, it now conservatively returns false (assuming no significant drop) instead of panicking.
3b0c42e to
fc87a13
Compare
This comment has been minimized.
This comment has been minimized.
|
@lcnr anything else that needs to be changed? |
| @@ -0,0 +1,16 @@ | |||
| // edition: 2015..2021 | |||
There was a problem hiding this comment.
| // edition: 2015..2021 | |
| //@ edition: 2015..2021 |
The MCVE I've posted in #149746 (comment) which you're using here as a test, deliberately has an @ there: It's a compiletest directive.
|
@lcnr anything else that needs to be changed? |
|
r? @fmease |
|
|
|
I am sorry that I did not quickly re-review this PR. For non-critical work, it is expected that reviewers sometimes take 1-2 weeks. Pinging people after a few days is fine if the issue is blocking other important work or if it fixes a critical issue. Please be more patient going forward. r? lcnr |
sorry sorry, i will take care of this from the next time |
There was a problem hiding this comment.
There's something a lot bigger here, which is why I didn't take the time to look at this PR until now.
At its core, a lot of code in the compiler assumes that if a type is well-formed, so are its fields. This is simply not true, neither currently nor in general:
- currently there's #149283
- in general, there's lcnr/random-rust-snippets#23
This is separate to the issue in this PR, where the struct itself is not well-formed and we don't emit an error for it. These we could handle by changing normalize_erasing_regions to emit a span_delayed_bug, returning an ty::Error instead. Doing that is kind of whatever however if the issue also just exists for otherwise well-formed programs.
Outside of codegen, using normalize_erasing_regions can pretty much always go wrong. Quickly looking through to source, the following additional calls to normalize_erasing_regions may also result in reachable ICE
- (we recur into struct fields)
rust/compiler/rustc_middle/src/ty/layout.rs
Line 545 in e951f47
- (should ICE for reference to alias as struct field)
rust/compiler/rustc_middle/src/ty/layout.rs
Line 906 in e951f47
- pretty much all (indirect) uses of
normalize_erasing_regionsin CTFE and MIR opts can result in ICE as long as they happen in generic bodies - using
normalize_erasing_regionsin borrowck for the not explicitly mentioned fields ofFoo { ..Default::default() }expressions should ICE - using
normalize_erasing_regionsinresolve_instance_rawICEs mir opts and CTFE using it
I would kind of like to figure out a more general strategy here instead of fixing these issues as they crop up. Anyways, fine with this PR for now, but want to start a T-types discussion about this and think about a general solution here.
| // If normalization fails, we assume it doesn't have a significant drop (false) | ||
| // to avoid crashing, allowing the compiler to emit the underlying type error later. | ||
| self.tcx | ||
| .try_normalize_erasing_regions(typing_env, ty) |
There was a problem hiding this comment.
can you instead replace the normalize_erasing_regions call in has_significant_drop with its try_ variant. r=me after.
There was a problem hiding this comment.
Done! I've moved the fix to has_significant_drop itself, replacing normalize_erasing_regions with try_normalize_erasing_regions and returning false when normalization fails.
badc3d2 to
cce05f0
Compare
This comment has been minimized.
This comment has been minimized.
compiler/rustc_middle/src/ty/util.rs
Outdated
| // FIX: Use try_normalize to avoid crashing. If it fails, return false. | ||
| tcx.try_normalize_erasing_regions(typing_env, query_ty) | ||
| .map(|erased| tcx.has_significant_drop_raw(typing_env.as_query_input(erased))) | ||
| .unwrap_or(false) |
There was a problem hiding this comment.
why false? in case the query_ty has infer we're currently conservatively returning true and we should the the same here
There was a problem hiding this comment.
Ah, good point! I completely missed that 😭🤦♂️we're already returning true conservatively for the has_infer() case. You're right - we should be consistent here. Updated to return true instead to match that behavior.
cce05f0 to
ac448c9
Compare
|
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. |
|
@bors r+ rollup |
Rollup of 6 pull requests Successful merges: - #149800 (Fix ICE in normalization during closure capture analysis (#149746)) - #150182 (Don't export upstream monomorphizations from compiler-builtins) - #150216 (Tidying up tests/ui/issues 15 tests [6/N]) - #150308 (Update bors configuration) - #150314 (rustc-dev-guide subtree update) - #150319 (use new term in description of --target) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of #149800 - Delta17920:fix-ice-149746, r=lcnr Fix ICE in normalization during closure capture analysis (#149746) This fixes an internal compiler error that occurred when normalizing associated types during closure capture analysis. The Fix: Modified rustc_middle/src/ty/normalize_erasing_regions.rs to gracefully handle projection normalization failures instead of panicking when analyzing closure captures. Regression Test: Added tests/ui/associated-types/normalization-ice-issue-149746.rs, a reproduction case involving complex associated type projections (<() as Owner>::Ty<T>) that previously crashed the compiler. Verified it now emits a standard type error (E0277). Fixes #149746
Rollup of 6 pull requests Successful merges: - rust-lang/rust#149800 (Fix ICE in normalization during closure capture analysis (rust-lang/rust#149746)) - rust-lang/rust#150182 (Don't export upstream monomorphizations from compiler-builtins) - rust-lang/rust#150216 (Tidying up tests/ui/issues 15 tests [6/N]) - rust-lang/rust#150308 (Update bors configuration) - rust-lang/rust#150314 (rustc-dev-guide subtree update) - rust-lang/rust#150319 (use new term in description of --target) r? `@ghost` `@rustbot` modify labels: rollup
Rollup of 6 pull requests Successful merges: - rust-lang/rust#149800 (Fix ICE in normalization during closure capture analysis (rust-lang/rust#149746)) - rust-lang/rust#150182 (Don't export upstream monomorphizations from compiler-builtins) - rust-lang/rust#150216 (Tidying up tests/ui/issues 15 tests [6/N]) - rust-lang/rust#150308 (Update bors configuration) - rust-lang/rust#150314 (rustc-dev-guide subtree update) - rust-lang/rust#150319 (use new term in description of --target) r? `@ghost` `@rustbot` modify labels: rollup
Rollup of 6 pull requests Successful merges: - rust-lang/rust#149800 (Fix ICE in normalization during closure capture analysis (rust-lang/rust#149746)) - rust-lang/rust#150182 (Don't export upstream monomorphizations from compiler-builtins) - rust-lang/rust#150216 (Tidying up tests/ui/issues 15 tests [6/N]) - rust-lang/rust#150308 (Update bors configuration) - rust-lang/rust#150314 (rustc-dev-guide subtree update) - rust-lang/rust#150319 (use new term in description of --target) r? `@ghost` `@rustbot` modify labels: rollup
This fixes an internal compiler error that occurred when normalizing associated types during closure capture analysis.
The Fix: Modified rustc_middle/src/ty/normalize_erasing_regions.rs to gracefully handle projection normalization failures instead of panicking when analyzing closure captures.
Regression Test: Added tests/ui/associated-types/normalization-ice-issue-149746.rs, a reproduction case involving complex associated type projections (<() as Owner>::Ty) that previously crashed the compiler. Verified it now emits a standard type error (E0277).
Fixes #149746