Auto merge of #3779 - mikerite:fix-3704, r=phansch

Improve `iter_cloned_collect` suggestions

Fixes #3704

3 files changed, 60 insertions, 23 deletions

diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs
index c9b27ef1615..739f6e3990a 100644
--- a/clippy_lints/src/methods/mod.rs
+++ b/clippy_lints/src/methods/mod.rs
@@ -1477,17 +1477,22 @@ fn lint_cstring_as_ptr(cx: &LateContext<'_, '_>, expr: &hir::Expr, new: &hir::Ex
     }
 }
 
-fn lint_iter_cloned_collect(cx: &LateContext<'_, '_>, expr: &hir::Expr, iter_args: &[hir::Expr]) {
-    if match_type(cx, cx.tables.expr_ty(expr), &paths::VEC)
-        && derefs_to_slice(cx, &iter_args[0], cx.tables.expr_ty(&iter_args[0])).is_some()
-    {
-        span_lint(
-            cx,
-            ITER_CLONED_COLLECT,
-            expr.span,
-            "called `cloned().collect()` on a slice to create a `Vec`. Calling `to_vec()` is both faster and \
-             more readable",
-        );
+fn lint_iter_cloned_collect<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &hir::Expr, iter_args: &'tcx [hir::Expr]) {
+    if match_type(cx, cx.tables.expr_ty(expr), &paths::VEC) {
+        if let Some(slice) = derefs_to_slice(cx, &iter_args[0], cx.tables.expr_ty(&iter_args[0])) {
+            if let Some(to_replace) = expr.span.trim_start(slice.span.source_callsite()) {
+                span_lint_and_sugg(
+                    cx,
+                    ITER_CLONED_COLLECT,
+                    to_replace,
+                    "called `iter().cloned().collect()` on a slice to create a `Vec`. Calling `to_vec()` is both faster and \
+                     more readable",
+                    "try",
+                    ".to_vec()".to_string(),
+                    Applicability::MachineApplicable,
+                );
+            }
+        }
     }
 }
 
@@ -1573,7 +1578,7 @@ fn lint_unnecessary_fold(cx: &LateContext<'_, '_>, expr: &hir::Expr, fold_args:
     };
 }
 
