needless_borrows_for_generic_args: Fix for &mut

This commit fixes a bug introduced in #12706, where the behavior of the
lint has been changed, to avoid suggestions that introduce a move. The
motivation in the commit message is quite poor (if the detection for
significant drops is not sufficient because it's not transitive, the
proper fix would be to make it transitive). However, #12454, the linked
issue, provides a good reason for the change — if the value being
borrowed is bound to a variable, then moving it will only introduce
friction into future refactorings.

Thus #12706 changes the logic so that the lint triggers if the value
being borrowed is Copy, or is the result of a function call, simplifying
the logic to the point where analysing "is this the only use of this
value" isn't necessary.

However, said PR also introduces an undocumented carveout, where
referents that themselves are mutable references are treated as Copy,
to catch some cases that we do want to lint against. However, that is
not sound — it's possible to consume a mutable reference by moving it.

To avoid emitting false suggestions, this PR reintroduces the
referent_used_exactly_once logic and runs that check for referents that
are themselves mutable references.

Thinking about the code shape of &mut x, where x: &mut T, raises the
point that while removing the &mut outright won't work, the extra
indirection is still undesirable, and perhaps instead we should suggest
reborrowing: &mut *x. That, however, is left as possible future work.

Fixes #12856

Author: meithecatte

clippy_lints/src/needless_borrows_for_generic_args.rs

@@ -1,6 +1,6 @@
 use clippy_config::msrvs::{self, Msrv};
 use clippy_utils::diagnostics::span_lint_and_then;
-use clippy_utils::mir::PossibleBorrowerMap;
+use clippy_utils::mir::{enclosing_mir, expr_local, local_assignments, used_exactly_once, PossibleBorrowerMap};
 use clippy_utils::source::snippet_with_context;
 use clippy_utils::ty::{implements_trait, is_copy};
 use clippy_utils::{expr_use_ctxt, peel_n_hir_expr_refs, DefinedTy, ExprUseNode};
@@ -11,6 +11,7 @@ use rustc_hir::{Body, Expr, ExprKind, Mutability, Path, QPath};
 use rustc_index::bit_set::BitSet;
 use rustc_infer::infer::TyCtxtInferExt;
 use rustc_lint::{LateContext, LateLintPass};
+use rustc_middle::mir::{Rvalue, StatementKind};
 use rustc_middle::ty::{
     self, ClauseKind, EarlyBinder, FnSig, GenericArg, GenericArgKind, ParamTy, ProjectionPredicate, Ty,
 };
@@ -105,6 +106,7 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessBorrowsForGenericArgs<'tcx> {
             }
             && let count = needless_borrow_count(
                 cx,
+                &mut self.possible_borrowers,
                 fn_id,
                 cx.typeck_results().node_args(hir_id),
                 i,
@@ -153,9 +155,14 @@ fn path_has_args(p: &QPath<'_>) -> bool {
 /// The following constraints will be checked:
 /// * The borrowed expression meets all the generic type's constraints.
 /// * The generic type appears only once in the functions signature.
-/// * The borrowed value is Copy itself OR not a variable (created by a function call)
+/// * The borrowed value is:
+///   - `Copy` itself, or
+///   - the only use of a mutable reference, or
+///   - not a variable (created by a function call)
+#[expect(clippy::too_many_arguments)]
 fn needless_borrow_count<'tcx>(
     cx: &LateContext<'tcx>,
+    possible_borrowers: &mut Vec<(LocalDefId, PossibleBorrowerMap<'tcx, 'tcx>)>,
     fn_id: DefId,
     callee_args: ty::GenericArgsRef<'tcx>,
     arg_index: usize,
@@ -230,9 +237,9 @@ fn needless_borrow_count<'tcx>(
 
         let referent_ty = cx.typeck_results().expr_ty(referent);
 
-        if (!is_copy(cx, referent_ty) && !referent_ty.is_ref())
-            && let ExprKind::AddrOf(_, _, inner) = reference.kind
-            && !matches!(inner.kind, ExprKind::Call(..) | ExprKind::MethodCall(..))
+        if !(is_copy(cx, referent_ty)
+            || referent_ty.is_ref() && referent_used_exactly_once(cx, possible_borrowers, reference)
+            || matches!(referent.kind, ExprKind::Call(..) | ExprKind::MethodCall(..)))
         {
             return false;
         }
@@ -335,6 +342,37 @@ fn is_mixed_projection_predicate<'tcx>(
     }
 }
 
+fn referent_used_exactly_once<'tcx>(
+    cx: &LateContext<'tcx>,
+    possible_borrowers: &mut Vec<(LocalDefId, PossibleBorrowerMap<'tcx, 'tcx>)>,
+    reference: &Expr<'tcx>,
+) -> bool {
+    if let Some(mir) = enclosing_mir(cx.tcx, reference.hir_id)
+        && let Some(local) = expr_local(cx.tcx, reference)
+        && let [location] = *local_assignments(mir, local).as_slice()
+        && let block_data = &mir.basic_blocks[location.block]
+        && let Some(statement) = block_data.statements.get(location.statement_index)
+        && let StatementKind::Assign(box (_, Rvalue::Ref(_, _, place))) = statement.kind
+        && !place.is_indirect_first_projection()
+    {
+        let body_owner_local_def_id = cx.tcx.hir().enclosing_body_owner(reference.hir_id);
+        if possible_borrowers
+            .last()
+            .map_or(true, |&(local_def_id, _)| local_def_id != body_owner_local_def_id)
+        {
+            possible_borrowers.push((body_owner_local_def_id, PossibleBorrowerMap::new(cx, mir)));
+        }
+        let possible_borrower = &mut possible_borrowers.last_mut().unwrap().1;
+        // If `only_borrowers` were used here, the `copyable_iterator::warn` test would fail. The reason is
+        // that `PossibleBorrowerVisitor::visit_terminator` considers `place.local` a possible borrower of
+        // itself. See the comment in that method for an explanation as to why.
+        possible_borrower.bounded_borrowers(&[local], &[local, place.local], place.local, location)
+            && used_exactly_once(mir, place.local).unwrap_or(false)
+    } else {
+        false
+    }
+}
+
 // Iteratively replaces `param_ty` with `new_ty` in `args`, and similarly for each resulting
 // projected type that is a type parameter. Returns `false` if replacing the types would have an
 // effect on the function signature beyond substituting `new_ty` for `param_ty`.

tests/ui/needless_borrows_for_generic_args.fixed

@@ -333,4 +333,12 @@ fn main() {
         f(&y); // Don't lint
         f("".to_owned()); // Lint
     }
+    {
+        fn takes_writer<T: std::io::Write>(_: T) {}
+
+        fn f(mut buffer: &mut Vec<u8>) {
+            takes_writer(&mut buffer); // Don't lint, would make buffer unavailable later
+            buffer.extend(b"\n");
+        }
+    }
 }

tests/ui/needless_borrows_for_generic_args.rs

@@ -333,4 +333,12 @@ fn main() {
         f(&y); // Don't lint
         f(&"".to_owned()); // Lint
     }
+    {
+        fn takes_writer<T: std::io::Write>(_: T) {}
+
+        fn f(mut buffer: &mut Vec<u8>) {
+            takes_writer(&mut buffer); // Don't lint, would make buffer unavailable later
+            buffer.extend(b"\n");
+        }
+    }
 }