Normalize capture place `ty`s to prevent ICE by JohnTitor · Pull Request #152165 · rust-lang/rust

JohnTitor · 2026-02-05T12:16:16Z

Fixes #151579
Fixes #120811
r? @lcnr

lcnr · 2026-02-08T20:05:09Z

compiler/rustc_middle/src/ty/closure.rs

+        HirPlaceBase::Upvar(upvar_id) => {
+            ty::TypingEnv::post_analysis(tcx, upvar_id.closure_expr_id)
+        }
+        _ => ty::TypingEnv::fully_monomorphized(),


This is hacky and subtly wrong in two ways.

even if the type is not from an upvar, we still need the right param_env to normalize

we use this during MIR build, so it should probably use TypingMode::Borrowck or whatever

see https://rustc-dev-guide.rust-lang.org/typing-parameter-envs.html

Ah I see, I've switched to use try_structurally_resolve_ty as mentioned in the issue: 176bf7d (this PR)
This way, we can avoid deep normalization here, right?

lcnr

ah, I didn't see we always get these types from the typeck_results. We should deeply normalize them in writeback and then assume that they are normalized afterwards

View changes since this review

lcnr · 2026-02-12T11:21:45Z

compiler/rustc_hir_typeck/src/upvar.rs

    }

+    fn try_structurally_resolve_place_ty(&self, span: Span, place: &mut Place<'tcx>) {
+        place.base_ty = self.try_structurally_resolve_type(span, place.base_ty);


use deeply_normalize here. We deeply normalize types in the writeback results, so I think we should do the same here

Just replacing didn't work so I had to do something like this instead: 750cd53

Is this what you meant for?

can you use at.deeply_normalize with both solvers?

I've tried but had to construct TraitEngine there which seems an unusual way (I haven't seen such a way in other places).
Instead, I guess using ObligationCtxt might be better here, other places do this way and it shoudl call at.deeply_normalize internally: a7ea420
How about this?

lcnr · 2026-02-13T13:59:05Z

compiler/rustc_hir_typeck/src/upvar.rs

                    UpvarMigrationInfo::CapturingPrecise {
                        source_expr: capture.info.path_expr_id,
-                        var_name: capture.to_string(self.tcx),
+                        var_name: self.place_to_string_for_migration(capture),


can we move this normalization even further up so that catpure.place is already deeply normalized and we don't have to do so here?

Moved: c22ffdb
I wonder if we should reduce scope by if next_solver as we now normalize stuff more generally, i.e. other than emitting lint rust_2021_incompatible_closure_captures and it would bring unnecessary cost for most cases 🤔

lcnr · 2026-02-14T18:33:02Z

compiler/rustc_hir_typeck/src/upvar.rs

        }
    }

+    fn try_structurally_resolve_place_ty(&self, span: Span, place: &mut Place<'tcx>) {


deeply_normalize should normalize arbitrary TypeFoldable things. Is Place type foldable?

Also, change this function name :>

Ah, you're right, it's TypeFoldable. Addressed: 7def3bc
And I noticed we have another win, it would fix #120811 (N.B. crashes/120911 is a dup of 120811, so I removed it).

lcnr · 2026-02-15T12:30:46Z

I messed up by reviewing your commit directly yesterday, see c22ffdb#r177216090

JohnTitor · 2026-02-16T12:29:33Z

@lcnr Sorry, I missed it 🙏
I think I addressed your comments: ecdea2c
I need to call normalize_capture_place before capture_information.push to prevent ICEs.

lcnr · 2026-02-16T12:30:50Z

compiler/rustc_hir_typeck/src/upvar.rs


        assert_eq!(self.tcx.hir_body_owner_def_id(body.id()), closure_def_id);
+
+        // Used by `ExprUseVisitor` when collecting closure capture information.


This comment feels is odd. It states information which is clearly visible 3 lines down. You could state sth like "We need to create a separate FnCtxt for nested bodies"

Creating FnCtxts for nested bodies is also something we normally do, so I don't know if that is that useful either. I think it's fine to just not have a comment here at all

Ok, removed in ebefb53.

lcnr · 2026-02-16T12:49:53Z

compiler/rustc_hir_typeck/src/upvar.rs

    }
