Enhance needless_borrow to consider trait implementations

Author: smoelius

clippy_lints/src/dereference.rs

@@ -1,8 +1,8 @@
 use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_hir_and_then};
 use clippy_utils::source::{snippet_with_applicability, snippet_with_context};
 use clippy_utils::sugg::has_enclosing_paren;
-use clippy_utils::ty::{expr_sig, peel_mid_ty_refs, variant_of_res};
-use clippy_utils::{get_parent_expr, is_lint_allowed, path_to_local, walk_to_expr_usage};
+use clippy_utils::ty::{contains_ty, expr_sig, implements_trait, is_copy, peel_mid_ty_refs, variant_of_res};
+use clippy_utils::{fn_def_id, get_parent_expr, is_lint_allowed, path_to_local, walk_to_expr_usage};
 use rustc_ast::util::parser::{PREC_POSTFIX, PREC_PREFIX};
 use rustc_data_structures::fx::FxIndexMap;
 use rustc_errors::Applicability;
@@ -15,7 +15,10 @@ use rustc_hir::{
 use rustc_infer::infer::TyCtxtInferExt;
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_middle::ty::adjustment::{Adjust, Adjustment, AutoBorrow, AutoBorrowMutability};
-use rustc_middle::ty::{self, Ty, TyCtxt, TypeVisitable, TypeckResults};
+use rustc_middle::ty::{
+    self, subst::GenericArgKind, PredicateKind, TraitPredicate, TraitRef, Ty, TyCtxt, TypeFoldable, TypeVisitable,
+    TypeckResults, TypeckResults,
+};
 use rustc_session::{declare_tool_lint, impl_lint_pass};
 use rustc_span::{symbol::sym, Span, Symbol};
 use rustc_trait_selection::infer::InferCtxtExt;
@@ -170,6 +173,7 @@ struct StateData {
 struct DerefedBorrow {
     count: usize,
     msg: &'static str,
+    snip_expr: Option<HirId>,
 }
 
 enum State {
@@ -331,20 +335,23 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing {
                         let deref_msg =
                             "this expression creates a reference which is immediately dereferenced by the compiler";
                         let borrow_msg = "this expression borrows a value the compiler would automatically borrow";
+                        let impl_msg = "the borrowed expression implements the required traits";
 
-                        let (required_refs, msg) = if position.can_auto_borrow() {
-                            (1, if deref_count == 1 { borrow_msg } else { deref_msg })
+                        let (required_refs, msg, snip_expr) = if position.can_auto_borrow() {
+                            (1, if deref_count == 1 { borrow_msg } else { deref_msg }, None)
+                        } else if let Position::ImplArg(hir_id) = position {
+                            (0, impl_msg, Some(hir_id))
                         } else if let Some(&Adjust::Borrow(AutoBorrow::Ref(_, mutability))) =
                             next_adjust.map(|a| &a.kind)
                         {
                             if matches!(mutability, AutoBorrowMutability::Mut { .. }) && !position.is_reborrow_stable()
                             {
-                                (3, deref_msg)
+                                (3, deref_msg, None)
                             } else {
-                                (2, deref_msg)
+                                (2, deref_msg, None)
                             }
                         } else {
-                            (2, deref_msg)
+                            (2, deref_msg, None)
                         };
 
                         if deref_count >= required_refs {
@@ -354,6 +361,7 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing {
                                     // can't be removed without breaking the code. See earlier comment.
                                     count: deref_count - required_refs,
                                     msg,
+                                    snip_expr,
                                 }),
                                 StateData { span: expr.span, hir_id: expr.hir_id, position },
                             ));
@@ -521,7 +529,7 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing {
                             spans: vec![pat.span],
                             app,
                             replacements: vec![(pat.span, snip.into())],
-                            hir_id: pat.hir_id
+                            hir_id: pat.hir_id,
                         }),
                     );
                 }
@@ -605,6 +613,7 @@ enum Position {
     /// The method is defined on a reference type. e.g. `impl Foo for &T`
     MethodReceiverRefImpl,
     Callee,
+    ImplArg(HirId),
     FieldAccess(Symbol),
     Postfix,
     Deref,
@@ -638,7 +647,7 @@ impl Position {
             | Self::Callee
             | Self::FieldAccess(_)
             | Self::Postfix => PREC_POSTFIX,
-            Self::Deref => PREC_PREFIX,
+            Self::ImplArg(_) | Self::Deref => PREC_PREFIX,
             Self::DerefStable(p) | Self::ReborrowStable(p) | Self::Other(p) => p,
         }
     }
