From c1eff7f3a2896fc69c60b03be426be62eff4e5bc Mon Sep 17 00:00:00 2001 From: lengyijun Date: Thu, 3 Aug 2023 11:37:23 +0800 Subject: [PATCH] [iter_overeager_cloned]: detect .cloned().filter() and .cloned().find() Key idea: ``` // before iter.cloned().filter(|x| unimplemented!() ) // after iter.filter(|&x| unimplemented!() ).cloned() // before iter.cloned().filter( foo ) // after iter.filter(|&x| foo(x) ).cloned() ``` --- .../src/methods/iter_overeager_cloned.rs | 51 ++++++++++++---- clippy_lints/src/methods/mod.rs | 24 +++++--- tests/ui/iter_overeager_cloned.fixed | 26 +++++++-- tests/ui/iter_overeager_cloned.rs | 24 ++++++-- tests/ui/iter_overeager_cloned.stderr | 58 ++++++++++++++++++- 5 files changed, 156 insertions(+), 27 deletions(-) diff --git a/clippy_lints/src/methods/iter_overeager_cloned.rs b/clippy_lints/src/methods/iter_overeager_cloned.rs index 9f7ec19aa598..018db3dabfef 100644 --- a/clippy_lints/src/methods/iter_overeager_cloned.rs +++ b/clippy_lints/src/methods/iter_overeager_cloned.rs @@ -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) { + #[allow(clippy::single_match_else)] + let new_closure = match predicate_expr.kind { + rustc_hir::ExprKind::Closure(_) => predicate.replacen('|', "|&", 1), + _ => 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 2874513c049b..7cc45d4d0211 100644 --- a/tests/ui/iter_overeager_cloned.fixed +++ b/tests/ui/iter_overeager_cloned.fixed @@ -21,8 +21,27 @@ 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(); + } + + { + let vec: Vec<(String, String)> = vec![]; + let _ = vec.iter().filter(|&(x, y)| x == "2").cloned(); + } // Not implemented yet let _ = vec.iter().cloned().map(|x| x.len()); @@ -30,9 +49,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 26f39734a512..7c056bd85bdd 100644 --- a/tests/ui/iter_overeager_cloned.rs +++ b/tests/ui/iter_overeager_cloned.rs @@ -22,18 +22,34 @@ 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); + } + + { + let vec: Vec<(String, String)> = vec![]; + let _ = vec.iter().cloned().filter(|(x, y)| x == "2"); + } + // 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 eaac48be8809..bab1fa49d54b 100644 --- a/tests/ui/iter_overeager_cloned.stderr +++ b/tests/ui/iter_overeager_cloned.stderr @@ -66,5 +66,61 @@ LL ~ .iter() LL ~ .flatten().cloned(); | -error: aborting due to 7 previous errors +error: unnecessarily eager cloning of iterator items + --> $DIR/iter_overeager_cloned.rs:25: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:27: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:31: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:32: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:38: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:39: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:44:17 + | +LL | let _ = vec.iter().cloned().filter(|(x, y)| x == "2"); + | ^^^^^^^^^^----------------------------------- + | | + | help: try: `.filter(|&(x, y)| x == "2").cloned()` + +error: aborting due to 14 previous errors