hir_ty_lowering: fix anon const type recovery

[Dnreikronos]

View all comments

fixes #158818

check_crate used to check whether coroutine closures need a by-move body in the same parallel pass that type-checks body owners.

That ordering is the bug. With an async closure inside a const argument, the by-move check asks for type_of on the nested coroutine. This can type-check the anon const before the enclosing body lowers the const argument and feeds its expected type. The query caches {type error}, then normal lowering later feeds the const parameter's actual type for the same key, so rustc sees two different values and ICEs.

I moved the by-move work into a second pass. The first pass gets through normal body type checking and feeds the anon const types. The second pass only starts after that is done, so it cannot ask for the nested coroutine type too early.

I think this is a much better place to fix it than the HIR-path recovery I tried earlier. The race starts in check_crate, and fixing the ordering there covers path expressions, type positions, and the generic argument cases without keeping a second, incomplete version of generic arg lowering in type_of.

The UI tests cover the original repro and the nearby shapes that came up while working on it. They run with -Zthreads=0 and now produce the normal type mismatch instead of an ICE.

[rustbot]

HIR ty lowering was modified

cc @fmease

[rustbot]

r? @davidtwco

rustbot has assigned @davidtwco.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler, types
• compiler, types expanded to 75 candidates
• Random selection from 20 candidates

[Dnreikronos]

@fmease i pushed a much narrower version.

i dropped the type-relative recovery path completely, so there is no separate iat/inherent assoc selection anymore. i also removed the generic-arg lowering helper from generics.rs. the remaining type_of code only accepts the exact simple shape from the repro: one resolved segment, one explicit const arg, one own const param, no parent generics, no self, no inferred args, no constraints, no late-bound regions.

imo this is aligned with the review feedback while still fixing the bug. there is still a tiny guard in type_of to make sure the anon const maps to that one const param, but idk how to remove even that without either reusing an existing lowering hook or changing the shape of the fix more deeply. ltm if you want zero matching in type_of. in that case i think the next step is finding a proper existing hook, or maybe backing out of this query-side recovery approach.

[davidtwco]

I'm going to re-roll this because I'm not too confident in it, the implementation strikes me as overfitting to the specific test case rather than the general problem.

View changes since this review

[davidtwco]

r? compiler

[Dnreikronos]

I'm going to re-roll this because I'm not too confident in it, the implementation strikes me as overfitting to the specific test case rather than the general problem.

View changes since this review

yeah, fair.
the reason i went this way is that the bug looks like a query consistency issue to me: type_of for the anon const can run before normal generic arg lowering gets to feed the expected const type. then later the normal path feeds the real type, and we can end up with the inconsistency that caused the ice.

my first attempt tried to recover more cases, but after fmease's comments i think that was the wrong direction. it was starting to duplicate too much of generic arg lowering and type-relative/iat selection in type_of, and i agree that code shouldn't live there.

but yeah, requiring exactly one generic arg and exactly one own param is probably overcorrecting. i'll keep this limited to direct resolved function paths, but make it handle simple multi-arg cases where this const arg's hir id can be matched to the corresponding own const param without doing real lowering. i'll still bail out for stuff that needs real lowering, like parent generics, self, late-bound regions, constraints, inferred args, defaults, type-relative paths, and const param types that depend on other generics.

ltm if that still feels like the wrong place for this. i'm not completely sure, but imo that seems like the least bad middle ground between the first version and this overfit one.

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[TaKO8Ki]

If there isn’t a strong reason to restrict this to a single generic argument, could you support multiple arguments as well? The current restriction still leaves similar ICEs for valid code such as:

fn f<T, const N: u8>() {
    f::<u8, { async || {} }>();
}

[Dnreikronos]

If there isn't a strong reason to restrict this to a single generic argument, could you support multiple arguments as well?

Yeah, no strong reason to keep it restricted to one arg, so I lifted that. Pushed in 1d2d34dfb06, tests in 2d620d4acc1.

Your exact repro is a regression test now and gives a clean expected u8, found {async closure} instead of the ICE:

fn f<T, const N: u8>() {
    f::<u8, { async || {} }>();
}

