Rust-clippy: Warn about immediately applied Deref

[ChrisJefferson]

I've discovered recently I've been writing Rust incorrectly for some time, I would often write code like this:

fn test2(x: &mut i32) {
    *x += 1;
    print!("{}", *x);
}

fn test1(mut x: &mut i32) {
    test2(&mut x);
    test2(&mut x);
}

fn main() {
    let mut x = 2;
    test1(&mut x);
}

My mistake was assuming I need to write test2(&mut x), as just x is fine. This would also let me change the type of test1 to x : &mut i32.

The only reason this code works (I believe) is that the extra &mut is immediately removed by Deref. I think there should be a warning when an & or &mut is immediately removed by Deref.

[flip1995]

I never saw fn test1(mut x: &mut i32) { ... } in any rust code, yet. On the other hand, I just checked the rustc error that appears when you leave out the first mut:

error[E0596]: cannot borrow `x` as mutable, as it is not declared as mutable
 --> src/main.rs:7:11
  |
6 | fn test1(x: &mut i32) {
  |          - help: consider changing this to be mutable: `mut x`
7 |     test2(&mut x);
  |           ^^^^^^ cannot borrow as mutable

and it suggests to insert this mut. Which makes sense as a general suggestion.

I don't really see an use case for mut x: &mut _. So I think we should already lint mut x: &mut _, except there is a real use case for writing this? cc @oli-obk bless us with your expertise!

[oli-obk]

As a first step we could fix the rustc diagnostic and make it suggest &mut *x, since that does a reborrow of the original mutable reference. I believe there's no situation where it can go wrong.

Once that works nicely, we can indeed lint about mut x: &mut _.

[ChrisJefferson]

I should have said, i only added the first mut because rustc told me to.

[goffrie]

mut x: &mut T is certainly useful! see e.g. https://github.com/alexcrichton/flate2-rs/blob/master/src/gz/bufread.rs#L233 (arbitrary example - I found a number of them in common crates.)

On the other hand, I also found many instances of this antipattern. Annoyingly, though, many of them are of the form

fn whoops<T: Trait>(mut x: &mut T) { delegate(&mut x); }
fn delegate<T: Trait>(x: &mut T);
impl<T: Trait> Trait for &mut T {..}

so the call to delegate is not actually derefing x, but using the wrapping impl.

[oli-obk]

mut x: &mut T is certainly useful!

Right, but that function actually contains a move into and out of the mutable variable. As long as the function only ever borrows it mutably, it still is useless to be mut. We should be able to test this very easily by writing a MIR lint via an MIR visitor that just checks whether there are no moves from or to the argument in question. The relevant visitor functions should usually have a https://doc.rust-lang.org/nightly/nightly-rustc/rustc/mir/visit/enum.PlaceContext.html argument that can be checked for mutable borrows and moves. If neither exist, it should be an immutable reference, if no moves exist the variable should be immutable, even if the reference is not

[spunit262]

A &mut &mut T is necessary if T: ?Sized but you want to pass it to a function that only takes &mut T where T: Sized. I've ran into this while writing a object safe wrapper trait for Hash.

impl<T: Hash + Any> DynHash for T {
    fn dyn_hash(&self, state: &mut dyn Hasher) {
        let mut state = state;
        self.hash(&mut state)
    }
}

I ended up writing it as a mut rebind because I though that looked better but that's a purely stylist choice.

[goffrie]

Indeed, I just ran into the same issue with rand:

fn make_str<R: Rng + ?Sized>(mut rng: &mut R) -> String {
    (&mut rng).sample_iter(&Alphanumeric).take(10).collect()
}

[goffrie]

I suppose it could be written as (&mut &mut *rng), but that seems like a rather pointless transformation.

[ejmg]

Just wanted to chime in:

I've been working a lot more with rand, and this pattern creeped into my code on each occasion without me realizing it; each time I started getting snagged by weird E0596 error messages, I thought I completely forgot how to program in rust until I realized I fell into the same unthinking habit trusting rust's error messages (generally a good strategy, of course) and slowly injected this mut anti-pattern(?) into my code.

In reflection, this is the not first case I've hit this; I think I did this to myself ~5 months ago, but it was far enough back that I clearly did not learn my lesson :slightly_smiling_face:. After googling it again tonight to further understand why it happens and finding this issue, I think adding a lint for this is a great idea; especially for those of us that know to trust rust's type errors and clippy/rustfmt suggestions, but are still inexperienced enough to get driven accidentally into a confusing uses of mut and &mut around function parameters and argument passing.

That all said, I think tonight is the last time I accidentally trip on this so readily :sweat_smile: