From b0d7aea946e88e87af136fb0fe5c4a7e87b6c16e Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Tue, 9 Oct 2018 19:25:03 -0700 Subject: [PATCH 1/6] 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 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 From 88ee209a1d2596efa1582cb7f993aca4308bf1c7 Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Tue, 9 Oct 2018 20:01:12 -0700 Subject: [PATCH 2/6] Corrected single-character string constant used as pattern found in dogfood test --- clippy_lints/src/misc.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/misc.rs b/clippy_lints/src/misc.rs index 4c6a9d6bd11..5f8480d8282 100644 --- a/clippy_lints/src/misc.rs +++ b/clippy_lints/src/misc.rs @@ -546,7 +546,7 @@ fn check_to_owned(cx: &LateContext<'_, '_>, expr: &Expr, other: &Expr) { } 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("*"); + let other_without_deref = other_snippet.trim_left_matches('*'); (expr.span.to(other.span), format!("{} == {}", snip.to_string(), other_without_deref)) } else { From d41615548e47f57c60ac8aed5d47a74dba048c13 Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Wed, 10 Oct 2018 04:51:06 -0700 Subject: [PATCH 3/6] cmp_owned add test for multiple dereference --- tests/ui/cmp_owned.rs | 5 +++++ tests/ui/cmp_owned.stderr | 12 +++++++++--- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/tests/ui/cmp_owned.rs b/tests/ui/cmp_owned.rs index 65351cd9b9d..dc0880e7089 100644 --- a/tests/ui/cmp_owned.rs +++ b/tests/ui/cmp_owned.rs @@ -40,6 +40,11 @@ fn main() { let y = &Baz; y.to_owned() == *x; + + let x = &&Baz; + let y = &Baz; + + y.to_owned() == **x; } struct Foo; diff --git a/tests/ui/cmp_owned.stderr b/tests/ui/cmp_owned.stderr index 2613d3b7500..0982467aeee 100644 --- a/tests/ui/cmp_owned.stderr +++ b/tests/ui/cmp_owned.stderr @@ -43,10 +43,16 @@ error: this creates an owned instance just for comparison | ^^^^^^^^^^^^^^^^^^ help: try: `y == x` error: this creates an owned instance just for comparison - --> $DIR/cmp_owned.rs:49:9 + --> $DIR/cmp_owned.rs:47:5 | -49 | self.to_owned() == *other +47 | y.to_owned() == **x; + | ^^^^^^^^^^^^^^^^^^^ help: try: `y == x` + +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 8 previous errors +error: aborting due to 9 previous errors From 0b65462ca52599162949df162239ce55de96b4b3 Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Thu, 11 Oct 2018 05:03:02 -0700 Subject: [PATCH 4/6] cmp_owned current suggestion for multiple deref --- clippy_lints/src/misc.rs | 2 +- tests/ui/cmp_owned.stderr | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/misc.rs b/clippy_lints/src/misc.rs index 5f8480d8282..be863cd7bc8 100644 --- a/clippy_lints/src/misc.rs +++ b/clippy_lints/src/misc.rs @@ -546,7 +546,7 @@ fn check_to_owned(cx: &LateContext<'_, '_>, expr: &Expr, other: &Expr) { } 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('*'); + let other_without_deref = other_snippet.replacen('*', "", 1); (expr.span.to(other.span), format!("{} == {}", snip.to_string(), other_without_deref)) } else { diff --git a/tests/ui/cmp_owned.stderr b/tests/ui/cmp_owned.stderr index 0982467aeee..1db60be54d6 100644 --- a/tests/ui/cmp_owned.stderr +++ b/tests/ui/cmp_owned.stderr @@ -46,7 +46,7 @@ error: this creates an owned instance just for comparison --> $DIR/cmp_owned.rs:47:5 | 47 | y.to_owned() == **x; - | ^^^^^^^^^^^^^^^^^^^ help: try: `y == x` + | ^^^^^^^^^^^^^^^^^^^ help: try: `y == *x` error: this creates an owned instance just for comparison --> $DIR/cmp_owned.rs:54:9 From c9718fa589552476ee277c52a35271663383cf6a Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Fri, 12 Oct 2018 04:34:41 -0700 Subject: [PATCH 5/6] cmp_owned correct error message if rhs is deref --- clippy_lints/src/misc.rs | 4 ++++ tests/ui/cmp_owned.stderr | 4 ++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/misc.rs b/clippy_lints/src/misc.rs index be863cd7bc8..0a65953313e 100644 --- a/clippy_lints/src/misc.rs +++ b/clippy_lints/src/misc.rs @@ -579,6 +579,10 @@ fn check_to_owned(cx: &LateContext<'_, '_>, expr: &Expr, other: &Expr) { } } } + if other_gets_derefed { + db.span_label(lint_span, "try implementing the comparison without allocating"); + return; + } db.span_suggestion_with_applicability( lint_span, "try", diff --git a/tests/ui/cmp_owned.stderr b/tests/ui/cmp_owned.stderr index 1db60be54d6..a7371ab4b6c 100644 --- a/tests/ui/cmp_owned.stderr +++ b/tests/ui/cmp_owned.stderr @@ -40,13 +40,13 @@ error: this creates an owned instance just for comparison --> $DIR/cmp_owned.rs:42:5 | 42 | y.to_owned() == *x; - | ^^^^^^^^^^^^^^^^^^ help: try: `y == x` + | ^^^^^^^^^^^^^^^^^^ try implementing the comparison without allocating error: this creates an owned instance just for comparison --> $DIR/cmp_owned.rs:47:5 | 47 | y.to_owned() == **x; - | ^^^^^^^^^^^^^^^^^^^ help: try: `y == *x` + | ^^^^^^^^^^^^^^^^^^^ try implementing the comparison without allocating error: this creates an owned instance just for comparison --> $DIR/cmp_owned.rs:54:9 From 352863065cb644a4f59fa5655601960e34bf77e7 Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Fri, 12 Oct 2018 04:48:54 -0700 Subject: [PATCH 6/6] cmp_owned refactor --- clippy_lints/src/misc.rs | 45 +++++++++++++--------------------------- 1 file changed, 14 insertions(+), 31 deletions(-) diff --git a/clippy_lints/src/misc.rs b/clippy_lints/src/misc.rs index 0a65953313e..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; @@ -540,18 +540,10 @@ fn check_to_owned(cx: &LateContext<'_, '_>, expr: &Expr, other: &Expr) { _ => 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.replacen('*', "", 1); - - (expr.span.to(other.span), format!("{} == {}", snip.to_string(), other_without_deref)) + let lint_span = if other_gets_derefed { + expr.span.to(other.span) } else { - // suggest dropping the to_owned on the left - (expr.span, snip.to_string()) + expr.span }; span_lint_and_then( @@ -560,29 +552,20 @@ fn check_to_owned(cx: &LateContext<'_, '_>, expr: &Expr, other: &Expr) { 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(lint_span, "try 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 { + // suggest deref on the left + format!("*{}", snip) + } else { + // suggest dropping the to_owned on the left + snip.to_string() + }; + db.span_suggestion_with_applicability( lint_span, "try",