Auto merge of #9386 - smoelius:further-enhance-needless-borrow, r=Jarcho

Further enhance `needless_borrow`, mildly refactor `redundant_clone`

This PR does the following:
* Moves some code from `redundant_clone` into a new `clippy_utils` module called `mir`, and wraps that code in a function called `dropped_without_further_use`.
* Relaxes the "is copyable" condition condition from #9136 by also suggesting to remove borrows from values dropped without further use. The changes involve the just mentioned function.
* Separates `redundant_clone` into modules.

Strictly speaking, the last bullet is independent of the others. `redundant_clone` is somewhat hairy, IMO. Separating it into modules makes it slightly less so, by helping to delineate what depends upon what.

I've tried to break everything up into digestible commits.

r? `@Jarcho`

(`@Jarcho` I hope you don't mind.)

changelog: continuation of #9136

Author: bors

clippy_dev/src/serve.rs

@@ -49,7 +49,7 @@ fn mtime(path: impl AsRef<Path>) -> SystemTime {
             .into_iter()
             .flatten()
             .flatten()
-            .map(|entry| mtime(&entry.path()))
+            .map(|entry| mtime(entry.path()))
             .max()
             .unwrap_or(SystemTime::UNIX_EPOCH)
     } else {

clippy_dev/src/update_lints.rs

@@ -128,7 +128,7 @@ fn generate_lint_files(
     for (lint_group, lints) in Lint::by_lint_group(usable_lints.into_iter().chain(internal_lints)) {
         let content = gen_lint_group_list(&lint_group, lints.iter());
         process_file(
-            &format!("clippy_lints/src/lib.register_{lint_group}.rs"),
+            format!("clippy_lints/src/lib.register_{lint_group}.rs"),
             update_mode,
             &content,
         );

clippy_lints/src/dereference.rs

@@ -1,4 +1,5 @@
 use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_hir_and_then};
+use clippy_utils::mir::{enclosing_mir, expr_local, local_assignments, used_exactly_once, PossibleBorrowerMap};
 use clippy_utils::source::{snippet_with_applicability, snippet_with_context};
 use clippy_utils::sugg::has_enclosing_paren;
 use clippy_utils::ty::{expr_sig, is_copy, peel_mid_ty_refs, ty_sig, variant_of_res};
@@ -11,13 +12,16 @@ use rustc_data_structures::fx::FxIndexMap;
 use rustc_errors::Applicability;
 use rustc_hir::intravisit::{walk_ty, Visitor};
 use rustc_hir::{
-    self as hir, def_id::DefId, BindingAnnotation, Body, BodyId, BorrowKind, Closure, Expr, ExprKind, FnRetTy,
-    GenericArg, HirId, ImplItem, ImplItemKind, Item, ItemKind, Local, MatchSource, Mutability, Node, Pat, PatKind,
-    Path, QPath, TraitItem, TraitItemKind, TyKind, UnOp,
+    self as hir,
+    def_id::{DefId, LocalDefId},
+    BindingAnnotation, Body, BodyId, BorrowKind, Closure, Expr, ExprKind, FnRetTy, GenericArg, HirId, ImplItem,
+    ImplItemKind, Item, ItemKind, Local, MatchSource, Mutability, Node, Pat, PatKind, Path, QPath, TraitItem,
+    TraitItemKind, TyKind, UnOp,
 };
 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::adjustment::{Adjust, Adjustment, AutoBorrow, AutoBorrowMutability};
 use rustc_middle::ty::{
     self, Binder, BoundVariableKind, EarlyBinder, FnSig, GenericArgKind, List, ParamTy, PredicateKind,
@@ -141,15 +145,15 @@ declare_clippy_lint! {
     "dereferencing when the compiler would automatically dereference"
 }
 
-impl_lint_pass!(Dereferencing => [
+impl_lint_pass!(Dereferencing<'_> => [
     EXPLICIT_DEREF_METHODS,
     NEEDLESS_BORROW,
     REF_BINDING_TO_REFERENCE,
     EXPLICIT_AUTO_DEREF,
 ]);
 
 #[derive(Default)]
-pub struct Dereferencing {
+pub struct Dereferencing<'tcx> {
     state: Option<(State, StateData)>,
 
     // While parsing a `deref` method call in ufcs form, the path to the function is itself an
@@ -170,11 +174,16 @@ pub struct Dereferencing {
     /// e.g. `m!(x) | Foo::Bar(ref x)`
     ref_locals: FxIndexMap<HirId, Option<RefPat>>,
 
+    /// Stack of (body owner, `PossibleBorrowerMap`) pairs. Used by
+    /// `needless_borrow_impl_arg_position` to determine when a borrowed expression can instead
+    /// be moved.
+    possible_borrowers: Vec<(LocalDefId, PossibleBorrowerMap<'tcx, 'tcx>)>,
+
     // `IntoIterator` for arrays requires Rust 1.53.
     msrv: Option<RustcVersion>,
 }
 
-impl Dereferencing {
+impl<'tcx> Dereferencing<'tcx> {
     #[must_use]
     pub fn new(msrv: Option<RustcVersion>) -> Self {
         Self {
@@ -244,7 +253,7 @@ struct RefPat {
     hir_id: HirId,
 }
 
-impl<'tcx> LateLintPass<'tcx> for Dereferencing {
+impl<'tcx> LateLintPass<'tcx> for Dereferencing<'tcx> {
     #[expect(clippy::too_many_lines)]
     fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
         // Skip path expressions from deref calls. e.g. `Deref::deref(e)`
@@ -278,7 +287,7 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing {
         match (self.state.take(), kind) {
             (None, kind) => {
                 let expr_ty = typeck.expr_ty(expr);
-                let (position, adjustments) = walk_parents(cx, expr, self.msrv);
+                let (position, adjustments) = walk_parents(cx, &mut self.possible_borrowers, expr, self.msrv);
                 match kind {
                     RefOp::Deref => {
                         if let Position::FieldAccess {
@@ -550,6 +559,12 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing {
     }
 
     fn check_body_post(&mut self, cx: &LateContext<'tcx>, body: &'tcx Body<'_>) {
+        if self.possible_borrowers.last().map_or(false, |&(local_def_id, _)| {
+            local_def_id == cx.tcx.hir().body_owner_def_id(body.id())
+        }) {
+            self.possible_borrowers.pop();
+        }
+
         if Some(body.id()) == self.current_body {
             for pat in self.ref_locals.drain(..).filter_map(|(_, x)| x) {
                 let replacements = pat.replacements;
@@ -682,6 +697,7 @@ impl Position {
 #[expect(clippy::too_many_lines)]
 fn walk_parents<'tcx>(
     cx: &LateContext<'tcx>,
+    possible_borrowers: &mut Vec<(LocalDefId, PossibleBorrowerMap<'tcx, 'tcx>)>,
     e: &'tcx Expr<'_>,
     msrv: Option<RustcVersion>,
 ) -> (Position, &'tcx [Adjustment<'tcx>]) {
@@ -796,7 +812,16 @@ fn walk_parents<'tcx>(
                             Some(hir_ty) => binding_ty_auto_deref_stability(cx, hir_ty, precedence, ty.bound_vars()),
                             None => {
                                 if let ty::Param(param_ty) = ty.skip_binder().kind() {
-                                    needless_borrow_impl_arg_position(cx, parent, i, *param_ty, e, precedence, msrv)
+                                    needless_borrow_impl_arg_position(
+                                        cx,
+                                        possible_borrowers,
+                                        parent,
+                                        i,
+                                        *param_ty,
+                                        e,
+                                        precedence,
+                                        msrv,
+                                    )
                                 } else {
                                     ty_auto_deref_stability(cx, cx.tcx.erase_late_bound_regions(ty), precedence)
                                         .position_for_arg()
@@ -844,7 +869,16 @@ fn walk_parents<'tcx>(
                     args.iter().position(|arg| arg.hir_id == child_id).map(|i| {
                         let ty = cx.tcx.fn_sig(id).skip_binder().inputs()[i + 1];
                         if let ty::Param(param_ty) = ty.kind() {
-                            needless_borrow_impl_arg_position(cx, parent, i + 1, *param_ty, e, precedence, msrv)
+                            needless_borrow_impl_arg_position(
+                                cx,
+                                possible_borrowers,
+                                parent,
+                                i + 1,
+                                *param_ty,
+                                e,
+                                precedence,
+                                msrv,
+                            )
                         } else {
                             ty_auto_deref_stability(
                                 cx,
@@ -1018,8 +1052,10 @@ fn ty_contains_infer(ty: &hir::Ty<'_>) -> bool {
 // If the conditions are met, returns `Some(Position::ImplArg(..))`; otherwise, returns `None`.
 //   The "is copyable" condition is to avoid the case where removing the `&` means `e` would have to
 // be moved, but it cannot be.
+#[expect(clippy::too_many_arguments)]
 fn needless_borrow_impl_arg_position<'tcx>(
     cx: &LateContext<'tcx>,
+    possible_borrowers: &mut Vec<(LocalDefId, PossibleBorrowerMap<'tcx, 'tcx>)>,
     parent: &Expr<'tcx>,
     arg_index: usize,
     param_ty: ParamTy,
@@ -1082,10 +1118,13 @@ fn needless_borrow_impl_arg_position<'tcx>(
     // elements are modified each time `check_referent` is called.
     let mut substs_with_referent_ty = substs_with_expr_ty.to_vec();
 
-    let mut check_referent = |referent| {
+    let mut check_reference_and_referent = |reference, referent| {
         let referent_ty = cx.typeck_results().expr_ty(referent);
 
-        if !is_copy(cx, referent_ty) {
+        if !is_copy(cx, referent_ty)
+            && (referent_ty.has_significant_drop(cx.tcx, cx.param_env)
+                || !referent_used_exactly_once(cx, possible_borrowers, reference))
+        {
             return false;
         }
 
@@ -1127,7 +1166,7 @@ fn needless_borrow_impl_arg_position<'tcx>(
 
     let mut needless_borrow = false;
     while let ExprKind::AddrOf(_, _, referent) = expr.kind {
-        if !check_referent(referent) {
+        if !check_reference_and_referent(expr, referent) {
             break;
         }
         expr = referent;
@@ -1155,6 +1194,36 @@ fn has_ref_mut_self_method(cx: &LateContext<'_>, trait_def_id: DefId) -> bool {
         })
 }
 
+fn referent_used_exactly_once<'a, 'tcx>(
+    cx: &'a LateContext<'tcx>,
+    possible_borrowers: &mut Vec<(LocalDefId, PossibleBorrowerMap<'tcx, 'tcx>)>,
+    reference: &Expr<'tcx>,
+) -> bool {
+    let mir = enclosing_mir(cx.tcx, reference.hir_id);
+    if let Some(local) = expr_local(cx.tcx, reference)
+        && let [location] = *local_assignments(mir, local).as_slice()
+        && let StatementKind::Assign(box (_, Rvalue::Ref(_, _, place))) =
+            mir.basic_blocks[location.block].statements[location.statement_index].kind
+        && !place.has_deref()
+    {
+        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 `substs`, 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`.
@@ -1439,8 +1508,8 @@ fn report<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, state: State, data
     }
 }
 
-impl Dereferencing {
-    fn check_local_usage<'tcx>(&mut self, cx: &LateContext<'tcx>, e: &Expr<'tcx>, local: HirId) {
+impl<'tcx> Dereferencing<'tcx> {
+    fn check_local_usage(&mut self, cx: &LateContext<'tcx>, e: &Expr<'tcx>, local: HirId) {
         if let Some(outer_pat) = self.ref_locals.get_mut(&local) {
             if let Some(pat) = outer_pat {
                 // Check for auto-deref

clippy_lints/src/lib.rs

@@ -38,7 +38,6 @@ extern crate rustc_infer;
 extern crate rustc_lexer;
 extern crate rustc_lint;
 extern crate rustc_middle;
-extern crate rustc_mir_dataflow;
 extern crate rustc_parse;
 extern crate rustc_session;
 extern crate rustc_span;
@@ -418,7 +417,7 @@ pub fn register_pre_expansion_lints(store: &mut rustc_lint::LintStore, sess: &Se
 
     let msrv = conf.msrv.as_ref().and_then(|s| {
         parse_msrv(s, None, None).or_else(|| {
-            sess.err(&format!(
+            sess.err(format!(
                 "error reading Clippy's configuration file. `{s}` is not a valid Rust version"
             ));
             None
@@ -434,7 +433,7 @@ fn read_msrv(conf: &Conf, sess: &Session) -> Option<RustcVersion> {
         .and_then(|v| parse_msrv(&v, None, None));
     let clippy_msrv = conf.msrv.as_ref().and_then(|s| {
         parse_msrv(s, None, None).or_else(|| {
-            sess.err(&format!(
+            sess.err(format!(
                 "error reading Clippy's configuration file. `{s}` is not a valid Rust version"
             ));
             None
@@ -445,7 +444,7 @@ fn read_msrv(conf: &Conf, sess: &Session) -> Option<RustcVersion> {
         if let Some(clippy_msrv) = clippy_msrv {
             // if both files have an msrv, let's compare them and emit a warning if they differ
             if clippy_msrv != cargo_msrv {
-                sess.warn(&format!(
+                sess.warn(format!(
                     "the MSRV in `clippy.toml` and `Cargo.toml` differ; using `{clippy_msrv}` from `clippy.toml`"
                 ));
             }
@@ -474,7 +473,7 @@ pub fn read_conf(sess: &Session) -> Conf {
     let TryConf { conf, errors, warnings } = utils::conf::read(&file_name);
     // all conf errors are non-fatal, we just use the default conf in case of error
     for error in errors {
-        sess.err(&format!(
+        sess.err(format!(
             "error reading Clippy's configuration file `{}`: {}",
             file_name.display(),
             format_error(error)

clippy_lints/src/nonstandard_macro_braces.rs

@@ -266,7 +266,7 @@ impl<'de> Deserialize<'de> for MacroMatcher {
                         .iter()
                         .find(|b| b.0 == brace)
                         .map(|(o, c)| ((*o).to_owned(), (*c).to_owned()))
-                        .ok_or_else(|| de::Error::custom(&format!("expected one of `(`, `{{`, `[` found `{brace}`")))?,
+                        .ok_or_else(|| de::Error::custom(format!("expected one of `(`, `{{`, `[` found `{brace}`")))?,
                 })
             }
         }