Improve projection predicate handling

smoelius · smoelius · commit 8bc8af3d71ce · 2022-07-23T06:52:45.000-04:00
diff --git a/clippy_lints/src/dereference.rs b/clippy_lints/src/dereference.rs
@@ -1,7 +1,7 @@
 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, is_copy, is_substs_for_type, peel_mid_ty_refs, variant_of_res};
+use clippy_utils::ty::{contains_ty, expr_sig, 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;
@@ -12,16 +12,19 @@ use rustc_hir::{
     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::ty::adjustment::{Adjust, Adjustment, AutoBorrow, AutoBorrowMutability};
 use rustc_middle::ty::{
-    self, subst::Subst, EarlyBinder, ParamTy, PredicateKind, TraitPredicate, Ty, TyCtxt, TypeVisitable, TypeckResults,
+    self, subst::Subst, EarlyBinder, FnSig, ParamTy, PredicateKind, ProjectionPredicate, 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 as _;
 use rustc_trait_selection::traits::{query::evaluate_obligation::InferCtxtExt as _, Obligation, ObligationCause};
+use std::collections::VecDeque;
 
 declare_clippy_lint! {
     /// ### What it does
@@ -958,6 +961,7 @@ 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 fn_sig = cx.tcx.fn_sig(callee_def_id).skip_binder();
     let substs_with_expr_ty = cx
         .typeck_results()
         .node_substs(if let ExprKind::Call(callee, _) = parent.kind {
@@ -967,11 +971,11 @@ fn needless_borrow_impl_arg_position<'tcx>(
         });
 
     let predicates = cx.tcx.param_env(callee_def_id).caller_bounds();
-    let trait_predicates = predicates
+    let trait_predicates_for_param_ty = 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))
+                && trait_predicate.trait_ref.self_ty() == param_ty.to_ty(cx.tcx)
             {
                 Some(trait_predicate)
             } else {
@@ -982,9 +986,7 @@ fn needless_borrow_impl_arg_position<'tcx>(
     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))
-            {
+            if let PredicateKind::Projection(projection_predicate) = predicate.kind().skip_binder() {
                 Some(projection_predicate)
             } else {
                 None
@@ -993,57 +995,43 @@ fn needless_borrow_impl_arg_position<'tcx>(
         .collect::<Vec<_>>();
 
     // If no traits were found, or only the `Sized` or `Any` traits were found, return.
-    if trait_predicates.iter().all(|trait_predicate| {
+    if trait_predicates_for_param_ty.iter().all(|trait_predicate| {
         let trait_def_id = trait_predicate.def_id();
         Some(trait_def_id) == sized_trait_def_id || cx.tcx.is_diagnostic_item(sym::Any, trait_def_id)
     }) {
         return Position::Other(precedence);
     }
 
-    // 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
+    let has_ref_mut_self_method = trait_predicates_for_param_ty
         .iter()
-        .enumerate()
-        .any(|(i, ty)| i != arg_index && contains_ty(ty, param_ty.to_ty(cx.tcx)))
-    {
-        return Position::Other(precedence);
-    }
+        .any(|TraitPredicate { trait_ref, .. }| has_ref_mut_self_method(cx, trait_ref.def_id));
 
-    let check_referent = |referent| {
+    // `substs_with_referent_ty` can be constructed outside of `check_referent` because the same
+    // 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 referent_ty = cx.typeck_results().expr_ty(referent);
 
         if !is_copy(cx, referent_ty) {
             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))
-        {
+        if has_ref_mut_self_method && !matches!(referent_ty.kind(), ty::Ref(_, _, Mutability::Mut)) {
             return false;
         }
 
-        let mut substs_with_referent_ty = substs_with_expr_ty.to_vec();
-        substs_with_referent_ty[param_ty.index as usize] = ty::GenericArg::from(referent_ty);
-        for projection_predicate in &projection_predicates {
-            if let ty::Term::Ty(term_ty) = projection_predicate.term
-                && let ty::Param(term_param_ty) = term_ty.kind()
-            {
-                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);
-                substs_with_referent_ty[term_param_ty.index as usize] = ty::GenericArg::from(projected_ty);
-            }
+        if !replace_types(
+            cx,
+            param_ty,
+            referent_ty,
+            fn_sig,
+            arg_index,
+            &projection_predicates,
+            &mut substs_with_referent_ty,
+        ) {
+            return false;
         }
 
         predicates.iter().all(|predicate| {
@@ -1085,6 +1073,62 @@ fn has_ref_mut_self_method(cx: &LateContext<'_>, trait_def_id: DefId) -> bool {
         })
 }
 
+// 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`.
+// See: https://github.com/rust-lang/rust-clippy/pull/9136#discussion_r927212757
+fn replace_types<'tcx>(
+    cx: &LateContext<'tcx>,
+    param_ty: ParamTy,
+    new_ty: Ty<'tcx>,
+    fn_sig: FnSig<'tcx>,
+    arg_index: usize,
+    projection_predicates: &[ProjectionPredicate<'tcx>],
+    substs: &mut [ty::GenericArg<'tcx>],
+) -> bool {
+    let mut replaced = BitSet::new_empty(substs.len());
+
+    let mut deque = VecDeque::with_capacity(substs.len());
+    deque.push_back((param_ty, new_ty));
+
+    while let Some((param_ty, new_ty)) = deque.pop_front() {
+        let new_generic_arg = ty::GenericArg::from(new_ty);
+
+        // If `replaced.is_empty()`, then `param_ty` and `new_ty` are those initially passed in.
+        if !(substs[param_ty.index as usize] == new_generic_arg
+            || fn_sig
+                .inputs_and_output
+                .iter()
+                .enumerate()
+                .all(|(i, ty)| (replaced.is_empty() && i == arg_index) || !contains_ty(ty, param_ty.to_ty(cx.tcx))))
+        {
+            return false;
+        }
+
+        // The next line should not panic, but panicking seems better than going into an infinite loop.
+        assert!(replaced.insert(param_ty.index));
+
+        substs[param_ty.index as usize] = new_generic_arg;
+
+        for projection_predicate in projection_predicates {
+            if projection_predicate.projection_ty.self_ty() == param_ty.to_ty(cx.tcx)
+                && let ty::Term::Ty(term_ty) = projection_predicate.term
+                && let ty::Param(term_param_ty) = term_ty.kind()
+            {
+                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(new_ty, &[]));
+                let projected_ty = cx.tcx.normalize_erasing_regions(cx.param_env, projection);
+
+                deque.push_back((*term_param_ty, projected_ty));
+            }
+        }
+    }
+
+    true
+}
+
 // 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 {
diff --git a/clippy_utils/src/ty.rs b/clippy_utils/src/ty.rs
@@ -12,7 +12,7 @@ use rustc_hir::{Expr, FnDecl, LangItem, TyKind, Unsafety};
 use rustc_infer::infer::TyCtxtInferExt;
 use rustc_lint::LateContext;
 use rustc_middle::mir::interpret::{ConstValue, Scalar};
-use rustc_middle::ty::subst::{GenericArg, GenericArgKind, Subst, SubstsRef};
+use rustc_middle::ty::subst::{GenericArg, GenericArgKind, Subst};
 use rustc_middle::ty::{
     self, AdtDef, Binder, BoundRegion, DefIdTree, FnSig, IntTy, ParamEnv, Predicate, PredicateKind,
     ProjectionPredicate, ProjectionTy, Region, RegionKind, TraitPredicate, Ty, TyCtxt, TypeSuperVisitable,
@@ -97,12 +97,12 @@ pub fn get_input_traits_and_projections<'tcx>(
     for predicate in cx.tcx.param_env(callee_def_id).caller_bounds() {
         match predicate.kind().skip_binder() {
             PredicateKind::Trait(trait_predicate) => {
-                if is_substs_for_type(trait_predicate.trait_ref.substs, input) {
+                if trait_predicate.trait_ref.self_ty() == input {
                     trait_predicates.push(trait_predicate);
                 }
             },
             PredicateKind::Projection(projection_predicate) => {
-                if is_substs_for_type(projection_predicate.projection_ty.substs, input) {
+                if projection_predicate.projection_ty.self_ty() == input {
                     projection_predicates.push(projection_predicate);
                 }
             },
@@ -314,19 +314,6 @@ 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() {
diff --git a/tests/ui/needless_borrow.fixed b/tests/ui/needless_borrow.fixed
@@ -131,11 +131,14 @@ fn main() {
     let _ = std::process::Command::new("ls").args(["-a", "-l"]).status().unwrap();
     let _ = std::path::Path::new(".").join(".");
     deref_target_is_x(X);
+    multiple_constraints([[""]]);
+    multiple_constraints_normalizes_to_same(X, X);
 
     only_sized(&""); // Don't lint. `Sized` is only bound
     let _ = std::any::Any::type_id(&""); // Don't lint. `Any` is only bound
     let _ = Box::new(&""); // Don't lint. Type parameter appears in return type
     ref_as_ref_path(&""); // Don't lint. Argument type is not a type parameter
+    multiple_constraints_does_not_normalize_to_same(&[[""]], &[""]); // Don't lint. Projected type appears in arguments
 }
 
 #[allow(clippy::needless_borrowed_reference)]
@@ -209,6 +212,23 @@ where
 {
 }
 
+fn multiple_constraints<T, U, V, X, Y>(_: T)
+where
+    T: IntoIterator<Item = U> + IntoIterator<Item = X>,
+    U: IntoIterator<Item = V>,
+    V: AsRef<str>,
+    X: IntoIterator<Item = Y>,
+    Y: AsRef<std::ffi::OsStr>,
+{
+}
+
+fn multiple_constraints_normalizes_to_same<T, U, V>(_: T, _: V)
+where
+    T: std::ops::Deref<Target = U>,
+    U: std::ops::Deref<Target = V>,
+{
+}
+
 fn only_sized<T>(_: T) {}
 
 fn ref_as_ref_path<T: 'static>(_: &'static T)
@@ -217,6 +237,14 @@ where
 {
 }
 
+fn multiple_constraints_does_not_normalize_to_same<T, U, V>(_: T, _: U)
+where
+    T: IntoIterator<Item = U>,
+    U: IntoIterator<Item = V>,
+    V: AsRef<str>,
+{
+}
+
 // https://github.com/rust-lang/rust-clippy/pull/9136#pullrequestreview-1037379321
 #[allow(dead_code)]
 mod copyable_iterator {
diff --git a/tests/ui/needless_borrow.rs b/tests/ui/needless_borrow.rs
@@ -131,11 +131,14 @@ fn main() {
     let _ = std::process::Command::new("ls").args(&["-a", "-l"]).status().unwrap();
     let _ = std::path::Path::new(".").join(&&".");
     deref_target_is_x(&X);
+    multiple_constraints(&[[""]]);
+    multiple_constraints_normalizes_to_same(&X, X);
 
     only_sized(&""); // Don't lint. `Sized` is only bound
     let _ = std::any::Any::type_id(&""); // Don't lint. `Any` is only bound
     let _ = Box::new(&""); // Don't lint. Type parameter appears in return type
     ref_as_ref_path(&""); // Don't lint. Argument type is not a type parameter
+    multiple_constraints_does_not_normalize_to_same(&[[""]], &[""]); // Don't lint. Projected type appears in arguments
 }
 
 #[allow(clippy::needless_borrowed_reference)]
@@ -209,6 +212,23 @@ where
 {
 }
 
+fn multiple_constraints<T, U, V, X, Y>(_: T)
+where
+    T: IntoIterator<Item = U> + IntoIterator<Item = X>,
+    U: IntoIterator<Item = V>,
+    V: AsRef<str>,
+    X: IntoIterator<Item = Y>,
+    Y: AsRef<std::ffi::OsStr>,
+{
+}
+
+fn multiple_constraints_normalizes_to_same<T, U, V>(_: T, _: V)
+where
+    T: std::ops::Deref<Target = U>,
+    U: std::ops::Deref<Target = V>,
+{
+}
+
 fn only_sized<T>(_: T) {}
 
 fn ref_as_ref_path<T: 'static>(_: &'static T)
@@ -217,6 +237,14 @@ where
 {
 }
 
+fn multiple_constraints_does_not_normalize_to_same<T, U, V>(_: T, _: U)
+where
+    T: IntoIterator<Item = U>,
+    U: IntoIterator<Item = V>,
+    V: AsRef<str>,
+{
+}
+
 // https://github.com/rust-lang/rust-clippy/pull/9136#pullrequestreview-1037379321
 #[allow(dead_code)]
 mod copyable_iterator {
diff --git a/tests/ui/needless_borrow.stderr b/tests/ui/needless_borrow.stderr
@@ -138,17 +138,29 @@ error: the borrowed expression implements the required traits
 LL |     deref_target_is_x(&X);
    |                       ^^ help: change this to: `X`
 
+error: the borrowed expression implements the required traits
+  --> $DIR/needless_borrow.rs:134:26
+   |
+LL |     multiple_constraints(&[[""]]);
+   |                          ^^^^^^^ help: change this to: `[[""]]`
+
+error: the borrowed expression implements the required traits
+  --> $DIR/needless_borrow.rs:135:45
+   |
+LL |     multiple_constraints_normalizes_to_same(&X, X);
+   |                                             ^^ help: change this to: `X`
+
 error: this expression borrows a value the compiler would automatically borrow
-  --> $DIR/needless_borrow.rs:182:13
+  --> $DIR/needless_borrow.rs:185:13
    |
 LL |             (&self.f)()
    |             ^^^^^^^^^ help: change this to: `(self.f)`
 
 error: this expression borrows a value the compiler would automatically borrow
-  --> $DIR/needless_borrow.rs:191:13
+  --> $DIR/needless_borrow.rs:194:13
    |
 LL |             (&mut self.f)()
    |             ^^^^^^^^^^^^^ help: change this to: `(self.f)`
 
-error: aborting due to 25 previous errors
+error: aborting due to 27 previous errors
 
