Address review comments

Author: smoelius

clippy_lints/src/dereference.rs

@@ -1,22 +1,26 @@
 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::{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 clippy_utils::ty::{
+    contains_ty, expr_sig, get_input_traits_and_projections, implements_trait, is_copy, peel_mid_ty_refs,
+    variant_of_res,
+};
+use clippy_utils::{
+    fn_def_id, get_parent_expr, is_lint_allowed, match_def_path, path_to_local, paths, walk_to_expr_usage,
+};
 use rustc_ast::util::parser::{PREC_POSTFIX, PREC_PREFIX};
 use rustc_data_structures::fx::FxIndexMap;
 use rustc_errors::Applicability;
 use rustc_hir::intravisit::{walk_ty, Visitor};
 use rustc_hir::{
-    self as hir, BindingAnnotation, Body, BodyId, BorrowKind, 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, BindingAnnotation, Body, BodyId, BorrowKind, Expr, ExprKind, FnRetTy, GenericArg,
+    HirId, ImplItem, ImplItemKind, Item, ItemKind, Local, MatchSource, Mutability, Node, Pat, PatKind, Path, QPath,
+    TraitItem, TraitItemKind, TyKind, UnOp,
 };
 use rustc_infer::infer::TyCtxtInferExt;
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_middle::ty::adjustment::{Adjust, Adjustment, AutoBorrow, AutoBorrowMutability};
-use rustc_middle::ty::subst::GenericArgKind;
-use rustc_middle::ty::{self, PredicateKind, TraitPredicate, TraitRef, Ty, TyCtxt, TypeFoldable, TypeckResults};
+use rustc_middle::ty::{self, layout::LayoutOf, ParamTy, TraitPredicate, Ty, TyCtxt, TypeFoldable, TypeckResults};
 use rustc_session::{declare_tool_lint, impl_lint_pass};
 use rustc_span::{symbol::sym, Span, Symbol};
 use rustc_trait_selection::infer::InferCtxtExt;
@@ -763,8 +767,13 @@ fn walk_parents<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (Position, &
                             // 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)),
+                            None => {
+                                if let ty::Param(param_ty) = ty.skip_binder().kind() {
+                                    needless_borrow_impl_arg_position(cx, parent, i, *param_ty, e, precedence)
+                                } else {
+                                    param_auto_deref_stability(ty.skip_binder(), precedence)
+                                }
+                            },
                         })
                     }),
                 ExprKind::MethodCall(_, args, _) => {
@@ -807,8 +816,11 @@ fn walk_parents<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (Position, &
                             }
                         } else {
                             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))
+                            if let ty::Param(param_ty) = ty.kind() {
+                                needless_borrow_impl_arg_position(cx, parent, i, *param_ty, e, precedence)
+                            } else {
+                                param_auto_deref_stability(ty, precedence)
+                            }
                         }
                     })
                 },
@@ -931,6 +943,13 @@ fn ty_contains_infer(ty: &hir::Ty<'_>) -> bool {
     v.0
 }
 
+// The maximum size (in bytes) to consider removing a reference from. Its purpose is to help limit
+// the performance impact in cases where a value would have to be moved, but it cannot be (see "is
+// copyable" comment just below). The choice of this parameter should not affect situations beyond
+// that. At the time the parameter was chosen, the largest type flagged by this lint in Clippy
+// itself was 160 bytes.
+const NEEDLESS_BORROW_SIZE_LIMIT: u64 = 256;
+
 // 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
