Wrong error: cannot move out of captured outer variable in an FnMut closure

[Boscop]

error: cannot move out of captured outer variable in an FnMut closure

struct Foo {
    a: i32,
    b: i32,
    bar: Bar,
}

struct Bar;

impl Bar {
    fn f<F: FnMut()>(&self, mut f: F) {
        f();
    }
}

fn main() {
    let mut foo = Foo { a: 1, b: 2, bar: Bar };
    let a = &mut foo.a;
    let b = &mut foo.b;
    (|| { // works
        *if true {a} else {b} = 42;
    })();
    
    let mut foo = Foo { a: 1, b: 2, bar: Bar };
    let a = &mut foo.a;
    let b = &mut foo.b;
    foo.bar.f(|| { // doesn't work
        *if true {a} else {b} = 42;
    });
}

https://play.rust-lang.org/?gist=4ce6948a92c2fcb281b3cade8574691d&version=nightly

But the second case should work too!

[kennytm]

It works if you change the FnMut to FnOnce.

[Boscop]

But why can't it also work with FnMut?

(Btw, the error should say it has to be FnOnce then.)

[nikomatsakis]

@Boscop because you can only move things one time -- an FnMut closure can be invoked more than once.

(Btw, the error should say it has to be FnOnce then.)

I don't entirely agree, but I also don't disagree. This kind of error is often hard to decide how to diagnose. There is a kind of tension here: the closure wants to take an action (a move), but the signature of f does not permit it (because it requires a closure that can be invoked more than once). It may be that the signature is in error (it should be generalized to accept an FnOnce) or that the closure is wrong (it is trying to do things it should not do). At minimum, we're clearly not doing a good job of capturing this 'tension' in the diagnostic.

Interestingly, if the closure is not given directly as an argument to f we give a somewhat better error:

error[E0525]: expected a closure that implements the `FnMut` trait, but this closure only implements `FnOnce`
  --> src/main.rs:19:19
   |
19 |       let closure = || { // doesn't work
   |  ___________________^
20 | |         *if true {a} else {b} = 42;
21 | |     };
   | |_____^
22 |       foo.bar.f(closure);
   |               - the requirement to implement `FnMut` derives from here
   |
note: closure is `FnOnce` because it moves the variable `a` out of its environment
  --> src/main.rs:20:19
   |
20 |         *if true {a} else {b} = 42;
   |                

This is because of the tracking that @estebank (I think? or was it someone else...) put in to track why we decide a closure's "defining trait". Right now, that checking is not coming into play because we are deciding that the closure should be FnMut solely because of the signature of f.

That happens in this code:

rust/src/librustc_typeck/check/closure.rs

Lines 151 to 155 in 70f7d58

	if let Some(kind) = opt_kind {
	self.demand_eqtype(expr.span,
	kind.to_ty(self.tcx),
	substs.closure_kind_ty(expr_def_id, self.tcx));
	}

When we go down that path, we never wind up storing the "origin" of the kind. In contrast, the upvar inference that would otherwise kick in does this:

rust/src/librustc_typeck/check/upvar.rs

Lines 182 to 188 in 70f7d58

	// If we have an origin, store it.
	if let Some(origin) = delegate.current_origin {
	self.tables
	.borrow_mut()
	.closure_kind_origins_mut()
	.insert(closure_hir_id, origin);
	}

So we probably need to be storing some kind of information in closure_kind_origins_mut and then improving the borrowcheck. Then we could give an error like:

error[E0507]: cannot move out of captured outer variable in an `FnMut` closure
  --> src/main.rs:20:19
   |
17 |     let a = &mut foo.a;
   |         - captured outer variable
...
20 |         *if true {a} else {b} = 42;
   |                   ^ cannot move out of captured outer variable in an `FnMut` closure
note: closure is `FnMut` because of the requirements of `f()`
  --> src/main.rs:19:19
   |
19 |         foo.bar.f(||
   |         ^^^^^^^^^
   |            

[estebank]

Current output:

error[E0507]: cannot move out of `a`, a captured variable in an `FnMut` closure
  --> src/main.rs:27:19
   |
24 |     let a = &mut foo.a;
   |         - captured outer variable
25 |     let b = &mut foo.b;
26 |     foo.bar.f(|| { // doesn't work
   |               -- captured by this `FnMut` closure
27 |         *if true {a} else {b} = 42;
   |                   ^ move occurs because `a` has type `&mut i32`, which does not implement the `Copy` trait

error[E0507]: cannot move out of `b`, a captured variable in an `FnMut` closure
  --> src/main.rs:27:28
   |
25 |     let b = &mut foo.b;
   |         - captured outer variable
26 |     foo.bar.f(|| { // doesn't work
   |               -- captured by this `FnMut` closure
27 |         *if true {a} else {b} = 42;
   |                            ^ move occurs because `b` has type `&mut i32`, which does not implement the `Copy` trait

error[E0525]: expected a closure that implements the `FnMut` trait, but this closure only implements `FnOnce`
  --> src/main.rs:19:19
   |
19 |     let closure = || { // doesn't work
   |                   ^^ this closure implements `FnOnce`, not `FnMut`
20 |         *if true {a} else {b} = 42;
   |                   - closure is `FnOnce` because it moves the variable `a` out of its environment
21 |     };
22 |     foo.bar.f(closure);
   |             - ------- the requirement to implement `FnMut` derives from here
   |             |
   |             required by a bound introduced by this call
   |
note: required by a bound in `Bar::f`
  --> src/main.rs:10:13
   |
10 |     fn f<F: FnMut()>(&self, mut f: F) {
   |             ^^^^^^^ required by this bound in `Bar::f`

[Boscop]

I came across this issue again..
The code below compiles and is the intended thing to do, but rustc doesn't mention this solution, I think it should.
Reasoning: This solution doesn't require changing any of the involved types, so ideally rustc should suggest this.

    foo.bar.f(|| {
        let a = &mut *a;
        let b = &mut *b;
        *if true {a} else {b} = 42;
    });