# Compiler should warn on moves of mutable copy variables

**URL:** <https://internals.rust-lang.org/t/compiler-should-warn-on-moves-of-mutable-copy-variables/19636>\
**Category:** compiler\
**Created:** [October 1, 2023, 7:35pm UTC](https://internals.rust-lang.org/t/compiler-should-warn-on-moves-of-mutable-copy-variables/19636 "2023-10-01T19:35:48Z")\
**Posts on this page:** 11\
**Page:** 1

<div class="post-metadata">

**Author:** ![Pzixel](https://sea2.discourse-cdn.com/flex002/user_avatar/internals.rust-lang.org/pzixel/32/4349_2.png) [@Pzixel](https://internals.rust-lang.org/u/Pzixel)\
**Post date:** [October 1, 2023, 7:35pm UTC](https://internals.rust-lang.org/t/compiler-should-warn-on-moves-of-mutable-copy-variables/19636/1 "2023-10-01T19:35:48Z")

</div>

Here is a code I think should not be accepted without warnings by the compiler:

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

```

I've discussed this quite a lot and some people agree with me and some say "but you said move, its behavior is absolutely expected". Well in reality it it more something like this:

```rust
fn main() {
    let mut i = 0;
    let rc = std::rc::Rc::new(10);
    let mut f = {
        let rc = rc.clone();
        move || {
            i += *rc;
            i
        }
    };
    println!("{}", f());
    println!("{}", i);
}

```

Now the thing is that you have `move` required to make `Rc` work, but you actually captured `i` by value unintentionally. Note that I'm only talking about `mut` bindings that get mutated within closure - immutable copies being moved are absolutely fine. So this being said, I think it's a really rare case when you want to do this, because original code can be always rewritten into:

```rust
fn main() {
    let i = 0;
    let f = move || {
      let i = i + 1; // we want to have a personal mutable copy
      i
    };
    println!("{}", f());
    println!("{}", i);
}

```

or

```rust
fn main() {
    let mut i = 0;
    {
        let i = &mut i; // you want to have a reference that you mutate in the closure
        let f = move || {
            *i += 1;
            i
        };
        println!("{}", f());
    }
    println!("{}", i);
}

```

But the original code makes a copy which can be easily missed and then mutates it. This remind me of a similar situation in C# where `get` property creates a new copy every time you call it and if you're trying to modify it it generates an error, because it's well expected to be a mistake:

```cs
class Program {
    struct Foo {
        public int x;
    }

    class Bar {
        public Foo Foo { get; }
    }

    static void Main() {
        var bar = new Bar();
        bar.Foo.x += 1; 
  // error CS1612: Cannot modify the return value of 'Bar.Foo' because it is not a variable
    }
}

```

I think that compiler should ask for an explicit `let` in case if this move was intentional or to create a reference/smart pointer/... in case if it was supposed to modify a shared location. In its current state it looks like a pitfall.

I wanted to end with more realistic example of what I was doing something along these:

```rust
#[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 syncronization" I immediately thought "well I'm running on a current-thread executor, so it figured out I don't need syncronization 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.

Hope this all makes sense.

---

<div class="post-metadata">

**Author:** ![jrose](https://sea2.discourse-cdn.com/flex002/user_avatar/internals.rust-lang.org/jrose/32/9591_2.png) [@jrose](https://internals.rust-lang.org/u/jrose)\
**Post date:** [October 1, 2023, 7:53pm UTC](https://internals.rust-lang.org/t/compiler-should-warn-on-moves-of-mutable-copy-variables/19636/2 "2023-10-01T19:53:02Z")

</div>

I want to note that your “I want a personal copy” rewrite is incorrect because the mutation doesn’t persist across multiple calls, as it would for the original `move` case. But you can still do a rewrite similar to your explicit `&mut` case, so it doesn’t rule out a lint here entirely. I appreciate that you took the time to write out the “how do you address this warning” part.

I’d suggest a small tweak to the rule: warn if a `move`d Copy binding is both _mutated_ after capture (either inside or outside the closure) as well as being _referenced again_ outside the closure. If there’s no further mutation or no further reference from multiple places, there’s no possibility of misinterpretation.

---

<div class="post-metadata">

**Author:** ![Pzixel](https://sea2.discourse-cdn.com/flex002/user_avatar/internals.rust-lang.org/pzixel/32/4349_2.png) [@Pzixel](https://internals.rust-lang.org/u/Pzixel)\
**Post date:** [October 1, 2023, 7:59pm UTC](https://internals.rust-lang.org/t/compiler-should-warn-on-moves-of-mutable-copy-variables/19636/3 "2023-10-01T19:59:57Z")

</div>

Yes, to be absolutely more precise `let` binding should be marked as mutable to make it mutable accross the closure, I just thought it would distract a little bit from the core concept as I already felt like I'm failing at delivering the idea. But you're absolutely right, thank for the edit and suggestion.

---

<div class="post-metadata">

**Author:** ![quinedot](https://sea2.discourse-cdn.com/flex002/user_avatar/internals.rust-lang.org/quinedot/32/7294_2.png) [@quinedot](https://internals.rust-lang.org/u/quinedot)\
**Post date:** [October 1, 2023, 11:56pm UTC](https://internals.rust-lang.org/t/compiler-should-warn-on-moves-of-mutable-copy-variables/19636/4 "2023-10-01T23:56:29Z")

</div>

> <https://github.com/rust-lang/rust/issues/73467>
>
> https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=6dee1348…129e0c1938c67eb66a8f62b0
> \`\`\`rust
> 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.

---

<div class="post-metadata">

**Author:** ![SkiFire13](https://sea2.discourse-cdn.com/flex002/user_avatar/internals.rust-lang.org/skifire13/32/7579_2.png) [@SkiFire13](https://internals.rust-lang.org/u/SkiFire13)\
**Post date:** [October 2, 2023, 6:19am UTC](https://internals.rust-lang.org/t/compiler-should-warn-on-moves-of-mutable-copy-variables/19636/5 "2023-10-02T06:19:44Z")

</div>

What if the closure is supposed to be short lived though? Like the one passed to `unwrap_or_else`. In that case the variable could still be mutated after the closure is created, but there is still no possibility of misinterpretation.

So I would add to the requirements that the closure should still be alive while the copy variable is mutated again or, if the closure is the one that mutates the variable, that the variable is referenced outside the closure after it is called.

---

<div class="post-metadata">

**Author:** ![jrose](https://sea2.discourse-cdn.com/flex002/user_avatar/internals.rust-lang.org/jrose/32/9591_2.png) [@jrose](https://internals.rust-lang.org/u/jrose)\
**Post date:** [October 4, 2023, 4:10am UTC](https://internals.rust-lang.org/t/compiler-should-warn-on-moves-of-mutable-copy-variables/19636/6 "2023-10-04T04:10:26Z")

</div>

~~Hm, it’s not possible to tell if a closure passed by value is still alive after the call completes, though. Maybe that means this isn’t feasible.~~ Never mind, lifetimes do provide enough information for this, short of transmutation.

---

<div class="post-metadata">

**Author:** ![AngelicosPhosphoros](https://sea2.discourse-cdn.com/flex002/user_avatar/internals.rust-lang.org/angelicosphosphoros/32/7346_2.png) [@AngelicosPhosphoros](https://internals.rust-lang.org/u/AngelicosPhosphoros)\
**Post date:** [October 6, 2023, 10:08am UTC](https://internals.rust-lang.org/t/compiler-should-warn-on-moves-of-mutable-copy-variables/19636/7 "2023-10-06T10:08:12Z")

</div>

There are cases when behaviour described in OP post is desirable.

For example, this code (which ensures that iterator values are unique):

```rust
my_it
  .filter({
     let mut prev_values = HashSet::new();
     move |&val| {
         prev_values.insert (val)
     }
  })

```

---

<div class="post-metadata">

**Author:** ![Pzixel](https://sea2.discourse-cdn.com/flex002/user_avatar/internals.rust-lang.org/pzixel/32/4349_2.png) [@Pzixel](https://internals.rust-lang.org/u/Pzixel)\
**Post date:** [October 6, 2023, 12:28pm UTC](https://internals.rust-lang.org/t/compiler-should-warn-on-moves-of-mutable-copy-variables/19636/8 "2023-10-06T12:28:30Z")

</div>

Yes, this code is fine, we have to big differences here:

1. HashSet is not a Copy type
2. Therefore this code doesn't create shallow copies and modifies them - lambda modifies a single instance of the binding and therefore everything works as expected. This is a bit artificial example but it shows the problem:

```rust
#[derive(Debug, Clone, Copy)]
struct HashSet<T>([Option<T>; 10]);

impl<T: Eq + Copy> HashSet<T> {
    fn new() -> Self {
        HashSet([None; 10])
    }

    fn insert(mut self, val: T) -> bool {
        let val = Some(val);
        if self.0.contains(&val) {
            return false;
        }
        for i in 0..self.0.len() {
            if self.0[i].is_none() {
                self.0[i] = val;
                return true;
            }
        }
        panic!("HashSet is full")
    }
}

fn main() {
    let my_it = vec![1, 1, 1, 2, 2, 3].into_iter();
    let x = my_it
        .filter({
            let mut prev_values = HashSet::new();
            move |&val| {
                prev_values.insert (val)
            }
        });
    for i in x {
        println!("{}", i);
    }
}

```

---

<div class="post-metadata">

**Author:** ![AngelicosPhosphoros](https://sea2.discourse-cdn.com/flex002/user_avatar/internals.rust-lang.org/angelicosphosphoros/32/7346_2.png) [@AngelicosPhosphoros](https://internals.rust-lang.org/u/AngelicosPhosphoros)\
**Post date:** [October 6, 2023, 12:43pm UTC](https://internals.rust-lang.org/t/compiler-should-warn-on-moves-of-mutable-copy-variables/19636/9 "2023-10-06T12:43:35Z")

</div>

Okay, abother example then:

```rust
My_iter
   .inspect({
      let mut idx = 0;
      move |val|{
         println!("val at idx {idx} is {val}");
         idx += 1;
      }
    })

```

Perhaps, we should warn only if we have usages of mutable variable outside of lambda?

---

<div class="post-metadata">

**Author:** ![robinm](https://sea2.discourse-cdn.com/flex002/user_avatar/internals.rust-lang.org/robinm/32/6255_2.png) [@robinm](https://internals.rust-lang.org/u/robinm)\
**Post date:** [October 6, 2023, 1:17pm UTC](https://internals.rust-lang.org/t/compiler-should-warn-on-moves-of-mutable-copy-variables/19636/10 "2023-10-06T13:17:38Z")

</div>

> [@AngelicosPhosphoros](#):
>
> Perhaps, we should warn only if we have usages of mutable variable outside of lambda?

I think that this was the idea 🙂 :

> [@jrose](#):
>
> I’d suggest a small tweak to the rule: warn if a `move`d Copy binding is both _mutated_ after capture (either inside or outside the closure) as well as being _referenced again_ outside the closure. If there’s no further mutation or no further reference from multiple places, there’s no possibility of misinterpretation.

---

<div class="post-metadata">

**Author:** ![system](https://sea2.discourse-cdn.com/flex002/user_avatar/internals.rust-lang.org/system/32/14092_2.png) [@system](https://internals.rust-lang.org/u/system)\
**Post date:** [January 4, 2024, 1:17pm UTC](https://internals.rust-lang.org/t/compiler-should-warn-on-moves-of-mutable-copy-variables/19636/11 "2024-01-04T13:17:54Z")

</div>

This topic was automatically closed 90 days after the last reply. New replies are no longer allowed.