-fn lint_iter_nth(cx: &LateContext<'_, '_>, expr: &hir::Expr, iter_args: &[hir::Expr], is_mut: bool) {
+fn lint_iter_nth<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &hir::Expr, iter_args: &'tcx [hir::Expr], is_mut: bool) {
     let mut_str = if is_mut { "_mut" } else { "" };
     let caller_type = if derefs_to_slice(cx, &iter_args[0], cx.tables.expr_ty(&iter_args[0])).is_some() {
         "slice"
@@ -1596,7 +1601,7 @@ fn lint_iter_nth(cx: &LateContext<'_, '_>, expr: &hir::Expr, iter_args: &[hir::E
     );
 }
 
-fn lint_get_unwrap(cx: &LateContext<'_, '_>, expr: &hir::Expr, get_args: &[hir::Expr], is_mut: bool) {
+fn lint_get_unwrap<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &hir::Expr, get_args: &'tcx [hir::Expr], is_mut: bool) {
     // Note: we don't want to lint `get_mut().unwrap` for HashMap or BTreeMap,
     // because they do not implement `IndexMut`
     let mut applicability = Applicability::MachineApplicable;
@@ -1681,7 +1686,11 @@ fn lint_iter_skip_next(cx: &LateContext<'_, '_>, expr: &hir::Expr) {
     }
 }
 
-fn derefs_to_slice(cx: &LateContext<'_, '_>, expr: &hir::Expr, ty: Ty<'_>) -> Option<sugg::Sugg<'static>> {
+fn derefs_to_slice<'a, 'tcx>(
+    cx: &LateContext<'a, 'tcx>,
+    expr: &'tcx hir::Expr,
+    ty: Ty<'tcx>,
+) -> Option<&'tcx hir::Expr> {
     fn may_slice(cx: &LateContext<'_, '_>, ty: Ty<'_>) -> bool {
         match ty.sty {
             ty::Slice(_) => true,
@@ -1695,17 +1704,17 @@ fn derefs_to_slice(cx: &LateContext<'_, '_>, expr: &hir::Expr, ty: Ty<'_>) -> Op
 
     if let hir::ExprKind::MethodCall(ref path, _, ref args) = expr.node {
         if path.ident.name == "iter" && may_slice(cx, cx.tables.expr_ty(&args[0])) {
-            sugg::Sugg::hir_opt(cx, &args[0]).map(sugg::Sugg::addr)
+            Some(&args[0])
         } else {
             None
         }
     } else {
         match ty.sty {
-            ty::Slice(_) => sugg::Sugg::hir_opt(cx, expr),
-            ty::Adt(def, _) if def.is_box() && may_slice(cx, ty.boxed_ty()) => sugg::Sugg::hir_opt(cx, expr),
+            ty::Slice(_) => Some(expr),
+            ty::Adt(def, _) if def.is_box() && may_slice(cx, ty.boxed_ty()) => Some(expr),
             ty::Ref(_, inner, _) => {
                 if may_slice(cx, inner) {
-                    sugg::Sugg::hir_opt(cx, expr)
+                    Some(expr)
                 } else {
                     None
                 }
diff --git a/tests/ui/unnecessary_clone.rs b/tests/ui/unnecessary_clone.rs
index fee6b30a97b..570536d0a56 100644
--- a/tests/ui/unnecessary_clone.rs
+++ b/tests/ui/unnecessary_clone.rs
@@ -66,6 +66,18 @@ fn iter_clone_collect() {
     let v2: Vec<isize> = v.iter().cloned().collect();
     let v3: HashSet<isize> = v.iter().cloned().collect();
     let v4: VecDeque<isize> = v.iter().cloned().collect();
+
+    // Handle macro expansion in suggestion
+    let _: Vec<isize> = vec![1, 2, 3].iter().cloned().collect();
+
+    // Issue #3704
+    unsafe {
+        let _: Vec<u8> = std::ffi::CStr::from_ptr(std::ptr::null())
+            .to_bytes()
+            .iter()
+            .cloned()
+            .collect();
+    }
 }
 
 mod many_derefs {
diff --git a/tests/ui/unnecessary_clone.stderr b/tests/ui/unnecessary_clone.stderr
index 5cd9b2d337f..a014478597f 100644
--- a/tests/ui/unnecessary_clone.stderr
+++ b/tests/ui/unnecessary_clone.stderr
@@ -78,19 +78,35 @@ help: or try being explicit about what type to clone
 LL |     let z: &Vec<_> = &std::vec::Vec<i32>::clone(y);
    |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
-error: called `cloned().collect()` on a slice to create a `Vec`. Calling `to_vec()` is both faster and more readable
-  --> $DIR/unnecessary_clone.rs:66:26
+error: called `iter().cloned().collect()` on a slice to create a `Vec`. Calling `to_vec()` is both faster and more readable
+  --> $DIR/unnecessary_clone.rs:66:27
    |
 LL |     let v2: Vec<isize> = v.iter().cloned().collect();
-   |                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |                           ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `.to_vec()`
    |
    = note: `-D clippy::iter-cloned-collect` implied by `-D warnings`
 
+error: called `iter().cloned().collect()` on a slice to create a `Vec`. Calling `to_vec()` is both faster and more readable
+  --> $DIR/unnecessary_clone.rs:71:38
+   |
+LL |     let _: Vec<isize> = vec![1, 2, 3].iter().cloned().collect();
+   |                                      ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `.to_vec()`
+
+error: called `iter().cloned().collect()` on a slice to create a `Vec`. Calling `to_vec()` is both faster and more readable
+  --> $DIR/unnecessary_clone.rs:76:24
+   |
+LL |               .to_bytes()
+   |  ________________________^
+LL | |             .iter()
+LL | |             .cloned()
+LL | |             .collect();
+   | |______________________^ help: try: `.to_vec()`
+
 error: using `clone` on a `Copy` type
-  --> $DIR/unnecessary_clone.rs:102:20
+  --> $DIR/unnecessary_clone.rs:114:20
    |
 LL |         let _: E = a.clone();
    |                    ^^^^^^^^^ help: try dereferencing it: `*****a`
 
-error: aborting due to 13 previous errors
+error: aborting due to 15 previous errors