Remove `identity_future` indirection #104833

Swatinem · 2022-11-24T17:08:42Z

This was previously needed because the indirection used to hide some unexplained lifetime errors, which it turned out were related to the min_choice algorithm.

Removing the indirection also solves a couple of cycle errors, large moves and makes async blocks support the #[track_caller]annotation.

Fixes #104826.

rustbot · 2022-11-24T17:08:50Z

r? @lcnr

(rustbot has picked a reviewer for you, use r? to override)

bors · 2022-11-27T08:06:05Z

☔ The latest upstream changes (presumably #96451) made this pull request unmergeable. Please resolve the merge conflicts.

Swatinem · 2022-12-08T18:22:39Z

Fun fact, now that I changed from ResumeTy to &'static mut Context<'_> in #105250, the identity_future indirection seems to have been hiding lifetime errors related to the inferred lifetime of Context<'_> which is also considered to be captured. So I had to manually bend that to be a 'static as well.

Though the three failing tests are still there. Though src/test/ui/impl-trait/in-trait/default-body-with-rpit.rs has changed a bit now. The impl Debug can now be correctly inferred with recent improvements to rpitit, but it is now complaining about a hidden lifetime for &'static str. It can somehow not infer that "" is indeed a &'static str?

bors · 2022-12-14T02:02:10Z

☔ The latest upstream changes (presumably #105667) made this pull request unmergeable. Please resolve the merge conflicts.

bors · 2022-12-20T04:40:15Z

☔ The latest upstream changes (presumably #105918) made this pull request unmergeable. Please resolve the merge conflicts.

Swatinem · 2023-01-31T12:26:41Z

The issue with HirId is an interesting one.
I believe the unused HirId is the one being allocated at the top of lower_expr_mut for the outer Expr.
For async move {}, I end up with something like Expr(Expr { id: NodeId(28), kind: Async(Value, NodeId(29), ....
NodeId(28) is the one it is complaining about it seems. I’m now early-returning in lower_expr_mut because make_async_expr returns an Expr directly instead of an ExprKind.

I could just return the ExprKind::Closure directly, but then the compiler ICEs because I fail to lower one of the NodeIds completely.

I believe the problem is that I have two NodeId that I have to deal with, one for the ast::Expr itself, and one for the inner ast::ExprKind::Async.
Previously, the identity_future() call got a HirId corresponding to the ast::Expr, and the generator got one corresponding to the ast::ExprKind::Async.
But now the call does not exist anymore, and one of the Ids gets unused.

I will have to read up on how all these things interact with each other. If someone who knows these things better than me knows a quick solution, feel free to educate me :-)

compiler/rustc_ast_lowering/src/expr.rs

bors · 2023-01-31T17:56:15Z

☔ The latest upstream changes (presumably #106399) made this pull request unmergeable. Please resolve the merge conflicts.

Swatinem · 2023-01-31T21:24:58Z

In the meantime while I still struggle to properly understand HIR lowering…

@nbdd0121 as you added #[track_caller]: Is the intention that it works on async blocks or not?
Because now it does, as async blocks are (generator) closures, and the tests/ui/async-await/track-caller/async-block.rs test does not error anymore:

    let _ = #[track_caller] async {
        //~^ ERROR attribute should be applied to a function definition [E0739]
    };

Swatinem · 2023-02-01T09:55:54Z

After a short detour messing with HIR and Ids, we are back to the known issues:

    [ui] tests/ui/impl-trait/in-trait/default-body-with-rpit.rs
    [ui] tests/ui/regions/issue-72051-member-region-hang.rs
    [ui] tests/ui/self/self_lifetime-async.rs

bors · 2023-03-14T10:13:02Z

⌛ Testing commit 9f03cfc with merge 669e751...

bors · 2023-03-14T13:45:01Z

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing 669e751 to master...

rust-timer · 2023-03-14T15:30:27Z

Finished benchmarking commit (669e751): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.5%	[-0.5%, -0.5%]	1
Improvements ✅ (secondary)	-3.9%	[-14.3%, -0.9%]	19
All ❌✅ (primary)	-0.5%	[-0.5%, -0.5%]	1

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.0%	[1.1%, 4.0%]	5
Improvements ✅ (primary)	-3.2%	[-3.2%, -3.2%]	1
Improvements ✅ (secondary)	-3.1%	[-4.4%, -2.0%]	4
All ❌✅ (primary)	-3.2%	[-3.2%, -3.2%]	1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.1%	[3.1%, 3.1%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-4.6%	[-14.6%, -1.9%]	17
All ❌✅ (primary)	-	-	0

…mpiler-errors Remove `identity_future` indirection This was previously needed because the indirection used to hide some unexplained lifetime errors, which it turned out were related to the `min_choice` algorithm. Removing the indirection also solves a couple of cycle errors, large moves and makes async blocks support the `#[track_caller]`annotation. Fixes rust-lang#104826.

…errors Remove the `NodeId` of `ast::ExprKind::Async` This is a followup to rust-lang#104833 (review). In my original attempt, I was using `LoweringContext::expr`, which was not correct as it creates a fresh `DefId`. It now uses the correct `DefId` for the wrapping `Expr`, and also makes forwarding `#[track_caller]` attributes more explicit.

Remove the `NodeId` of `ast::ExprKind::Async` This is a followup to rust-lang/rust#104833 (review). In my original attempt, I was using `LoweringContext::expr`, which was not correct as it creates a fresh `DefId`. It now uses the correct `DefId` for the wrapping `Expr`, and also makes forwarding `#[track_caller]` attributes more explicit.

This function/lang_item was introduced in rust-lang#104321 as a temporary workaround of future lowering. The usage and need for it went away in rust-lang#104833. After a bootstrap update, the function itself can be removed from `std`.

…rk-Simulacrum Remove `identity_future` from stdlib This function/lang_item was introduced in rust-lang#104321 as a temporary workaround of future lowering. The usage and need for it went away in rust-lang#104833. After a bootstrap update, the function itself can be removed from `std`.

This function/lang_item was introduced in rust-lang#104321 as a temporary workaround of future lowering. The usage and need for it went away in rust-lang#104833. After a bootstrap update, the function itself can be removed from `std`.

Remove the `NodeId` of `ast::ExprKind::Async` This is a followup to rust-lang/rust#104833 (review). In my original attempt, I was using `LoweringContext::expr`, which was not correct as it creates a fresh `DefId`. It now uses the correct `DefId` for the wrapping `Expr`, and also makes forwarding `#[track_caller]` attributes more explicit.

…th-late-bound-vars-again, r=jackh726 Normalize opaques with late-bound vars again We have a hack in the compiler where if an opaque has escaping late-bound vars, we skip revealing it even though we *could* reveal it from a technical perspective. First of all, this is weird, since we really should be revealing all opaques in `Reveal::All` mode. Second of all, it causes subtle bugs (linked below). I attempted to fix this in rust-lang#100980, which was unfortunately reverted due to perf regressions on codebases that used really deeply nested futures in some interesting ways. The worst of which was rust-lang#103423, which caused the project to hang on build. Another one was rust-lang#104842, which was just a slow-down, but not a hang. I took some time afterwards to investigate how to rework `normalize_erasing_regions` to take advantage of better caching, but that effort kinda fizzled out (rust-lang#104133). However, recently, I was made aware of more bugs whose root cause is not revealing opaques during codegen. That made me want to fix this again -- in the process, interestingly, I took the the minimized example from rust-lang#103423 (comment), and it doesn't seem to hang any more... Thinking about this harder, there have been some changes to the way we lower and typecheck async futures that may have reduced the pathologically large number of outlives obligations (see description of rust-lang#103423) that we were encountering when normalizing opaques with bound vars the last time around: * rust-lang#104321 (lower `async { .. }` directly as a generator that implements `Future`, removing the `from_generator` shim) * rust-lang#104833 (removing an `identity_future` fn that was wrapping desugared future generators) ... so given that I can see: * No significant regression on rust perf bot (rust-lang#107620 (comment)) * No timeouts in crater run I did (rust-lang#107620 (comment), rechecked failing crates in rust-lang#107620 (comment)) ... and given that this PR: * Fixes rust-lang#104601 * Fixes rust-lang#107557 * Fixes rust-lang#109464 * Allows us to remove a `DefiningAnchor::Bubble` from codegen (75a8f68) I'm inclined to give this another shot at landing this. Best case, it just works -- worst case, we get more examples to study how we need to improve the compiler to make this work. r? types

rustbot assigned lcnr Nov 24, 2022

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. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Nov 24, 2022