Skip over args when determining if async-closure's inner coroutine consumes its upvars #128520

compiler-errors · 2024-08-01T22:46:48Z

#125306 implements a strategy for when we have an async move || async-closure that is inferred to be async FnOnce, it will force the inner coroutine to also be move, since we cannot borrow any upvars from the parent async-closure (since FnOnce is not lending):

rust/compiler/rustc_hir_typeck/src/upvar.rs

Lines 211 to 229 in 8e86c95

    
           // There are several curious situations with coroutine-closures where 
        
           // analysis is too aggressive with borrows when the coroutine-closure is 
        
           // marked `move`. Specifically: 
        
           // 
        
           // 1. If the coroutine-closure was inferred to be `FnOnce` during signature 
        
           // inference, then it's still possible that we try to borrow upvars from 
        
           // the coroutine-closure because they are not used by the coroutine body 
        
           // in a way that forces a move. See the test: 
        
           // `async-await/async-closures/force-move-due-to-inferred-kind.rs`. 
        
           // 
        
           // 2. If the coroutine-closure is forced to be `FnOnce` due to the way it 
        
           // uses its upvars, but not *all* upvars would force the closure to `FnOnce`. 
        
           // See the test: `async-await/async-closures/force-move-due-to-actually-fnonce.rs`. 
        
           // 
        
           // This would lead to an impossible to satisfy situation, since `AsyncFnOnce` 
        
           // coroutine bodies can't borrow from their parent closure. To fix this, 
        
           // we force the inner coroutine to also be `move`. This only matters for 
        
           // coroutine-closures that are `move` since otherwise they themselves will 
        
           // be borrowing from the outer environment, so there's no self-borrows occuring.

However, when this strategy was implemented, it reused the ExprUseVisitor data from visiting the whole coroutine, which includes additional statements due to async-specific argument desugaring:

rust/compiler/rustc_ast_lowering/src/item.rs

Lines 1197 to 1228 in 8e86c95

    
           // Async function parameters are lowered into the closure body so that they are 
        
           // captured and so that the drop order matches the equivalent non-async functions. 
        
           // 
        
           // from: 
        
           // 
        
           //     async fn foo(<pattern>: <ty>, <pattern>: <ty>, <pattern>: <ty>) { 
        
           //         <body> 
        
           //     } 
        
           // 
        
           // into: 
        
           // 
        
           //     fn foo(__arg0: <ty>, __arg1: <ty>, __arg2: <ty>) { 
        
           //       async move { 
        
           //         let __arg2 = __arg2; 
        
           //         let <pattern> = __arg2; 
        
           //         let __arg1 = __arg1; 
        
           //         let <pattern> = __arg1; 
        
           //         let __arg0 = __arg0; 
        
           //         let <pattern> = __arg0; 
        
           //         drop-temps { <body> } // see comments later in fn for details 
        
           //       } 
        
           //     } 
        
           // 
        
           // If `<pattern>` is a simple ident, then it is lowered to a single 
        
           // `let <pattern> = <pattern>;` statement as an optimization. 
        
           // 
        
           // Note that the body is embedded in `drop-temps`; an 
        
           // equivalent desugaring would be `return { <body> 
        
           // };`. The key point is that we wish to drop all the 
        
           // let-bound variables and temporaries created in the body 
        
           // (and its tail expression!) before we drop the 
        
           // parameters (c.f. rust-lang/rust#64512).

Well, it turns out that we don't care about these argument desugaring parameters, because arguments to the async-closure are not the async-closure's captures -- they exist for only one invocation of the closure, and they're always consumed by construction (see the argument desugaring above), so they will force the coroutine's inferred kind to FnOnce. (Unless they're Copy, because we never consider Copy types to be consumed):

rust/compiler/rustc_hir_typeck/src/expr_use_visitor.rs

Lines 60 to 66 in 8e86c95

    
           /// The value found at `place` is being copied. 
        
           /// `diag_expr_id` is the id used for diagnostics (see `consume` for more details). 
        
           fn copy(&mut self, place_with_id: &PlaceWithHirId<'tcx>, diag_expr_id: HirId) { 
        
               // In most cases, copying data from `x` is equivalent to doing `*&x`, so by default 
        
               // we treat a copy of `x` as a borrow of `x`. 
        
               self.borrow(place_with_id, diag_expr_id, ty::BorrowKind::ImmBorrow) 
        
           }

However, since we were visiting these arg exprs, this resulted in us too-aggressively applying move to the inner coroutine, resulting in regressions. For example, this PR fixes #128516. Fascinatingly, the note above about how we never consume Copy types is why this only regressed when the argument types weren't all Copy.

I tried to leave some comments inline to make this more clear :)

