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

Variables not always moved into async move block #78633

Open
Ploppz opened this issue Nov 1, 2020 · 10 comments
Open

Variables not always moved into async move block #78633

Ploppz opened this issue Nov 1, 2020 · 10 comments
Labels
A-async-await Area: Async & Await A-borrow-checker Area: The borrow checker A-closures Area: Closures (`|…| { … }`) AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Ploppz
Copy link

Ploppz commented Nov 1, 2020

Playground:

use std::cell::UnsafeCell;
use std::sync::{Arc, Mutex};

#[tokio::main]
async fn main() {
    let arena = 0;
    let mut track = Vec::new();
    async move { track.push(&arena) }.await;
}

rustc v1.47.0 stable

Error:

error[E0597]: `arena` does not live long enough
 --> src/main.rs:8:29
  |
7 |     let mut track = Vec::new();
  |         --------- lifetime `'1` appears in the type of `track`
8 |     async move { track.push(&arena) }.await;
  |                  -----------^^^^^^- - `arena` dropped here while still borrowed
  |                  |          |
  |                  |          borrowed value does not live long enough
  |                  argument requires that `arena` is borrowed for `'1`

error: aborting due to previous error; 2 warnings emitted

Which is because track is captured by reference and not by value, while arena is captured by value.

By submitting this issue, I assume that the intended semantics for move is:

move value(s) into the closure/block, even if the closure/block only seems to need to store reference(s)

which would make this a bug.

@Ploppz Ploppz added the C-bug Category: This is a bug. label Nov 1, 2020
@Ploppz Ploppz changed the title Variables not always into async move block Variables not always moved into async move block Nov 1, 2020
@jonas-schievink jonas-schievink added A-async-await Area: Async & Await T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 1, 2020
@jyn514
Copy link
Member

jyn514 commented Nov 1, 2020

Slightly minimized (playground):

async fn f() {
    let arena = 0;
    async move { &arena }.await;
}
error[E0515]: cannot return reference to local data `arena`
 --> src/lib.rs:5:18
  |
5 |     async move { &arena }.await;
  |                  ^^^^^^ returns a reference to data owned by the current function

error: aborting due to previous error

@jyn514 jyn514 added the A-borrow-checker Area: The borrow checker label Nov 1, 2020
@jyn514
Copy link
Member

jyn514 commented Nov 1, 2020

Actually that's not quite right because it returns a reference. I think https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=51d3946643a798e189b33af33ec4e88f is closer.

@jyn514
Copy link
Member

jyn514 commented Nov 1, 2020

I think the issue is that this is a self-referential struct: the future stores both track and arena, so by putting a pointer to arena into track you end up with a future that points to itself, which isn't allowed. by-move and by-value aren't related.

@jyn514 jyn514 removed the C-bug Category: This is a bug. label Nov 1, 2020
@jyn514
Copy link
Member

jyn514 commented Nov 1, 2020

@Ploppz
Copy link
Author

Ploppz commented Nov 1, 2020

That works because if you drop(track) or drop(arena) you actually only drop the reference I think?

I think this fix to my initial playground, adding let mut track = track; to coerce the compiler to capture by value, shows that the problem is that it actually captures by reference by default
https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=4ddfa084cc64cbfe3465e0228ce2d7b1

Edit:
And shows that it's not a problem that the future is self referential.
And yes it works if you remove the move, because it captures both variables by reference. The problem when move is there is precisely, in my understanding, that it captures arena by value and track by reference so that arena is dropped before track.

@jyn514
Copy link
Member

jyn514 commented Nov 1, 2020

That works because if you drop(track) or drop(arena) you actually only drop the reference I think?

It breaks without drop, too, that was just to make it more clear. I guess it didn't work 😆

I think this fix to my initial playground, adding let mut track = track; to coerce the compiler to capture by value, shows that the problem is that it actually captures by reference by default

That's because you're pushing to a new, non-captured variable now - it's no longer stored in the future. If you capture after the push it still breaks: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=4d3a959ef9a3cae41bcde778822bedda

@camelid camelid added the A-closures Area: Closures (`|…| { … }`) label Nov 1, 2020
@nikomatsakis
Copy link
Contributor

The errors are correct but confusing. In the move case, the arena variable is owned by the closure (well, generator), so you can't push it onto the vector that is owned by the creator stack frame, since the lifetime of the closure is not related to the lifetime of that vector. But in the non-move case, the arena variable remains owned by the other stack frame, so you can.

@nikomatsakis
Copy link
Contributor

Er, I explained that slightly wrong. For the move case both arena and track are owned by the generator, but that still requires it to be self-referential (i.e., to put a reference to one field into the vector).

@tmandry
Copy link
Member

tmandry commented Nov 3, 2020

This also isn't specific to async, we can see the same behavior with regular closures:

fn main() {
    let arena = 0;
    let mut track = Vec::new();
    (move || {
        //let mut track = track;
        track.push(&arena)
    })();
}

in this case uncommenting the commented line, or removing move, is enough to let the example compile. The error message in this case:

error[E0521]: borrowed data escapes outside of closure
 --> src/main.rs:9:9
  |
6 |     let mut track = Vec::new();
  |         --------- `track` declared here, outside of the closure body
...
9 |         track.push(&arena)
  |         ^^^^^^^^^^^^^^^^^^

is different from the error message in the async case:

error[E0597]: `arena` does not live long enough
  --> src/main.rs:10:20
   |
7  |     let mut track = Vec::new();
   |         --------- lifetime `'1` appears in the type of `track`
...
10 |         track.push(&arena)
   |         -----------^^^^^^-
   |         |          |
   |         |          borrowed value does not live long enough
   |         argument requires that `arena` is borrowed for `'1`
11 |     }
   |     - `arena` dropped here while still borrowed

...and honestly, I think both of them could use some improvement. The async one actually seems a little better to me personally.

@tmandry
Copy link
Member

tmandry commented Nov 3, 2020

As discussed in the wg-async-foundations meeting, we could support this in the async case at least, since async generators are allowed to be self-borrowing. But since we handle their captures the same as we handle closures, we don't support it today.

@tmandry tmandry added the AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. label Nov 3, 2020
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-borrow-checker Area: The borrow checker A-closures Area: Closures (`|…| { … }`) AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants