diff --git a/clippy_lints/src/map_clone.rs b/clippy_lints/src/map_clone.rs index 1546964e4..0fb0d8d69 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 5c4194882..aea892407 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 96a615ae5..e5560b34b 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 63889055a..504f4a01a 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