Auto merge of rust-lang#11207 - Centri3:rust-lang#11182, r=giraffate

[`needless_pass_by_ref_mut`]: Do not lint if passed as a fn-like argument

Fixes rust-lang#11182 and also fixes rust-lang#11199 (though this is kind of a duplicate)

There's likely a case or two I've missed, so this likely needs a bit more work but it seems to work fine with the tests I've added.

PS, the diff for the test is useless because it iterates over a hashmap before linting. Seems to work fine but we could maybe change this for consistency's sake

changelog: [`needless_pass_by_ref_mut`]: No longer lints if the function is passed as a fn-like argument

Author: bors

clippy_lints/src/needless_pass_by_ref_mut.rs

@@ -1,16 +1,16 @@
 use super::needless_pass_by_value::requires_exact_signature;
-use clippy_utils::diagnostics::span_lint_and_then;
+use clippy_utils::diagnostics::span_lint_hir_and_then;
 use clippy_utils::source::snippet;
-use clippy_utils::{is_from_proc_macro, is_self};
-use if_chain::if_chain;
-use rustc_data_structures::fx::FxHashSet;
+use clippy_utils::{get_parent_node, is_from_proc_macro, is_self};
+use rustc_data_structures::fx::{FxHashSet, FxIndexMap};
 use rustc_errors::Applicability;
-use rustc_hir::intravisit::FnKind;
-use rustc_hir::{Body, FnDecl, HirId, HirIdMap, HirIdSet, Impl, ItemKind, Mutability, Node, PatKind};
+use rustc_hir::intravisit::{walk_qpath, FnKind, Visitor};
+use rustc_hir::{Body, ExprKind, FnDecl, HirId, HirIdMap, HirIdSet, Impl, ItemKind, Mutability, Node, PatKind, QPath};
 use rustc_hir_typeck::expr_use_visitor as euv;
 use rustc_infer::infer::TyCtxtInferExt;
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_middle::hir::map::associated_body;
+use rustc_middle::hir::nested_filter::OnlyBodies;
 use rustc_middle::mir::FakeReadCause;
 use rustc_middle::ty::{self, Ty, UpvarId, UpvarPath};
 use rustc_session::{declare_tool_lint, impl_lint_pass};
@@ -48,20 +48,24 @@ declare_clippy_lint! {
     "using a `&mut` argument when it's not mutated"
 }
 
-#[derive(Copy, Clone)]
-pub struct NeedlessPassByRefMut {
+#[derive(Clone)]
+pub struct NeedlessPassByRefMut<'tcx> {
     avoid_breaking_exported_api: bool,
+    used_fn_def_ids: FxHashSet<LocalDefId>,
+    fn_def_ids_to_maybe_unused_mut: FxIndexMap<LocalDefId, Vec<rustc_hir::Ty<'tcx>>>,
 }
 
-impl NeedlessPassByRefMut {
+impl NeedlessPassByRefMut<'_> {
     pub fn new(avoid_breaking_exported_api: bool) -> Self {
         Self {
             avoid_breaking_exported_api,
+            used_fn_def_ids: FxHashSet::default(),
+            fn_def_ids_to_maybe_unused_mut: FxIndexMap::default(),
         }
     }
 }
 
-impl_lint_pass!(NeedlessPassByRefMut => [NEEDLESS_PASS_BY_REF_MUT]);
+impl_lint_pass!(NeedlessPassByRefMut<'_> => [NEEDLESS_PASS_BY_REF_MUT]);
 
 fn should_skip<'tcx>(
     cx: &LateContext<'tcx>,
@@ -89,12 +93,12 @@ fn should_skip<'tcx>(
     is_from_proc_macro(cx, &input)
 }
 
