Unused mutabilty

There is warning " variable does not need to be mutable" exists in Rust. But why such warning is not exists for methods with &mut self?

struct A;

impl A {
    fn test1(&mut self) { // ?
    }

    fn test2(mut self) { // variable does not need to be mutable
    }

}

This is because &mut T means unique reference to T, with mutability being a side effect of safe ergonomic defaults.

To read more about this way of thinking: https://limpet.net/mbrubeck/2019/02/07/rust-a-unique-perspective.html

2 Likes

&mut refers to the data pointed to by the variable. You can't reassign the variable, but can modify the data.

mut refers to the variable name itself, not its data. You can reassign the variable. Owned data can always be modified.

There is no reason to have a mut binding when you are not using its mut-ness, so the lint makes sense.

There is, however, reasons to have a function / method take an exclusive (&mut) borrow rather than a shared (&) one even if the body is not mutating anything (mainly API: stability and / or soundness)

9 Likes

But what sense is having method that takes data by &mut and not modifying through reference, besides api stability?

The ensure that you have exclusive access to the value. This is easiest seen with Mutex. Mutex::get_mut gives you access the the underlying data without taking a lock. Why is this safe? Because &mut Mutex<T> means you have exclusive access to the Mutex, this allows you to get exclusive access to the underlying value, knowing that there can be no other threads that can access the value.

Most interior mutability types have a similar api. Such as Cell, RefCell, and RwLock these bypass the normal checks and are safe becuase &mut T means exclusive access.

2 Likes

You're again explaining what &mut means, but avoiding the original question, which was as I understand it "why not warn when the mut-part is unnecessary".

I think the Mutex example is interesting. But if you call Mutex::get_mut you are actually "using" the mut. More relevant is to consider the call of lock. For this the mut part is formally unnecessary, but it guarantees that the lock always succeeds at runtime. Thus having a mut-reference might be useful and a warning bad. Though, for this example, one might argue that one should use get_mut() instead of lock().unwrap() anyway and then the warning would go away...

(So maybe there is a better example for where a warning would be bad?)

Mutex::get_mut() is a perfect example for this, because if you look at its source code, it only calls UnsafeCell::get() and poison::Flag::borrow(), both of which only take &self, but if the &mut self didn't guarantee uniqueness of the borrow, the call to UnsafeCell::get() wouldn't be safe. The &mut self is thus needed without ever being explicitly used, so it wouldn't make sense to warn for it.

Because that's not what &mut T means! I think this warning is bad because it leads people to think that mutability is the core part to focus on, when it isn't. People should focus on if they need exclusive access or shared access, not in terms of mutability.

Thinking in terms of mutability will invariably lead to confusion once you start to really get into lifetimes. I have seen this many times with numerous different people asking about why they have lifetime issues, and it always boils down to "&mut T means unique, not mutable".

Adding this warning will only make things worse.

Along the lines of Mutex::get_mut, consider the following

fn get_ref<T: ?Sized>(mutex: &mut Mutex<T>) -> &T {
    // let's ignore poisoning for now, it's not relavant
    Mutex::get_mut(mutex).unwrap()
}

let mut my_mutex: Mutex<String> = ...;
let foo: &String = get_ref(&mut my_mutex);

In this example, foo's pointee can't be mutated because foo is a shared reference (not mutable by default), and String provides no interior mutability facilities. Now, since there is no mutability to be seen, but the &mut Mutex<String> is still necessary. Why? Because &mut Mutex<T> means unique, and we need unique access to the mutex to guarantee that we don't have any data races.

tldr: This lint is fundementally misguided, and it would lead people to use a different model for references than the compiler uses. This will invariably lead to confusion later when those differences are highlighted in more advanced lifetime usages.

2 Likes

Can you be more precise about what you mean by "that" and by "this warning"? I understand the question about a similar warning, not the same. So the question could also be "Why not have a warning that warns if people make a reference exclusive that does not have to be exclusive for the code to compile". (And let's assume no unsafe is involved and ignore the point about api stability, so only local code.)

For your example: You're calling get_mut whose signature is pub fn get_mut(&mut self), so of course mutex needs to be an exclusive reference for it even to compile.

When I said "that" and "this warning", I was refering to,

Where the quoted text misrepresents what &mut T is.


I don't think that this is a consistent lint. We don't lint on unneeded unsafe on functions or traits, even though we could. This is because both (&mut T and unsafe) are making guarantees about the api. We do lint on unneeded mut bindings and unsafe blocks, because they are implementation details.

If we ignore api-stability, that would make a better case for this lint, but I don't think we need it.

Also, we shouldn't make a lint that is actively wrong for unsafe code. If you have a type that contains pointers, any function that only uses those pointers and reads from other fields will compile without &mut T, but that may be unsound.

In any case the api reasons that @Yandros laid out are enough to answer the question.

4 Likes