Add needless_pass_by_ref_mut lint

[GuillaumeGomez]

changelog: [needless_pass_by_ref_mut]: This PR add a new lint needless_pass_by_ref_mut which emits a warning in case a &mut function argument isn't used mutably. It doesn't warn on trait and trait impls functions.

Fixes #8863.

[rustbot]

r? @Jarcho

(rustbot has picked a reviewer for you, use r? to override)

[GuillaumeGomez]

I found two cases where it doesn't work:

fn foo2(s: &mut Vec<u32>) {
    s.push(8);
}

fn foo3(s: &mut Vec<u32>) -> &mut Vec<u32> {
    s
}

[GuillaumeGomez]

I fixed the two cases. If people could please check if I didn't miss other cases? :)

[Centri3]

I found a couple after running lintcheck:

https://github.com/dtolnay/anyhow/blob/1f17666ef1d4d3cd1f05a9ee5e79f75d66801c66/src/ptr.rs#LL151C1-L157C6
https://github.com/rust-random/rand/blob/0c4c6c02d818ec7ec6a9d7458297ca3993e4c2fe/rand_core/src/impls.rs#LL37C1-L53C2
https://github.com/rayon-rs/rayon/blob/2c9794343df0ac20643f27a9b9be6514f670a730/src/iter/map_with.rs#LL310C1-L325C6
https://github.com/dtolnay/syn/blob/35e6821d9f3b73a587e024aa5ff01357a2c090bf/src/token.rs#LL1036C1-L1052C6

The first (anyhow) is really bad, as it's necessary for it to be sound. We should probably consider creation of NonNull (or anything else with a mut raw pointer) as necessary.

Great work on this, though :)

[GuillaumeGomez]

Thanks a lot for the cases! For NonNull, I'll check what's going on.

[Centri3]

I know this isn't done yet, but I thought I'd give a small review anyway ^^

[GuillaumeGomez]

I know this isn't done yet, but I thought I'd give a small review anyway ^^

Very much appreciated, thanks! I'll fix them when I resume this work tomorrow.

[emilk]

I'd like to make sure these are also caught:

impl Bar {
    // Should warn about `vec`
    pub fn mushroom(&self, vec: &mut Vec<i32>) -> usize {
        vec.len()
    }

    // Should warn about `vec`
    pub fn badger(&mut self, vec: &mut Vec<i32>) -> usize {
        vec.len()
    }
}

Additionally, shouldn't we warn about &mut self too? e.g.

struct Foo {
    x: i32
}

impl Foo {
    pub fn get(&mut self) -> i32 {
        self.x
    }
}

---

I have never worked in this repository before - how do I easily test a single lint like this? cargo test has a bunch of unrelated failures on my machine (M1 Mac)

[Centri3]

I have never worked in this repository before - how do I easily test a single lint like this? cargo test has a bunch of unrelated failures on my machine (M1 Mac)

TESTNAME=<name> cargo uitest. The name just has to be a substring of any test's name, in this case needless_pass_by_ref_mut should work.

[GuillaumeGomez]

@emilk: I'll add them to the tests within this PR. The more I have, the better.

[GuillaumeGomez]

Additionally, shouldn't we warn about &mut self too? e.g.

This is tricky because you might want to actually force your object instance to be mutably borrowed even if you don't use it mutably internally. However, we can always add another lint (allow by default) which could check that too.

[GuillaumeGomez]

@Centri3 I was able to fix two of the four problems you linked (the alias and the NonNull::from). For the last two, I need to be able to make a small code to reproduce the issue. So to do that: how can I run my own clippy onto rayon for example? I didn't find explanations for that in the clippy book unfortunately...

[Centri3]

The easiest way is lintcheck; cargo lintcheck, if you want you can add more crates in lintcheck/lintcheck_crates.toml (though rayon should be there by default)

[GuillaumeGomez]

Thanks! That was quite useful and indeed, rayon was there. Allowed me to discover new corner cases. Continuing to move on this slowly but surely.

[bors]

