needless_pass_by_ref_mut should not trigger for cfg-gated mod/fn

[oxalica]

Summary

In target specific modules, some functions may only mutate arguments for a specific platform. Keeping the same signature to take &mut for all of them makes higher level abstractions simpler.

If we expose this platform differences, it will propagate upwards and force every users to #[cfg] between & and &mut. This violates the abstraction goal of the library.

A solution to this issue is to treat every #[cfg] fn and #[cfg] mod { pub fn } as part of public interface, and suppress warnings that would cause signature changes on them.

Lint Name

needless_pass_by_ref_mut

Reproducer

pub struct Resource(i32);

impl Resource {
    // Real public interface.
    pub fn work(&mut self) {
        imp::work(&mut self.0);
    }
}

// Semi-public interface. Abstractions over platforms.
#[cfg(unix)]
mod imp {
    // Immutable for some platforms.
    pub fn work(data: &mut i32) { // <- needless_pass_by_ref_mut
        println!("{data}");
    }
}

#[cfg(windows)]
mod imp {
    // Mutable for some other platforms.
    pub fn work(data: &mut i32) {
        *data += 1;
        println!("{data}");
    }
}

I saw this happen:

warning: this argument is a mutable reference, but not used mutably
  --> src/lib.rs:13:23
   |
14 |     pub fn work(data: &mut i32) { // <- needless_pass_by_ref_mut
   |                       ^^^^^^^^ help: consider changing to: `&i32`
   |
   = 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

I expected to see this happen:

Version

Rust Playground, nightly.
Clippy 0.1.73 (2023-07-18 903e279)

Additional Labels

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

I think this fits I-suggestion-causes-error because the change breaks on other cfg-gated platforms.

[Centri3]

cc @GuillaumeGomez

Likely you'll want to traverse up the DefId tree using parent_module/parent_module_from_def_id to see if any have a #[cfg(...)] attribute, plus the function in question ofc.

[GuillaumeGomez]

I don't think it's a good idea to disable the lint if we enter a cfg gated item. In this case, it enters the "I know what I'm doing, let's disable the lint for this item" zone.

[Centri3]

Then we could perhaps at least add a note then to allow it if it's inside a cfg, because this is likely rather common.

[GuillaumeGomez]

I don't understand what you mean by "add a note".

[Centri3]

Should've clarified; like with how show_semver_warning works already, but calling diag.note instead of diag.warn to say something like "this is cfg-gated and may require further changes"

[GuillaumeGomez]

Oh I see. Yes that sounds like a nice idea indeed.