Address second round of review comments

smoelius · smoelius · commit 43ea74d98725 · 2022-07-20T06:09:06.000-04:00
diff --git a/clippy_lints/src/dereference.rs b/clippy_lints/src/dereference.rs
@@ -1,13 +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::{
-    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 clippy_utils::ty::{contains_ty, expr_sig, is_copy, is_substs_for_type, 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;
@@ -20,10 +15,13 @@ 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, TraitPredicate, Ty, TyCtxt, TypeVisitable, TypeckResults};
+use rustc_middle::ty::{
+    self, subst::Subst, EarlyBinder, ParamTy, PredicateKind, TraitPredicate, Ty, TyCtxt, TypeVisitable, TypeckResults,
+};
 use rustc_session::{declare_tool_lint, impl_lint_pass};
 use rustc_span::{symbol::sym, Span, Symbol};
-use rustc_trait_selection::infer::InferCtxtExt;
+use rustc_trait_selection::infer::InferCtxtExt as _;
+use rustc_trait_selection::traits::{query::evaluate_obligation::InferCtxtExt as _, Obligation, ObligationCause};
 
 declare_clippy_lint! {
     /// ### What it does
@@ -658,7 +656,7 @@ impl Position {
 /// Walks up the parent expressions attempting to determine both how stable the auto-deref result
 /// is, and which adjustments will be applied to it. Note this will not consider auto-borrow
 /// locations as those follow different rules.
-#[allow(clippy::too_many_lines)]
+#[expect(clippy::too_many_lines)]
 fn walk_parents<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (Position, &'tcx [Adjustment<'tcx>]) {
     let mut adjustments = [].as_slice();
     let mut precedence = 0i8;
@@ -943,19 +941,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
 // 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.
+#[allow(clippy::too_many_lines)]
 fn needless_borrow_impl_arg_position<'tcx>(
     cx: &LateContext<'tcx>,
     parent: &Expr<'tcx>,
@@ -967,14 +959,45 @@ fn needless_borrow_impl_arg_position<'tcx>(
     let sized_trait_def_id = cx.tcx.lang_items().sized_trait();
 
     let Some(callee_def_id) = fn_def_id(cx, parent) else { return Position::Other(precedence) };
+    let callee_generics = cx.tcx.generics_of(callee_def_id);
+    let substs_with_expr_ty = cx
+        .typeck_results()
+        .node_substs(if let ExprKind::Call(callee, _) = parent.kind {
+            callee.hir_id
+        } else {
+            parent.hir_id
+        });
 
-    let (trait_predicates, projection_predicates) =
-        get_input_traits_and_projections(cx, callee_def_id, param_ty.to_ty(cx.tcx));
+    let predicates = cx.tcx.param_env(callee_def_id).caller_bounds();
+    let trait_predicates = predicates
+        .iter()
+        .filter_map(|predicate| {
+            if let PredicateKind::Trait(trait_predicate) = predicate.kind().skip_binder()
+                && is_substs_for_type(trait_predicate.trait_ref.substs, param_ty.to_ty(cx.tcx))
+            {
+                Some(trait_predicate)
+            } else {
+                None
+            }
+        })
+        .collect::<Vec<_>>();
+    let projection_predicates = predicates
+        .iter()
+        .filter_map(|predicate| {
+            if let PredicateKind::Projection(projection_predicate) = predicate.kind().skip_binder()
+                && is_substs_for_type(projection_predicate.projection_ty.substs, param_ty.to_ty(cx.tcx))
+            {
+                Some(projection_predicate)
+            } else {
+                None
+            }
+        })
+        .collect::<Vec<_>>();
 
     // 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)
+        Some(trait_def_id) == sized_trait_def_id || cx.tcx.is_diagnostic_item(sym::Any, trait_def_id)
     }) {
         return Position::Other(precedence);
     }
@@ -996,11 +1019,7 @@ fn needless_borrow_impl_arg_position<'tcx>(
     let check_referent = |referent| {
         let referent_ty = cx.typeck_results().expr_ty(referent);
 
-        if !is_copy(cx, referent_ty)
-            || cx
-                .layout_of(referent_ty)
-                .map_or(true, |layout| layout.size.bytes() > NEEDLESS_BORROW_SIZE_LIMIT)
-        {
+        if !is_copy(cx, referent_ty) {
             return false;
         }
 
@@ -1013,41 +1032,49 @@ fn needless_borrow_impl_arg_position<'tcx>(
             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()
-                })
+        let substs_with_expr_ty = substs_with_expr_ty.split_at(callee_generics.parent_count);
+        let substs_with_referent_ty = substs_with_expr_ty
+            .0
+            .iter()
+            .copied()
+            .chain(
+                callee_generics
+                    .params
+                    .iter()
+                    .zip(substs_with_expr_ty.1.iter())
+                    .map(|(param, arg)| {
+                        if param.index == param_ty.index {
+                            ty::GenericArg::from(referent_ty)
+                        } else {
+                            projection_predicates
+                                .iter()
+                                .find_map(|projection_predicate| {
+                                    if let ty::Term::Ty(term_ty) = projection_predicate.term
+                                        && term_ty.is_param(param.index)
+                                    {
+                                        let item_def_id = projection_predicate.projection_ty.item_def_id;
+                                        let assoc_item = cx.tcx.associated_item(item_def_id);
+                                        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);
+                                        Some(ty::GenericArg::from(projected_ty))
+                                    } else {
+                                        None
+                                    }
+                                })
+                                .unwrap_or(*arg)
+                        }
+                    }),
+            )
+            .collect::<Vec<_>>();
+
+        let predicates = EarlyBinder(predicates).subst(cx.tcx, &substs_with_referent_ty);
+        if !predicates.iter().all(|predicate| {
+            let obligation = Obligation::new(ObligationCause::dummy(), cx.param_env, predicate);
+            cx.tcx
+                .infer_ctxt()
+                .enter(|infcx| infcx.predicate_must_hold_modulo_regions(&obligation))
         }) {
             return false;
         }
