Rework support for async closures; allow them to return futures that borrow from the closure's captures #120361

compiler-errors · 2024-01-26T01:20:39Z

This PR implements a new lowering for async closures via TyKind::CoroutineClosure which handles the curious relationship between the closure and the coroutine that it returns.

I wrote up a bunch in this hackmd which will be copied to the dev guide after this PR lands, and hopefully left sufficient comments in the source code explaining why this change is as large as it is.

This also necessitates that they begin implementing the AsyncFn-family of traits, rather than the Fn-family of traits -- if you need Fn implementations, you should probably use the non-sugar || async {} syntax instead.

Notably this PR does not yet implement async Fn() syntax sugar for bounds, but I expect to add those soon (edit: #120392). For now, users must use AsyncFn() traits directly, which necessitates adding the async_fn_traits feature gate as well. I will add this as a follow-up very soon.

r? oli-obk

This is based on top of #120322, but that PR is minimal.

rustbot · 2024-01-26T04:45:39Z

Type relation code was changed

cc @compiler-errors, @lcnr

Some changes occurred in need_type_info.rs

cc @lcnr

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes might have occurred in exhaustiveness checking

cc @Nadrieril

This PR changes MIR

cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @celinval, @vakaras

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

bors · 2024-01-26T06:12:58Z

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

RalfJung · 2024-01-26T07:07:18Z

This also necessitates that they begin implementing the AsyncFn-family of traits, rather than the Fn-family of traits -- if you need Fn implementations, you should probably use the non-sugar || async {} syntax instead.

Is there an issue tracking "closures should be able to return a value borrowed from their captures"?

compiler-errors · 2024-01-26T14:25:05Z

@RalfJung: Can you explain what you mean? Not really sure what you're asking for.

edit: Maybe you meant why I added the AsyncFn* family of traits? If so, see #119305. I can add elaborate if you'd like.

RalfJung · 2024-01-26T15:53:13Z

Can you explain what you mean? Not really sure what you're asking for.

There was discussion $somewhere recently that the Output type of Fn and FnMut should have been generic over the lifetime of the borrow of the closure context. If that was the case then I think we wouldn't need these AsyncFn traits?

I see #119305 discusses this to some extent. I just wonder if we should track somewhere the idea of having LendingFn* eventually (or whether it is even possible to make the existing traits lending).

oli-obk

did a commit-by-commit review, I'll do another review of the entire diff in one go, too

compiler/rustc_hir_typeck/src/upvar.rs

oli-obk · 2024-01-29T14:10:05Z

compiler/rustc_mir_transform/src/shim.rs

add a mir-opt test showing this shim's MIR

compiler/rustc_mir_transform/src/coroutine/by_move_body.rs

compiler/rustc_mir_transform/src/lib.rs

compiler/rustc_borrowck/src/type_check/input_output.rs

compiler-errors · 2024-01-29T17:11:13Z

@bors try

bors · 2024-02-06T15:04:05Z

⌛ Testing commit ed7fca1 with merge 4a2fe44...

bors · 2024-02-06T17:18:08Z

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 4a2fe44 to master...

rust-timer · 2024-02-06T18:32:50Z

Finished benchmarking commit (4a2fe44): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

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.5%	[0.2%, 0.7%]	11
Regressions ❌ (secondary)	0.6%	[0.4%, 0.7%]	11
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.2%	[-0.2%, -0.2%]	1
All ❌✅ (primary)	0.5%	[0.2%, 0.7%]	11

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)	1.2%	[1.2%, 1.3%]	2
Regressions ❌ (secondary)	8.2%	[5.9%, 10.5%]	2
Improvements ✅ (primary)	-3.2%	[-3.2%, -3.2%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.2%	[-3.2%, 1.3%]	3

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)	2.7%	[2.1%, 3.4%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.1%	[-2.1%, -2.1%]	1
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 661.097s -> 662.774s (0.25%)
Artifact size: 308.13 MiB -> 308.21 MiB (0.03%)

… r=oli-obk Print kind of coroutine closure Make sure that we print "async closure" when we have an async closure, rather than calling it generically a ["coroutine-closure"](rust-lang#120361). Fixes rust-lang#120886 r? oli-obk

Rollup merge of rust-lang#120896 - compiler-errors:coro-closure-kind, r=oli-obk Print kind of coroutine closure Make sure that we print "async closure" when we have an async closure, rather than calling it generically a ["coroutine-closure"](rust-lang#120361). Fixes rust-lang#120886 r? oli-obk

Print kind of coroutine closure Make sure that we print "async closure" when we have an async closure, rather than calling it generically a ["coroutine-closure"](rust-lang/rust#120361). Fixes #120886 r? oli-obk

Kobzol · 2024-02-13T07:35:53Z

Probably just codegen noise, the changes were reverted in followup commits.

@rustbot label: +perf-regression-triaged

… r=oli-obk An async closure may implement `FnMut`/`Fn` if it has no self-borrows There's no reason that async closures may not implement `FnMut` or `Fn` if they don't actually borrow anything with the closure's env lifetime. Specifically, rust-lang#123660 made it so that we don't always need to borrow captures from the closure's env. See the doc comment on `should_reborrow_from_env_of_parent_coroutine_closure`: https://github.com/rust-lang/rust/blob/c00957a3e269219413041a4e3565f33b1f9d0779/compiler/rustc_hir_typeck/src/upvar.rs#L1777-L1823 If there are no such borrows, then we are free to implement `FnMut` and `Fn` as permitted by our closure's inferred `ClosureKind`. As far as I can tell, this change makes `async || {}` work in precisely the set of places they used to work before rust-lang#120361. Fixes rust-lang#125247. r? oli-obk

Rollup merge of rust-lang#125259 - compiler-errors:fn-mut-as-a-treat, r=oli-obk An async closure may implement `FnMut`/`Fn` if it has no self-borrows There's no reason that async closures may not implement `FnMut` or `Fn` if they don't actually borrow anything with the closure's env lifetime. Specifically, rust-lang#123660 made it so that we don't always need to borrow captures from the closure's env. See the doc comment on `should_reborrow_from_env_of_parent_coroutine_closure`: https://github.com/rust-lang/rust/blob/c00957a3e269219413041a4e3565f33b1f9d0779/compiler/rustc_hir_typeck/src/upvar.rs#L1777-L1823 If there are no such borrows, then we are free to implement `FnMut` and `Fn` as permitted by our closure's inferred `ClosureKind`. As far as I can tell, this change makes `async || {}` work in precisely the set of places they used to work before rust-lang#120361. Fixes rust-lang#125247. r? oli-obk

rustbot assigned oli-obk Jan 26, 2024