-impl<'tcx> LateLintPass<'tcx> for NeedlessPassByRefMut {
+impl<'tcx> LateLintPass<'tcx> for NeedlessPassByRefMut<'tcx> {
     fn check_fn(
         &mut self,
         cx: &LateContext<'tcx>,
         kind: FnKind<'tcx>,
-        decl: &'tcx FnDecl<'_>,
+        decl: &'tcx FnDecl<'tcx>,
         body: &'tcx Body<'_>,
         span: Span,
         fn_def_id: LocalDefId,
@@ -140,7 +144,6 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessPassByRefMut {
         if it.peek().is_none() {
             return;
         }
-
         // Collect variables mutably used and spans which will need dereferencings from the
         // function body.
         let MutablyUsedVariablesCtxt { mutably_used_vars, .. } = {
@@ -165,30 +168,45 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessPassByRefMut {
             }
             ctx
         };
-
-        let show_semver_warning = self.avoid_breaking_exported_api && cx.effective_visibilities.is_exported(fn_def_id);
         for ((&input, &_), arg) in it {
             // Only take `&mut` arguments.
-            if_chain! {
-                if let PatKind::Binding(_, canonical_id, ..) = arg.pat.kind;
-                if !mutably_used_vars.contains(&canonical_id);
-                if let rustc_hir::TyKind::Ref(_, inner_ty) = input.kind;
-                then {
-                    // If the argument is never used mutably, we emit the warning.
-                    let sp = input.span;
-                    span_lint_and_then(
+            if let PatKind::Binding(_, canonical_id, ..) = arg.pat.kind
+                && !mutably_used_vars.contains(&canonical_id)
+            {
+                self.fn_def_ids_to_maybe_unused_mut.entry(fn_def_id).or_default().push(input);
+            }
+        }
+    }
+
+    fn check_crate_post(&mut self, cx: &LateContext<'tcx>) {
+        cx.tcx.hir().visit_all_item_likes_in_crate(&mut FnNeedsMutVisitor {
+            cx,
+            used_fn_def_ids: &mut self.used_fn_def_ids,
+        });
+
+        for (fn_def_id, unused) in self
+            .fn_def_ids_to_maybe_unused_mut
+            .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);
+
+            for input in unused {
+                // If the argument is never used mutably, we emit the warning.
+                let sp = input.span;
+                if let rustc_hir::TyKind::Ref(_, inner_ty) = input.kind {
+                    span_lint_hir_and_then(
                         cx,
                         NEEDLESS_PASS_BY_REF_MUT,
+                        cx.tcx.hir().local_def_id_to_hir_id(*fn_def_id),
                         sp,
                         "this argument is a mutable reference, but not used mutably",
                         |diag| {
                             diag.span_suggestion(
                                 sp,
                                 "consider changing to".to_string(),
-                                format!(
-                                    "&{}",
-                                    snippet(cx, cx.tcx.hir().span(inner_ty.ty.hir_id), "_"),
-                                ),
+                                format!("&{}", snippet(cx, cx.tcx.hir().span(inner_ty.ty.hir_id), "_"),),
                                 Applicability::Unspecified,
                             );
                             if show_semver_warning {
@@ -316,3 +334,44 @@ impl<'tcx> euv::Delegate<'tcx> for MutablyUsedVariablesCtxt {
         self.prev_bind = Some(id);
     }
 }
+
+/// A final pass to check for paths referencing this function that require the argument to be
+/// `&mut`, basically if the function is ever used as a `fn`-like argument.
+struct FnNeedsMutVisitor<'a, 'tcx> {
+    cx: &'a LateContext<'tcx>,
+    used_fn_def_ids: &'a mut FxHashSet<LocalDefId>,
+}
+
+impl<'tcx> Visitor<'tcx> for FnNeedsMutVisitor<'_, 'tcx> {
+    type NestedFilter = OnlyBodies;
+
+    fn nested_visit_map(&mut self) -> Self::Map {
+        self.cx.tcx.hir()
+    }
+
+    fn visit_qpath(&mut self, qpath: &'tcx QPath<'tcx>, hir_id: HirId, _: Span) {
+        walk_qpath(self, qpath, hir_id);
+
+        let Self { cx, used_fn_def_ids } = self;
+
+        // #11182; do not lint if mutability is required elsewhere
+        if let Node::Expr(expr) = cx.tcx.hir().get(hir_id)
+            && let Some(parent) = get_parent_node(cx.tcx, expr.hir_id)
+            && let ty::FnDef(def_id, _) = cx.tcx.typeck(cx.tcx.hir().enclosing_body_owner(hir_id)).expr_ty(expr).kind()
+            && let Some(def_id) = def_id.as_local()
+        {
+            if let Node::Expr(e) = parent
+                && let ExprKind::Call(call, _) = e.kind
+                && call.hir_id == expr.hir_id
+            {
+                return;
+            }
+
+            // We don't need to check each argument individually as you cannot coerce a function
+            // taking `&mut` -> `&`, for some reason, so if we've gotten this far we know it's
+            // passed as a `fn`-like argument (or is unified) and should ignore every "unused"
+            // argument entirely
+            used_fn_def_ids.insert(def_id);
+        }
+    }
+}

tests/ui/infinite_loop.stderr

@@ -1,11 +1,3 @@
-error: this argument is a mutable reference, but not used mutably
-  --> $DIR/infinite_loop.rs:7:17
-   |
-LL | fn fn_mutref(i: &mut i32) {
-   |                 ^^^^^^^^ help: consider changing to: `&i32`
-   |
-   = note: `-D clippy::needless-pass-by-ref-mut` implied by `-D warnings`
-
 error: variables in the condition are not mutated in the loop body
   --> $DIR/infinite_loop.rs:20:11
    |
@@ -99,5 +91,13 @@ LL |     while y < 10 {
    = note: this loop contains `return`s or `break`s
    = help: rewrite it as `if cond { loop { } }`
 
+error: this argument is a mutable reference, but not used mutably
+  --> $DIR/infinite_loop.rs:7:17
+   |
+LL | fn fn_mutref(i: &mut i32) {
+   |                 ^^^^^^^^ help: consider changing to: `&i32`
+   |
+   = note: `-D clippy::needless-pass-by-ref-mut` implied by `-D warnings`
+
 error: aborting due to 12 previous errors
 

tests/ui/let_underscore_future.stderr

@@ -1,11 +1,3 @@
-error: this argument is a mutable reference, but not used mutably
-  --> $DIR/let_underscore_future.rs:11:35
-   |
-LL | fn do_something_to_future(future: &mut impl Future<Output = ()>) {}
-   |                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider changing to: `&impl Future<Output = ()>`
-   |
-   = note: `-D clippy::needless-pass-by-ref-mut` implied by `-D warnings`
-
 error: non-binding `let` on a future
   --> $DIR/let_underscore_future.rs:14:5
    |
@@ -31,5 +23,13 @@ LL |     let _ = future;
    |
    = help: consider awaiting the future or dropping explicitly with `std::mem::drop`
 
+error: this argument is a mutable reference, but not used mutably
+  --> $DIR/let_underscore_future.rs:11:35
+   |
+LL | fn do_something_to_future(future: &mut impl Future<Output = ()>) {}
+   |                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider changing to: `&impl Future<Output = ()>`
+   |
+   = note: `-D clippy::needless-pass-by-ref-mut` implied by `-D warnings`
+
 error: aborting due to 4 previous errors
 

tests/ui/mut_key.stderr

@@ -12,14 +12,6 @@ error: mutable key type
 LL | fn should_not_take_this_arg(m: &mut HashMap<Key, usize>, _n: usize) -> HashSet<Key> {
    |                                                                        ^^^^^^^^^^^^
 
-error: this argument is a mutable reference, but not used mutably
-  --> $DIR/mut_key.rs:31:32
-   |
-LL | fn should_not_take_this_arg(m: &mut HashMap<Key, usize>, _n: usize) -> HashSet<Key> {
-   |                                ^^^^^^^^^^^^^^^^^^^^^^^^ help: consider changing to: `&HashMap<Key, usize>`
-   |
-   = note: `-D clippy::needless-pass-by-ref-mut` implied by `-D warnings`
-
 error: mutable key type
   --> $DIR/mut_key.rs:32:5
    |
@@ -110,5 +102,13 @@ error: mutable key type
 LL |     let _map = HashMap::<Arc<Cell<usize>>, usize>::new();
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
+error: this argument is a mutable reference, but not used mutably
+  --> $DIR/mut_key.rs:31:32
+   |
+LL | fn should_not_take_this_arg(m: &mut HashMap<Key, usize>, _n: usize) -> HashSet<Key> {
+   |                                ^^^^^^^^^^^^^^^^^^^^^^^^ help: consider changing to: `&HashMap<Key, usize>`
+   |
+   = note: `-D clippy::needless-pass-by-ref-mut` implied by `-D warnings`
+
 error: aborting due to 18 previous errors
 

tests/ui/mut_reference.stderr

@@ -1,17 +1,3 @@
-error: this argument is a mutable reference, but not used mutably
-  --> $DIR/mut_reference.rs:4:33
-   |
-LL | fn takes_a_mutable_reference(a: &mut i32) {}
-   |                                 ^^^^^^^^ help: consider changing to: `&i32`
-   |
-   = note: `-D clippy::needless-pass-by-ref-mut` implied by `-D warnings`
-
-error: this argument is a mutable reference, but not used mutably
-  --> $DIR/mut_reference.rs:11:44
-   |
-LL |     fn takes_a_mutable_reference(&self, a: &mut i32) {}
-   |                                            ^^^^^^^^ help: consider changing to: `&i32`
-
 error: the function `takes_an_immutable_reference` doesn't need a mutable reference
   --> $DIR/mut_reference.rs:17:34
    |
@@ -32,5 +18,13 @@ error: the method `takes_an_immutable_reference` doesn't need a mutable referenc
 LL |     my_struct.takes_an_immutable_reference(&mut 42);
    |                                            ^^^^^^^
 
-error: aborting due to 5 previous errors
+error: this argument is a mutable reference, but not used mutably
+  --> $DIR/mut_reference.rs:11:44
+   |
+LL |     fn takes_a_mutable_reference(&self, a: &mut i32) {}
+   |                                            ^^^^^^^^ help: consider changing to: `&i32`
+   |
+   = note: `-D clippy::needless-pass-by-ref-mut` implied by `-D warnings`
+
+error: aborting due to 4 previous errors
 

tests/ui/needless_pass_by_ref_mut.rs

@@ -1,4 +1,5 @@
-#![allow(unused)]
+#![allow(clippy::if_same_then_else, clippy::no_effect)]
+#![feature(lint_reasons)]
 
 use std::ptr::NonNull;
 
@@ -155,15 +156,48 @@ async fn a8(x: i32, a: &mut i32, y: i32, z: &mut i32) {
     println!("{:?}", z);
 }
 
+// Should not warn (passed as closure which takes `&mut`).
+fn passed_as_closure(s: &mut u32) {}
+
+// Should not warn.
+fn passed_as_local(s: &mut u32) {}
+
+// Should not warn.
+fn ty_unify_1(s: &mut u32) {}
+
+// Should not warn.
+fn ty_unify_2(s: &mut u32) {}
+
+// Should not warn.
+fn passed_as_field(s: &mut u32) {}
+
+fn closure_takes_mut(s: fn(&mut u32)) {}
+
+struct A {
+    s: fn(&mut u32),
+}
+
+// Should warn.
+fn used_as_path(s: &mut u32) {}
+
+// Make sure lint attributes work fine
+#[expect(clippy::needless_pass_by_ref_mut)]
+fn lint_attr(s: &mut u32) {}
+
 fn main() {
-    // let mut u = 0;
-    // let mut v = vec![0];
-    // foo(&mut v, &0, &mut u);
-    // foo2(&mut v);
-    // foo3(&mut v);
-    // foo4(&mut v);
-    // foo5(&mut v);
-    // alias_check(&mut v);
-    // alias_check2(&mut v);
-    // println!("{u}");
+    let mut u = 0;
+    let mut v = vec![0];
+    foo(&mut v, &0, &mut u);
+    foo2(&mut v);
+    foo3(&mut v);
+    foo4(&mut v);
+    foo5(&mut v);
+    alias_check(&mut v);
+    alias_check2(&mut v);
+    println!("{u}");
+    closure_takes_mut(passed_as_closure);
+    A { s: passed_as_field };
+    used_as_path;
+    let _: fn(&mut u32) = passed_as_local;
+    let _ = if v[0] == 0 { ty_unify_1 } else { ty_unify_2 };
 }