diff --git a/clippy_lints/src/methods/iter_overeager_cloned.rs b/clippy_lints/src/methods/iter_overeager_cloned.rs index 9f7ec19aa598..4b1ca0d9b9cf 100644 --- a/clippy_lints/src/methods/iter_overeager_cloned.rs +++ b/clippy_lints/src/methods/iter_overeager_cloned.rs @@ -2,7 +2,7 @@ use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::source::snippet_opt; use clippy_utils::ty::{implements_trait, is_copy}; use rustc_errors::Applicability; -use rustc_hir::Expr; +use rustc_hir::{Expr, ExprKind}; use rustc_lint::LateContext; use rustc_middle::ty; use rustc_span::sym; @@ -10,12 +10,28 @@ use rustc_span::sym; use super::ITER_OVEREAGER_CLONED; use crate::redundant_clone::REDUNDANT_CLONE; +#[derive(Clone, Copy)] +pub(super) enum Op<'a> { + // rm `.cloned()` + // e.g. `count` + RmCloned, + + // later `.cloned()` + // and add `&` to the parameter of closure parameter + // e.g. `find` `filter` + FixClosure(&'a str, &'a Expr<'a>), + + // later `.cloned()` + // e.g. `skip` `take` + LaterCloned, +} + pub(super) fn check<'tcx>( cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, cloned_call: &'tcx Expr<'_>, cloned_recv: &'tcx Expr<'_>, - is_count: bool, + op: Op<'tcx>, needs_into_iter: bool, ) { let typeck = cx.typeck_results(); @@ -35,10 +51,9 @@ pub(super) fn check<'tcx>( return; } - 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()") + let (lint, msg, trailing_clone) = match op { + Op::RmCloned => (REDUNDANT_CLONE, "unneeded cloning of iterator items", ""), + Op::LaterCloned | Op::FixClosure(_, _) => (ITER_OVEREAGER_CLONED, "unnecessarily eager cloning of iterator items", ".cloned()"), }; span_lint_and_then( @@ -47,11 +62,27 @@ pub(super) fn check<'tcx>( 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", snip, Applicability::MachineApplicable); + match op { + Op::RmCloned | Op::LaterCloned => { + 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", snip, Applicability::MachineApplicable); + } + } + Op::FixClosure(name, predicate_expr) => { + if let Some(predicate) = snippet_opt(cx, predicate_expr.span) { + let new_closure = if let ExprKind::Closure(_) = predicate_expr.kind { + predicate.replacen('|', "|&", 1) + } else { + format!("|&x| {predicate}(x)") + }; + let snip = format!(".{name}({new_closure}).cloned()" ); + let replace_span = expr.span.with_lo(cloned_recv.span.hi()); + diag.span_suggestion(replace_span, "try", snip, Applicability::MachineApplicable); + } + } } } ); diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 42756b27d014..729eda37af62 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -3919,7 +3919,7 @@ impl Methods { } }, ("count", []) if is_trait_method(cx, expr, sym::Iterator) => match method_call(recv) { - Some(("cloned", recv2, [], _, _)) => iter_overeager_cloned::check(cx, expr, recv, recv2, true, false), + Some(("cloned", recv2, [], _, _)) => iter_overeager_cloned::check(cx, expr, recv, recv2, iter_overeager_cloned::Op::RmCloned , false), Some((name2 @ ("into_iter" | "iter" | "iter_mut"), recv2, [], _, _)) => { iter_count::check(cx, expr, recv2, name2); }, @@ -3973,6 +3973,13 @@ impl Methods { string_extend_chars::check(cx, expr, recv, arg); extend_with_drain::check(cx, expr, recv, arg); }, + (name @ ( "filter" | "find" ) , [arg]) => { + if let Some(("cloned", recv2, [], _span2, _)) = method_call(recv) { + // if `arg` has side-effect, the semantic will change + iter_overeager_cloned::check(cx, expr, recv, recv2, + iter_overeager_cloned::Op::FixClosure(name, arg), false); + } + } ("filter_map", [arg]) => { unnecessary_filter_map::check(cx, expr, arg, name); filter_map_bool_then::check(cx, expr, arg, call_span); @@ -3987,7 +3994,7 @@ impl Methods { }, ("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, recv, recv2, false, true), + Some(("cloned", recv2, [], _, _)) => iter_overeager_cloned::check(cx, expr, recv, recv2, iter_overeager_cloned::Op::LaterCloned , true), _ => {}, }, ("fold", [init, acc]) => { @@ -4021,7 +4028,8 @@ impl Methods { }, ("last", []) => { if let Some(("cloned", recv2, [], _span2, _)) = method_call(recv) { - iter_overeager_cloned::check(cx, expr, recv, recv2, false, false); + iter_overeager_cloned::check(cx, expr, recv, recv2, + iter_overeager_cloned::Op::LaterCloned , false); } }, ("lock", []) => { @@ -4058,7 +4066,7 @@ impl Methods { ("next", []) => { if let Some((name2, recv2, args2, _, _)) = method_call(recv) { match (name2, args2) { - ("cloned", []) => iter_overeager_cloned::check(cx, expr, recv, recv2, false, false), + ("cloned", []) => iter_overeager_cloned::check(cx, expr, recv, recv2, iter_overeager_cloned::Op::LaterCloned, 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), @@ -4071,7 +4079,7 @@ impl Methods { }, ("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, recv, recv2, false, false), + Some(("cloned", recv2, [], _, _)) => iter_overeager_cloned::check(cx, expr, recv, recv2, iter_overeager_cloned::Op::LaterCloned , 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), @@ -4126,7 +4134,8 @@ impl Methods { iter_skip_zero::check(cx, expr, arg); if let Some(("cloned", recv2, [], _span2, _)) = method_call(recv) { - iter_overeager_cloned::check(cx, expr, recv, recv2, false, false); + iter_overeager_cloned::check(cx, expr, recv, recv2, + iter_overeager_cloned::Op::LaterCloned , false); } } ("sort", []) => { @@ -4152,7 +4161,8 @@ impl Methods { ("step_by", [arg]) => iterator_step_by_zero::check(cx, expr, arg), ("take", [_arg]) => { if let Some(("cloned", recv2, [], _span2, _)) = method_call(recv) { - iter_overeager_cloned::check(cx, expr, recv, recv2, false, false); + iter_overeager_cloned::check(cx, expr, recv, recv2, + iter_overeager_cloned::Op::LaterCloned, false); } }, ("take", []) => needless_option_take::check(cx, expr, recv), diff --git a/tests/ui/iter_overeager_cloned.fixed b/tests/ui/iter_overeager_cloned.fixed index ba26fe33d892..9605b449b409 100644 --- a/tests/ui/iter_overeager_cloned.fixed +++ b/tests/ui/iter_overeager_cloned.fixed @@ -20,8 +20,41 @@ fn main() { .iter() .flatten().cloned(); - // Not implemented yet - let _ = vec.iter().cloned().filter(|x| x.starts_with('2')); + let _ = vec.iter().filter(|&x| x.starts_with('2')).cloned(); + + let _ = vec.iter().find(|&x| x == "2").cloned(); + + { + let f = |x: &String| x.starts_with('2'); + let _ = vec.iter().filter(|&x| f(x)).cloned(); + let _ = vec.iter().find(|&x| f(x)).cloned(); + } + + { + let vec: Vec<(String, String)> = vec![]; + let f = move |x: &(String, String)| x.0.starts_with('2'); + let _ = vec.iter().filter(|&x| f(x)).cloned(); + let _ = vec.iter().find(|&x| f(x)).cloned(); + } + + fn test_move<'a>( + iter: impl Iterator + 'a, + target: String, + ) -> impl Iterator + 'a { + iter.filter(move |&(&a, b)| a == 1 && b == &target).cloned() + } + + { + #[derive(Clone)] + struct S<'a> { + a: &'a u32, + b: String, + } + + fn bar<'a>(iter: impl Iterator> + 'a, target: String) -> impl Iterator> + 'a { + iter.filter(move |&S { a, b }| **a == 1 && b == &target).cloned() + } + } // Not implemented yet let _ = vec.iter().cloned().map(|x| x.len()); @@ -29,9 +62,6 @@ fn main() { // This would fail if changed. let _ = vec.iter().cloned().map(|x| x + "2"); - // Not implemented yet - let _ = vec.iter().cloned().find(|x| x == "2"); - // Not implemented yet let _ = vec.iter().cloned().for_each(|x| assert!(!x.is_empty())); diff --git a/tests/ui/iter_overeager_cloned.rs b/tests/ui/iter_overeager_cloned.rs index 2d3c4ea7c201..8d75f039d44e 100644 --- a/tests/ui/iter_overeager_cloned.rs +++ b/tests/ui/iter_overeager_cloned.rs @@ -21,18 +21,48 @@ fn main() { .cloned() .flatten(); - // Not implemented yet let _ = vec.iter().cloned().filter(|x| x.starts_with('2')); + let _ = vec.iter().cloned().find(|x| x == "2"); + + { + let f = |x: &String| x.starts_with('2'); + let _ = vec.iter().cloned().filter(f); + let _ = vec.iter().cloned().find(f); + } + + { + let vec: Vec<(String, String)> = vec![]; + let f = move |x: &(String, String)| x.0.starts_with('2'); + let _ = vec.iter().cloned().filter(f); + let _ = vec.iter().cloned().find(f); + } + + fn test_move<'a>( + iter: impl Iterator + 'a, + target: String, + ) -> impl Iterator + 'a { + iter.cloned().filter(move |(&a, b)| a == 1 && b == &target) + } + + { + #[derive(Clone)] + struct S<'a> { + a: &'a u32, + b: String, + } + + fn bar<'a>(iter: impl Iterator> + 'a, target: String) -> impl Iterator> + 'a { + iter.cloned().filter(move |S { a, b }| **a == 1 && b == &target) + } + } + // Not implemented yet let _ = vec.iter().cloned().map(|x| x.len()); // This would fail if changed. let _ = vec.iter().cloned().map(|x| x + "2"); - // Not implemented yet - let _ = vec.iter().cloned().find(|x| x == "2"); - // Not implemented yet let _ = vec.iter().cloned().for_each(|x| assert!(!x.is_empty())); diff --git a/tests/ui/iter_overeager_cloned.stderr b/tests/ui/iter_overeager_cloned.stderr index 2be2b7e07a46..bdbb04d7f082 100644 --- a/tests/ui/iter_overeager_cloned.stderr +++ b/tests/ui/iter_overeager_cloned.stderr @@ -66,5 +66,69 @@ LL ~ .iter() LL ~ .flatten().cloned(); | -error: aborting due to 7 previous errors +error: unnecessarily eager cloning of iterator items + --> $DIR/iter_overeager_cloned.rs:24:13 + | +LL | let _ = vec.iter().cloned().filter(|x| x.starts_with('2')); + | ^^^^^^^^^^---------------------------------------- + | | + | help: try: `.filter(|&x| x.starts_with('2')).cloned()` + +error: unnecessarily eager cloning of iterator items + --> $DIR/iter_overeager_cloned.rs:26:13 + | +LL | let _ = vec.iter().cloned().find(|x| x == "2"); + | ^^^^^^^^^^---------------------------- + | | + | help: try: `.find(|&x| x == "2").cloned()` + +error: unnecessarily eager cloning of iterator items + --> $DIR/iter_overeager_cloned.rs:30:17 + | +LL | let _ = vec.iter().cloned().filter(f); + | ^^^^^^^^^^------------------- + | | + | help: try: `.filter(|&x| f(x)).cloned()` + +error: unnecessarily eager cloning of iterator items + --> $DIR/iter_overeager_cloned.rs:31:17 + | +LL | let _ = vec.iter().cloned().find(f); + | ^^^^^^^^^^----------------- + | | + | help: try: `.find(|&x| f(x)).cloned()` + +error: unnecessarily eager cloning of iterator items + --> $DIR/iter_overeager_cloned.rs:37:17 + | +LL | let _ = vec.iter().cloned().filter(f); + | ^^^^^^^^^^------------------- + | | + | help: try: `.filter(|&x| f(x)).cloned()` + +error: unnecessarily eager cloning of iterator items + --> $DIR/iter_overeager_cloned.rs:38:17 + | +LL | let _ = vec.iter().cloned().find(f); + | ^^^^^^^^^^----------------- + | | + | help: try: `.find(|&x| f(x)).cloned()` + +error: unnecessarily eager cloning of iterator items + --> $DIR/iter_overeager_cloned.rs:45:9 + | +LL | iter.cloned().filter(move |(&a, b)| a == 1 && b == &target) + | ^^^^------------------------------------------------------- + | | + | help: try: `.filter(move |&(&a, b)| a == 1 && b == &target).cloned()` + +error: unnecessarily eager cloning of iterator items + --> $DIR/iter_overeager_cloned.rs:56:13 + | +LL | iter.cloned().filter(move |S { a, b }| **a == 1 && b == &target) + | ^^^^------------------------------------------------------------ + | | + | help: try: `.filter(move |&S { a, b }| **a == 1 && b == &target).cloned()` + +error: aborting due to 15 previous errors