Fix needless_borrow suggestion when calling a trait method taking self

Jarcho · Jarcho · commit 5a6a6d6e0d63 · 2022-01-30T14:14:14.000-05:00
diff --git a/clippy_lints/src/dereference.rs b/clippy_lints/src/dereference.rs
@@ -11,11 +11,13 @@ use rustc_hir::{
     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::{self, Ty, TyCtxt, TyS, TypeFoldable, TypeckResults};
 use rustc_session::{declare_tool_lint, impl_lint_pass};
 use rustc_span::{symbol::sym, Span, Symbol};
+use rustc_trait_selection::infer::InferCtxtExt;
 
 declare_clippy_lint! {
     /// ### What it does
@@ -155,7 +157,6 @@ pub struct Dereferencing {
 
 struct DerefedBorrow {
     count: usize,
-    required_precedence: i8,
     msg: &'static str,
     position: Position,
 }
@@ -312,19 +313,19 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing {
                             "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 (required_refs, required_precedence, msg) = if position.can_auto_borrow() {
-                            (1, PREC_POSTFIX, if deref_count == 1 { borrow_msg } else { deref_msg })
+                        let (required_refs, msg) = if position.can_auto_borrow() {
+                            (1, if deref_count == 1 { borrow_msg } else { deref_msg })
                         } 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, 0, deref_msg)
+                                (3, deref_msg)
                             } else {
-                                (2, 0, deref_msg)
+                                (2, deref_msg)
                             }
                         } else {
-                            (2, 0, deref_msg)
+                            (2, deref_msg)
                         };
 
                         if deref_count >= required_refs {
@@ -333,7 +334,6 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing {
                                     // One of the required refs is for the current borrow expression, the remaining ones
                                     // can't be removed without breaking the code. See earlier comment.
                                     count: deref_count - required_refs,
-                                    required_precedence,
                                     msg,
                                     position,
                                 }),
@@ -542,6 +542,8 @@ fn deref_method_same_type(result_ty: Ty<'_>, arg_ty: Ty<'_>) -> bool {
 #[derive(Clone, Copy)]
 enum Position {
     MethodReceiver,
+    /// The method is defined on a reference type. e.g. `impl Foo for &T`
+    MethodReceiverRefImpl,
     Callee,
     FieldAccess(Symbol),
     Postfix,
@@ -568,6 +570,13 @@ impl Position {
     fn lint_explicit_deref(self) -> bool {
         matches!(self, Self::Other | Self::DerefStable | Self::ReborrowStable)
     }
+
+    fn needs_parens(self, precedence: i8) -> bool {
+        matches!(
+            self,
+            Self::MethodReceiver | Self::MethodReceiverRefImpl | Self::Callee | Self::FieldAccess(_) | Self::Postfix
+        ) && precedence < PREC_POSTFIX
+    }
 }
 
 /// Walks up the parent expressions attempting to determine both how stable the auto-deref result
@@ -673,10 +682,34 @@ fn walk_parents<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (Position, &
                     let id = cx.typeck_results().type_dependent_def_id(parent.hir_id).unwrap();
                     args.iter().position(|arg| arg.hir_id == child_id).map(|i| {
                         if i == 0 {
-                            if e.hir_id == child_id {
-                                Position::MethodReceiver
-                            } else {
+                            // Check for calls to trait methods where the trait is implemented on a reference.
+                            // Two cases need to be handled:
+                            // * `self` methods on `&T` will never have auto-borrow
+                            // * `&self` methods on `&T` can have auto-borrow, but `&self` methods on `T` will take
+                            //   priority.
+                            if e.hir_id != child_id {
                                 Position::ReborrowStable
+                            } else if let Some(trait_id) = cx.tcx.trait_of_item(id)
+                                && let arg_ty = cx.tcx.erase_regions(cx.typeck_results().expr_ty_adjusted(e))
+                                && let ty::Ref(_, sub_ty, _) = *arg_ty.kind()
+                                && let subs = cx.typeck_results().node_substs_opt(child_id).unwrap_or_else(
+                                    || cx.tcx.mk_substs([].iter())
+                                ) && let impl_ty = if cx.tcx.fn_sig(id).skip_binder().inputs()[0].is_ref() {
+                                    // Trait methods taking `&self`
+                                    sub_ty
+                                } else {
+                                    // Trait methods taking `self`
+                                    arg_ty
+                                } && impl_ty.is_ref()
+                                && cx.tcx.infer_ctxt().enter(|infcx|
+                                    infcx
+                                        .type_implements_trait(trait_id, impl_ty, subs, cx.param_env)
+                                        .must_apply_modulo_regions()
+                                )
+                            {
+                                Position::MethodReceiverRefImpl
+                            } else {
+                                Position::MethodReceiver
                             }
                         } else {
                             param_auto_deref_stability(cx.tcx.fn_sig(id).skip_binder().inputs()[i])
@@ -912,7 +945,7 @@ fn report(cx: &LateContext<'_>, expr: &Expr<'_>, state: State, span: Span) {
                 span,
                 state.msg,
                 "change this to",
-                if state.required_precedence > expr.precedence().order() && !has_enclosing_paren(&snip) {
+                if state.position.needs_parens(expr.precedence().order()) && !has_enclosing_paren(&snip) {
                     format!("({})", snip)
                 } else {
                     snip.into()
diff --git a/tests/ui/needless_borrow.fixed b/tests/ui/needless_borrow.fixed
@@ -83,6 +83,36 @@ fn main() {
     let _ = x.0;
     let x = &x as *const (i32, i32);
     let _ = unsafe { (*x).0 };
+
+    // Issue #8367
+    trait Foo {
+        fn foo(self);
+    }
+    impl Foo for &'_ () {
+        fn foo(self) {}
+    }
+    (&()).foo(); // Don't lint. `()` doesn't implement `Foo`
+    (&()).foo();
+
+    impl Foo for i32 {
+        fn foo(self) {}
+    }
+    impl Foo for &'_ i32 {
+        fn foo(self) {}
+    }
+    (&5).foo(); // Don't lint. `5` will call `<i32 as Foo>::foo`
+    (&5).foo();
+
+    trait FooRef {
+        fn foo_ref(&self);
+    }
+    impl FooRef for () {
+        fn foo_ref(&self) {}
+    }
+    impl FooRef for &'_ () {
+        fn foo_ref(&self) {}
+    }
+    (&&()).foo_ref(); // Don't lint. `&()` will call `<() as FooRef>::foo_ref`
 }
 
 #[allow(clippy::needless_borrowed_reference)]
diff --git a/tests/ui/needless_borrow.rs b/tests/ui/needless_borrow.rs
@@ -83,6 +83,36 @@ fn main() {
     let _ = (&x).0;
     let x = &x as *const (i32, i32);
     let _ = unsafe { (&*x).0 };
+
+    // Issue #8367
+    trait Foo {
+        fn foo(self);
+    }
+    impl Foo for &'_ () {
+        fn foo(self) {}
+    }
+    (&()).foo(); // Don't lint. `()` doesn't implement `Foo`
+    (&&()).foo();
+
+    impl Foo for i32 {
+        fn foo(self) {}
+    }
+    impl Foo for &'_ i32 {
+        fn foo(self) {}
+    }
+    (&5).foo(); // Don't lint. `5` will call `<i32 as Foo>::foo`
+    (&&5).foo();
+
+    trait FooRef {
+        fn foo_ref(&self);
+    }
+    impl FooRef for () {
+        fn foo_ref(&self) {}
+    }
+    impl FooRef for &'_ () {
+        fn foo_ref(&self) {}
+    }
+    (&&()).foo_ref(); // Don't lint. `&()` will call `<() as FooRef>::foo_ref`
 }
 
 #[allow(clippy::needless_borrowed_reference)]
diff --git a/tests/ui/needless_borrow.stderr b/tests/ui/needless_borrow.stderr
@@ -126,5 +126,17 @@ error: this expression borrows a value the compiler would automatically borrow
 LL |     let _ = unsafe { (&*x).0 };
    |                      ^^^^^ help: change this to: `(*x)`
 
-error: aborting due to 21 previous errors
+error: this expression creates a reference which is immediately dereferenced by the compiler
+  --> $DIR/needless_borrow.rs:95:5
+   |
+LL |     (&&()).foo();
+   |     ^^^^^^ help: change this to: `(&())`
+
+error: this expression creates a reference which is immediately dereferenced by the compiler
+  --> $DIR/needless_borrow.rs:104:5
+   |
+LL |     (&&5).foo();
+   |     ^^^^^ help: change this to: `(&5)`
+
+error: aborting due to 23 previous errors
 
