Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Async closures are not allowed to reference all captured lifetimes if one of them is invariant #123241

Closed
tmandry opened this issue Mar 30, 2024 · 7 comments · Fixed by #123660
Closed
Labels
A-async-await Area: Async & Await A-closures Area: Closures (`|…| { … }`) AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. C-bug Category: This is a bug. F-async_closure `#![feature(async_closure)]` T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-async Working group: Async & await

Comments

@tmandry
Copy link
Member

tmandry commented Mar 30, 2024

Full example

async fn go<'a>(value: &'a i32) {
    let closure = async |scope: ScopeRef<'_, 'a>| {
        let _future1 = scope.spawn(async {
            let _v = *value;
        });
    };
}

yields

error: lifetime may not live long enough
  --> examples/repro.rs:52:63
   |
52 |       let closure = async |scope: ScopeRef<'_, 'a, i32>| -> i32 {
   |  ___________________-------------------------------------------_^
   | |                   |                      |
   | |                   |                      let's call the lifetime of this reference `'2`
   | |                   lifetime `'1` represents this closure's body
53 | |         let _future1 = scope.spawn(async { value });
54 | |         22
55 | |     };
   | |_____^ closure was supposed to return data with lifetime `'2` but it is returning data with lifetime `'1`
   |
   = note: closure implements `Fn`, so references to captured variables can't escape the closure
   = note: requirement occurs because of the type `Scope<'_, '_, i32>`, which makes the generic argument `'_` invariant
   = note: the struct `Scope<'scope, 'env, R>` is invariant over the parameter `'scope`

closure was supposed to return data with lifetime '2 ('scope) but it is returning data with lifetime '1 (closure body)

It's reasonable that we would create a lifetime representing the closure body, but naturally, the future should be allowed to reference it. The invariant lifetime must be mucking things up somehow.

(spawn has no lifetime params so I would not expect there to be a new lifetime created when it is called.)

note: closure implements Fn, so references to captured variables can't escape the closure

The closure definitely shouldn't try to implement Fn in this case, I don't know why it does.

note: requirement occurs because of the type Scope<'_, '_, i32>, which makes the generic argument '_ invariant
note: the struct Scope<'scope, 'env, R> is invariant over the parameter 'scope

It's correct that this lifetime is invariant. We must write to a vec of futures that outlive 'scope, so any variance would be unsound.

cc @compiler-errors

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Mar 30, 2024
@compiler-errors
Copy link
Member

@tmandry: Several things here:

Firstly, regarding this message:

The closure definitely shouldn't try to implement Fn in this case, I don't know why it does.

It's just that the borrowck diagnostics code isn't yet modified to say async Fn for async closure borrow errors; just a diagnostics issue. Don't worry too much about it, since it should be fixed separately.

Secondly, I think this all might just be a red herring with the lifetime invariance. The "which makes the generic argument '_ invariant" message is just a side-effect of some other error reporting and doesn't actually have to do with the issue here, probably?

So the real problem here is that the future that you pass into scope.spawn() actually borrows the captured value reference from the async closure, explaining the "but it is returning data with lifetime '1 (closure body)" part of the error message.

This can be fixed by passing an async move instead.

The fact that Rust chooses to borrow rather than copy the captured value (ref) into the inner async block is a consequence of some closure capture rules that I've been interested in investigating further, but I haven't really spent the time wrapping my head around them totally.

My vibe is that the preference to borrow rather than copy/move args can allow more code to compile in some cases, but obviously this may cause other problems (e.g. this issue).

This also probably has something to do with how the first example, scope_with_box, ends up implicitly copying the value ref rather than reborrowing it from the outer closure. Specifically, I'm actually surprised that the scope_with_box example doesn't end up giving a similar error about returning data referencing the captures from the outer closure.