@@ -940,106 +959,132 @@ fn ty_contains_infer(ty: &hir::Ty<'_>) -> bool {
 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> {
+    arg_index: usize,
+    param_ty: ParamTy,
+    mut expr: &Expr<'tcx>,
+    precedence: i8,
+) -> 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<_>>();
+    let Some(callee_def_id) = fn_def_id(cx, parent) else { return Position::Other(precedence) };
 
-    // If only `Sized` was found, return.
-    if traits
-        .iter()
-        .all(|&(trait_def_id, _)| Some(trait_def_id) == sized_trait_def_id)
-    {
-        return None;
-    }
+    let (trait_predicates, projection_predicates) =
+        get_input_traits_and_projections(cx, callee_def_id, param_ty.to_ty(cx.tcx));
 
-    if !matches!(child_arg_ty.kind(), ty::Param(_)) {
-        return None;
+    // If no traits were found, or only the `Sized` or `Any` traits were found, return.
+    if trait_predicates.iter().all(|trait_predicate| {
+        let trait_def_id = trait_predicate.def_id();
+        Some(trait_def_id) == sized_trait_def_id || match_def_path(cx, trait_def_id, &paths::ANY_TRAIT)
+    }) {
+        return Position::Other(precedence);
     }
 
-    // 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 `param_ty` 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))
+        .any(|(i, ty)| i != arg_index && contains_ty(ty, param_ty.to_ty(cx.tcx)))
     {
-        return None;
+        return Position::Other(precedence);
     }
 
     let check_referent = |referent| {
         let referent_ty = cx.typeck_results().expr_ty(referent);
 
-        if !is_copy(cx, referent_ty) {
+        if !is_copy(cx, referent_ty)
+            || cx
+                .layout_of(referent_ty)
+                .map_or(true, |layout| layout.size.bytes() > NEEDLESS_BORROW_SIZE_LIMIT)
+        {
             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;
-            }
+        // https://github.com/rust-lang/rust-clippy/pull/9136#pullrequestreview-1037379321
+        if !matches!(referent_ty.kind(), ty::Ref(_, _, Mutability::Mut))
+            && trait_predicates
+                .iter()
+                .any(|TraitPredicate { trait_ref, .. }| has_ref_mut_self_method(cx, trait_ref.def_id))
+        {
+            return false;
+        }
+
+        if !trait_predicates.iter().all(|TraitPredicate { trait_ref, .. }| {
+            // The use of `has_escaping_bound_vars` addresses:
+            // https://github.com/rust-lang/rust-clippy/pull/9136#issuecomment-1184024609
+            let substs = &trait_ref.substs[1..];
+            substs.iter().all(|arg| !arg.has_escaping_bound_vars())
+                && implements_trait(cx, referent_ty, trait_ref.def_id, substs)
+        }) {
+            return false;
+        }
+
+        if !projection_predicates.iter().all(|projection_predicate| {
+            let item_def_id = projection_predicate.projection_ty.item_def_id;
+            let assoc_item = cx.tcx.associated_item(item_def_id);
+            let ty::Term::Ty(expected_ty) = projection_predicate.term else { return false };
+            let projection = cx
+                .tcx
+                .mk_projection(assoc_item.def_id, cx.tcx.mk_substs_trait(referent_ty, &[]));
+            let projected_ty = cx.tcx.normalize_erasing_regions(cx.param_env, projection);
+            // This code is not general. It handles just two specific cases:
+            // * the expected type matches the referent's projected type exactly
+            // * the expected type is a parameter with trait bounds and the referent's projected type satisfies
+            //   those trait bounds
+            expected_ty == projected_ty
+                || (matches!(expected_ty.kind(), ty::Param(_)) && {
+                    let (expected_ty_trait_predicates, expected_ty_projection_predicates) =
+                        get_input_traits_and_projections(cx, callee_def_id, expected_ty);
+                    expected_ty_trait_predicates
+                        .iter()
+                        .all(|TraitPredicate { trait_ref, .. }| {
+                            let substs = &trait_ref.substs[1..];
+                            substs.iter().all(|arg| !arg.has_escaping_bound_vars())
+                                && implements_trait(cx, projected_ty, trait_ref.def_id, substs)
+                        })
+                        && expected_ty_projection_predicates.is_empty()
+                })
+        }) {
+            return false;
         }
-        needless_borrow
-    };
 
-    let Node::Expr(mut descendent) = cx.tcx.hir().get(child_id) else { return None };
+        true
+    };
 
     let mut needless_borrow = false;
-    while let ExprKind::AddrOf(_, _, referent) = descendent.kind {
+    while let ExprKind::AddrOf(_, _, referent) = expr.kind {
         if !check_referent(referent) {
             break;
         }
-        descendent = referent;
+        expr = referent;
         needless_borrow = true;
     }
 
     if needless_borrow {
-        Some(Position::ImplArg(descendent.hir_id))
+        Position::ImplArg(expr.hir_id)
     } else {
-        None
+        Position::Other(precedence)
     }
 }
 
+fn has_ref_mut_self_method(cx: &LateContext<'_>, trait_def_id: DefId) -> bool {
+    cx.tcx
+        .associated_items(trait_def_id)
+        .in_definition_order()
+        .any(|assoc_item| {
+            if assoc_item.fn_has_self_parameter {
+                let self_ty = cx.tcx.fn_sig(assoc_item.def_id).skip_binder().inputs()[0];
+                matches!(self_ty.kind(), ty::Ref(_, _, Mutability::Mut))
+            } else {
+                false
+            }
+        })
+}
+
 // 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 {

clippy_lints/src/methods/unnecessary_to_owned.rs

@@ -3,18 +3,18 @@ use super::unnecessary_iter_cloned::{self, is_into_iter};
 use clippy_utils::diagnostics::span_lint_and_sugg;
 use clippy_utils::source::snippet_opt;
 use clippy_utils::ty::{
-    contains_ty, get_associated_type, get_iterator_item_ty, implements_trait, is_copy, peel_mid_ty_refs,
+    contains_ty, get_associated_type, get_input_traits_and_projections, get_iterator_item_ty, implements_trait,
+    is_copy, peel_mid_ty_refs,
 };
-use clippy_utils::{meets_msrv, msrvs};
-
 use clippy_utils::{fn_def_id, get_parent_expr, is_diag_item_method, is_diag_trait_item};
+use clippy_utils::{meets_msrv, msrvs};
 use rustc_errors::Applicability;
 use rustc_hir::{def_id::DefId, BorrowKind, Expr, ExprKind};
 use rustc_lint::LateContext;
 use rustc_middle::mir::Mutability;
+use rustc_middle::ty;
 use rustc_middle::ty::adjustment::{Adjust, Adjustment, OverloadedDeref};
 use rustc_middle::ty::subst::{GenericArg, GenericArgKind, SubstsRef};
-use rustc_middle::ty::{self, PredicateKind, ProjectionPredicate, TraitPredicate, Ty};
 use rustc_semver::RustcVersion;
 use rustc_span::{sym, Symbol};
 use std::cmp::max;
@@ -352,42 +352,6 @@ fn get_callee_substs_and_args<'tcx>(
     None
 }
 
-/// Returns the `TraitPredicate`s and `ProjectionPredicate`s for a function's input type.
-fn get_input_traits_and_projections<'tcx>(
-    cx: &LateContext<'tcx>,
-    callee_def_id: DefId,
-    input: Ty<'tcx>,
-) -> (Vec<TraitPredicate<'tcx>>, Vec<ProjectionPredicate<'tcx>>) {
-    let mut trait_predicates = Vec::new();
-    let mut projection_predicates = Vec::new();
-    for (predicate, _) in cx.tcx.predicates_of(callee_def_id).predicates.iter() {
-        // `substs` should have 1 + n elements. The first is the type on the left hand side of an
-        // `as`. The remaining n are trait parameters.
-        let is_input_substs = |substs: SubstsRef<'tcx>| {
-            if_chain! {
-                if let Some(arg) = substs.iter().next();
-                if let GenericArgKind::Type(arg_ty) = arg.unpack();
-                if arg_ty == input;
-                then { true } else { false }
-            }
-        };
-        match predicate.kind().skip_binder() {
-            PredicateKind::Trait(trait_predicate) => {
-                if is_input_substs(trait_predicate.trait_ref.substs) {
-                    trait_predicates.push(trait_predicate);
-                }
-            },
-            PredicateKind::Projection(projection_predicate) => {
-                if is_input_substs(projection_predicate.projection_ty.substs) {
-                    projection_predicates.push(projection_predicate);
-                }
-            },
-            _ => {},
-        }
-    }
-    (trait_predicates, projection_predicates)
-}
-
 /// Composes two substitutions by applying the latter to the types of the former.
 fn compose_substs<'tcx>(
     cx: &LateContext<'tcx>,

clippy_utils/src/paths.rs

@@ -15,6 +15,7 @@ pub const APPLICABILITY_VALUES: [[&str; 3]; 4] = [
 ];
 #[cfg(feature = "internal")]
 pub const DIAGNOSTIC_BUILDER: [&str; 3] = ["rustc_errors", "diagnostic_builder", "DiagnosticBuilder"];
+pub const ANY_TRAIT: [&str; 3] = ["core", "any", "Any"];
 pub const ARC_PTR_EQ: [&str; 4] = ["alloc", "sync", "Arc", "ptr_eq"];
 pub const ASMUT_TRAIT: [&str; 3] = ["core", "convert", "AsMut"];
 pub const ASREF_TRAIT: [&str; 3] = ["core", "convert", "AsRef"];