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

Error message for closure with async block needs improvement #68119

Open
tmandry opened this issue Jan 11, 2020 · 4 comments
Open

Error message for closure with async block needs improvement #68119

tmandry opened this issue Jan 11, 2020 · 4 comments
Labels
A-async-await Area: Async & Await A-closures Area: closures (`|args| { .. }`) A-diagnostics Area: Messages for errors, warnings, and lints AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. C-enhancement Category: An issue proposing an enhancement or a PR with one. D-confusing Diagnostics: Confusing error or lint that should be reworked. P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@tmandry
Copy link
Member

tmandry commented Jan 11, 2020

Playground

fn do_stuff(foo: Option<Foo>) {
    require_fn_trait(|| async {
        if foo.map_or(false, |f| f.foo()) {
            panic!("foo");
        }
        //ready(())
    })
}

gives

error[E0507]: cannot move out of `foo`, a captured variable in an `Fn` closure
  --> src/lib.rs:14:31
   |
13 |   fn do_stuff(foo: Option<Foo>) {
   |               --- captured outer variable
14 |       require_fn_trait(|| async {
   |  _______________________________^
15 | |         if foo.map_or(false, |f| f.foo()) {
   | |            ---
   | |            |
   | |            move occurs because `foo` has type `std::option::Option<Foo>`, which does not implement the `Copy` trait
   | |            move occurs due to use in generator
16 | |             panic!("foo");
17 | |         }
18 | |         //ready(())
19 | |     })
   | |_____^ move out of `foo` occurs here
   |
help: consider borrowing the `Option`'s content
   |
14 |     require_fn_trait(|| async {
15 |         if foo.map_or(false, |f| f.foo()) {
16 |             panic!("foo");
17 |         }
18 |         //ready(())
19 |     }.as_ref())
   |

Note move occurs due to use in generator. Also, the code is being output twice, for some reason.

If you take this out of the async context by removing async and uncommenting ready(()), you get a better suggestion to use as_ref() and an all-around cleaner error message:

error[E0507]: cannot move out of `foo`, a captured variable in an `Fn` closure
  --> src/lib.rs:15:12
   |
13 | fn do_stuff(foo: Option<Foo>) {
   |             --- captured outer variable
14 |     require_fn_trait(|| {
15 |         if foo.map_or(false, |f| f.foo()) {
   |            ^^^
   |            |
   |            move occurs because `foo` has type `std::option::Option<Foo>`, which does not implement the `Copy` trait
   |            help: consider borrowing the `Option`'s content: `foo.as_ref()`

Thanks to @JakeEhrlich for originally reporting this.

@jonas-schievink jonas-schievink added A-async-await Area: Async & Await A-closures Area: closures (`|args| { .. }`) A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 11, 2020
@JohnTitor JohnTitor added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Jan 12, 2020
@tmandry tmandry changed the title Error message for closure with async block mentions generators Error message for closure with async block needs improvement Jan 13, 2020
@nikomatsakis
Copy link
Contributor

It took me a while to process what's going on here. I guess the biggest problem is the "move of foo occurs here" that highlights the whole async block.

In contrast, we don't show (for closures) the "closure creation" as the point of a move (although it technically is). We just show the point within the closure body that's responsible.

Looking more closely at the example, it seems like there are actually several moves happening

  • into the outer closure
  • into the async block
  • then into map_or call

and so I guess we are suppressing one but not the rest. I'd have to go digging a bit to give more specifics on how to fix.

@tmandry
Copy link
Member Author

tmandry commented Jan 14, 2020

@rustbot modify labels to +AsyncAwait-OnDeck

Good diagnostics are a priority

@tmandry tmandry added the AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. label Jan 14, 2020
@tmandry tmandry added this to To do in wg-async work Feb 11, 2020
@tmandry tmandry added D-confusing Diagnostics: Confusing error or lint that should be reworked. P-medium Medium priority labels Mar 3, 2020
@tmandry tmandry removed this from To do in wg-async work Mar 3, 2020
@folex
Copy link
Contributor

folex commented Jan 12, 2021

I also got bitten by this error message
link to playground

    arr.into_iter().map(move |_e| {
        async move {
            let value = cloned_value.clone();
            println!("{:?}", value.lock().ok())
        }
    })

And the error got me confused because I'm not familiar with the "generator" concept - I have seen it in some blog posts but never met it in practice.

move occurs due to use in generator

Consider a very similar situation

    arr.into_iter().map(move |_e| {
        move || {
            let value = cloned_value.clone();
            println!("{:?}", value.lock().ok())
        }
    })

which yields

move occurs due to use in closure

I've much more accustomed to thinking about closures - these I imagine as structures that capture the context in their fields. So this leads me to a solution much faster.

However, note that E0507 talks about a borrowed value, but here it is hard to see where it is borrowed since everything is marked as move

E0507 A borrowed value was moved out.

These are just my thoughts on a topic, hope that might help you! Thanks a lot for your tremendous works!

@sd2k
Copy link

sd2k commented Oct 27, 2022

I seem to get bitten by this every time I'm working with futures::stream::iter and some Arcs. The solution is invariably to change this (from the above playground):

    arr.into_iter().map(move |_e| {
        async move {
            let value = cloned_value.clone();
            println!("{:?}", value.lock().ok())
        }
    })

to

    arr.into_iter().map(move |_e| {
        let value = cloned_value.clone();
        async move {
            println!("{:?}", value.lock().ok())
        }
    })

i.e. moving the clone outside the async move block. I've probably had to do this at least 10 times in the last year, and it still takes me a minute or two to realise where it needs to happen each time. Perhaps it would be worth suggesting that change in the diagnostic if the variable is being cloned? Something like:

error[[E0507]](https://doc.rust-lang.org/stable/error-index.html#E0507): cannot move out of `cloned_value`, a captured variable in an `FnMut` closure
  --> src/lib.rs:15:20
   |
13 |       let cloned_value = value.clone();
   |           ------------ captured outer variable
14 |       arr.into_iter().map(move |_e| {
   |                           --------- captured by this `FnMut` closure
15 |           async move {
   |  ____________________^
16 | |             let value = cloned_value.clone();
   | |                         ------------
   | |                         |
   | |                         variable moved due to use in generator
   | |                         move occurs because `cloned_value` has type `Arc<Mutex<Value>>`, which does not implement the `Copy` trait
17 | |             println!("{:?}", value.lock().ok())
18 | |         }
   | |_________^ move out of `cloned_value` occurs here
   |
help: try cloning `cloned_value` before the `async move` block:
   |
13 |       let cloned_value = value.clone();
14 |       arr.into_iter().map(move |_e| {
15 |           let value = cloned_value.clone();
16 |           async move {
17 |               println!("{:?}, value.lock().ok())
18 |           }

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 (`|args| { .. }`) A-diagnostics Area: Messages for errors, warnings, and lints AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. C-enhancement Category: An issue proposing an enhancement or a PR with one. D-confusing Diagnostics: Confusing error or lint that should be reworked. P-medium Medium priority 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

7 participants