Compiler should warn on moves of mutable copy variables

compiler
1

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

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:

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:

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

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:

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:

#[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.

7 Likes
2

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 moved 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.

3 Likes
3

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.

1 Like
4
3 Likes
5

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.

1 Like
6

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.

7

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

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

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

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:
#[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);
    }
}
9

Okay, abother example then:

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?

10

I think that this was the idea :slight_smile: :

2 Likes
11

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