Skip to content

async move closure owning Fn and calling it #128516

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
ChayimFriedman2 opened this issue Aug 1, 2024 · 1 comment · Fixed by #128520
Closed

async move closure owning Fn and calling it #128516

ChayimFriedman2 opened this issue Aug 1, 2024 · 1 comment · Fixed by #128520
Assignees
Labels
A-async-closures `async || {}` C-bug Category: This is a bug.

Comments

@ChayimFriedman2
Copy link
Contributor

I tried this code (https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=1062a9d9bd18705abc0f2109c7df7ef3):

#![feature(async_closure)]

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

I expected to see this happen: this compiles successfully. In my understanding of async closures, the closure should own f but its returned future should borrow it from the closure, leading to no error. Furthermore, if all of returned closure's arguments are Copy, this compiles successfully - I don't think the arguments' types should change anything.

Instead, this happened: it errors with:

error[E0507]: cannot move out of `f` which is behind a shared reference
 --> src/lib.rs:4:5
  |
4 |     async move |_| f()
  |     ^^^^^^^^^^^^^^ -
  |     |              |
  |     |              variable moved due to use in coroutine
  |     |              move occurs because `f` has type `impl Fn()`, which does not implement the `Copy` trait
  |     `f` is moved here
  |
help: if `impl Fn()` implemented `Clone`, you could clone the value
 --> src/lib.rs:3:15
  |
3 | fn wrapper(f: impl Fn()) -> impl async Fn(String) {
  |               ^^^^^^^^^ consider constraining this type parameter with `Clone`
4 |     async move |_| f()
  |                    - you could clone this value

For more information about this error, try `rustc --explain E0507`.

@rustbot label +F-async_closure

@ChayimFriedman2 ChayimFriedman2 added the C-bug Category: This is a bug. label Aug 1, 2024
@rustbot rustbot added needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. A-async-closures `async || {}` labels Aug 1, 2024
@compiler-errors compiler-errors self-assigned this Aug 1, 2024
@compiler-errors
Copy link
Member

I'll look into it. I agree that something particularly weird is going on here -- each future returned by calling the async Fn should borrow the Fn to do the call.

@saethlin saethlin removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Aug 4, 2024
@bors bors closed this as completed in 2a177c2 Aug 8, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Aug 8, 2024
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 :)
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
A-async-closures `async || {}` C-bug Category: This is a bug.
Projects
None yet
4 participants