Auto merge of rust-lang#12706 - pacak:less-aggressive-needless-borrows, r=dswij

less aggressive needless_borrows_for_generic_args

Current implementation looks for significant drops, that can change the behavior, but that's not enough - value might not have a `Drop` itself but one of its children might have it.

A good example is passing a reference to `PathBuf` to `std::fs::File::open`. There's no benefits to pass `PathBuf` by value, but since `clippy` can't see `Drop` on `Vec` several layers down it complains forcing pass by value and making it impossible to use the same name later.

New implementation only looks at copy values or values created in place     so existing variable will never be moved but things that take a string reference created and value is created inplace `&"".to_owned()` will make it to suggest to use `"".to_owned()` still.

Fixes rust-lang/rust-clippy#12454

changelog: [`needless_borrows_for_generic_args`]: avoid moving variables

Author: bors

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::{enclosing_mir, expr_local, local_assignments, used_exactly_once, PossibleBorrowerMap};
+use clippy_utils::mir::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,7 +11,6 @@ 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, List, ParamTy, ProjectionPredicate, Ty,
 };
@@ -106,7 +105,6 @@ 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,
@@ -155,11 +153,9 @@ 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 will not be moved if it is used later in the function.
-#[expect(clippy::too_many_arguments)]
+/// * The borrowed value is Copy itself OR not a variable (created by a function call)
 fn needless_borrow_count<'tcx>(
     cx: &LateContext<'tcx>,
-    possible_borrowers: &mut Vec<(LocalDefId, PossibleBorrowerMap<'tcx, 'tcx>)>,
     fn_id: DefId,
     callee_args: &'tcx List<GenericArg<'tcx>>,
     arg_index: usize,
@@ -234,9 +230,9 @@ fn needless_borrow_count<'tcx>(
 
         let referent_ty = cx.typeck_results().expr_ty(referent);
 
-        if !is_copy(cx, referent_ty)
-            && (referent_ty.has_significant_drop(cx.tcx, cx.param_env)
-                || !referent_used_exactly_once(cx, possible_borrowers, reference))
+        if (!is_copy(cx, referent_ty) && !referent_ty.is_ref())
+            && let ExprKind::AddrOf(_, _, inner) = reference.kind
+            && !matches!(inner.kind, ExprKind::Call(..) | ExprKind::MethodCall(..))
         {
             return false;
         }
@@ -339,37 +335,6 @@ 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

@@ -141,8 +141,8 @@ fn main() {
         let f = |arg| {
             let loc = "loc".to_owned();
             let _ = std::fs::write("x", &env); // Don't lint. In environment
-            let _ = std::fs::write("x", arg);
-            let _ = std::fs::write("x", loc);
+            let _ = std::fs::write("x", &arg);
+            let _ = std::fs::write("x", &loc);
         };
         let _ = std::fs::write("x", &env); // Don't lint. Borrowed by `f`
         f(arg);
@@ -158,13 +158,13 @@ fn main() {
         fn f(_: impl Debug) {}
 
         let x = X;
-        f(&x); // Don't lint. Has significant drop
+        f(&x); // Don't lint, not copy, passed by a reference to a variable
     }
     {
         fn f(_: impl AsRef<str>) {}
 
         let x = String::new();
-        f(x);
+        f(&x);
     }
     {
         fn f(_: impl AsRef<str>) {}
@@ -299,4 +299,38 @@ fn main() {
             check_str(&owner.0); // Don't lint. `owner` can't be partially moved because it impl Drop
         }
     }
+    {
+        #[derive(Debug)]
+        struct X(Vec<u8>);
+
+        fn f(_: impl Debug) {}
+
+        let x = X(vec![]);
+        f(&x); // Don't lint, makes x unavailable later
+    }
+    {
+        #[derive(Debug)]
+        struct X;
+
+        impl Drop for X {
+            fn drop(&mut self) {}
+        }
+
+        fn f(_: impl Debug) {}
+
+        #[derive(Debug)]
+        struct Y(X);
+
+        let y = Y(X);
+        f(&y); // Don't lint. Not copy, passed by a reference to value
+    }
+    {
+        fn f(_: impl AsRef<str>) {}
+        let x = String::new();
+        f(&x); // Don't lint, not a copy, makes it unavailable later
+        f(String::new()); // Lint, makes no difference
+        let y = "".to_owned();
+        f(&y); // Don't lint
+        f("".to_owned()); // Lint
+    }
 }

