New Lint: detect unused `&mut` references in function parameters

[leonardo-m]

Hi! Clippy in the Rust Playground spots this problem:

fn foo(x: &u32) {
    let _y = x;
}
fn main() {
    foo(&mut 10);
}


warning: The function/method `foo` doesn't need a mutable reference
note: `#[warn(clippy::unnecessary_mut_passed)]` on by default

But it doesn't find the inverted problem here:

fn foo(x: &mut u32) { // This doesn't need to take a mut
    let _y = x;
}
fn main() {
    foo(&mut 10);
}

While this is caught by Rustc too:

fn foo(mut x: u32) {
    let _y = x;
}
fn main() {
    foo(10);
}

warning: variable does not need to be mutable

[flip1995]

This would be an entirely new lint. Detecting if a mutable reference is actually necessary is way harder, than just detecting if a mutable reference is given to a function, that only takes a shared reference.

[Logarithmus]

@flip1995 can't clippy just temporary replace &mut with &, test if it still compiles, and if it is, then suggest a lint?

[flip1995]

Sadly not.

[Logarithmus]

@flip1995 why? Sorry, I'm not familiar with clippy's internals.

[flip1995]

Maybe a relevant info before the explanation: Clippy doesn't do the compilation. Clippy just calls rustc programmatically and uses the rustc queries to do the analysis. But all the heavy lifting, like parsing, type checking, ... is done by rustc

Clippy can only run one compiler pass. Doing something like this would mean that Clippy has to modify a file on disk and then run the compiler again, and after that revert the modification of the file. Perf concerns aside (we'd probably have to do this multiple times for multiple lints), this would be unexpected behavior of a linter from the users perspective. At least I wouldn't expect that my static analyzer modifies files on disk. rustfix and rustfmt sure, they have to modify files; but the linter... that would be a surprise.

Another option might be to only re-compile the function. But that wouldn't work because we'd had to manually rerun every rustc pass on that function. And rustc is not designed for this. Also specifically in this case, there might also be global concerns, because a function signature is changed.

Don't be sorry when asking questions. It's the best way to learn :)

[Logarithmus]

@flip1995 thanks for a detailed explanation. You are concerned that modifying source code files might confuse the user. What prevents clippy from copying them to a temp directory or loading into RAM, modifying the copies and then passing to rustc?

[flip1995]

Clippy would still create files on disk (in /tmp) or would use more RAM. You can't really compile a single file in Rust, like you do in C++. In Rust the smallest compilation unit is a crate (in simplified terms). So you'd have to copy the whole crate. And then you still need the context of the dependencies. At this point we'd almost be rebuilding cargo into Clippy.

I like to be strict here and keep the behavior that Clippy doesn't do anything regarding the file system. (Except for the target/ dir, which is also technically created by cargo/rustc and not by Clippy)

[y21]

In unsafe code, &mut self is often necessary even if it would compile with just &self because that would otherwise make it unsound, so I think we should be careful with how the lint interacts with unsafe code and perhaps not lint at all if the body contains any unsafe {}.

For example:

struct LeakyBox<T>(*mut T);
impl<T> LeakyBox<T> {
  pub fn new(v: T) -> Self {
    Self(Box::into_raw(Box::new(v)))
  }

  pub fn get_mut(&self) -> &mut T {
    unsafe { &mut *self.0 }
  }
}

This code compiles fine and LeakyBox::get_mut doesn't require &mut self to compile, but it's unsound, because this one doesn't assert uniqueness over the LeakyBox and lets you obtain two aliasing &muts. So this really should take &mut self.