@@ -751,12 +760,14 @@ fn walk_parents<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (Position, &
                     .iter()
                     .position(|arg| arg.hir_id == child_id)
                     .zip(expr_sig(cx, func))
-                    .and_then(|(i, sig)| sig.input_with_hir(i))
-                    .map(|(hir_ty, ty)| match hir_ty {
-                        // Type inference for closures can depend on how they're called. Only go by the explicit
-                        // types here.
-                        Some(ty) => binding_ty_auto_deref_stability(ty, precedence),
-                        None => param_auto_deref_stability(ty.skip_binder(), precedence),
+                    .and_then(|(i, sig)| {
+                        sig.input_with_hir(i).map(|(hir_ty, ty)| match hir_ty {
+                            // Type inference for closures can depend on how they're called. Only go by the explicit
+                            // types here.
+                            Some(ty) => binding_ty_auto_deref_stability(ty, precedence),
+                            None => needless_borrow_impl_arg_position(cx, parent, i, ty.skip_binder(), child_id)
+                                .unwrap_or_else(|| param_auto_deref_stability(ty.skip_binder(), precedence)),
+                        })
                     }),
                 ExprKind::MethodCall(_, args, _) => {
                     let id = cx.typeck_results().type_dependent_def_id(parent.hir_id).unwrap();
@@ -797,7 +808,9 @@ fn walk_parents<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (Position, &
                                 Position::MethodReceiver
                             }
                         } else {
-                            param_auto_deref_stability(cx.tcx.fn_sig(id).skip_binder().inputs()[i], precedence)
+                            let ty = cx.tcx.fn_sig(id).skip_binder().inputs()[i];
+                            needless_borrow_impl_arg_position(cx, parent, i, ty, child_id)
+                                .unwrap_or_else(|| param_auto_deref_stability(ty, precedence))
                         }
                     })
                 },
@@ -920,6 +933,111 @@ fn ty_contains_infer(ty: &hir::Ty<'_>) -> bool {
     v.0
 }
 
