Auto merge of #3662 - mikerite:fix-498, r=oli-obk

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

4 files changed, 81 insertions, 28 deletions

diff --git a/clippy_lints/src/map_clone.rs b/clippy_lints/src/map_clone.rs
index 1546964e426..4db0ca759db 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;
@@ -18,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:**
 ///
@@ -69,19 +68,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 +101,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 5c419488286..af417815ed1 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<String> = vec![String::new()].iter().cloned().collect();
     let _: Vec<u32> = vec![42, 43].iter().cloned().collect();
     let _: Option<u64> = 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 #498
+    let _ = std::env::args();
 }
diff --git a/tests/ui/map_clone.rs b/tests/ui/map_clone.rs
index 96a615ae54c..7dd2ce30202 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<String> = vec![String::new()].iter().map(|x| x.clone()).collect();
     let _: Vec<u32> = vec![42, 43].iter().map(|&x| x).collect();
     let _: Option<u64> = 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 #498
+    let _ = std::env::args().map(|v| v.clone());
 }
diff --git a/tests/ui/map_clone.stderr b/tests/ui/map_clone.stderr
index 63889055aa0..504f4a01a4c 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<i8> = 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<i8> = 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<String> = 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<u32> = 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