From b0d7aea946e88e87af136fb0fe5c4a7e87b6c16e Mon Sep 17 00:00:00 2001
From: Josh Mcguigan <joshmcg88@gmail.com>
Date: Tue, 9 Oct 2018 19:25:03 -0700
Subject: [PATCH] Fixes 3289, cmp_owned wording and false positive

---
 clippy_lints/src/misc.rs  | 26 ++++++++++++++++++++++----
 tests/ui/cmp_owned.rs     | 15 +++++++++++++++
 tests/ui/cmp_owned.stderr | 14 ++++++++++----
 3 files changed, 47 insertions(+), 8 deletions(-)

diff --git a/clippy_lints/src/misc.rs b/clippy_lints/src/misc.rs
index 4e6d5b72efc..4c6a9d6bd11 100644
--- a/clippy_lints/src/misc.rs
+++ b/clippy_lints/src/misc.rs
@@ -535,10 +535,29 @@ 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, try_hint) = if deref_arg_impl_partial_eq_other {
+        // suggest deref on the left
+        (expr.span, format!("*{}", snip))
+    } else if other_gets_derefed {
+        // suggest dropping the to_owned on the left and the deref on the right
+        let other_snippet = snippet(cx, other.span, "..").into_owned();
+        let other_without_deref = other_snippet.trim_left_matches("*");
+
+        (expr.span.to(other.span), format!("{} == {}", snip.to_string(), other_without_deref))
+    } else {
+        // suggest dropping the to_owned on the left
+        (expr.span, snip.to_string())
+    };
+
     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
@@ -554,15 +573,14 @@ fn check_to_owned(cx: &LateContext<'_, '_>, expr: &Expr, other: &Expr) {
                             // 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");
+                            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() };
             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 031809f5df5..65351cd9b9d 100644
--- a/tests/ui/cmp_owned.rs
+++ b/tests/ui/cmp_owned.rs
@@ -35,6 +35,11 @@ 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;
 }
 
 struct Foo;
@@ -67,3 +72,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..2613d3b7500 100644
--- a/tests/ui/cmp_owned.stderr
+++ b/tests/ui/cmp_owned.stderr
@@ -37,10 +37,16 @@ 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;
+   |     ^^^^^^^^^^^^^^^^^^ help: try: `y == x`
 
-error: aborting due to 7 previous errors
+error: this creates an owned instance just for comparison
+  --> $DIR/cmp_owned.rs:49:9
+   |
+49 |         self.to_owned() == *other
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^ try implementing the comparison without allocating
+
+error: aborting due to 8 previous errors