diff --git a/clippy_utils/src/paths.rs b/clippy_utils/src/paths.rs
@@ -15,7 +15,6 @@ 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"];
diff --git a/clippy_utils/src/ty.rs b/clippy_utils/src/ty.rs
@@ -95,26 +95,14 @@ pub fn get_input_traits_and_projections<'tcx>(
     let mut trait_predicates = Vec::new();
     let mut projection_predicates = Vec::new();
     for predicate in cx.tcx.param_env(callee_def_id).caller_bounds() {
-        // `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 let Some(arg) = substs.iter().next()
-                && let GenericArgKind::Type(arg_ty) = arg.unpack()
-                && arg_ty == input
-            {
-                true
-            } else {
-                false
-            }
-        };
         match predicate.kind().skip_binder() {
             PredicateKind::Trait(trait_predicate) => {
-                if is_input_substs(trait_predicate.trait_ref.substs) {
+                if is_substs_for_type(trait_predicate.trait_ref.substs, input) {
                     trait_predicates.push(trait_predicate);
                 }
             },
             PredicateKind::Projection(projection_predicate) => {
-                if is_input_substs(projection_predicate.projection_ty.substs) {
+                if is_substs_for_type(projection_predicate.projection_ty.substs, input) {
                     projection_predicates.push(projection_predicate);
                 }
             },
@@ -326,6 +314,19 @@ pub fn is_recursively_primitive_type(ty: Ty<'_>) -> bool {
     }
 }
 
+// Returns `true` if `substs` has 1 + n elements and the first is `ty`. Think of the first as the
+// type on the left hand side of an `as`, and the rest as trait parameters.
+pub fn is_substs_for_type<'tcx>(substs: SubstsRef<'tcx>, ty: Ty<'tcx>) -> bool {
+    if let Some(arg) = substs.iter().next()
+        && let GenericArgKind::Type(arg_ty) = arg.unpack()
+        && arg_ty == ty
+    {
+        true
+    } else {
+        false
+    }
+}
+
 /// Checks if the type is a reference equals to a diagnostic item
 pub fn is_type_ref_to_diagnostic_item(cx: &LateContext<'_>, ty: Ty<'_>, diag_item: Symbol) -> bool {
     match ty.kind() {
