From 6e7a813ed20f9f0639c1565da083824d039566b8 Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Mon, 18 Feb 2019 07:30:50 +0200 Subject: [PATCH] Improve `iter_cloned_collect` suggestions Fixes #3704 --- clippy_lints/src/methods/mod.rs | 41 +++++++++++++++++-------------- tests/ui/unnecessary_clone.rs | 12 +++++++++ tests/ui/unnecessary_clone.stderr | 26 ++++++++++++++++---- 3 files changed, 56 insertions(+), 23 deletions(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index c9b27ef1615..b1bd4341015 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 check_fold_with_op( }; } -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,7 @@ fn lint_iter_skip_next(cx: &LateContext<'_, '_>, expr: &hir::Expr) { } } -fn derefs_to_slice(cx: &LateContext<'_, '_>, expr: &hir::Expr, ty: Ty<'_>) -> Option> { +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 +1700,17 @@ fn may_slice(cx: &LateContext<'_, '_>, ty: Ty<'_>) -> bool { 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..e7fceab01c6 100644 --- a/tests/ui/unnecessary_clone.rs +++ b/tests/ui/unnecessary_clone.rs @@ -66,6 +66,18 @@ fn iter_clone_collect() { let v2: Vec = v.iter().cloned().collect(); let v3: HashSet = v.iter().cloned().collect(); let v4: VecDeque = v.iter().cloned().collect(); + + // Handle macro expansion in suggestion + let _ : Vec = vec![1, 2, 3].iter().cloned().collect(); + + // Issue #3704 + unsafe { + let _: Vec = 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..13c5a43c8e9 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::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 = 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:39 + | +LL | let _ : Vec = 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