Merge pull request #3291 from JoshMcguigan/cmp_owned-3289

cmp_owned wording and false positive

3 files changed, 63 insertions, 26 deletions

diff --git a/clippy_lints/src/misc.rs b/clippy_lints/src/misc.rs
index 4e6d5b72efc..1cf7345e8df 100644
--- a/clippy_lints/src/misc.rs
+++ b/clippy_lints/src/misc.rs
@@ -21,7 +21,7 @@ use crate::utils::{get_item_name, get_parent_expr, implements_trait, in_constant
             iter_input_pats, last_path_segment, match_qpath, match_trait_method, paths, snippet, span_lint,
             span_lint_and_then, walk_ptrs_ty, SpanlessEq};
 use crate::utils::sugg::Sugg;
-use crate::syntax::ast::{LitKind, CRATE_NODE_ID};
+use crate::syntax::ast::LitKind;
 use crate::consts::{constant, Constant};
 use crate::rustc_errors::Applicability;
 
@@ -535,34 +535,39 @@ fn check_to_owned(cx: &LateContext<'_, '_>, expr: &Expr, other: &Expr) {
         return;
     }
 
+    let other_gets_derefed = match other.node {
+        ExprKind::Unary(UnDeref, _) => true,
+        _ => false,
+    };
+
+    let lint_span = if other_gets_derefed {
+        expr.span.to(other.span)
+    } else {
+        expr.span
+    };
+
     span_lint_and_then(
         cx,
         CMP_OWNED,
-        expr.span,
+        lint_span,
         "this creates an owned instance just for comparison",
         |db| {
-            // this is as good as our recursion check can get, we can't prove that the
-            // current function is
-            // called by
-            // PartialEq::eq, but we can at least ensure that this code is not part of it
-            let parent_fn = cx.tcx.hir.get_parent(expr.id);
-            let parent_impl = cx.tcx.hir.get_parent(parent_fn);
-            if parent_impl != CRATE_NODE_ID {
-                if let Node::Item(item) = cx.tcx.hir.get(parent_impl) {
-                    if let ItemKind::Impl(.., Some(ref trait_ref), _, _) = item.node {
-                        if trait_ref.path.def.def_id() == partial_eq_trait_id {
-                            // we are implementing PartialEq, don't suggest not doing `to_owned`, otherwise
-                            // we go into
-                            // recursion
-                            db.span_label(expr.span, "try calling implementing the comparison without allocating");
-                            return;
-                        }
-                    }
-                }
+            // this also catches PartialEq implementations that call to_owned
+            if other_gets_derefed {
+                db.span_label(lint_span, "try implementing the comparison without allocating");
+                return;
             }
-            let try_hint = if deref_arg_impl_partial_eq_other { format!("*{}", snip) } else { snip.to_string() };
+
+            let try_hint = if deref_arg_impl_partial_eq_other {
+                // suggest deref on the left
+                format!("*{}", snip)
+            } else {
+                // suggest dropping the to_owned on the left
+                snip.to_string()
+            };
+
             db.span_suggestion_with_applicability(
-                expr.span,
+                lint_span,
                 "try",
                 try_hint,
                 Applicability::MachineApplicable, // snippet
diff --git a/tests/ui/cmp_owned.rs b/tests/ui/cmp_owned.rs
index 978e919ff43..dc3d62ddfa6 100644
--- a/tests/ui/cmp_owned.rs
+++ b/tests/ui/cmp_owned.rs
@@ -35,6 +35,16 @@ fn main() {
     "abc".chars().filter(|c| c.to_owned() != 'X');
 
     "abc".chars().filter(|c| *c != 'X');
+
+    let x = &Baz;
+    let y = &Baz;
+
+    y.to_owned() == *x;
+
+    let x = &&Baz;
+    let y = &Baz;
+
+    y.to_owned() == **x;
 }
 
 struct Foo;
@@ -67,3 +77,13 @@ impl std::borrow::Borrow<Foo> for Bar {
         &FOO
     }
 }
+
+#[derive(PartialEq)]
+struct Baz;
+
+impl ToOwned for Baz {
+    type Owned = Baz;
+    fn to_owned(&self) -> Baz {
+        Baz
+    }
+}
diff --git a/tests/ui/cmp_owned.stderr b/tests/ui/cmp_owned.stderr
index 5434b68de9f..a7371ab4b6c 100644
--- a/tests/ui/cmp_owned.stderr
+++ b/tests/ui/cmp_owned.stderr
@@ -37,10 +37,22 @@ error: this creates an owned instance just for comparison
    |                              ^^^^^^^^^^^^ help: try: `*c`
 
 error: this creates an owned instance just for comparison
-  --> $DIR/cmp_owned.rs:44:9
+  --> $DIR/cmp_owned.rs:42:5
    |
-44 |         self.to_owned() == *other
-   |         ^^^^^^^^^^^^^^^ try calling implementing the comparison without allocating
+42 |     y.to_owned() == *x;
+   |     ^^^^^^^^^^^^^^^^^^ try implementing the comparison without allocating
 
-error: aborting due to 7 previous errors
+error: this creates an owned instance just for comparison
+  --> $DIR/cmp_owned.rs:47:5
+   |
+47 |     y.to_owned() == **x;
+   |     ^^^^^^^^^^^^^^^^^^^ try implementing the comparison without allocating
+
+error: this creates an owned instance just for comparison
+  --> $DIR/cmp_owned.rs:54:9
+   |
+54 |         self.to_owned() == *other
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^ try implementing the comparison without allocating
+
+error: aborting due to 9 previous errors