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

"cannot move out of captured outer variable in an FnMut closure" should mention the outer variable #31752

Closed
Manishearth opened this issue Feb 18, 2016 · 10 comments
Labels
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.

Comments

@Manishearth
Copy link
Member

Manishearth commented Feb 18, 2016

use std::thread;

fn main() {
    let things = vec![1,2,3,4];
    let mut buf = vec![];
    let handle_thread: Vec<_> = things.iter().map(|&d| {
        thread::spawn(move || {
            do_thing(&mut buf);
        })
    }).collect();
}

fn do_thing(x: &mut [u8]) {}

(https://gist.github.com/772bde36b6cddebcd46a)

gives

<anon>:11:17: 13:4 error: cannot move out of captured outer variable in an `FnMut` closure [E0507]
<anon>:11       thread::spawn(move || {
<anon>:12           do_thing(&mut buf);
<anon>:13       })
error: aborting due to previous error

Except that it's totally confusing as to which outer variable is being talked about. In this case it's obvious since there's a single capture, but this won't always be the case.

The error should at least mention the name of the captured variable, and preferably point out where the variable was captured and how to fix it.

@Manishearth Manishearth added I-papercut A-diagnostics Area: Messages for errors, warnings, and lints labels Feb 18, 2016
@sheganinans
Copy link

sheganinans commented Apr 19, 2016

I've ran into this error, until this issue is closed maybe it would be good to have an example of a fix here. Thanks!

@Manishearth
Copy link
Member Author

Manishearth commented Apr 19, 2016

The fix is to figure out which captured variable is being moved and not move it. In this case, the variable buf is owned by the outer closure and needs to be kept around since the outer closure will get called multiple times. However, the inner move closure is moving it, so it gets used up after the first call. In this specific case to fix this error you would do something like let buf = &mut buf right before spawning the thread. This causes a lifetime error because the program example is an untenable one, but it's more or less what a general fix would look like.

@sheganinans
Copy link

sheganinans commented Apr 21, 2016

Perfect, thanks!

On Tue, Apr 19, 2016 at 8:53 AM, Manish Goregaokar <notifications@github.com

wrote:

The fix is to figure out which captured variable is being moved and not
move it. In this case, the variable buf is owned by the outer closure and
needs to be kept around since the outer closure will get called multiple
times. However, the inner move closure is moving it, so it gets used up
after the first call. In this specific case to fix this error you would do
something like let buf = &mut buf right before spawning the thread. This
causes a lifetime error because the program example is an untenable one,
but it's more or less what a general fix would look like.


You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#31752 (comment)

@kornelski
Copy link
Contributor

kornelski commented Jul 24, 2016

I've ran into this too.

    let mut tmp = 1;
    let c = &mut tmp;

    vec![0;10].iter().flat_map(|&a| {
        (0..10).map(move |b|{
            a + b + *c
        })
    });

cannot move out of captured outer variable in an FnMut closure

The error does not say that the problematic variable is c.

In real code, with multiple variables used and type inference hiding the details, it is really hard to figure out what this error is about.

@jonhoo
Copy link
Contributor

jonhoo commented Jan 20, 2017

Yet another example of this (simplified from an actual use-case I ran into):

fn main() {
    use std::thread;
    use std::sync::mpsc;

    let (tx, rx) = mpsc::channel();
    let n = 10;
    let foos: Vec<_> = (0..n)
        .into_iter()
        .map(|i| thread::spawn(move || -> bool { rx.recv().unwrap() }))
        .collect();
}

@jonhoo
Copy link
Contributor

jonhoo commented Mar 7, 2017

The compiler is a little better at this now that it highlights which variable is the problem, but I think we could still do better. Consider

fn foo(mut f: Box<FnMut()>) {
    f();
}

fn main() {
    let x = vec![];
    x.push(true);
    let y = true;
    foo(Box::new(move || y = false) as Box<_>);
}
error: cannot borrow immutable local variable `x` as mutable
 --> x.rs:7:5
  |
6 |     let x = vec![];
  |         - use `mut x` here to make mutable
7 |     x.push(true);
  |     ^ cannot borrow mutably

error: cannot assign to captured outer variable in an `FnMut` closure
 --> x.rs:9:26
  |
9 |     foo(Box::new(move || y = false) as Box<_>);
  |                          ^^^^^^^^^

For x, the compiler helpfully tells the user that they can make x mut, and everything will be fine. For y, the solution is the same (and, arguably, the problem is the same), but the compiler makes no such recommendation.

@steveklabnik steveklabnik added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 9, 2017
arielb1 pushed a commit to arielb1/rust that referenced this issue Apr 27, 2017
frewsxcv added a commit to frewsxcv/rust that referenced this issue Apr 27, 2017
frewsxcv added a commit to frewsxcv/rust that referenced this issue Apr 27, 2017
@blanham
Copy link

blanham commented May 6, 2017

I believe this was fixed in PR #41523

@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented May 6, 2017

Agreed, closing.

@jonhoo notes that the diagnostic for assignment isn't quite as good, but as shown below we still highlight the span for the variable assignment which doesn't work, so I suspect it's fairly clear which variable can't be assigned to. If you disagree, please open a new tracking issue for that.

fn foo(mut f: Box<FnMut()>) {
    f();
}

fn main() {
    let y = true;
    foo(Box::new(move || y = false) as Box<_>);
}
error: cannot assign to captured outer variable in an `FnMut` closure
 --> test.rs:7:26
  |
7 |     foo(Box::new(move || y = false) as Box<_>);
  |                          ^^^^^^^^^

error: aborting due to previous error

@jonhoo
Copy link
Contributor

jonhoo commented May 6, 2017

Hmm, I still think it'd be better if the error was:

error: cannot assign to captured outer variable in an `FnMut` closure
 --> test.rs:7:26
  |
6 |     let y = true;
  |         - use `mut y` here to make mutable
7 |     foo(Box::new(move || y = false) as Box<_>);
  |                          ^^^^^^^^^

error: aborting due to previous error

to bring it in line with the non-closure case...

@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented May 6, 2017

Opened #41790 to track the assignment case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

No branches or pull requests

7 participants