@fmease fmease added A-closures Area: Closures (`|…| { … }`) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. C-bug Category: This is a bug. A-async-await Area: Async & Await A-variance Area: Variance (https://doc.rust-lang.org/nomicon/subtyping.html) F-async_closure `#![feature(async_closure)]` and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. A-variance Area: Variance (https://doc.rust-lang.org/nomicon/subtyping.html) labels Mar 30, 2024
@tmandry
Copy link
Member Author

tmandry commented Mar 31, 2024

Thanks for your support @compiler-errors! Five stars would minimize again.

I'm also surprised by the difference from scope_with_box, and am interested if it points to something clever we could do for the closure case. From a lang perspective I'd really like it if we could make the original code compile. Using move to mean "copy my reference instead of borrowing it" is really quite subtle and not the meaning most people would associate with that keyword.

That said, in this particular API I think you always want move.

@compiler-errors
Copy link
Member

@tmandry: I think my next step will be to understand the box case. I'm not certain if it's worth thinking about what to change until we can completely understand what's going on here.

I do agree that the code going from fail->pass with the addition of a move kw is really unnecessarily subtle, tho.

Regarding diagnostics, I don't know if there's a way to get borrowck to understand how to suggest this change, though, given that it's pretty far along past closure capture analysis.

And the same question for making closure capture analysis more sophisticated too. Again--will probably need to understand exactly what is happening better.

@tmandry
Copy link
Member Author

tmandry commented Mar 31, 2024

I think I found an important difference: In the box case the future is bound to outlive 'scope (by the definition of BoxFuture), but not in the closure case. This is an important bound because we need to be able to poll the future for as long as 'scope is live.

Adding such a bound (see line 31) fails compilation with a couple of 'static obligations, for some reason.

@compiler-errors
Copy link
Member

compiler-errors commented Mar 31, 2024

Well partly this is because we don't need to prove that outlives bound for all 'scope, but just for all 'scope shorter than 'env. We don't have conditional higher ranked lifetime bounds, yet.

The type erasure of dyn probably side-steps this problem for the box case, so we're not accidentally requiring for<'scope> 'env: 'scope to hold by accident.

@traviscross
Copy link
Contributor

@rustbot labels +WG-async +AsyncAwait-Triaged

We discussed this in the async triage call and, based on the discussion here in this thread, this is definitely triaged, and it seems that more investigation is needed.

@rustbot rustbot added AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. WG-async Working group: Async & await labels Apr 8, 2024
@compiler-errors
Copy link
Member

Investigated this. The minimal version of this is:

#![feature(async_closure)]

fn outlives<'a, T: 'a>(_: T) {}

fn hello<'a>(x: &'a i32) {
    let c = async || {
        outlives::<'a>(async {
            let y = *x;
        });
    };
}

And it should be fixed by #123660.

@bors bors closed this as completed in ee73660 Apr 11, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Apr 11, 2024
Rollup merge of rust-lang#123660 - compiler-errors:coroutine-closure-env, r=oli-obk

Make the computation of `coroutine_captures_by_ref_ty` more sophisticated

Currently, we treat all the by-(mut/)ref borrows of a coroutine-closure as having a "closure env" borrowed lifetime.

When we have the given code:
```rust
let x: &'a i32 = ...;
let c = async || {
    let _x = *x;
};
```

Then when we call:
```rust
c()
// which, because `AsyncFn` takes a `&self`, we insert an autoref:
(&c /* &'env {coroutine-closure} */)()
```

We will return a future whose captures contain `&'env i32` instead of `&'a i32`, which is way more restrictive than necessary. We should be able to drop `c` while the future is alive since it's not actually borrowing any data *originating from within* the closure's captures, but since the capture has that `'env` lifetime, this is not possible.

This wouldn't be true, for example, if the closure captured `i32` instead of `&'a i32`, because the `'env` lifetime is actually *necessary* since the data (`i32`) is owned by the closure.

This PR identifies two criteria where we *need* to take the borrow with the closure env lifetime:
1. If the closure borrows data from inside the closure's captures. This is not true if the parent capture is by-ref, OR if the parent capture is by-move and the child capture begins with a deref projection. This is the example described above.
2. If we're dealing with mutable references, since we cannot reborrow `&'env mut &'a mut i32` into `&'a mut i32`, *only* `&'env mut i32`.

See the documentation on `should_reborrow_from_env_of_parent_coroutine_closure` for more info.

**important:** As disclaimer states on that function, luckily, if this heuristic is not correct, then the program is not unsound, since we still borrowck and validate the choices made from this function -- the only side-effect is that the user may receive unnecessary borrowck errors.

Fixes rust-lang#123241
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-async-await Area: Async & Await A-closures Area: Closures (`|…| { … }`) AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. C-bug Category: This is a bug. F-async_closure `#![feature(async_closure)]` T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-async Working group: Async & await
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants