From 51f6aeae51e220f990799a2f4b377ade40b72a49 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Thu, 11 May 2017 16:37:46 +0200 Subject: [PATCH 1/2] Reduce code duplication --- clippy_lints/src/misc.rs | 30 ++++++++++-------------------- 1 file changed, 10 insertions(+), 20 deletions(-) diff --git a/clippy_lints/src/misc.rs b/clippy_lints/src/misc.rs index c2fc932a678..3a202c189ec 100644 --- a/clippy_lints/src/misc.rs +++ b/clippy_lints/src/misc.rs @@ -456,26 +456,16 @@ fn check_to_owned(cx: &LateContext, expr: &Expr, other: &Expr, left: bool, op: S return; } - if left { - span_lint(cx, - CMP_OWNED, - expr.span, - &format!("this creates an owned instance just for comparison. Consider using `{} {} {}` to \ - compare without allocation", - snip, - snippet(cx, op, "=="), - snippet(cx, other.span, ".."))); - } else { - span_lint(cx, - CMP_OWNED, - expr.span, - &format!("this creates an owned instance just for comparison. Consider using `{} {} {}` to \ - compare without allocation", - snippet(cx, other.span, ".."), - snippet(cx, op, "=="), - snip)); - } - + let other = snippet(cx, other.span, ".."); + let (snip, other) = if left { (snip, other) } else { (other, snip) }; + span_lint(cx, + CMP_OWNED, + expr.span, + &format!("this creates an owned instance just for comparison. Consider using `{} {} {}` to \ + compare without allocation", + snip, + snippet(cx, op, "=="), + other)); } fn is_str_arg(cx: &LateContext, args: &[Expr]) -> bool { From 672045689e18bc15ae76bacc70540c0b4fe769b9 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Thu, 11 May 2017 18:59:36 +0200 Subject: [PATCH 2/2] Fix #1730 --- clippy_lints/src/misc.rs | 67 ++++++++++++++++++++------------- clippy_lints/src/utils/paths.rs | 2 + tests/ui/cmp_owned.rs | 36 ++++++++++++++++-- tests/ui/cmp_owned.stderr | 36 ++++++++++++------ 4 files changed, 100 insertions(+), 41 deletions(-) diff --git a/clippy_lints/src/misc.rs b/clippy_lints/src/misc.rs index 3a202c189ec..dbadce6193c 100644 --- a/clippy_lints/src/misc.rs +++ b/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, right, left, false, cmp.span) + check_to_owned(cx, left, right); + 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 => { - let name = name.as_str(); - if name == "to_string" || name == "to_owned" && is_str_arg(cx, args) { - (cx.tables.expr_ty(&args[0]), snippet(cx, args[0].span, "..")) + ExprMethodCall(.., ref args) if args.len() == 1 => { + if match_trait_method(cx, expr, &paths::TO_STRING) || match_trait_method(cx, expr, &paths::TO_OWNED) { + (cx.tables.expr_ty_adjusted(&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,30 +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 + 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 + && !implements_trait(cx, arg_ty, partial_eq_trait_id, &[other_ty], None) { return; } - let other = snippet(cx, other.span, ".."); - let (snip, other) = if left { (snip, other) } else { (other, snip) }; - span_lint(cx, - CMP_OWNED, - expr.span, - &format!("this creates an owned instance just for comparison. Consider using `{} {} {}` to \ - compare without allocation", - snip, - snippet(cx, op, "=="), - other)); -} - -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) + span_lint_and_then(cx, CMP_OWNED, expr.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 map::NodeItem(item) = cx.tcx.hir.get(parent_impl) { + if let ItemImpl(.., 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; + } + } + } + } + db.span_suggestion(expr.span, "try", snip.to_string()); + }); } /// Heuristic to see if an expression is used. Should be compatible with `unused_variables`'s idea diff --git a/clippy_lints/src/utils/paths.rs b/clippy_lints/src/utils/paths.rs index 57d2e5a33f1..f7e0d0c6aae 100644 --- a/clippy_lints/src/utils/paths.rs +++ b/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", "", "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"]; diff --git a/tests/ui/cmp_owned.rs b/tests/ui/cmp_owned.rs index 12dcc8262c2..e586b164cc6 100644 --- a/tests/ui/cmp_owned.rs +++ b/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 for Bar { + fn eq(&self, _: &Foo) -> bool { + true + } +} + +impl std::borrow::Borrow for Bar { + fn borrow(&self) -> &Foo { + static FOO: Foo = Foo; + &FOO + } } diff --git a/tests/ui/cmp_owned.stderr b/tests/ui/cmp_owned.stderr index a476110be61..14cad03a0f2 100644 --- a/tests/ui/cmp_owned.stderr +++ b/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 - --> $DIR/cmp_owned.rs:22:10 +error: this creates an owned instance just for comparison + --> $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