Duplicating mut Copy variables doesn't warn or error

[rocallahan]

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

fn main() {
    let mut x = 0;
    let closure = move || {
        let mut closure2 = move || {
            x += 1;
        };
        closure2();
        println!("{}", x);
    };
    closure();
}

The inner closure essentially forks the mutable variable x into two variables. I would expect rustc to give a warning or error, but there is no warning or error. I understand the logic behind allowing this but I think it can lead to surprising results. rust-analyzer warns that assignment to x in closure2 is never read. This led me to a real bug in our code.

Seems to me that duplicating a mut Copy variable like this deserves some kind of warning at least.

[pitaj]

Looks like this is already a warning in 1.45.0-beta2 (and nightly ofc)

warning: unused variable: `x`
 --> src/main.rs:5:13
  |
5 |             x += 1;
  |             ^
  |
  = note: `#[warn(unused_variables)]` on by default
  = help: did you mean to capture by reference instead?

[jonas-schievink]

Closing as fixed

[rocallahan]

I actually think this should be reopened. Here's a slight variation that gives no warning on nightly:
https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=acc65b891ae68e390f0ed94e76df458b

fn main() {
    let mut x = 0;
    let closure = move || {
        let mut closure2 = move || {
            x += 1;
            x
        };
        closure2();
        println!("{}", x);
    };
    closure();
}

Since closure2 is now using its duplicate of x, the unused-variable warning goes away. However, I think it's still a problem that x is being duplicated here.

[IAkumaI]

Two years passed and this issue is still without any attention.
Much more simple example and topic which describes it: https://users.rust-lang.org/t/why-passing-mut-variable-to-another-thread-isnt-an-error/89857

rustc 1.67.0 (fc594f1 2023-01-24)

let mut a = 1;
for _ in 0..10 {
    tokio::spawn(async move {
        a += 1;
        if a > 3 {
            // this will never reached
        }
    })
    .await
    .unwrap();
}

UPD: Edited to more realistic example

[kpreid]

I've also myself managed to hit this issue while trying to hurriedly set up a closure producing diagnostic output ("how many times did this event happen" or similar) in addition to its existing behavior.

[IAkumaI]

@kpreid Yeah, I found the real bug today too. It had to be an archiver which slices zips to 512 Mb each, but it didn't. And it took some time to realize that count the zip size in mut usize which increments in spawn_blocking was not the best idea.

[Pzixel]

A simpler example from my recent code:

let mut i = 0;
let mut f = move || {
  i += 1;
  i
};
println!("{}", f());
println!("{}", i);

Which formerly was

#[tokio::main(flavor = "current_thread")]
async fn main() {
    let mut i = 0;

    let n = 5;
    let futures = (0..n).map(|_| async move {
        // some dozens of lines of code here
        i += 1;
        println!("Done {} out of {}", i, n);
    });

    for f in futures {
        f.await;
    }
}

The thing is that counter didn't exist in the fist place - it was just bunch of futures with their own work. So I added a counter - and while it stroke me a little bit "why does it compile? Don't I need some kind of synchronization" I immediately thought "well I'm running on a current-thread executor, so it figured out I don't need synchronization and regular reference is enough, since compiler is happy then it most definitely should be valid". And it's easy to imagine I was very wrong about that. I agree that I was tired and probably should have looked better, but in the end I really think that compiler should warn about such issues, C# compiler does just this in a very similar situation. I think this is the best approach and doesn't harm anyone - rust always has been about explicitness so I think this is a great place to apply it.

[kpreid]

This came up on URLO again.

I think what the compiler should do is warn when, for some binding foo,

• a closure captures foo by value,
• the captured foo is mutated within the closure, and
• the original foo is used after the closure was constructed.

Note that these rules don’t mention Copy. Rather, they describe a situation which is already an error if foo's type is not Copy, so they should hopefully preempt this surprise that arises only with Copy. Also, if a non-move closure writes || { let mut foo = foo; ... } to force a move/copy, then that naturally silences the warning since the new shadowing binding isn't the capture.