Auto merge of rust-lang#11647 - flip1995:needless-pass-by-ref-mut-pub…

…-api, r=xFrednet Honor `avoid-breaking-exported-api` in `needless_pass_by_ref_mut` Until now, the lint only emitted a warning, when breaking public API. Now it doesn't lint at all when the config value is not set to `false`, bringing it in line with the other lints using this config value. Also ensures that this config value is documented in the lint. changelog: none (I don't think a changelog is necessary, since this lint is in `nursery`) --- Fixes rust-lang/rust-clippy#11374 cc `@GuillaumeGomez` Marking as draft: Does this lint even break public API? If I change a function signature from `fn foo(x: &mut T)` to `fn foo(x: &T)`, I can still call it with `foo(&mut x)`. The only "breaking" thing is that the `clippy::unnecessary_mut_passed` lint will complain that `&mut` at the callsite is not necessary, possibly trickling down to the crate user having to remote a `mut` from a variable. [Playground](https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=058165a7663902e84af1d23e35c10d66). Are there examples where this actually breaks public API, that I'm missing?
flip1995 · Jul 3, 2024 · 918ae1b · 918ae1b
2 parents 3af2005 + ae47b97
commit 918ae1b
Show file tree

Hide file tree

Showing 12 changed files with 86 additions and 54 deletions.
diff --git a/book/src/lint_configuration.md b/book/src/lint_configuration.md
@@ -348,6 +348,7 @@ Suppress lints whenever the suggested change would cause breakage for other crat
 * [`enum_variant_names`](https://rust-lang.github.io/rust-clippy/master/index.html#enum_variant_names)
 * [`large_types_passed_by_value`](https://rust-lang.github.io/rust-clippy/master/index.html#large_types_passed_by_value)
 * [`linkedlist`](https://rust-lang.github.io/rust-clippy/master/index.html#linkedlist)
+* [`needless_pass_by_ref_mut`](https://rust-lang.github.io/rust-clippy/master/index.html#needless_pass_by_ref_mut)
 * [`option_option`](https://rust-lang.github.io/rust-clippy/master/index.html#option_option)
 * [`rc_buffer`](https://rust-lang.github.io/rust-clippy/master/index.html#rc_buffer)
 * [`rc_mutex`](https://rust-lang.github.io/rust-clippy/master/index.html#rc_mutex)

diff --git a/clippy_config/src/conf.rs b/clippy_config/src/conf.rs
@@ -263,7 +263,7 @@ define_Conf! {
     /// arithmetic-side-effects-allowed-unary = ["SomeType", "AnotherType"]
     /// ```
     (arithmetic_side_effects_allowed_unary: FxHashSet<String> = <_>::default()),
-    /// Lint: ENUM_VARIANT_NAMES, LARGE_TYPES_PASSED_BY_VALUE, TRIVIALLY_COPY_PASS_BY_REF, UNNECESSARY_WRAPS, UNUSED_SELF, UPPER_CASE_ACRONYMS, WRONG_SELF_CONVENTION, BOX_COLLECTION, REDUNDANT_ALLOCATION, RC_BUFFER, VEC_BOX, OPTION_OPTION, LINKEDLIST, RC_MUTEX, UNNECESSARY_BOX_RETURNS, SINGLE_CALL_FN.
+    /// Lint: ENUM_VARIANT_NAMES, LARGE_TYPES_PASSED_BY_VALUE, TRIVIALLY_COPY_PASS_BY_REF, UNNECESSARY_WRAPS, UNUSED_SELF, UPPER_CASE_ACRONYMS, WRONG_SELF_CONVENTION, BOX_COLLECTION, REDUNDANT_ALLOCATION, RC_BUFFER, VEC_BOX, OPTION_OPTION, LINKEDLIST, RC_MUTEX, UNNECESSARY_BOX_RETURNS, SINGLE_CALL_FN, NEEDLESS_PASS_BY_REF_MUT.
     ///
     /// Suppress lints whenever the suggested change would cause breakage for other crates.
     (avoid_breaking_exported_api: bool = true),

diff --git a/clippy_lints/src/needless_pass_by_ref_mut.rs b/clippy_lints/src/needless_pass_by_ref_mut.rs
@@ -27,7 +27,7 @@ declare_clippy_lint! {
     /// Check if a `&mut` function argument is actually used mutably.
     ///
     /// Be careful if the function is publicly reexported as it would break compatibility with
-    /// users of this function.
+    /// users of this function, when the users pass this function as an argument.
     ///
     /// ### Why is this bad?
     /// Less `mut` means less fights with the borrow checker. It can also lead to more
@@ -138,6 +138,10 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessPassByRefMut<'tcx> {
             return;
         }
 
+        if self.avoid_breaking_exported_api && cx.effective_visibilities.is_exported(fn_def_id) {
+            return;
+        }
+
         let hir_id = cx.tcx.local_def_id_to_hir_id(fn_def_id);
         let is_async = match kind {
             FnKind::ItemFn(.., header) => {
@@ -262,9 +266,6 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessPassByRefMut<'tcx> {
             .iter()
             .filter(|(def_id, _)| !self.used_fn_def_ids.contains(def_id))
         {
-            let show_semver_warning =
-                self.avoid_breaking_exported_api && cx.effective_visibilities.is_exported(*fn_def_id);
-
             let mut is_cfged = None;
             for input in unused {
                 // If the argument is never used mutably, we emit the warning.
@@ -284,7 +285,7 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessPassByRefMut<'tcx> {
                                 format!("&{}", snippet(cx, cx.tcx.hir().span(inner_ty.ty.hir_id), "_"),),
                                 Applicability::Unspecified,
                             );
-                            if show_semver_warning {
+                            if cx.effective_visibilities.is_exported(*fn_def_id) {
                                 diag.warn("changing this function will impact semver compatibility");
                             }
                             if *is_cfged {

diff --git a/tests/ui-toml/needless_pass_by_ref_mut/clippy.toml b/tests/ui-toml/needless_pass_by_ref_mut/clippy.toml
@@ -0,0 +1 @@
+avoid-breaking-exported-api = false
diff --git a/tests/ui-toml/needless_pass_by_ref_mut/needless_pass_by_ref_mut.fixed b/tests/ui-toml/needless_pass_by_ref_mut/needless_pass_by_ref_mut.fixed
@@ -0,0 +1,10 @@
+#![warn(clippy::needless_pass_by_ref_mut)]
+#![allow(clippy::ptr_arg)]
+
+// Should warn
+pub fn pub_foo(s: &Vec<u32>, b: &u32, x: &mut u32) {
+    //~^ ERROR: this argument is a mutable reference, but not used mutably
+    *x += *b + s.len() as u32;
+}
+
+fn main() {}
diff --git a/tests/ui-toml/needless_pass_by_ref_mut/needless_pass_by_ref_mut.rs b/tests/ui-toml/needless_pass_by_ref_mut/needless_pass_by_ref_mut.rs
@@ -0,0 +1,10 @@
+#![warn(clippy::needless_pass_by_ref_mut)]
+#![allow(clippy::ptr_arg)]
+
+// Should warn
+pub fn pub_foo(s: &mut Vec<u32>, b: &u32, x: &mut u32) {
+    //~^ ERROR: this argument is a mutable reference, but not used mutably
+    *x += *b + s.len() as u32;
+}
+
+fn main() {}
diff --git a/tests/ui-toml/needless_pass_by_ref_mut/needless_pass_by_ref_mut.stderr b/tests/ui-toml/needless_pass_by_ref_mut/needless_pass_by_ref_mut.stderr
@@ -0,0 +1,12 @@
+error: this argument is a mutable reference, but not used mutably
+  --> tests/ui-toml/needless_pass_by_ref_mut/needless_pass_by_ref_mut.rs:5:19
+   |
+LL | pub fn pub_foo(s: &mut Vec<u32>, b: &u32, x: &mut u32) {
+   |                   ^^^^^^^^^^^^^ help: consider changing to: `&Vec<u32>`
+   |
+   = warning: changing this function will impact semver compatibility
+   = note: `-D clippy::needless-pass-by-ref-mut` implied by `-D warnings`
+   = help: to override `-D warnings` add `#[allow(clippy::needless_pass_by_ref_mut)]`
+
+error: aborting due to 1 previous error
+
diff --git a/tests/ui/needless_pass_by_ref_mut.rs b/tests/ui/needless_pass_by_ref_mut.rs
@@ -232,43 +232,48 @@ async fn async_vec2(b: &mut Vec<bool>) {
 }
 fn non_mut(n: &str) {}
 //Should warn
-pub async fn call_in_closure1(n: &mut str) {
+async fn call_in_closure1(n: &mut str) {
     (|| non_mut(n))()
 }
 fn str_mut(str: &mut String) -> bool {
     str.pop().is_some()
 }
 //Should not warn
-pub async fn call_in_closure2(str: &mut String) {
+async fn call_in_closure2(str: &mut String) {
     (|| str_mut(str))();
 }
 
 // Should not warn.
-pub async fn closure(n: &mut usize) -> impl '_ + FnMut() {
+async fn closure(n: &mut usize) -> impl '_ + FnMut() {
     || {
         *n += 1;
     }
 }
 
 // Should warn.
-pub fn closure2(n: &mut usize) -> impl '_ + FnMut() -> usize {
+fn closure2(n: &mut usize) -> impl '_ + FnMut() -> usize {
     //~^ ERROR: this argument is a mutable reference, but not used mutably
     || *n + 1
 }
 
 // Should not warn.
-pub async fn closure3(n: &mut usize) {
+async fn closure3(n: &mut usize) {
     (|| *n += 1)();
 }
 
 // Should warn.
-pub async fn closure4(n: &mut usize) {
+async fn closure4(n: &mut usize) {
     //~^ ERROR: this argument is a mutable reference, but not used mutably
     (|| {
         let _x = *n + 1;
     })();
 }
 
+// Should not warn: pub
+pub fn pub_foo(s: &mut Vec<u32>, b: &u32, x: &mut u32) {
+    *x += *b + s.len() as u32;
+}
+
 // Should not warn.
 async fn _f(v: &mut Vec<()>) {
     let x = || v.pop();
@@ -365,4 +370,5 @@ fn main() {
     used_as_path;
     let _: fn(&mut u32) = passed_as_local;
     let _ = if v[0] == 0 { ty_unify_1 } else { ty_unify_2 };
+    pub_foo(&mut v, &0, &mut u);
 }
diff --git a/tests/ui/needless_pass_by_ref_mut.stderr b/tests/ui/needless_pass_by_ref_mut.stderr
@@ -108,109 +108,103 @@ LL | async fn inner_async3(x: &mut i32, y: &mut u32) {
    |                          ^^^^^^^^ help: consider changing to: `&i32`
 
 error: this argument is a mutable reference, but not used mutably
-  --> tests/ui/needless_pass_by_ref_mut.rs:235:34
+  --> tests/ui/needless_pass_by_ref_mut.rs:235:30
    |
-LL | pub async fn call_in_closure1(n: &mut str) {
-   |                                  ^^^^^^^^ help: consider changing to: `&str`
-   |
-   = warning: changing this function will impact semver compatibility
+LL | async fn call_in_closure1(n: &mut str) {
+   |                              ^^^^^^^^ help: consider changing to: `&str`
 
 error: this argument is a mutable reference, but not used mutably
-  --> tests/ui/needless_pass_by_ref_mut.rs:254:20
-   |
-LL | pub fn closure2(n: &mut usize) -> impl '_ + FnMut() -> usize {
-   |                    ^^^^^^^^^^ help: consider changing to: `&usize`
+  --> tests/ui/needless_pass_by_ref_mut.rs:254:16
    |
-   = warning: changing this function will impact semver compatibility
+LL | fn closure2(n: &mut usize) -> impl '_ + FnMut() -> usize {
+   |                ^^^^^^^^^^ help: consider changing to: `&usize`
 
 error: this argument is a mutable reference, but not used mutably
-  --> tests/ui/needless_pass_by_ref_mut.rs:265:26
-   |
-LL | pub async fn closure4(n: &mut usize) {
-   |                          ^^^^^^^^^^ help: consider changing to: `&usize`
+  --> tests/ui/needless_pass_by_ref_mut.rs:265:22
    |
-   = warning: changing this function will impact semver compatibility
+LL | async fn closure4(n: &mut usize) {
+   |                      ^^^^^^^^^^ help: consider changing to: `&usize`
 
 error: this argument is a mutable reference, but not used mutably
-  --> tests/ui/needless_pass_by_ref_mut.rs:314:12
+  --> tests/ui/needless_pass_by_ref_mut.rs:319:12
    |
 LL |     fn bar(&mut self) {}
    |            ^^^^^^^^^ help: consider changing to: `&self`
 
 error: this argument is a mutable reference, but not used mutably
-  --> tests/ui/needless_pass_by_ref_mut.rs:316:18
+  --> tests/ui/needless_pass_by_ref_mut.rs:321:18
    |
 LL |     async fn foo(&mut self, u: &mut i32, v: &mut u32) {
    |                  ^^^^^^^^^ help: consider changing to: `&self`
 
 error: this argument is a mutable reference, but not used mutably
-  --> tests/ui/needless_pass_by_ref_mut.rs:316:45
+  --> tests/ui/needless_pass_by_ref_mut.rs:321:45
    |
 LL |     async fn foo(&mut self, u: &mut i32, v: &mut u32) {
    |                                             ^^^^^^^^ help: consider changing to: `&u32`
 
 error: this argument is a mutable reference, but not used mutably
-  --> tests/ui/needless_pass_by_ref_mut.rs:324:46
+  --> tests/ui/needless_pass_by_ref_mut.rs:329:46
    |
 LL |     async fn foo2(&mut self, u: &mut i32, v: &mut u32) {
    |                                              ^^^^^^^^ help: consider changing to: `&u32`
 
 error: this argument is a mutable reference, but not used mutably
-  --> tests/ui/needless_pass_by_ref_mut.rs:340:18
+  --> tests/ui/needless_pass_by_ref_mut.rs:345:18
    |
 LL | fn _empty_tup(x: &mut (())) {}
    |                  ^^^^^^^^^ help: consider changing to: `&()`
 
 error: this argument is a mutable reference, but not used mutably
-  --> tests/ui/needless_pass_by_ref_mut.rs:341:19
+  --> tests/ui/needless_pass_by_ref_mut.rs:346:19
    |
 LL | fn _single_tup(x: &mut ((i32,))) {}
    |                   ^^^^^^^^^^^^^ help: consider changing to: `&(i32,)`
 
 error: this argument is a mutable reference, but not used mutably
-  --> tests/ui/needless_pass_by_ref_mut.rs:342:18
+  --> tests/ui/needless_pass_by_ref_mut.rs:347:18
    |
 LL | fn _multi_tup(x: &mut ((i32, u32))) {}
    |                  ^^^^^^^^^^^^^^^^^ help: consider changing to: `&(i32, u32)`
 
 error: this argument is a mutable reference, but not used mutably
-  --> tests/ui/needless_pass_by_ref_mut.rs:343:11
+  --> tests/ui/needless_pass_by_ref_mut.rs:348:11
    |
 LL | fn _fn(x: &mut (fn())) {}
    |           ^^^^^^^^^^^ help: consider changing to: `&fn()`
 
 error: this argument is a mutable reference, but not used mutably
-  --> tests/ui/needless_pass_by_ref_mut.rs:345:23
+  --> tests/ui/needless_pass_by_ref_mut.rs:350:23
    |
 LL | fn _extern_rust_fn(x: &mut extern "Rust" fn()) {}
    |                       ^^^^^^^^^^^^^^^^^^^^^^^ help: consider changing to: `&extern "Rust" fn()`
 
 error: this argument is a mutable reference, but not used mutably
-  --> tests/ui/needless_pass_by_ref_mut.rs:346:20
+  --> tests/ui/needless_pass_by_ref_mut.rs:351:20
    |
 LL | fn _extern_c_fn(x: &mut extern "C" fn()) {}
    |                    ^^^^^^^^^^^^^^^^^^^^ help: consider changing to: `&extern "C" fn()`
 
 error: this argument is a mutable reference, but not used mutably
-  --> tests/ui/needless_pass_by_ref_mut.rs:347:18
+  --> tests/ui/needless_pass_by_ref_mut.rs:352:18
    |
 LL | fn _unsafe_fn(x: &mut unsafe fn()) {}
    |                  ^^^^^^^^^^^^^^^^ help: consider changing to: `&unsafe fn()`
 
 error: this argument is a mutable reference, but not used mutably
-  --> tests/ui/needless_pass_by_ref_mut.rs:348:25
+  --> tests/ui/needless_pass_by_ref_mut.rs:353:25
    |
 LL | fn _unsafe_extern_fn(x: &mut unsafe extern "C" fn()) {}
    |                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider changing to: `&unsafe extern "C" fn()`
 
 error: this argument is a mutable reference, but not used mutably
-  --> tests/ui/needless_pass_by_ref_mut.rs:349:20
+  --> tests/ui/needless_pass_by_ref_mut.rs:354:20
    |
 LL | fn _fn_with_arg(x: &mut unsafe extern "C" fn(i32)) {}
    |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider changing to: `&unsafe extern "C" fn(i32)`
 
 error: this argument is a mutable reference, but not used mutably
-  --> tests/ui/needless_pass_by_ref_mut.rs:350:20
+  --> tests/ui/needless_pass_by_ref_mut.rs:355:20
    |
 LL | fn _fn_with_ret(x: &mut unsafe extern "C" fn() -> (i32)) {}
    |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider changing to: `&unsafe extern "C" fn() -> (i32)`

diff --git a/tests/ui/needless_pass_by_ref_mut2.fixed b/tests/ui/needless_pass_by_ref_mut2.fixed
@@ -5,15 +5,15 @@
 #![allow(clippy::redundant_closure_call)]
 #![warn(clippy::needless_pass_by_ref_mut)]
 
-pub async fn inner_async3(x: &i32, y: &mut u32) {
+async fn inner_async3(x: &i32, y: &mut u32) {
     //~^ ERROR: this argument is a mutable reference, but not used mutably
     async {
         *y += 1;
     }
     .await;
 }
 
-pub async fn inner_async4(u: &mut i32, v: &u32) {
+async fn inner_async4(u: &mut i32, v: &u32) {
     //~^ ERROR: this argument is a mutable reference, but not used mutably
     async {
         *u += 1;

diff --git a/tests/ui/needless_pass_by_ref_mut2.rs b/tests/ui/needless_pass_by_ref_mut2.rs
@@ -5,15 +5,15 @@
 #![allow(clippy::redundant_closure_call)]
 #![warn(clippy::needless_pass_by_ref_mut)]
 
-pub async fn inner_async3(x: &mut i32, y: &mut u32) {
+async fn inner_async3(x: &mut i32, y: &mut u32) {
     //~^ ERROR: this argument is a mutable reference, but not used mutably
     async {
         *y += 1;
     }
     .await;
 }
 
-pub async fn inner_async4(u: &mut i32, v: &mut u32) {
+async fn inner_async4(u: &mut i32, v: &mut u32) {
     //~^ ERROR: this argument is a mutable reference, but not used mutably
     async {
         *u += 1;

diff --git a/tests/ui/needless_pass_by_ref_mut2.stderr b/tests/ui/needless_pass_by_ref_mut2.stderr
@@ -1,20 +1,17 @@
 error: this argument is a mutable reference, but not used mutably
-  --> tests/ui/needless_pass_by_ref_mut2.rs:8:30
+  --> tests/ui/needless_pass_by_ref_mut2.rs:8:26
    |
-LL | pub async fn inner_async3(x: &mut i32, y: &mut u32) {
-   |                              ^^^^^^^^ help: consider changing to: `&i32`
+LL | async fn inner_async3(x: &mut i32, y: &mut u32) {
+   |                          ^^^^^^^^ help: consider changing to: `&i32`
    |
-   = warning: changing this function will impact semver compatibility
    = note: `-D clippy::needless-pass-by-ref-mut` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(clippy::needless_pass_by_ref_mut)]`
 
 error: this argument is a mutable reference, but not used mutably
-  --> tests/ui/needless_pass_by_ref_mut2.rs:16:43
+  --> tests/ui/needless_pass_by_ref_mut2.rs:16:39
    |
-LL | pub async fn inner_async4(u: &mut i32, v: &mut u32) {
-   |                                           ^^^^^^^^ help: consider changing to: `&u32`
-   |
-   = warning: changing this function will impact semver compatibility
+LL | async fn inner_async4(u: &mut i32, v: &mut u32) {
+   |                                       ^^^^^^^^ help: consider changing to: `&u32`
 
 error: aborting due to 2 previous errors