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-block diagnostics do not suggest async move when it may be needed #66107

Closed
WildCryptoFox opened this issue Nov 5, 2019 · 10 comments · Fixed by #70906
Closed

async-block diagnostics do not suggest async move when it may be needed #66107

WildCryptoFox opened this issue Nov 5, 2019 · 10 comments · Fixed by #70906

Comments

@WildCryptoFox
Copy link
Contributor

@WildCryptoFox WildCryptoFox commented Nov 5, 2019

(playground)

//* f hits lifetime errors
fn f(a: &usize) -> (impl '_ + Future<Output = ()>, &usize) {
    (async { *a; }, a)
}
// */

// g compiles as expected
fn g(a: &usize) -> (impl '_ + Future<Output = ()>, &usize) {
    async fn h(a: &usize) { *a; }
    (h(a), a)
}
error[E0515]: cannot return value referencing function parameter `a`
 --> src/main.rs:5:5
  |
5 |     (async { *a; }, a)
  |     ^^^^^^^^^^-^^^^^^^
  |     |         |
  |     |         `a` is borrowed here
  |     returns a value referencing data owned by the current function

This issue has been assigned to @gizmondo via this comment.

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Nov 12, 2019

The error is correct, so what we are looking for here is a diagnostic improvement. The problem is that async { *a }, much like a closure, defaults to borrowing a in place -- so when we try to return, it has a reference to a as a is going out of scope.

To fix the error, you need async move (playground).

The situation is exactly analogous to closures and move closures. Note that in the analogous closure example (playground), we give a different error, complete with a suggestion:

error[E0373]: closure may outlive the current function, but it borrows `a`, which is owned by the current function
 --> src/main.rs:5:6
  |
5 |     (|| { *a; }, a)
  |      ^^    - `a` is borrowed here
  |      |
  |      may outlive borrowed value `a`
  |
note: closure is returned here
 --> src/main.rs:5:5
  |
5 |     (|| { *a; }, a)
  |     ^^^^^^^^^^^^^^^
help: to force the closure to take ownership of `a` (and any other referenced variables), use the `move` keyword
  |
5 |     (move || { *a; }, a)
  |      ^^^^^^^

It seems like we should adapt the async { } case to be closer to the closure wording.

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Nov 12, 2019

Marking this as OnDeck since it seems like this might be a common mistake, and I think the fix should be relatively straightforward.

@nikomatsakis nikomatsakis changed the title async-block lifetime issue with shared reference in the return type (async-fn works as expected) async-block diagnostics do not suggest async move when it may be needed Nov 12, 2019
@WildCryptoFox
Copy link
Contributor Author

@WildCryptoFox WildCryptoFox commented Nov 13, 2019

Ah thanks @nikomatsakis this makes sense.

@gilescope
Copy link
Contributor

@gilescope gilescope commented Dec 10, 2019

@rustbot claim

@tmandry tmandry moved this from To do to In progress in wg-async-foundations work Mar 3, 2020
@tmandry
Copy link
Contributor

@tmandry tmandry commented Mar 3, 2020

Ping from triage. @gilescope are you working on this?

@tmandry tmandry moved this from In progress to Assigned in wg-async-foundations work Mar 3, 2020
@tmandry
Copy link
Contributor

@tmandry tmandry commented Mar 17, 2020

Ping. @gilescope are you interested in continuing to work on this, or should I un-assign?

@tmandry tmandry moved this from Claimed to To do in wg-async-foundations work Mar 24, 2020
@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Mar 31, 2020

@rustbot claim

I'm going to "claim" this issue, but only to write mentoring instructions! If anybody is interested in taking a crack at it, I'd be happy to help.

@gilescope
Copy link
Contributor

@gilescope gilescope commented Apr 3, 2020

Sorry - if anyone else wants it that’s cool. I seem to have fingers in quite a few pies at the moment.

Sent with GitHawk

@tmandry tmandry moved this from On deck to Claimed in wg-async-foundations work Apr 7, 2020
@gizmondo
Copy link
Contributor

@gizmondo gizmondo commented Apr 7, 2020

@rustbot claim

@rustbot rustbot assigned rustbot and unassigned nikomatsakis Apr 7, 2020
@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Apr 8, 2020

@gizmondo thanks!!

@tmandry tmandry moved this from Claimed to In progress in wg-async-foundations work Apr 9, 2020
@bors bors closed this in 6f8fc4d Apr 9, 2020
wg-async-foundations work automation moved this from In progress to Done Apr 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment