Merge pull request #1749 from Manishearth/cmp_owned

Improve cmp_owned and stop suggestiong `self == other` inside a `PartialEq` impl

clippy_lints/src/misc.rs

@@ -6,11 +6,12 @@ use rustc::middle::const_val::ConstVal;
 use rustc::ty;
 use rustc_const_eval::ConstContext;
 use rustc_const_math::ConstFloat;
-use syntax::codemap::{Span, Spanned, ExpnFormat};
+use syntax::codemap::{Span, ExpnFormat};
 use utils::{get_item_name, get_parent_expr, implements_trait, in_macro, is_integer_literal, match_path, snippet,
-            span_lint, span_lint_and_then, walk_ptrs_ty, last_path_segment, iter_input_pats, in_constant};
+            span_lint, span_lint_and_then, walk_ptrs_ty, last_path_segment, iter_input_pats, in_constant,
+            match_trait_method, paths};
 use utils::sugg::Sugg;
-use syntax::ast::LitKind;
+use syntax::ast::{LitKind, CRATE_NODE_ID};
 
 /// **What it does:** Checks for function arguments and let bindings denoted as `ref`.
 ///
@@ -297,8 +298,8 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass {
                     if let ExprPath(QPath::Resolved(_, ref path)) = right.node {
                         check_nan(cx, path, expr);
                     }
-                    check_to_owned(cx, left, right, true, cmp.span);
+                    check_to_owned(cx, left, right);
-                    check_to_owned(cx, right, left, false, cmp.span)
+                    check_to_owned(cx, right, left);
                 }
                 if (op == BiEq || op == BiNe) && (is_float(cx, left) || is_float(cx, right)) {
                     if is_allowed(cx, left) || is_allowed(cx, right) {
@@ -422,12 +423,11 @@ fn is_float(cx: &LateContext, expr: &Expr) -> bool {
     matches!(walk_ptrs_ty(cx.tables.expr_ty(expr)).sty, ty::TyFloat(_))
 }
 
-fn check_to_owned(cx: &LateContext, expr: &Expr, other: &Expr, left: bool, op: Span) {
+fn check_to_owned(cx: &LateContext, expr: &Expr, other: &Expr) {
     let (arg_ty, snip) = match expr.node {
-        ExprMethodCall(Spanned { node: ref name, .. }, _, ref args) if args.len() == 1 => {
+        ExprMethodCall(.., ref args) if args.len() == 1 => {
-            let name = name.as_str();
+            if match_trait_method(cx, expr, &paths::TO_STRING) || match_trait_method(cx, expr, &paths::TO_OWNED) {
-            if name == "to_string" || name == "to_owned" && is_str_arg(cx, args) {
+                (cx.tables.expr_ty_adjusted(&args[0]), snippet(cx, args[0].span, ".."))
-                (cx.tables.expr_ty(&args[0]), snippet(cx, args[0].span, ".."))
             } else {
                 return;
             }
@@ -435,7 +435,7 @@ fn check_to_owned(cx: &LateContext, expr: &Expr, other: &Expr, left: bool, op: S
         ExprCall(ref path, ref v) if v.len() == 1 => {
             if let ExprPath(ref path) = path.node {
                 if match_path(path, &["String", "from_str"]) || match_path(path, &["String", "from"]) {
-                    (cx.tables.expr_ty(&v[0]), snippet(cx, v[0].span, ".."))
+                    (cx.tables.expr_ty_adjusted(&v[0]), snippet(cx, v[0].span, ".."))
                 } else {
                     return;
                 }
@@ -446,40 +446,43 @@ fn check_to_owned(cx: &LateContext, expr: &Expr, other: &Expr, left: bool, op: S
         _ => return,
     };
 
-    let other_ty = cx.tables.expr_ty(other);
+    let other_ty = cx.tables.expr_ty_adjusted(other);
     let partial_eq_trait_id = match cx.tcx.lang_items.eq_trait() {
         Some(id) => id,
         None => return,
     };
 
-    if !implements_trait(cx, arg_ty, partial_eq_trait_id, &[other_ty], None) {
+    // *arg impls PartialEq<other>
+    if !arg_ty
+        .builtin_deref(true, ty::LvaluePreference::NoPreference)
+        .map_or(false, |tam| implements_trait(cx, tam.ty, partial_eq_trait_id, &[other_ty], None))
+        // arg impls PartialEq<*other>
+        && !other_ty
+        .builtin_deref(true, ty::LvaluePreference::NoPreference)
+        .map_or(false, |tam| implements_trait(cx, arg_ty, partial_eq_trait_id, &[tam.ty], None))
+        // arg impls PartialEq<other>
+        && !implements_trait(cx, arg_ty, partial_eq_trait_id, &[other_ty], None) {
         return;
     }
 
-    if left {
+    span_lint_and_then(cx, CMP_OWNED, expr.span, "this creates an owned instance just for comparison", |db| {
-        span_lint(cx,
+        // this is as good as our recursion check can get, we can't prove that the current function is called by
-                  CMP_OWNED,
+        // PartialEq::eq, but we can at least ensure that this code is not part of it
-                  expr.span,
+        let parent_fn = cx.tcx.hir.get_parent(expr.id);
-                  &format!("this creates an owned instance just for comparison. Consider using `{} {} {}` to \
+        let parent_impl = cx.tcx.hir.get_parent(parent_fn);
-                            compare without allocation",
+        if parent_impl != CRATE_NODE_ID {
-                           snip,
+            if let map::NodeItem(item) = cx.tcx.hir.get(parent_impl) {
-                           snippet(cx, op, "=="),
+                if let ItemImpl(.., Some(ref trait_ref), _, _) = item.node {
-                           snippet(cx, other.span, "..")));
+                    if trait_ref.path.def.def_id() == partial_eq_trait_id {
-    } else {
+                        // we are implementing PartialEq, don't suggest not doing `to_owned`, otherwise we go into recursion
-        span_lint(cx,
+                        db.span_label(expr.span, "try calling implementing the comparison without allocating");
-                  CMP_OWNED,
+                        return;
-                  expr.span,
+                    }
-                  &format!("this creates an owned instance just for comparison. Consider using `{} {} {}` to \
+                }
-                            compare without allocation",
+            }
-                           snippet(cx, other.span, ".."),
+        }
-                           snippet(cx, op, "=="),
+        db.span_suggestion(expr.span, "try", snip.to_string());
-                           snip));
+    });
-    }
-
-}
-
-fn is_str_arg(cx: &LateContext, args: &[Expr]) -> bool {
-    args.len() == 1 && matches!(walk_ptrs_ty(cx.tables.expr_ty(&args[0])).sty, ty::TyStr)
 }
 
 /// Heuristic to see if an expression is used. Should be compatible with `unused_variables`'s idea

clippy_lints/src/utils/paths.rs

@@ -71,6 +71,8 @@ pub const RESULT_OK: [&'static str; 4] = ["core", "result", "Result", "Ok"];
 pub const SERDE_DE_VISITOR: [&'static str; 3] = ["serde", "de", "Visitor"];
 pub const SLICE_INTO_VEC: [&'static str; 4] = ["collections", "slice", "<impl [T]>", "into_vec"];
 pub const STRING: [&'static str; 3] = ["collections", "string", "String"];
+pub const TO_OWNED: [&'static str; 3] = ["collections", "borrow", "ToOwned"];
+pub const TO_STRING: [&'static str; 3] = ["collections", "string", "ToString"];
 pub const TRANSMUTE: [&'static str; 4] = ["core", "intrinsics", "", "transmute"];
 pub const VEC: [&'static str; 3] = ["collections", "vec", "Vec"];
 pub const VEC_DEQUE: [&'static str; 3] = ["collections", "vec_deque", "VecDeque"];

tests/ui/cmp_owned.rs

@@ -16,10 +16,40 @@ fn main() {
 
     x != "foo".to_owned();
 
-    // removed String::from_str(..), as it has finally been removed in 1.4.0
-    // as of 2015-08-14
-
     x != String::from("foo");
 
     42.to_string() == "42";
+
+    Foo.to_owned() == Foo;
+}
+
+struct Foo;
+
+impl PartialEq for Foo {
+    fn eq(&self, other: &Self) -> bool {
+        self.to_owned() == *other
+    }
+}
+
+impl ToOwned for Foo {
+    type Owned = Bar;
+    fn to_owned(&self) -> Bar {
+        Bar
+    }
+}
+
+#[derive(PartialEq)]
+struct Bar;
+
+impl PartialEq<Foo> for Bar {
+    fn eq(&self, _: &Foo) -> bool {
+        true
+    }
+}
+
+impl std::borrow::Borrow<Foo> for Bar {
+    fn borrow(&self) -> &Foo {
+        static FOO: Foo = Foo;
+        &FOO
+    }
 }

tests/ui/cmp_owned.stderr

@@ -1,8 +1,8 @@
-error: this creates an owned instance just for comparison. Consider using `x != "foo"` to compare without allocation
+error: this creates an owned instance just for comparison
  --> $DIR/cmp_owned.rs:8:14
   |
 8 |         x != "foo".to_string();
-  |              ^^^^^^^^^^^^^^^^^
+  |              ^^^^^^^^^^^^^^^^^ help: try `"foo"`
   |
 note: lint level defined here
  --> $DIR/cmp_owned.rs:4:8
@@ -10,23 +10,37 @@ note: lint level defined here
 4 | #[deny(cmp_owned)]
   |        ^^^^^^^^^
 
-error: this creates an owned instance just for comparison. Consider using `"foo" != x` to compare without allocation
+error: this creates an owned instance just for comparison
   --> $DIR/cmp_owned.rs:10:9
    |
 10 |         "foo".to_string() != x;
-   |         ^^^^^^^^^^^^^^^^^
+   |         ^^^^^^^^^^^^^^^^^ help: try `"foo"`
 
-error: this creates an owned instance just for comparison. Consider using `x != "foo"` to compare without allocation
+error: this creates an owned instance just for comparison
   --> $DIR/cmp_owned.rs:17:10
    |
 17 |     x != "foo".to_owned();
-   |          ^^^^^^^^^^^^^^^^
+   |          ^^^^^^^^^^^^^^^^ help: try `"foo"`
 
-error: this creates an owned instance just for comparison. Consider using `x != "foo"` to compare without allocation
+error: this creates an owned instance just for comparison
-  --> $DIR/cmp_owned.rs:22:10
+  --> $DIR/cmp_owned.rs:19:10
    |
-22 |     x != String::from("foo");
+19 |     x != String::from("foo");
-   |          ^^^^^^^^^^^^^^^^^^^
+   |          ^^^^^^^^^^^^^^^^^^^ help: try `"foo"`
 
-error: aborting due to 4 previous errors
+error: this creates an owned instance just for comparison
+  --> $DIR/cmp_owned.rs:23:5
+   |
+23 |     Foo.to_owned() == Foo;
+   |     ^^^^^^^^^^^^^^ help: try `Foo`
+
+warning: this creates an owned instance just for comparison
+  --> $DIR/cmp_owned.rs:30:9
+   |
+30 |         self.to_owned() == *other
+   |         ^^^^^^^^^^^^^^^ try calling implementing the comparison without allocating
+   |
+   = note: #[warn(cmp_owned)] on by default
+
+error: aborting due to 5 previous errors