The recovery matches args positionally against the generic params and only bails when there are more args than can line up, so multiple args just work.

btw while I was in there I ran into one more flavor of the same bug: trailing type params with defaults also drop out of the turbofish, so fn f<const N: u8, T = ()>() called as f::<{ ... }>() still fed {type error} and then u8 for the same key and double-fed. folded that in too (swap the exact-count != for >) with its own test. fyi both only repro under -Zthreads=0.

imo positional matching is the right call here. once you drop the exact-count requirement, the omittable-param cases (synthetic impl Trait, elided lifetimes, defaults) all fall out for free. idk if you'd prefer defaults handled more explicitly, ltm if so.

verified locally: your repro plus the three existing tests are green, and the whole const-generics anon-const suite passes (840). no rush on my end, nothing asap.

(the async-closure-in-a-const trick is a weirdly clean way to trip this race irl, kinda enjoyed chasing it.)

[fmease]

cc @BoxyUwU

[Dnreikronos]

@rustbot ready

[rustbot]

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.

[BoxyUwU]

This makes sense to me 🤔 This is basically just reverting #141950 though I think?

@bors try @rust-timer queue

though I'm not sure what alternative there is if this is perf sensitive. we can't just move this logic into type checking of closure expressions 🤔

[rust-bors]

☀️ Try build successful (CI)
Build commit: 8d05ea8 (8d05ea813c6b5434ca0a42c4ed59359853e7c45f)
Base parent: a872286 (a872286d0a1873caec0291ed4304de3170adbe16)

[rust-timer]

Finished benchmarking commit (8d05ea8): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up.

@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This perf run didn't have relevant results for this metric.

Max RSS (memory usage)

Results (primary 5.0%, secondary -5.8%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	5.0%	[5.0%, 5.0%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-5.8%	[-5.8%, -5.8%]	1
All ❌✅ (primary)	5.0%	[5.0%, 5.0%]	1

Cycles

Results (secondary 1.4%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.7%	[2.4%, 4.9%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.0%	[-3.0%, -3.0%]	1
All ❌✅ (primary)	-	-	0

Binary size

Results (primary 0.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	0.1%	[0.1%, 0.1%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.1%	[0.1%, 0.1%]	1

Bootstrap: 468.499s -> 468.404s (-0.02%)
Artifact size: 400.07 MiB -> 400.88 MiB (0.20%)

[Dnreikronos]

This makes sense to me 🤔 This is basically just reverting #141950 though I think?

@bors try @rust-timer queue

though I'm not sure what alternative there is if this is perf sensitive. we can't just move this logic into type checking of closure expressions 🤔

sup, Boxy :)
yeah, for the scheduling part, pretty much. #141950 moved this work into the main body-owner loop. This puts it back in its own pass, though it still lives in check_crate instead of going back to run_required_analyses.

what bites us here is that the loop is parallel. Doing typeck and then the by-move check for one owner doesn't tell us whether its enclosing owner has finished. For the coroutine inside the const arg, needs_coroutine_by_move_body_def_id can ask for its type_of before the enclosing body has lowered that arg and fed the anon const type. That first lookup ends up caching the error type. Later, normal typeck reaches the const arg and feeds u8 for the same query, and that's the ICE.

I tried fixing it around HIR lowering first, but that turned into chasing each place a const arg can show up, and it duplicated some generic arg matching along the way. Didn't feel like the right layer. Putting the by-move work after body typeck gives us the ordering we need and handles those cases in one place.

I don't see a clean way to tie this to closure expression type checking either...the thing we need is for the enclosing body to have already lowered the const arg, and a closure-local hook doesn't give us that ordering. imo the second pass is a little annoying after #141950, but it's also the least surprising fix to me. perf came back with no relevant changes, so I'd keep it like this so far.

But if you really want me to change something ltm

[rust-timer]

Insufficient permissions to issue commands to rust-timer.

[Dnreikronos]

@rustbot ready

[BoxyUwU]

@bors r+

[rust-bors]

📌 Commit 023a327 has been approved by BoxyUwU

It is now in the queue for this repository.