…e consumes its upvars

rustbot · 2024-08-01T22:46:56Z

r? @BoxyUwU

rustbot has assigned @BoxyUwU.
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

BoxyUwU · 2024-08-08T00:45:16Z

tests/ui/async-await/async-closures/non-copy-arg-does-not-force-inner-move.rs

+fn wrapper(f: impl Fn(String)) -> impl async Fn(String) {
+    async move |s| f(s)
+}


So, the closure captures f by value, and then the future captures the upvar by ref?

BoxyUwU · 2024-08-08T00:45:50Z

r=me unless I'm wrong about why that code compiles lol

BoxyUwU · 2024-08-08T00:49:09Z

@bors r+ rollup

bors · 2024-08-08T00:49:12Z

📌 Commit 5138586 has been approved by BoxyUwU

It is now in the queue for this repository.

Rollup of 7 pull requests Successful merges: - rust-lang#128520 (Skip over args when determining if async-closure's inner coroutine consumes its upvars) - rust-lang#128552 (Emit an error for invalid use of the `#[no_sanitize]` attribute) - rust-lang#128691 (Update `compiler-builtins` to 0.1.117) - rust-lang#128702 (Add -Zmetrics-dir=PATH to save diagnostic metadata to disk) - rust-lang#128797 (Fuchsia Test Runner: enable ffx repository server) - rust-lang#128798 (refactor(rustc_expand::mbe): Don't require full ExtCtxt when not necessary) - rust-lang#128800 (Add tracking issue to core-pattern-type) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#128520 - compiler-errors:more-precisely-force-move, r=BoxyUwU Skip over args when determining if async-closure's inner coroutine consumes its upvars rust-lang#125306 implements a strategy for when we have an `async move ||` async-closure that is inferred to be `async FnOnce`, it will force the inner coroutine to also be `move`, since we cannot borrow any upvars from the parent async-closure (since `FnOnce` is not lending): https://github.com/rust-lang/rust/blob/8e86c9567154dc5a9ada15ab196d23eae2bd7d89/compiler/rustc_hir_typeck/src/upvar.rs#L211-L229 However, when this strategy was implemented, it reused the `ExprUseVisitor` data from visiting the whole coroutine, which includes additional statements due to `async`-specific argument desugaring: https://github.com/rust-lang/rust/blob/8e86c9567154dc5a9ada15ab196d23eae2bd7d89/compiler/rustc_ast_lowering/src/item.rs#L1197-L1228 Well, it turns out that we don't care about these argument desugaring parameters, because arguments to the async-closure are not the *async-closure*'s captures -- they exist for only one invocation of the closure, and they're always consumed by construction (see the argument desugaring above), so they will force the coroutine's inferred kind to `FnOnce`. (Unless they're `Copy`, because we never consider `Copy` types to be consumed): https://github.com/rust-lang/rust/blob/8e86c9567154dc5a9ada15ab196d23eae2bd7d89/compiler/rustc_hir_typeck/src/expr_use_visitor.rs#L60-L66 However, since we *were* visiting these arg exprs, this resulted in us too-aggressively applying `move` to the inner coroutine, resulting in regressions. For example, this PR fixes rust-lang#128516. Fascinatingly, the note above about how we never consume `Copy` types is why this only regressed when the argument types weren't all `Copy`. I tried to leave some comments inline to make this more clear :)

Skip over args when determining if coroutine-closure's inner coroutin…

5138586

…e consumes its upvars

rustbot assigned BoxyUwU Aug 1, 2024

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. labels Aug 1, 2024

compiler-errors changed the title ~~Skip over args when determining if coroutine-closure's inner coroutine consumes its upvars~~ Skip over args when determining if async-closure's inner coroutine consumes its upvars Aug 1, 2024

