Suggests removed "unused" mut when not actually able to be removed

[emmiegit]

Summary

Clippy will suggest that mut be removed from function references if it is not used, even if the mutability cannot be removed because of function type requirements.

Lint Name

clippy::needless_pass_by_ref_mut

Reproducer

The codebase this occurs in is rather complicated, so I reduced it to a simple test case which reproduces the same false positive.

#[derive(Debug)]
struct Context {
    a: i32,
    b: i32,
}

type ActionFn = fn(&mut Context);

#[derive(Debug)]
struct Action {
    name: &'static str,
    call: ActionFn,
}

fn action_foo(ctx: &mut Context) {
    println!("Uses r/w access to parameter.");
    ctx.a += 1;
}

fn action_bar(ctx: &mut Context) {
    println!("Only needs r/o access, but must be &mut to match the function definition.");
    let _ = ctx.a * ctx.b;
}

fn main() {
    let action1 = Action {
        name: "Foo",
        call: action_foo,
    };

    let action2 = Action {
        name: "Bar",
        call: action_bar,
    };

    run_action(&action1);
    run_action(&action2);
}

fn run_action(action: &Action) {
    println!("Running action {}", action.name);
    let mut ctx = Context { a: 100, b: -20 };
    (action.call)(&mut ctx);
}

Here we have a fixed object Action which expects a specific type for its function pointer. However, while most implementers need for full mutable access, some do not, which then triggers the following Clippy lint:

warning: this argument is a mutable reference, but not used mutably
  --> src/main.rs:20:20
   |
20 | fn action_bar(ctx: &mut Context) {
   |                    ^^^^^^^^^^^^ help: consider changing to: `&Context`
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_pass_by_ref_mut
   = note: `#[warn(clippy::needless_pass_by_ref_mut)]` on by default

warning: `test2` (bin "test2") generated 1 warning
    Finished dev [unoptimized + debuginfo] target(s) in 0.00s

However, this is incorrect, as the mut cannot actually be removed, as this will change the function signature, causing a compiler error. That is, if I change line 20 to fn action_bar(ctx: &Context), then I get:

    Checking test2 v0.1.0 (/tmp/emmie/test2)
error[E0308]: mismatched types
  --> src/main.rs:33:15
   |
33 |         call: action_bar,
   |               ^^^^^^^^^^ types differ in mutability
   |
   = note: expected fn pointer `for<'a> fn(&'a mut Context)`
                 found fn item `for<'a> fn(&'a Context) {action_bar}`
   = note: when the arguments and return types match, functions can be coerced to function pointers

For more information about this error, try `rustc --explain E0308`.
error: could not compile `test2` (bin "test2") due to previous error

Because there is a legitimate type incompatibility reason why mut cannot be removed here, Clippy should not be recommending its removal.

Version

rustc 1.73.0-nightly (39f42ad9e 2023-07-19)
binary: rustc
commit-hash: 39f42ad9e8430a8abb06c262346e89593278c515
commit-date: 2023-07-19
host: x86_64-unknown-linux-gnu
release: 1.73.0-nightly
LLVM version: 16.0.5

clippy 0.1.73 (39f42ad 2023-07-19)

Additional Labels

@rustbot label +I-suggestion-causes-error

[y21]

As far as I can tell this is the same issue as #11182. Clippy should probably check if the function is being used as a value (i.e. if the expression is a path but not used as a function call, or perhaps checking if the fn item adjusts to a fn pointer anywhere), and not lint if so, because it might be assigned to a local with a different type annotation or passed to a function expecting a fn pointer with a mutable reference. It basically boils down to:

fn f(x: &mut i32) {}

let _: fn(&mut i32) = f; // mutable reference in `fn f` is in fact required for this to pass typeck

[emmiegit]

Ah I see, that is a further-reduced form which essentially has the same problem. I was not aware of that other issue, but it does seem they are likely caused by the same or similar underlying issue.