Auto merge of #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 https://github.com/rust-lang/rust-clippy/issues/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?
2024-07-03 07:55:05 +00:00 · 2024-07-03 07:55:05 +00:00 · 918ae1bec4
commit 918ae1bec4
parent 3af20058eb ae47b97655
12 changed files with 86 additions and 54 deletions
--- 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)
--- 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),
--- 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 {
--- 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
--- 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() {}
--- 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() {}
--- 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
+
--- 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);
 }
--- 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
+  --> tests/ui/needless_pass_by_ref_mut.rs:254:16
   |
-LL | pub fn closure2(n: &mut usize) -> impl '_ + FnMut() -> usize {
-   |                    ^^^^^^^^^^ help: consider changing to: `&usize`
-   |
-   = 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
+  --> tests/ui/needless_pass_by_ref_mut.rs:265:22
   |
-LL | pub async fn closure4(n: &mut usize) {
-   |                          ^^^^^^^^^^ help: consider changing to: `&usize`
-   |
-   = 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)`
--- a/tests/ui/needless_pass_by_ref_mut2.fixed
+++ b/tests/ui/needless_pass_by_ref_mut2.fixed
@ -5,7 +5,7 @@
 #![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;
@ -13,7 +13,7 @@ pub async fn inner_async3(x: &i32, y: &mut u32) {
    .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;
--- a/tests/ui/needless_pass_by_ref_mut2.rs
+++ b/tests/ui/needless_pass_by_ref_mut2.rs
@ -5,7 +5,7 @@
 #![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;
@ -13,7 +13,7 @@ pub async fn inner_async3(x: &mut i32, y: &mut u32) {
    .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;
--- 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