BoxyUwU reviewed Aug 8, 2024

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 8, 2024

tgross35 mentioned this pull request Aug 8, 2024

Rollup of 7 pull requests #128805

Merged

bors merged commit 2a177c2 into rust-lang:master Aug 8, 2024
6 checks passed

rustbot added this to the 1.82.0 milestone Aug 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Skip over args when determining if async-closure's inner coroutine consumes its upvars #128520

Skip over args when determining if async-closure's inner coroutine consumes its upvars #128520

compiler-errors commented Aug 1, 2024 •

edited

Loading

rustbot commented Aug 1, 2024

BoxyUwU Aug 8, 2024

compiler-errors Aug 8, 2024

BoxyUwU commented Aug 8, 2024

BoxyUwU commented Aug 8, 2024

bors commented Aug 8, 2024

	// There are several curious situations with coroutine-closures where
	// analysis is too aggressive with borrows when the coroutine-closure is
	// marked `move`. Specifically:
	//
	// 1. If the coroutine-closure was inferred to be `FnOnce` during signature
	// inference, then it's still possible that we try to borrow upvars from
	// the coroutine-closure because they are not used by the coroutine body
	// in a way that forces a move. See the test:
	// `async-await/async-closures/force-move-due-to-inferred-kind.rs`.
	//
	// 2. If the coroutine-closure is forced to be `FnOnce` due to the way it
	// uses its upvars, but not all upvars would force the closure to `FnOnce`.
	// See the test: `async-await/async-closures/force-move-due-to-actually-fnonce.rs`.
	//
	// This would lead to an impossible to satisfy situation, since `AsyncFnOnce`
	// coroutine bodies can't borrow from their parent closure. To fix this,
	// we force the inner coroutine to also be `move`. This only matters for
	// coroutine-closures that are `move` since otherwise they themselves will
	// be borrowing from the outer environment, so there's no self-borrows occuring.

	// Async function parameters are lowered into the closure body so that they are
	// captured and so that the drop order matches the equivalent non-async functions.
	//
	// from:
	//
	// async fn foo(<pattern>: <ty>, <pattern>: <ty>, <pattern>: <ty>) {
	// <body>
	// }
	//
	// into:
	//
	// fn foo(__arg0: <ty>, __arg1: <ty>, __arg2: <ty>) {
	// async move {
	// let __arg2 = __arg2;
	// let <pattern> = __arg2;
	// let __arg1 = __arg1;
	// let <pattern> = __arg1;
	// let __arg0 = __arg0;
	// let <pattern> = __arg0;
	// drop-temps { <body> } // see comments later in fn for details
	// }
	// }
	//
	// If `<pattern>` is a simple ident, then it is lowered to a single
	// `let <pattern> = <pattern>;` statement as an optimization.
	//
	// Note that the body is embedded in `drop-temps`; an
	// equivalent desugaring would be `return { <body>
	// };`. The key point is that we wish to drop all the
	// let-bound variables and temporaries created in the body
	// (and its tail expression!) before we drop the
	// parameters (c.f. rust-lang/rust#64512).

	/// The value found at `place` is being copied.
	/// `diag_expr_id` is the id used for diagnostics (see `consume` for more details).
	fn copy(&mut self, place_with_id: &PlaceWithHirId<'tcx>, diag_expr_id: HirId) {
	// In most cases, copying data from `x` is equivalent to doing `*&x`, so by default
	// we treat a copy of `x` as a borrow of `x`.
	self.borrow(place_with_id, diag_expr_id, ty::BorrowKind::ImmBorrow)
	}

Skip over args when determining if async-closure's inner coroutine consumes its upvars #128520

Skip over args when determining if async-closure's inner coroutine consumes its upvars #128520

Conversation

compiler-errors commented Aug 1, 2024 • edited Loading

rustbot commented Aug 1, 2024

BoxyUwU Aug 8, 2024

Choose a reason for hiding this comment

compiler-errors Aug 8, 2024

Choose a reason for hiding this comment

BoxyUwU commented Aug 8, 2024

BoxyUwU commented Aug 8, 2024

bors commented Aug 8, 2024

compiler-errors commented Aug 1, 2024 •

edited

Loading