From dd9d4fad0bd449626536b7c8161bb3a3a1f722c6 Mon Sep 17 00:00:00 2001 From: lengyijun Date: Sat, 5 Aug 2023 10:20:16 +0800 Subject: [PATCH] [Draft] [iter_overeager_cloned]: detect .cloned().map() and .cloned().for_each() key idea: for `f` in `map` and `for_each`, if `f` never move the parameter, the `cloned` can be removed --- .../src/methods/iter_overeager_cloned.rs | 71 ++++++++++++++++++- clippy_lints/src/methods/mod.rs | 14 ++-- tests/ui/iter_overeager_cloned.fixed | 6 +- tests/ui/iter_overeager_cloned.rs | 2 - tests/ui/iter_overeager_cloned.stderr | 18 ++++- 5 files changed, 97 insertions(+), 14 deletions(-) diff --git a/clippy_lints/src/methods/iter_overeager_cloned.rs b/clippy_lints/src/methods/iter_overeager_cloned.rs index 018db3dabfef..5d47779c95c9 100644 --- a/clippy_lints/src/methods/iter_overeager_cloned.rs +++ b/clippy_lints/src/methods/iter_overeager_cloned.rs @@ -1,9 +1,12 @@ use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::source::snippet_opt; use clippy_utils::ty::{implements_trait, is_copy}; +use rustc_ast::BindingAnnotation; use rustc_errors::Applicability; -use rustc_hir::Expr; +use rustc_hir::{Body, Expr}; use rustc_lint::LateContext; +use rustc_middle::mir::visit::Visitor; +use rustc_middle::mir::{self, Local, Location, Mutability, Operand, VarDebugInfoContents}; use rustc_middle::ty; use rustc_span::sym; @@ -16,6 +19,10 @@ pub(super) enum Op<'a> { // e.g. `count` RmCloned, + // rm `.cloned()` + // e.g. `map` `for_each` + NeedlessMove(&'a str, &'a Expr<'a>), + // later `.cloned()` // and add `&` to the parameter of closure parameter // e.g. `find` `filter` @@ -51,8 +58,32 @@ pub(super) fn check<'tcx>( return; } + if let Op::NeedlessMove(_, expr) = op { + if let rustc_hir::ExprKind::Closure(closure) = expr.kind + && let Body {params: [p], value: _, generator_kind: _ } = cx.tcx.hir().body(closure.body) { + match p.pat.kind { + rustc_hir::PatKind::Binding(BindingAnnotation(_, Mutability::Not), _, _, _) => { + let body = cx.tcx.optimized_mir(closure.def_id); + if let VarDebugInfoContents::Place(p) = &body.var_debug_info[0].value { + let mut find_move = FindMove::new(p.local); + find_move.visit_body(body); + if find_move.found { + return; + } + } else { + return; + } + } + rustc_hir::PatKind::Wild => {} + _ => return, + } + } else { + return; + } + } + let (lint, msg, trailing_clone) = match op { - Op::RmCloned => (REDUNDANT_CLONE, "unneeded cloning of iterator items", ""), + Op::RmCloned | Op::NeedlessMove(_, _) => (REDUNDANT_CLONE, "unneeded cloning of iterator items", ""), Op::LaterCloned | Op::FixClosure(_, _) => (ITER_OVEREAGER_CLONED, "unnecessarily eager cloning of iterator items", ".cloned()"), }; @@ -83,8 +114,44 @@ pub(super) fn check<'tcx>( diag.span_suggestion(replace_span, "try", snip, Applicability::MachineApplicable); } } + Op::NeedlessMove(_, _) => { + let method_span = expr.span.with_lo(cloned_call.span.hi()); + if let Some(snip) = snippet_opt(cx, method_span) { + let replace_span = expr.span.with_lo(cloned_recv.span.hi()); + // suggestion may not accurate: programmer need some manual fix + diag.span_suggestion(replace_span, "try", snip, Applicability::MachineApplicable); + } + } } } ); } } + +struct FindMove { + target: Local, + found: bool, +} + +impl FindMove { + fn new(target: Local) -> Self { + FindMove { target, found: false } + } +} + +impl<'tcx> mir::visit::Visitor<'tcx> for FindMove { + fn visit_operand(&mut self, operand: &Operand<'tcx>, _: Location) { + if self.found { + return; + } + + match &operand { + Operand::Move(p) | Operand::Copy(p) => { + if p.local == self.target { + self.found = true; + } + }, + Operand::Constant(_) => {}, + } + } +} diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 729eda37af62..805b69fd70b3 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -4001,9 +4001,11 @@ impl Methods { manual_try_fold::check(cx, expr, init, acc, call_span, &self.msrv); unnecessary_fold::check(cx, expr, init, acc, span); }, - ("for_each", [_]) => { - if let Some(("inspect", _, [_], span2, _)) = method_call(recv) { - inspect_for_each::check(cx, expr, span2); + ("for_each", [arg]) => { + match method_call(recv) { + Some(("inspect", _, [_], span2, _)) => inspect_for_each::check(cx, expr, span2), + Some(("cloned", recv2, [], _, _)) => iter_overeager_cloned::check(cx, expr, recv, recv2, iter_overeager_cloned::Op::NeedlessMove(name, arg), false), + _ => {} } }, ("get", [arg]) => { @@ -4038,8 +4040,10 @@ impl Methods { (name @ ("map" | "map_err"), [m_arg]) => { if name == "map" { map_clone::check(cx, expr, recv, m_arg, &self.msrv); - if let Some((map_name @ ("iter" | "into_iter"), recv2, _, _, _)) = method_call(recv) { - iter_kv_map::check(cx, map_name, expr, recv2, m_arg); + match method_call(recv) { + Some((map_name @ ("iter" | "into_iter"), recv2, _, _, _)) => iter_kv_map::check(cx, map_name, expr, recv2, m_arg), + Some(("cloned", recv2, [], _, _)) => iter_overeager_cloned::check(cx, expr, recv, recv2, iter_overeager_cloned::Op::NeedlessMove(name, m_arg), false), + _ => {} } } else { map_err_ignore::check(cx, expr, m_arg); diff --git a/tests/ui/iter_overeager_cloned.fixed b/tests/ui/iter_overeager_cloned.fixed index 7cc45d4d0211..7554d4440527 100644 --- a/tests/ui/iter_overeager_cloned.fixed +++ b/tests/ui/iter_overeager_cloned.fixed @@ -43,14 +43,12 @@ fn main() { let _ = vec.iter().filter(|&(x, y)| x == "2").cloned(); } - // Not implemented yet - let _ = vec.iter().cloned().map(|x| x.len()); + let _ = vec.iter().map(|x| x.len()); // This would fail if changed. let _ = vec.iter().cloned().map(|x| x + "2"); - // Not implemented yet - let _ = vec.iter().cloned().for_each(|x| assert!(!x.is_empty())); + let _ = vec.iter().for_each(|x| assert!(!x.is_empty())); // Not implemented yet let _ = vec.iter().cloned().all(|x| x.len() == 1); diff --git a/tests/ui/iter_overeager_cloned.rs b/tests/ui/iter_overeager_cloned.rs index 7c056bd85bdd..af1142442522 100644 --- a/tests/ui/iter_overeager_cloned.rs +++ b/tests/ui/iter_overeager_cloned.rs @@ -44,13 +44,11 @@ fn main() { 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().for_each(|x| assert!(!x.is_empty())); // Not implemented yet diff --git a/tests/ui/iter_overeager_cloned.stderr b/tests/ui/iter_overeager_cloned.stderr index bab1fa49d54b..ede9180f1c09 100644 --- a/tests/ui/iter_overeager_cloned.stderr +++ b/tests/ui/iter_overeager_cloned.stderr @@ -122,5 +122,21 @@ 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 +error: unneeded cloning of iterator items + --> $DIR/iter_overeager_cloned.rs:47:13 + | +LL | let _ = vec.iter().cloned().map(|x| x.len()); + | ^^^^^^^^^^-------------------------- + | | + | help: try: `.map(|x| x.len())` + +error: unneeded cloning of iterator items + --> $DIR/iter_overeager_cloned.rs:52:13 + | +LL | let _ = vec.iter().cloned().for_each(|x| assert!(!x.is_empty())); + | ^^^^^^^^^^---------------------------------------------- + | | + | help: try: `.for_each(|x| assert!(!x.is_empty()))` + +error: aborting due to 16 previous errors