Auto merge of rust-lang#9674 - smoelius:needless-borrow-fp, r=Jarcho

Fix `needless_borrow` false positive

The PR fixes the false positive exposed by `@BusyJay's` example in: rust-lang/rust-clippy#9111 (comment)

The current approach is described in rust-lang/rust-clippy#9674 (comment) and rust-lang/rust-clippy#9674 (comment).

The original approach appears below.

---

The proposed fix is to flag only "simple" trait implementations involving references, a concept
that I introduce next.

Intuitively, a trait implementation is "simple" if all it does is dereference and apply the trait
implementation of a type named by a type parameter. `AsRef` provides a good example of a simple
implementation: https://doc.rust-lang.org/std/convert/trait.AsRef.html#impl-AsRef%3CU%3E-for-%26T

We can make this idea more precise as follows. Given a trait implementation, first determine
whether the implementation is "used defined." If so, then examine its nested obligations.
Consider the implementation simple if-and-only-if:
- there is at least one nested obligation for the same trait
- for each type `X` in the nested obligation's substitution, either `X` is the same as that of
  the original obligation's substitution, or the original type is `&X`

For example, the following implementation from `@BusyJay's` example is "complex" (i.e., not simple)
because it produces no nested obligations:

```rust
impl<'a> Extend<&'a u8> for A { ... }
```

On the other hand, the following slightly modified implementation is simple, because it produces
a nested obligation for `Extend<X>`:

```rust
impl<'a, X> Extend<&'a X> for A where A: Extend<X> { ... }
```

How does flagging only simple implementations help? One way of interpreting the false positive in
`@BusyJay's` example is that it separates a reference from a concrete type. Doing so turns a
successful type inference into a failing one. By flagging only simple implementations, we
separate references from type variables only, thereby eliminating this class of false positives.

Note that `Deref` is a special case, as the obligations generated for it already involve the
underlying type.

r? `@Jarcho` (Sorry to keep pinging you with `needless_borrow` stuff. But my impression is no one knows this code better than you.)

changelog: fix `needless_borrow` false positive

Author: bors

clippy_lints/src/dereference.rs

@@ -1049,7 +1049,7 @@ 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)]
+#[expect(clippy::too_many_arguments, clippy::too_many_lines)]
 fn needless_borrow_impl_arg_position<'tcx>(
     cx: &LateContext<'tcx>,
     possible_borrowers: &mut Vec<(LocalDefId, PossibleBorrowerMap<'tcx, 'tcx>)>,
@@ -1092,7 +1092,7 @@ fn needless_borrow_impl_arg_position<'tcx>(
         .iter()
         .filter_map(|predicate| {
             if let PredicateKind::Trait(trait_predicate) = predicate.kind().skip_binder()
-                && trait_predicate.trait_ref.self_ty() == param_ty.to_ty(cx.tcx)
+                && trait_predicate.self_ty() == param_ty.to_ty(cx.tcx)
             {
                 Some(trait_predicate.trait_ref.def_id)
             } else {
@@ -1111,6 +1111,16 @@ fn needless_borrow_impl_arg_position<'tcx>(
         return Position::Other(precedence);
     }
 
+    // See:
+    // - https://github.com/rust-lang/rust-clippy/pull/9674#issuecomment-1289294201
+    // - https://github.com/rust-lang/rust-clippy/pull/9674#issuecomment-1292225232
+    if projection_predicates
+        .iter()
+        .any(|projection_predicate| is_mixed_projection_predicate(cx, callee_def_id, projection_predicate))
+    {
+        return Position::Other(precedence);
+    }
+
     // `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();
@@ -1190,8 +1200,39 @@ fn has_ref_mut_self_method(cx: &LateContext<'_>, trait_def_id: DefId) -> bool {
         })
 }
 
-fn referent_used_exactly_once<'tcx>(
+fn is_mixed_projection_predicate<'tcx>(
     cx: &LateContext<'tcx>,
+    callee_def_id: DefId,
+    projection_predicate: &ProjectionPredicate<'tcx>,
+) -> bool {
+    let generics = cx.tcx.generics_of(callee_def_id);
+    // The predicate requires the projected type to equal a type parameter from the parent context.
+    if let Some(term_ty) = projection_predicate.term.ty()
+        && let ty::Param(term_param_ty) = term_ty.kind()
+        && (term_param_ty.index as usize) < generics.parent_count
+    {
+        // The inner-most self type is a type parameter from the current function.
+        let mut projection_ty = projection_predicate.projection_ty;
+        loop {
+            match projection_ty.self_ty().kind() {
+                ty::Projection(inner_projection_ty) => {
+                    projection_ty = *inner_projection_ty;
+                }
+                ty::Param(param_ty) => {
+                    return (param_ty.index as usize) >= generics.parent_count;
+                }
+                _ => {
+                    return false;
+                }
+            }
+        }
+    } else {
+        false
+    }
+}
+
+fn referent_used_exactly_once<'a, 'tcx>(
+    cx: &'a LateContext<'tcx>,
     possible_borrowers: &mut Vec<(LocalDefId, PossibleBorrowerMap<'tcx, 'tcx>)>,
     reference: &Expr<'tcx>,
 ) -> bool {

tests/ui/needless_borrow.fixed

@@ -385,3 +385,26 @@ mod used_more_than_once {
     fn use_x(_: impl AsRef<str>) {}
     fn use_x_again(_: impl AsRef<str>) {}
 }
+
+// https://github.com/rust-lang/rust-clippy/issues/9111#issuecomment-1277114280
+#[allow(dead_code)]
+mod issue_9111 {
+    struct A;
+
+    impl Extend<u8> for A {
+        fn extend<T: IntoIterator<Item = u8>>(&mut self, _: T) {
+            unimplemented!()
+        }
+    }
+
+    impl<'a> Extend<&'a u8> for A {
+        fn extend<T: IntoIterator<Item = &'a u8>>(&mut self, _: T) {
+            unimplemented!()
+        }
+    }
+
+    fn main() {
+        let mut a = A;
+        a.extend(&[]); // vs a.extend([]);
+    }
+}

tests/ui/needless_borrow.rs

@@ -385,3 +385,26 @@ mod used_more_than_once {
     fn use_x(_: impl AsRef<str>) {}
     fn use_x_again(_: impl AsRef<str>) {}
 }
+
+// https://github.com/rust-lang/rust-clippy/issues/9111#issuecomment-1277114280
+#[allow(dead_code)]
+mod issue_9111 {
+    struct A;
+
+    impl Extend<u8> for A {
+        fn extend<T: IntoIterator<Item = u8>>(&mut self, _: T) {
+            unimplemented!()
+        }
+    }
+
+    impl<'a> Extend<&'a u8> for A {
+        fn extend<T: IntoIterator<Item = &'a u8>>(&mut self, _: T) {
+            unimplemented!()
+        }
+    }
+
+    fn main() {
+        let mut a = A;
+        a.extend(&[]); // vs a.extend([]);
+    }
+}