From 67a9f20c91fcb7281b46514bd866e353347a4416 Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Tue, 15 Jan 2019 08:09:47 +0200 Subject: [PATCH 1/4] Fix `map_clone` bad suggestion `cloned` requires that the elements of the iterator must be references. This change determines if that is the case by examining the type of the closure argument and suggesting `.cloned` only if it is a reference. When the closure argument is not a reference, it suggests removing the `map` call instead. A minor problem with this change is that the new check sometimes overlaps with the `clone_on_copy` lint. Fixes #498 --- clippy_lints/src/map_clone.rs | 67 ++++++++++++++++++++++++----------- tests/ui/map_clone.fixed | 12 +++++++ tests/ui/map_clone.rs | 12 +++++++ tests/ui/map_clone.stderr | 14 +++++--- 4 files changed, 80 insertions(+), 25 deletions(-) diff --git a/clippy_lints/src/map_clone.rs b/clippy_lints/src/map_clone.rs index 1546964e42612..0fb0d8d690f1b 100644 --- a/clippy_lints/src/map_clone.rs +++ b/clippy_lints/src/map_clone.rs @@ -5,6 +5,7 @@ use crate::utils::{ use if_chain::if_chain; use rustc::hir; use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass}; +use rustc::ty; use rustc::{declare_tool_lint, lint_array}; use rustc_errors::Applicability; use syntax::ast::Ident; @@ -69,19 +70,27 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { hir::PatKind::Ref(ref inner, _) => if let hir::PatKind::Binding( hir::BindingAnnotation::Unannotated, _, name, None ) = inner.node { - lint(cx, e.span, args[0].span, name, closure_expr); + if ident_eq(name, closure_expr) { + lint(cx, e.span, args[0].span); + } }, hir::PatKind::Binding(hir::BindingAnnotation::Unannotated, _, name, None) => { match closure_expr.node { hir::ExprKind::Unary(hir::UnOp::UnDeref, ref inner) => { - if !cx.tables.expr_ty(inner).is_box() { - lint(cx, e.span, args[0].span, name, inner); + if ident_eq(name, inner) && !cx.tables.expr_ty(inner).is_box() { + lint(cx, e.span, args[0].span); } }, hir::ExprKind::MethodCall(ref method, _, ref obj) => { - if method.ident.as_str() == "clone" + if ident_eq(name, &obj[0]) && method.ident.as_str() == "clone" && match_trait_method(cx, closure_expr, &paths::CLONE_TRAIT) { - lint(cx, e.span, args[0].span, name, &obj[0]); + + let obj_ty = cx.tables.expr_ty(&obj[0]); + if let ty::Ref(..) = obj_ty.sty { + lint(cx, e.span, args[0].span); + } else { + lint_needless_cloning(cx, e.span, args[0].span); + } } }, _ => {}, @@ -94,22 +103,38 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { } } -fn lint(cx: &LateContext<'_, '_>, replace: Span, root: Span, name: Ident, path: &hir::Expr) { +fn ident_eq(name: Ident, path: &hir::Expr) -> bool { if let hir::ExprKind::Path(hir::QPath::Resolved(None, ref path)) = path.node { - if path.segments.len() == 1 && path.segments[0].ident == name { - let mut applicability = Applicability::MachineApplicable; - span_lint_and_sugg( - cx, - MAP_CLONE, - replace, - "You are using an explicit closure for cloning elements", - "Consider calling the dedicated `cloned` method", - format!( - "{}.cloned()", - snippet_with_applicability(cx, root, "..", &mut applicability) - ), - applicability, - ) - } + path.segments.len() == 1 && path.segments[0].ident == name + } else { + false } } + +fn lint_needless_cloning(cx: &LateContext<'_, '_>, root: Span, receiver: Span) { + span_lint_and_sugg( + cx, + MAP_CLONE, + root.trim_start(receiver).unwrap(), + "You are needlessly cloning iterator elements", + "Remove the map call", + String::new(), + Applicability::MachineApplicable, + ) +} + +fn lint(cx: &LateContext<'_, '_>, replace: Span, root: Span) { + let mut applicability = Applicability::MachineApplicable; + span_lint_and_sugg( + cx, + MAP_CLONE, + replace, + "You are using an explicit closure for cloning elements", + "Consider calling the dedicated `cloned` method", + format!( + "{}.cloned()", + snippet_with_applicability(cx, root, "..", &mut applicability) + ), + applicability, + ) +} diff --git a/tests/ui/map_clone.fixed b/tests/ui/map_clone.fixed index 5c41948828663..aea8924073cbf 100644 --- a/tests/ui/map_clone.fixed +++ b/tests/ui/map_clone.fixed @@ -1,6 +1,7 @@ // run-rustfix #![warn(clippy::all, clippy::pedantic)] #![allow(clippy::iter_cloned_collect)] +#![allow(clippy::clone_on_copy)] #![allow(clippy::missing_docs_in_private_items)] fn main() { @@ -8,4 +9,15 @@ fn main() { let _: Vec = vec![String::new()].iter().cloned().collect(); let _: Vec = vec![42, 43].iter().cloned().collect(); let _: Option = Some(Box::new(16)).map(|b| *b); + + // Don't lint these + let v = vec![5_i8; 6]; + let a = 0; + let b = &a; + let _ = v.iter().map(|_x| *b); + let _ = v.iter().map(|_x| a.clone()); + let _ = v.iter().map(|&_x| a); + + // Issue #496 + let _ = std::env::args(); } diff --git a/tests/ui/map_clone.rs b/tests/ui/map_clone.rs index 96a615ae54c8c..e5560b34bb08a 100644 --- a/tests/ui/map_clone.rs +++ b/tests/ui/map_clone.rs @@ -1,6 +1,7 @@ // run-rustfix #![warn(clippy::all, clippy::pedantic)] #![allow(clippy::iter_cloned_collect)] +#![allow(clippy::clone_on_copy)] #![allow(clippy::missing_docs_in_private_items)] fn main() { @@ -8,4 +9,15 @@ fn main() { let _: Vec = vec![String::new()].iter().map(|x| x.clone()).collect(); let _: Vec = vec![42, 43].iter().map(|&x| x).collect(); let _: Option = Some(Box::new(16)).map(|b| *b); + + // Don't lint these + let v = vec![5_i8; 6]; + let a = 0; + let b = &a; + let _ = v.iter().map(|_x| *b); + let _ = v.iter().map(|_x| a.clone()); + let _ = v.iter().map(|&_x| a); + + // Issue #496 + let _ = std::env::args().map(|v| v.clone()); } diff --git a/tests/ui/map_clone.stderr b/tests/ui/map_clone.stderr index 63889055aa0e9..504f4a01a4cc6 100644 --- a/tests/ui/map_clone.stderr +++ b/tests/ui/map_clone.stderr @@ -1,5 +1,5 @@ error: You are using an explicit closure for cloning elements - --> $DIR/map_clone.rs:7:22 + --> $DIR/map_clone.rs:8:22 | LL | let _: Vec = vec![5_i8; 6].iter().map(|x| *x).collect(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: Consider calling the dedicated `cloned` method: `vec![5_i8; 6].iter().cloned()` @@ -7,16 +7,22 @@ LL | let _: Vec = vec![5_i8; 6].iter().map(|x| *x).collect(); = note: `-D clippy::map-clone` implied by `-D warnings` error: You are using an explicit closure for cloning elements - --> $DIR/map_clone.rs:8:26 + --> $DIR/map_clone.rs:9:26 | LL | let _: Vec = vec![String::new()].iter().map(|x| x.clone()).collect(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: Consider calling the dedicated `cloned` method: `vec![String::new()].iter().cloned()` error: You are using an explicit closure for cloning elements - --> $DIR/map_clone.rs:9:23 + --> $DIR/map_clone.rs:10:23 | LL | let _: Vec = vec![42, 43].iter().map(|&x| x).collect(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: Consider calling the dedicated `cloned` method: `vec![42, 43].iter().cloned()` -error: aborting due to 3 previous errors +error: You are needlessly cloning iterator elements + --> $DIR/map_clone.rs:22:29 + | +LL | let _ = std::env::args().map(|v| v.clone()); + | ^^^^^^^^^^^^^^^^^^^ help: Remove the map call + +error: aborting due to 4 previous errors From f96dc2e9e270da539ebd968203524e6141983643 Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Tue, 15 Jan 2019 08:15:12 +0200 Subject: [PATCH 2/4] Remove `map_clone` fixed known problem --- clippy_lints/src/map_clone.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/clippy_lints/src/map_clone.rs b/clippy_lints/src/map_clone.rs index 0fb0d8d690f1b..4db0ca759db80 100644 --- a/clippy_lints/src/map_clone.rs +++ b/clippy_lints/src/map_clone.rs @@ -19,9 +19,7 @@ pub struct Pass; /// /// **Why is this bad?** Readability, this can be written more concisely /// -/// **Known problems:** Sometimes `.cloned()` requires stricter trait -/// bound than `.map(|e| e.clone())` (which works because of the coercion). -/// See [#498](https://github.com/rust-lang-nursery/rust-clippy/issues/498). +/// **Known problems:** None /// /// **Example:** /// From f53f12b0c3d8cd619d657a50367d0f5fddf32548 Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Tue, 15 Jan 2019 08:17:55 +0200 Subject: [PATCH 3/4] Fix issue number in `map_clone` test --- tests/ui/map_clone.fixed | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/ui/map_clone.fixed b/tests/ui/map_clone.fixed index aea8924073cbf..af417815ed1d4 100644 --- a/tests/ui/map_clone.fixed +++ b/tests/ui/map_clone.fixed @@ -18,6 +18,6 @@ fn main() { let _ = v.iter().map(|_x| a.clone()); let _ = v.iter().map(|&_x| a); - // Issue #496 + // Issue #498 let _ = std::env::args(); } From 89de4c9766eeecd791c789ef63aab9df4b9d906c Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Tue, 15 Jan 2019 08:36:56 +0200 Subject: [PATCH 4/4] Really fix issue number in `map_clone` test --- tests/ui/map_clone.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/ui/map_clone.rs b/tests/ui/map_clone.rs index e5560b34bb08a..7dd2ce3020230 100644 --- a/tests/ui/map_clone.rs +++ b/tests/ui/map_clone.rs @@ -18,6 +18,6 @@ fn main() { let _ = v.iter().map(|_x| a.clone()); let _ = v.iter().map(|&_x| a); - // Issue #496 + // Issue #498 let _ = std::env::args().map(|v| v.clone()); }