☔ The latest upstream changes (presumably #10934) made this pull request unmergeable. Please resolve the merge conflicts.

[GuillaumeGomez]

From lintcheck, the remaining warnings seem legit. I'll add allow(needless_pass_by_ref_mut) in the failing clippy UI tests.

[GuillaumeGomez]

Just realized that avoid-breaking-exported-api is true by default. I don't think skipping this lint by default for public items is a good idea so I'll go for the first proposed solution instead.

Just in case, here's the diff to have the avoid-breaking-exported-api change:

From cdfb83030b2f78d672d9f6b08911cf8123edf338 Mon Sep 17 00:00:00 2001
From: Guillaume Gomez <guillaume1.gomez@gmail.com>
Date: Mon, 3 Jul 2023 22:19:32 +0200
Subject: [PATCH] Make NEEDLESS_PASS_BY_REF_MUT not emitted if
 `avoid-breaking-exported-api` option is enabled

---
 clippy_lints/src/lib.rs                       |  6 ++++-
 clippy_lints/src/needless_pass_by_ref_mut.rs  | 22 +++++++++++++++++--
 .../ui/should_impl_trait/method_list_2.stderr | 10 +--------
 3 files changed, 26 insertions(+), 12 deletions(-)

diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
index e1e4a83b0..040ed1a86 100644
--- a/clippy_lints/src/lib.rs
+++ b/clippy_lints/src/lib.rs
@@ -1058,7 +1058,11 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     let stack_size_threshold = conf.stack_size_threshold;
     store.register_late_pass(move |_| Box::new(large_stack_frames::LargeStackFrames::new(stack_size_threshold)));
     store.register_late_pass(|_| Box::new(single_range_in_vec_init::SingleRangeInVecInit));
-    store.register_late_pass(|_| Box::new(needless_pass_by_ref_mut::NeedlessPassByRefMut));
+    store.register_late_pass(move |_| {
+        Box::new(needless_pass_by_ref_mut::NeedlessPassByRefMut::new(
+            avoid_breaking_exported_api,
+        ))
+    });
     store.register_late_pass(|_| Box::new(incorrect_impls::IncorrectImpls));
     store.register_late_pass(move |_| {
         Box::new(single_call_fn::SingleCallFn {
diff --git a/clippy_lints/src/needless_pass_by_ref_mut.rs b/clippy_lints/src/needless_pass_by_ref_mut.rs
index c5dc87cdf..614b1f965 100644
--- a/clippy_lints/src/needless_pass_by_ref_mut.rs
+++ b/clippy_lints/src/needless_pass_by_ref_mut.rs
@@ -12,7 +12,7 @@ use rustc_infer::infer::TyCtxtInferExt;
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_middle::mir::FakeReadCause;
 use rustc_middle::ty::{self, Ty};
-use rustc_session::{declare_lint_pass, declare_tool_lint};
+use rustc_session::{declare_tool_lint, impl_lint_pass};
 use rustc_span::def_id::LocalDefId;
 use rustc_span::symbol::kw;
 use rustc_span::Span;
@@ -43,7 +43,21 @@ declare_clippy_lint! {
     suspicious,
     "using a `&mut` argument when it's not mutated"
 }
-declare_lint_pass!(NeedlessPassByRefMut => [NEEDLESS_PASS_BY_REF_MUT]);
+
+#[derive(Copy, Clone)]
+pub struct NeedlessPassByRefMut {
+    avoid_breaking_exported_api: bool,
+}
+
+impl NeedlessPassByRefMut {
+    pub fn new(avoid_breaking_exported_api: bool) -> Self {
+        Self {
+            avoid_breaking_exported_api,
+        }
+    }
+}
+
+impl_lint_pass!(NeedlessPassByRefMut => [NEEDLESS_PASS_BY_REF_MUT]);
 
 fn should_skip<'tcx>(
     cx: &LateContext<'tcx>,
@@ -81,6 +95,10 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessPassByRefMut {
         span: Span,
         fn_def_id: LocalDefId,
     ) {
+        if self.avoid_breaking_exported_api && cx.effective_visibilities.is_exported(fn_def_id) {
+            return;
+        }
+
         if span.from_expansion() {
             return;
         }
diff --git a/tests/ui/should_impl_trait/method_list_2.stderr b/tests/ui/should_impl_trait/method_list_2.stderr
index e47cb209b..10bfea68f 100644
--- a/tests/ui/should_impl_trait/method_list_2.stderr
+++ b/tests/ui/should_impl_trait/method_list_2.stderr
@@ -39,14 +39,6 @@ LL | |     }
    |
    = help: consider implementing the trait `std::hash::Hash` or choosing a less ambiguous method name
 
-error: this argument is a mutable reference, but not used mutably
-  --> $DIR/method_list_2.rs:38:31
-   |
-LL |     pub fn hash(&self, state: &mut T) {
-   |                               ^^^^^^ help: consider changing to: `&T`
-   |
-   = note: `-D clippy::needless-pass-by-ref-mut` implied by `-D warnings`
-
 error: method `index` can be confused for the standard trait method `std::ops::Index::index`
   --> $DIR/method_list_2.rs:42:5
    |
@@ -157,5 +149,5 @@ LL | |     }
    |
    = help: consider implementing the trait `std::ops::Sub` or choosing a less ambiguous method name
 
-error: aborting due to 16 previous errors
+error: aborting due to 15 previous errors
 
-- 
2.34.1

[llogiq]

Why do you think avoiding API breakage by default is a bad idea?

[Centri3]

Probably because it doesn't cause API breakage, just a clippy lint.

[emilk]

I just learned about avoid-breaking-exported-api now, and now I'm turning it off for my projects so I get more clippy lints :)

Whether or not avoid-breaking-exported-api should be on by default seems like a discussion outside the scope of this PR.

For consistency with how other lints work, it seems this lint should heed it too (just like similar lints do, e.g. unnecessary_wraps and unused_self)

[Centri3]

I disagree as changing &mut to & doesn't cause API breakage, you can pass &mut to a function expecting & just fine

[emilk]

I disagree as changing &mut to & doesn't cause API breakage, you can pass &mut to a function expecting & just fine

It's a good point, however:

If you have pub fn foo(bar: &mut Baz) { bar.non_mut_method() } you reserve the right to mutate bar in the future. If you follow the lint suggestion and change it to fn foo(bar: &Baz) you are not breaking the API, but you are changing the API so that you cannot mutate the argument in the future. Thus when you next release realize you want to mutate the argument, you have lost the ability to do so, unless you now introduce a breaking change by re-adding the mut.

[Centri3]

Those are the cases where you should probably allow the lint, though it isn't particularly vocal about this unfortunately (something should be added to the description noting this)

[llogiq]

Probably because it doesn't cause API breakage, just a clippy lint.

Would it be possible to shadow a trait method following the lint's suggestion? If so, it could break the build.

[llogiq]

That said, there is an argument to be made that we should make the default false for projects before 1.0 and true for those who have reached it 😀

[GuillaumeGomez]

That said, there is an argument to be made that we should make the default false for projects before 1.0 and true for those who have reached it grinning

I disagree with this position: clippy is supposed to tell me what it potential issues it sees, and then it's up to me to decide whether or not I apply them.

So something I can suggest is the following: if an item is publicly accessible, to not enable the possibility of automatically fixing it. What do you think about it?

[GuillaumeGomez]

So I did the following changes:

• Emit a warning in case it would change semver compatibility
• Making the lint not machine applicable ever: the reason why is simply as said previously: some code might not work after this change so better let users do this refactoring "by hand" to be sure they didn't miss something.

[Centri3]

Probably because it doesn't cause API breakage, just a clippy lint.

Would it be possible to shadow a trait method following the lint's suggestion? If so, it could break the build.

Most likely so, but I wouldn't consider this a "breaking change"; afaik that would be up to the library author to refactor (which, since it now isn't machine applicable, wouldn't be a big deal), as the only way I can think of for that to happen is the usage within the function itself, which changing shouldn't cause issues downstream

Perhaps something like &mut IterMut vs &IterMut as an argument (is that even used, though?)? Just first thing that came to mind. That might be able to break but I'm not sure if something like that is common enough to be an issue

[llogiq]

The reason for the avoid-breaking-exported-api is that we consider a warning that can only be fixed by allow or breaking code as a false positive, and we really want to avoid those. Actually breaking code indirectly like this is considered even worse, because the user may not be aware they're breaking their users' code. Now I agree that in this case the potential for breakage isn't that high (probably mostly due to traits implemented on &mut T, but not &T plus inference), and if the user gets a fair warning on public items, this is OK in my book.

Sorry I'm being pedantic here, but we've had some bad experience with users ditching clippy because of too many false positives, and we want this to be useful.

@bors r+

[bors]

📌 Commit f048f73 has been approved by llogiq

It is now in the queue for this repository.

[bors]

⌛ Testing commit f048f73 with merge b46033e...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: llogiq
Pushing b46033e to master...