tests/ui/needless_borrows_for_generic_args.rs

@@ -158,7 +158,7 @@ fn main() {
         fn f(_: impl Debug) {}
 
         let x = X;
-        f(&x); // Don't lint. Has significant drop
+        f(&x); // Don't lint, not copy, passed by a reference to a variable
     }
     {
         fn f(_: impl AsRef<str>) {}
@@ -299,4 +299,38 @@ fn main() {
             check_str(&owner.0); // Don't lint. `owner` can't be partially moved because it impl Drop
         }
     }
+    {
+        #[derive(Debug)]
+        struct X(Vec<u8>);
+
+        fn f(_: impl Debug) {}
+
+        let x = X(vec![]);
+        f(&x); // Don't lint, makes x unavailable later
+    }
+    {
+        #[derive(Debug)]
+        struct X;
+
+        impl Drop for X {
+            fn drop(&mut self) {}
+        }
+
+        fn f(_: impl Debug) {}
+
+        #[derive(Debug)]
+        struct Y(X);
+
+        let y = Y(X);
+        f(&y); // Don't lint. Not copy, passed by a reference to value
+    }
+    {
+        fn f(_: impl AsRef<str>) {}
+        let x = String::new();
+        f(&x); // Don't lint, not a copy, makes it unavailable later
+        f(&String::new()); // Lint, makes no difference
+        let y = "".to_owned();
+        f(&y); // Don't lint
+        f(&"".to_owned()); // Lint
+    }
 }

tests/ui/needless_borrows_for_generic_args.stderr

@@ -50,28 +50,22 @@ LL |         let _ = Command::new("ls").args(&["-a", "-l"]).status().unwrap();
    |                                         ^^^^^^^^^^^^^ help: change this to: `["-a", "-l"]`
 
 error: the borrowed expression implements the required traits
-  --> tests/ui/needless_borrows_for_generic_args.rs:144:41
-   |
-LL |             let _ = std::fs::write("x", &arg);
-   |                                         ^^^^ help: change this to: `arg`
-
-error: the borrowed expression implements the required traits
-  --> tests/ui/needless_borrows_for_generic_args.rs:145:41
+  --> tests/ui/needless_borrows_for_generic_args.rs:247:13
    |
-LL |             let _ = std::fs::write("x", &loc);
-   |                                         ^^^^ help: change this to: `loc`
+LL |         foo(&a);
+   |             ^^ help: change this to: `a`
 
 error: the borrowed expression implements the required traits
-  --> tests/ui/needless_borrows_for_generic_args.rs:167:11
+  --> tests/ui/needless_borrows_for_generic_args.rs:331:11
    |
-LL |         f(&x);
-   |           ^^ help: change this to: `x`
+LL |         f(&String::new()); // Lint, makes no difference
+   |           ^^^^^^^^^^^^^^ help: change this to: `String::new()`
 
 error: the borrowed expression implements the required traits
-  --> tests/ui/needless_borrows_for_generic_args.rs:247:13
+  --> tests/ui/needless_borrows_for_generic_args.rs:334:11
    |
-LL |         foo(&a);
-   |             ^^ help: change this to: `a`
+LL |         f(&"".to_owned()); // Lint
+   |           ^^^^^^^^^^^^^^ help: change this to: `"".to_owned()`
 
-error: aborting due to 12 previous errors
+error: aborting due to 11 previous errors