needless_pass_by_ref_mut suggests (potentially unsound) code that triggers mut_from_ref

[taiki-e]

Summary

needless_pass_by_ref_mut (warn by default) suggests code that triggers mut_from_ref (error by default).

The suggested code itself is also suspicious from a soundness point of view.

Mentioning @GuillaumeGomez, who implement this lint in #10900.

Reproducer

I tried this code:

#![allow(dead_code)]

use std::ptr::NonNull;

struct Data<T: ?Sized> {
    value: T,
}

unsafe fn get_mut_unchecked<T>(ptr: &mut NonNull<Data<T>>) -> &mut T {
    unsafe { &mut (*ptr.as_ptr()).value }
}

This happened:

warning: this argument is a mutable reference, but not used mutably
 --> src/lib.rs:7:37
  |
7 | unsafe fn get_mut_unchecked<T>(ptr: &mut NonNull<Data<T>>) -> &mut T {
  |                                     ^^^^^^^^^^^^^^^^^^^^^ help: consider changing to: `&NonNull<Data<T>>`
  |
  = 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

And when I applied the suggestion:

#![allow(dead_code)]

use std::ptr::NonNull;

struct Data<T: ?Sized> {
    value: T,
}

unsafe fn get_mut_unchecked<T>(ptr: &NonNull<Data<T>>) -> &mut T {
    unsafe { &mut (*ptr.as_ptr()).value }
}

Then, this happened:

error: mutable borrow from immutable input(s)
 --> src/lib.rs:9:59
  |
9 | unsafe fn get_mut_unchecked<T>(ptr: &NonNull<Data<T>>) -> &mut T {
  |                                                           ^^^^^^
  |
note: immutable borrow here
 --> src/lib.rs:9:37
  |
9 | unsafe fn get_mut_unchecked<T>(ptr: &NonNull<Data<T>>) -> &mut T {
  |                                     ^^^^^^^^^^^^^^^^^
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#mut_from_ref
  = note: `#[deny(clippy::mut_from_ref)]` on by default

Version

rustc 1.73.0-nightly (da6b55cc5 2023-07-17)
binary: rustc
commit-hash: da6b55cc5eaf76ed6acb7dc2f7d611e32af7c9a7
commit-date: 2023-07-17
host: aarch64-apple-darwin
release: 1.73.0-nightly
LLVM version: 16.0.5

Additional Labels

@rustbot label +I-false-positive +I-suggestion-causes-error

[y21]

The soundness bit is probably the same concern I'd talked about here #5546 (comment), in that we should be careful how such lints interact with unsafe code and perhaps not lint at all if the function is marked unsafe or contains any unsafe blocks that involve the parameter

[sunng87]

My project pgwire is affected by this as well for safe code. The CI report is here: https://github.com/sunng87/pgwire/actions/runs/5622952044/job/15236578055

If I followed suggestion to remove mut from &mut C in query.rs:343, I will be getting compilation error:

error[E0596]: cannot borrow `*client` as mutable, as it is behind a `&` reference
   --> src/api/query.rs:349:5
    |
349 |     client
    |     ^^^^^^ `client` is a `&` reference, so the data it refers to cannot be borrowed as mutable
    |
help: consider specifying this binding's type
    |
343 | pub async fn send_execution_response<C>(client: &mut C: &C, tag: Tag) -> PgWireResult<()>
    |                                               ++++++++

[taiki-e]

@sunng87 I believe your case is #11179.

[GuillaumeGomez]

I think the simplest approach is the one suggested in #11586 (to not run this lint in unsafe block/function).