diff options
| -rw-r--r-- | clippy_lints/src/methods/iter_overeager_cloned.rs | 97 | ||||
| -rw-r--r-- | clippy_lints/src/methods/mod.rs | 24 | ||||
| -rw-r--r-- | clippy_utils/src/ty.rs | 4 | ||||
| -rw-r--r-- | tests/ui/iter_overeager_cloned.fixed | 6 | ||||
| -rw-r--r-- | tests/ui/iter_overeager_cloned.rs | 3 | ||||
| -rw-r--r-- | tests/ui/iter_overeager_cloned.stderr | 42 |
6 files changed, 92 insertions, 84 deletions
diff --git a/clippy_lints/src/methods/iter_overeager_cloned.rs b/clippy_lints/src/methods/iter_overeager_cloned.rs index 54c9ca435a4..06a39c5997e 100644 --- a/clippy_lints/src/methods/iter_overeager_cloned.rs +++ b/clippy_lints/src/methods/iter_overeager_cloned.rs @@ -1,70 +1,59 @@ -use clippy_utils::diagnostics::span_lint_and_sugg; -use clippy_utils::source::snippet; -use clippy_utils::ty::{get_iterator_item_ty, implements_trait, is_copy}; -use itertools::Itertools; +use clippy_utils::diagnostics::span_lint_and_then; +use clippy_utils::source::snippet_opt; +use clippy_utils::ty::{get_associated_type, implements_trait, is_copy}; use rustc_errors::Applicability; -use rustc_hir as hir; +use rustc_hir::Expr; use rustc_lint::LateContext; use rustc_middle::ty; use rustc_span::sym; -use std::ops::Not; use super::ITER_OVEREAGER_CLONED; use crate::redundant_clone::REDUNDANT_CLONE; -/// lint overeager use of `cloned()` for `Iterator`s pub(super) fn check<'tcx>( cx: &LateContext<'tcx>, - expr: &'tcx hir::Expr<'_>, - recv: &'tcx hir::Expr<'_>, - name: &str, - map_arg: &[hir::Expr<'_>], + expr: &'tcx Expr<'_>, + cloned_call: &'tcx Expr<'_>, + cloned_recv: &'tcx Expr<'_>, + is_count: bool, + needs_into_iter: bool, ) { - // Check if it's iterator and get type associated with `Item`. - let inner_ty = if_chain! { - if let Some(iterator_trait_id) = cx.tcx.get_diagnostic_item(sym::Iterator); - let recv_ty = cx.typeck_results().expr_ty(recv); - if implements_trait(cx, recv_ty, iterator_trait_id, &[]); - if let Some(inner_ty) = get_iterator_item_ty(cx, cx.typeck_results().expr_ty_adjusted(recv)); - then { - inner_ty - } else { + let typeck = cx.typeck_results(); + if let Some(iter_id) = cx.tcx.get_diagnostic_item(sym::Iterator) + && let Some(method_id) = typeck.type_dependent_def_id(expr.hir_id) + && cx.tcx.trait_of_item(method_id) == Some(iter_id) + && let Some(method_id) = typeck.type_dependent_def_id(cloned_call.hir_id) + && cx.tcx.trait_of_item(method_id) == Some(iter_id) + && let cloned_recv_ty = typeck.expr_ty_adjusted(cloned_recv) + && let Some(iter_assoc_ty) = get_associated_type(cx, cloned_recv_ty, iter_id, "Item") + && matches!(*iter_assoc_ty.kind(), ty::Ref(_, ty, _) if !is_copy(cx, ty)) + { + if needs_into_iter + && let Some(into_iter_id) = cx.tcx.get_diagnostic_item(sym::IntoIterator) + && !implements_trait(cx, iter_assoc_ty, into_iter_id, &[]) + { return; } - }; - - match inner_ty.kind() { - ty::Ref(_, ty, _) if !is_copy(cx, *ty) => {}, - _ => return, - }; - let (lint, preserve_cloned) = match name { - "count" => (REDUNDANT_CLONE, false), - _ => (ITER_OVEREAGER_CLONED, true), - }; - let wildcard_params = map_arg.is_empty().not().then(|| "...").unwrap_or_default(); - let msg = format!( - "called `cloned().{}({})` on an `Iterator`. It may be more efficient to call `{}({}){}` instead", - name, - wildcard_params, - name, - wildcard_params, - preserve_cloned.then(|| ".cloned()").unwrap_or_default(), - ); + let (lint, msg, trailing_clone) = if is_count { + (REDUNDANT_CLONE, "unneeded cloning of iterator items", "") + } else { + (ITER_OVEREAGER_CLONED, "unnecessarily eager cloning of iterator items", ".cloned()") + }; - span_lint_and_sugg( - cx, - lint, - expr.span, - &msg, - "try this", - format!( - "{}.{}({}){}", - snippet(cx, recv.span, ".."), - name, - map_arg.iter().map(|a| snippet(cx, a.span, "..")).join(", "), - preserve_cloned.then(|| ".cloned()").unwrap_or_default(), - ), - Applicability::MachineApplicable, - ); + span_lint_and_then( + cx, + lint, + expr.span, + msg, + |diag| { + let method_span = expr.span.with_lo(cloned_call.span.hi()); + if let Some(mut snip) = snippet_opt(cx, method_span) { + snip.push_str(trailing_clone); + let replace_span = expr.span.with_lo(cloned_recv.span.hi()); + diag.span_suggestion(replace_span, "try this", snip, Applicability::MachineApplicable); + } + } + ); + } } diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 7308e74c323..2ebfcb7a5fe 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -2583,8 +2583,8 @@ impl Methods { }, _ => {}, }, - (name @ "count", args @ []) => match method_call(recv) { - Some(("cloned", [recv2], _)) => iter_overeager_cloned::check(cx, expr, recv2, name, args), + ("count", []) => match method_call(recv) { + Some(("cloned", [recv2], _)) => iter_overeager_cloned::check(cx, expr, recv, recv2, true, false), Some((name2 @ ("into_iter" | "iter" | "iter_mut"), [recv2], _)) => { iter_count::check(cx, expr, recv2, name2); }, @@ -2614,9 +2614,9 @@ impl Methods { flat_map_identity::check(cx, expr, arg, span); flat_map_option::check(cx, expr, arg, span); }, - (name @ "flatten", args @ []) => match method_call(recv) { + ("flatten", []) => match method_call(recv) { Some(("map", [recv, map_arg], map_span)) => map_flatten::check(cx, expr, recv, map_arg, map_span), - Some(("cloned", [recv2], _)) => iter_overeager_cloned::check(cx, expr, recv2, name, args), + Some(("cloned", [recv2], _)) => iter_overeager_cloned::check(cx, expr, recv, recv2, false, true), _ => {}, }, ("fold", [init, acc]) => unnecessary_fold::check(cx, expr, init, acc, span), @@ -2636,10 +2636,10 @@ impl Methods { unnecessary_join::check(cx, expr, recv, join_arg, span); } }, - ("last", args @ []) | ("skip", args @ [_]) => { + ("last", []) | ("skip", [_]) => { if let Some((name2, [recv2, args2 @ ..], _span2)) = method_call(recv) { if let ("cloned", []) = (name2, args2) { - iter_overeager_cloned::check(cx, expr, recv2, name, args); + iter_overeager_cloned::check(cx, expr, recv, recv2, false, false); } } }, @@ -2660,10 +2660,10 @@ impl Methods { map_identity::check(cx, expr, recv, m_arg, name, span); }, ("map_or", [def, map]) => option_map_or_none::check(cx, expr, recv, def, map), - (name @ "next", args @ []) => { + ("next", []) => { if let Some((name2, [recv2, args2 @ ..], _)) = method_call(recv) { match (name2, args2) { - ("cloned", []) => iter_overeager_cloned::check(cx, expr, recv2, name, args), + ("cloned", []) => iter_overeager_cloned::check(cx, expr, recv, recv2, false, false), ("filter", [arg]) => filter_next::check(cx, expr, recv2, arg), ("filter_map", [arg]) => filter_map_next::check(cx, expr, recv2, arg, self.msrv), ("iter", []) => iter_next_slice::check(cx, expr, recv2), @@ -2673,9 +2673,9 @@ impl Methods { } } }, - ("nth", args @ [n_arg]) => match method_call(recv) { + ("nth", [n_arg]) => match method_call(recv) { Some(("bytes", [recv2], _)) => bytes_nth::check(cx, expr, recv2, n_arg), - Some(("cloned", [recv2], _)) => iter_overeager_cloned::check(cx, expr, recv2, name, args), + Some(("cloned", [recv2], _)) => iter_overeager_cloned::check(cx, expr, recv, recv2, false, false), Some(("iter", [recv2], _)) => iter_nth::check(cx, expr, recv2, recv, n_arg, false), Some(("iter_mut", [recv2], _)) => iter_nth::check(cx, expr, recv2, recv, n_arg, true), _ => iter_nth_zero::check(cx, expr, recv, n_arg), @@ -2698,10 +2698,10 @@ impl Methods { } }, ("step_by", [arg]) => iterator_step_by_zero::check(cx, expr, arg), - ("take", args @ [_arg]) => { + ("take", [_arg]) => { if let Some((name2, [recv2, args2 @ ..], _span2)) = method_call(recv) { if let ("cloned", []) = (name2, args2) { - iter_overeager_cloned::check(cx, expr, recv2, name, args); + iter_overeager_cloned::check(cx, expr, recv, recv2, false, false); } } }, diff --git a/clippy_utils/src/ty.rs b/clippy_utils/src/ty.rs index a10515d2fec..227e97d37ec 100644 --- a/clippy_utils/src/ty.rs +++ b/clippy_utils/src/ty.rs @@ -78,9 +78,9 @@ pub fn get_associated_type<'tcx>( cx.tcx .associated_items(trait_id) .find_by_name_and_kind(cx.tcx, Ident::from_str(name), ty::AssocKind::Type, trait_id) - .map(|assoc| { + .and_then(|assoc| { let proj = cx.tcx.mk_projection(assoc.def_id, cx.tcx.mk_substs_trait(ty, &[])); - cx.tcx.normalize_erasing_regions(cx.param_env, proj) + cx.tcx.try_normalize_erasing_regions(cx.param_env, proj).ok() }) } diff --git a/tests/ui/iter_overeager_cloned.fixed b/tests/ui/iter_overeager_cloned.fixed index 7c2b05d837b..c100705d017 100644 --- a/tests/ui/iter_overeager_cloned.fixed +++ b/tests/ui/iter_overeager_cloned.fixed @@ -18,7 +18,8 @@ fn main() { let _ = vec.iter().filter(|x| x == &"2").nth(2).cloned(); let _ = [Some(Some("str".to_string())), Some(Some("str".to_string()))] - .iter().flatten().cloned(); + .iter() + .flatten().cloned(); // Not implemented yet let _ = vec.iter().cloned().filter(|x| x.starts_with('2')); @@ -43,6 +44,9 @@ fn main() { // Should probably stay as it is. let _ = [0, 1, 2, 3, 4].iter().cloned().take(10); + + // `&Range<_>` doesn't implement `IntoIterator` + let _ = [0..1, 2..5].iter().cloned().flatten(); } // #8527 diff --git a/tests/ui/iter_overeager_cloned.rs b/tests/ui/iter_overeager_cloned.rs index f2d0b155d2c..2caa8802066 100644 --- a/tests/ui/iter_overeager_cloned.rs +++ b/tests/ui/iter_overeager_cloned.rs @@ -45,6 +45,9 @@ fn main() { // Should probably stay as it is. let _ = [0, 1, 2, 3, 4].iter().cloned().take(10); + + // `&Range<_>` doesn't implement `IntoIterator` + let _ = [0..1, 2..5].iter().cloned().flatten(); } // #8527 diff --git a/tests/ui/iter_overeager_cloned.stderr b/tests/ui/iter_overeager_cloned.stderr index 0582700fd16..dcae7cecd33 100644 --- a/tests/ui/iter_overeager_cloned.stderr +++ b/tests/ui/iter_overeager_cloned.stderr @@ -1,44 +1,56 @@ -error: called `cloned().last()` on an `Iterator`. It may be more efficient to call `last().cloned()` instead +error: unnecessarily eager cloning of iterator items --> $DIR/iter_overeager_cloned.rs:8:29 | LL | let _: Option<String> = vec.iter().cloned().last(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `vec.iter().last().cloned()` + | ^^^^^^^^^^---------------- + | | + | help: try this: `.last().cloned()` | = note: `-D clippy::iter-overeager-cloned` implied by `-D warnings` -error: called `cloned().next()` on an `Iterator`. It may be more efficient to call `next().cloned()` instead +error: unnecessarily eager cloning of iterator items --> $DIR/iter_overeager_cloned.rs:10:29 | LL | let _: Option<String> = vec.iter().chain(vec.iter()).cloned().next(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `vec.iter().chain(vec.iter()).next().cloned()` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^---------------- + | | + | help: try this: `.next().cloned()` -error: called `cloned().count()` on an `Iterator`. It may be more efficient to call `count()` instead +error: unneeded cloning of iterator items --> $DIR/iter_overeager_cloned.rs:12:20 | LL | let _: usize = vec.iter().filter(|x| x == &"2").cloned().count(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `vec.iter().filter(|x| x == &"2").count()` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^----------------- + | | + | help: try this: `.count()` | = note: `-D clippy::redundant-clone` implied by `-D warnings` -error: called `cloned().take(...)` on an `Iterator`. It may be more efficient to call `take(...).cloned()` instead +error: unnecessarily eager cloning of iterator items --> $DIR/iter_overeager_cloned.rs:14:21 | LL | let _: Vec<_> = vec.iter().cloned().take(2).collect(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `vec.iter().take(2).cloned()` + | ^^^^^^^^^^----------------- + | | + | help: try this: `.take(2).cloned()` -error: called `cloned().skip(...)` on an `Iterator`. It may be more efficient to call `skip(...).cloned()` instead +error: unnecessarily eager cloning of iterator items --> $DIR/iter_overeager_cloned.rs:16:21 | LL | let _: Vec<_> = vec.iter().cloned().skip(2).collect(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `vec.iter().skip(2).cloned()` + | ^^^^^^^^^^----------------- + | | + | help: try this: `.skip(2).cloned()` -error: called `cloned().nth(...)` on an `Iterator`. It may be more efficient to call `nth(...).cloned()` instead +error: unnecessarily eager cloning of iterator items --> $DIR/iter_overeager_cloned.rs:18:13 | LL | let _ = vec.iter().filter(|x| x == &"2").cloned().nth(2); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `vec.iter().filter(|x| x == &"2").nth(2).cloned()` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^---------------- + | | + | help: try this: `.nth(2).cloned()` -error: called `cloned().flatten()` on an `Iterator`. It may be more efficient to call `flatten().cloned()` instead +error: unnecessarily eager cloning of iterator items --> $DIR/iter_overeager_cloned.rs:20:13 | LL | let _ = [Some(Some("str".to_string())), Some(Some("str".to_string()))] @@ -50,8 +62,8 @@ LL | | .flatten(); | help: try this | -LL ~ let _ = [Some(Some("str".to_string())), Some(Some("str".to_string()))] -LL ~ .iter().flatten().cloned(); +LL ~ .iter() +LL ~ .flatten().cloned(); | error: aborting due to 7 previous errors |