Move closure copies :Copy variables silently

[kpp]

The Good

Given the following code:

fn main() {
    let mut x = 0;
    dbg!(x);
}

https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=19261742184207a4075885cae58680e1

I got a reasonable warning:

warning: variable does not need to be mutable
 --> src/main.rs:2:9
  |
2 |     let mut x = 0;
  |         ----^
  |         |
  |         help: remove this `mut`

The Bad

Suggest we have the following code:

fn main() {
    let mut x = 0;
    let mut f = move || x = 42;
    f();
    dbg!(x);
}

https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=42c989cf63f57e5bad0f572bb705b69f

Neither rustc nor clippy gives me any warning. x = 42 affects a local copy of x, not the outer one, that's why dbg prints x=0. This can result in a buggy code.

The Ugly

async move closures are affected too. Replacing FnMut's with async move closures light-headedly will add unexpected bugs. Example from https://docs.rs/futures-preview/0.3.0-alpha.17/futures/stream/trait.StreamExt.html#method.for_each:

-let fut = stream::repeat(1).take(3).for_each(|item| {
+let fut = stream::repeat(1).take(3).for_each(async move |item| {
      x += item;
-     ready(())
 });

// futures 0.3
#![feature(async_await, async_closure)]
use futures::future;
use futures::stream::{self, StreamExt};

let mut x = 0;

{
    let fut = stream::repeat(1).take(3).for_each(async move |item| {
        x += item;
    });
    fut.await;
}

assert_eq!(x, 3); // BOOM! x = 0

[ltratt]

I've just found a real bug in a library caused by the lack of an error/warning for this problem in rustc/Clippy. Simplified slightly the problem is:

fn main() {
    let mut x = 0;
    let mut y = move |z: u32| {
        x += z;
        println!("inner x {}", x);
    };

    println!("outer x {}", x);
    y(2);
    println!("outer x {}", x);
}

Playground link

This outputs:

outer x 0
inner x 2
outer x 0

The problem in this case was moving single-threaded code to being multi-threaded: it compiles fine, but produces incorrect results because ints are Copyable. In other words, Rust's semantics are sound but (to put it mildly) surprising. That means that the above program is effectively equivalent to:

fn main() {
    let x = 0;
    let mut y = move |z: u32| {
        let mut x = x + z;
        println!("inner x {}", x);
    };

    println!("outer x {}", x);
    y(2);
    println!("outer x {}", x);
}

There are several mitigations I can imagine (putting aside issues of backwards compatibility):

• rustc can error/warn on move closures capturing mut variables.
• Clippy can error/warn about move closures capturing mut variables.
• rustc/Clippy can error/warn if a mut variable is mutated inside a closure and/or after the variable is captured.

Maybe there are others I haven't thought of.

[anatol1234]

This is also discussed here: https://internals.rust-lang.org/t/closure-value-moved-why-mut-needed/11082

@ltratt Since the closures that are produced are FnMut, it is probably a good idea to take look at the general case in which the closure is not only called once but multiple times:

fn main() {
    let mut x = 0;
    let mut y = move |z: u32| {
        x += z;
        println!("inner x {}", x);
    };

    println!("outer x {}", x);
    y(2);
    y(2);
    println!("outer x {}", x);
}

This outputs:

outer x 0
inner x 2
inner x 4
outer x 0

Your version with the inner local mutable binding of x outputs:

outer x 0
inner x 2
inner x 2
outer x 0

For a version that is effectively equivalent you would then need a struct for the captured x (common way of desugaring Rust closures):

struct Closure {
  x: u32,
}

impl Closure {
    fn call_mut(&mut self, z: u32){
        self.x += z;
        println!("inner x {}", self.x);
    }
}

fn main() {
    let x = 0;
    let mut y = Closure{x};

    println!("outer x {}", x);
    y.call_mut(2);
    y.call_mut(2);
    println!("outer x {}", x);
}

If I understand it correctly, all of your three suggestions for mitigation implicitly imply that rustc should allow mutating a captured non-mut variable from inside the closure. Applying an argument from the Rust internals thread, in the general case there would then be no possibility for the programmer to specify that the captured variable should not be mutated. It would, therefore, harm the expressiveness. On the contrary, the argument goes, a rule stating that variables captured by a closure are always mut-bound would follow anonymous bindings rules. The only idea how to partially preserve the expressiveness I have would be to introduce some special syntax for specifying mutability of captured variables in move closures, like movemut.

[kpp]

If I understand it correctly, all of your three suggestions for mitigation implicitly imply that rustc should allow mutating a captured non-mut variable from inside the closure

The reason I called this issue ugly:

    let mut x = 0;
    let mut f = move || x = 42;

Is that it implicitly creates a new variable x without let keyword. Having x on the right side of the expression should be ok (let y = x;) but not on the left side (x = 42).

[ltratt]

@anatol1234 I thought my 0 2 0 example was odd but I still didn't expect your 0 2 4 0 variation!

I don't fully grasp your last paragraph but I think the answer to this bit:

If I understand it correctly, all of your three suggestions for mitigation implicitly imply that rustc should allow mutating a captured non-mut variable from inside the closure.

is "no, I don't think that." My starting point is that the current behaviour where a combination of move, let mut and a Copyable datatype leads to two detached mutable variables sharing the same name but different scopes is surprising (as @kpp has just commented). My questions/thoughts were then "should this be a warning/error in rustc/Clippy"? I don't know what the best choice is. If nothing else, I'm sure that some code currently relies on this behaviour though I imagine that, mostly, it's hiding bugs.

[kpp]

These 0 2 2 0 and 0 2 4 0 issues gave me a flavor of static keyword in terms of C++.

So instead of movemut keyword we could've rewritten 0 2 2 0 as:

    let x = 0;
    let mut y = move |z: u32| {
        let mut x = x + z;
        println!("inner x {}", x);
    };

And 0 2 4 0 as:

    let x = 0;
    let mut y = move |z: u32| {
        let static mut x = x + z;
        println!("inner x {}", x);
    };

[anatol1234]

My starting point is that the current behaviour where a combination of move, let mut and a Copyable datatype leads to two detached mutable variables sharing the same name but different scopes is surprising (as @kpp has just commented). My questions/thoughts were then "should this be a warning/error in rustc/Clippy"?

@ltratt In this case, let's assume we had such a warning/error and everything else about rustc remains unchanged. How would you then fix the warning/error in this prominent example taken from the std::iter::from_fn doc:

let mut count = 0;
let counter = std::iter::from_fn(move || {
    // Increment our count. This is why we started at zero.
    count += 1;

    // Check to see if we've finished counting or not.
    if count < 6 {
        Some(count)
    } else {
        None
    }
});
assert_eq!(counter.collect::<Vec<_>>(), &[1, 2, 3, 4, 5]);

I hope I don't miss some detail, but I think there is everything in there. An outer variable with let mut binding of a Copyable value which is moved into a closure which maintains a copy of that variable which is mutated in each call of the closure. You could even add some code at the bottom which reads the startposition count from the outside and you would still have a perfectly reasonable example, I think.

[spunit262]

count is not used in the outer scope after being "moved", the result is the same as if the value wasn't Copy, so that example is not an error. If you added code at the bottom that used it, that should be an error, which could be fix by making a new let binding just for the closure, that way it's clear that the two bindings are unrelated.

As I said in the internals thread, it should be thought as the binding is being moved into the closure, not just the value.

[anatol1234]

@spunit262 If you apply this logic of moving bindings to reason about the semantics of the move keyword, then in order to be consistent this logic should apply indifferently of whether the captured value is Copy/non-Copy and indifferently of whether it is bound mut/non-mut in the outer scope, right? In this case, I guess, every attempt to use a variable after a "move" via move should yield some flavor of cannot find value x in this scope rather than use of moved value: x if x is non-Copy or no error if x is Copy.

Also, in the FnOnce case...

struct X;
fn main() {
    let x = X;
    let fnonce = || x;
    let x = X;
    let fnonce_move = move || x;
}

... would the semantics of fnonce and fnonce_move then be different in the sense that for fnonce the value of x is moved, while for fnonce_move the x binding is moved?