+// Checks whether:
+// * child is an expression of the form `&e` in an argument position requiring an `impl Trait`
+// * `e`'s type implements `Trait` and is copyable
+// 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.
+fn needless_borrow_impl_arg_position<'tcx>(
+    cx: &LateContext<'tcx>,
+    parent: &Expr<'tcx>,
+    child_arg_index: usize,
+    child_arg_ty: Ty<'tcx>,
+    child_id: HirId,
+) -> Option<Position> {
+    let sized_trait_def_id = cx.tcx.lang_items().sized_trait();
+
+    let Some(callee_def_id) = fn_def_id(cx, parent) else { return None };
+
+    let traits = cx
+        .tcx
+        .predicates_of(callee_def_id)
+        .predicates
+        .iter()
+        .filter_map(|(predicate, _)| {
+            if_chain! {
+                if let PredicateKind::Trait(TraitPredicate {
+                    trait_ref:
+                        TraitRef {
+                            def_id: trait_def_id,
+                            substs,
+                            ..
+                        },
+                    ..
+                }) = predicate.kind().skip_binder();
+                if let Some(arg) = substs.iter().next();
+                if let GenericArgKind::Type(arg_ty) = arg.unpack();
+                if arg_ty == child_arg_ty;
+                then { Some((trait_def_id, &substs[1..])) } else { None }
+            }
+        })
+        .collect::<Vec<_>>();
+
+    // If only `Sized` was found, return.
+    if traits
+        .iter()
+        .all(|&(trait_def_id, _)| Some(trait_def_id) == sized_trait_def_id)
+    {
+        return None;
+    }
+
+    // If `child_arg_ty` is a type parameter that appears in more than one place, then substituting
+    // it with `T` instead of `&T` could cause a type error.
+    if cx
+        .tcx
+        .fn_sig(callee_def_id)
+        .skip_binder()
+        .inputs_and_output
+        .iter()
+        .enumerate()
+        .any(|(i, ty)| i != child_arg_index && contains_ty(ty, child_arg_ty))
+    {
+        return None;
+    }
+
+    let check_referent = |referent| {
+        let referent_ty = cx.typeck_results().expr_ty(referent);
+
+        if !is_copy(cx, referent_ty) {
+            return false;
+        }
+
+        // * If an applicable trait is found and `referent_ty` implements it, `needless_borrow` is set to
+        //   `true`.
+        // * If an applicable trait is found that `referent_ty` does not implement, `false` is returned
+        //   immediately.
+        // * If no applicable traits are found, `needless_borrow` remains `false`.
+        let mut needless_borrow = false;
+        for &(trait_def_id, substs) in &traits {
+            if implements_trait(cx, referent_ty, trait_def_id, substs) {
+                // Ignore `Sized` since it is required by default.
+                needless_borrow = Some(trait_def_id) != sized_trait_def_id;
+            } else {
+                return false;
+            }
+        }
+        needless_borrow
+    };
+
+    let Node::Expr(mut descendent) = cx.tcx.hir().get(child_id) else { return None };
+
+    let mut needless_borrow = false;
+    while let ExprKind::AddrOf(_, _, referent) = descendent.kind {
+        if !check_referent(referent) {
+            break;
+        }
+        descendent = referent;
+        needless_borrow = true;
+    }
+
+    if needless_borrow {
+        Some(Position::ImplArg(descendent.hir_id))
+    } else {
+        None
+    }
+}
+
 // Checks whether a type is stable when switching to auto dereferencing,
 fn param_auto_deref_stability(ty: Ty<'_>, precedence: i8) -> Position {
     let ty::Ref(_, mut ty, _) = *ty.kind() else {
@@ -1026,7 +1144,8 @@ fn report<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, state: State, data
         },
         State::DerefedBorrow(state) => {
             let mut app = Applicability::MachineApplicable;
-            let (snip, snip_is_macro) = snippet_with_context(cx, expr.span, data.span.ctxt(), "..", &mut app);
+            let snip_expr = state.snip_expr.map_or(expr, |hir_id| cx.tcx.hir().expect_expr(hir_id));
+            let (snip, snip_is_macro) = snippet_with_context(cx, snip_expr.span, data.span.ctxt(), "..", &mut app);
             span_lint_hir_and_then(cx, NEEDLESS_BORROW, data.hir_id, data.span, state.msg, |diag| {
                 let sugg = if !snip_is_macro
                     && expr.precedence().order() < data.position.precedence()

tests/ui/needless_borrow.fixed

@@ -127,6 +127,11 @@ fn main() {
             0
         }
     }
+
+    let _ = std::process::Command::new("ls").args(["-a", "-l"]).status().unwrap();
+    let _ = std::path::Path::new(".").join(".");
+
+    let _ = Box::new(&""); // Don't lint. Type parameter has no trait bounds
 }
 
 #[allow(clippy::needless_borrowed_reference)]

tests/ui/needless_borrow.rs

@@ -127,6 +127,11 @@ fn main() {
             0
         }
     }
+
+    let _ = std::process::Command::new("ls").args(&["-a", "-l"]).status().unwrap();
+    let _ = std::path::Path::new(".").join(&&".");
+
+    let _ = Box::new(&""); // Don't lint. Type parameter has no trait bounds
 }
 
 #[allow(clippy::needless_borrowed_reference)]

tests/ui/needless_borrow.stderr

@@ -120,5 +120,17 @@ error: this expression creates a reference which is immediately dereferenced by
 LL |     (&&5).foo();
    |     ^^^^^ help: change this to: `(&5)`
 
-error: aborting due to 20 previous errors
+error: the borrowed expression implements the required traits
+  --> $DIR/needless_borrow.rs:131:51
+   |
+LL |     let _ = std::process::Command::new("ls").args(&["-a", "-l"]).status().unwrap();
+   |                                                   ^^^^^^^^^^^^^ help: change this to: `["-a", "-l"]`
+
+error: the borrowed expression implements the required traits
+  --> $DIR/needless_borrow.rs:132:44
+   |
+LL |     let _ = std::path::Path::new(".").join(&&".");
+   |                                            ^^^^^ help: change this to: `"."`
+
+error: aborting due to 22 previous errors