+    fn normalize_capture_place(&self, span: Span, place: &mut Place<'tcx>) {
+        *place = self.resolve_vars_if_possible(place.clone());
+


why &mut Place instead of taking by value and returning it again?

Fair enough, addressed in dddbf96

lcnr · 2026-02-16T12:52:09Z

compiler/rustc_hir_typeck/src/upvar.rs

-            restrict_capture_precision(place_with_id.place.clone(), dummy_capture_kind);
+        let span = self.fcx.tcx.hir_span(diag_expr_id);
+        let mut place = place_with_id.place.clone();
+        self.fcx.normalize_capture_place(span, &mut place);


could also eagerly normalize closure_fake_reads 🤔

though, I guess this is fine for now as making sure the ExprUseVisitor always handles things correctly seems like a bigger change

Alright, should I add FIXME or something else?

rustbot · 2026-02-17T11:32:37Z

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.

JohnTitor · 2026-02-17T11:33:19Z

(rebase mistake, sorry for pinging)

rustbot assigned lcnr Feb 5, 2026

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) labels Feb 5, 2026

This comment has been minimized.

Sign in to view

theemathas mentioned this pull request Feb 6, 2026

ICE : internal error: entered unreachable code #152205

Open

lcnr closed this Feb 8, 2026

rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 8, 2026

lcnr reopened this Feb 8, 2026

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 8, 2026

lcnr reviewed Feb 8, 2026

View reviewed changes

JohnTitor force-pushed the issue-151579 branch from a544343 to 176bf7d Compare February 11, 2026 10:53

This comment has been minimized.

Sign in to view

JohnTitor requested a review from lcnr February 11, 2026 11:04

lcnr reviewed Feb 12, 2026

View reviewed changes

JohnTitor requested a review from lcnr February 12, 2026 22:32

lcnr reviewed Feb 13, 2026

View reviewed changes

JohnTitor force-pushed the issue-151579 branch from a7ea420 to c22ffdb Compare February 14, 2026 11:44

This comment has been minimized.

Sign in to view

lcnr reviewed Feb 14, 2026

View reviewed changes

JohnTitor force-pushed the issue-151579 branch from c22ffdb to 88036b0 Compare February 15, 2026 04:11

This comment has been minimized.

Sign in to view

JohnTitor force-pushed the issue-151579 branch from 88036b0 to 7def3bc Compare February 15, 2026 04:11

JohnTitor force-pushed the issue-151579 branch from 7def3bc to ecdea2c Compare February 16, 2026 12:22

This comment has been minimized.

Sign in to view

lcnr reviewed Feb 16, 2026

View reviewed changes

This comment was marked as off-topic.

Sign in to view

JohnTitor added 9 commits February 17, 2026 20:32

Normalize capture place tys to prevent ICE

b7d6767

call try_structurally_resolve_type before emitting diag

0d9f020

Deeply normalize on next solver

1d395f1

improve normalization way

a8c0441

Move normalization forward up

52cf4ba

Normalize the whole Place

82b09a1

deeply normalize only on next solver

a4e7e0c

Remove comment about ExprUseVisitor

d79fe52

simplify normalize_capture_place

dddbf96

JohnTitor force-pushed the issue-151579 branch from 5e5de59 to dddbf96 Compare February 17, 2026 11:32


		assert_eq!(self.tcx.hir_body_owner_def_id(body.id()), closure_def_id);

		// Used by `ExprUseVisitor` when collecting closure capture information.

Uh oh!

Comments

Conversation

JohnTitor commented Feb 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

JohnTitor Feb 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lcnr left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

lcnr commented Feb 15, 2026

Uh oh!

This comment has been minimized.

JohnTitor commented Feb 16, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as off-topic.

rustbot commented Feb 17, 2026

Uh oh!

JohnTitor commented Feb 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

JohnTitor commented Feb 5, 2026 •

edited

Loading

JohnTitor Feb 11, 2026 •

edited

Loading

lcnr left a comment •

edited by rustbot

Loading