From e440065a0f930b527925b61c3a6dd5f4207dd7b6 Mon Sep 17 00:00:00 2001
From: lengyijun <sjtu5140809011@gmail.com>
Date: Sat, 5 Aug 2023 10:20:16 +0800
Subject: [PATCH] [`iter_overeager_cloned`]: detect .cloned().map() and
.cloned().for_each()
key idea:
for `f` in `.map(f)` and `.for_each(f)`:
1. `f` must be a closure with one parameter
2. don't lint if mutable paramter in clsure `f`: `|mut x| ...`
3. don't lint if parameter is moved
---
.../src/methods/iter_overeager_cloned.rs | 77 ++++++++++++++++++-
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, 102 insertions(+), 15 deletions(-)
diff --git a/clippy_lints/src/methods/iter_overeager_cloned.rs b/clippy_lints/src/methods/iter_overeager_cloned.rs
index 4b1ca0d9b9c..ee405a3e30c 100644
--- a/clippy_lints/src/methods/iter_overeager_cloned.rs
+++ b/clippy_lints/src/methods/iter_overeager_cloned.rs
@@ -1,14 +1,18 @@
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, ExprKind};
+use rustc_hir::{Body, Expr, ExprKind, HirId, HirIdSet, PatKind};
+use rustc_hir_typeck::expr_use_visitor::{Delegate, ExprUseVisitor, PlaceBase, PlaceWithHirId};
use rustc_lint::LateContext;
-use rustc_middle::ty;
+use rustc_middle::mir::{FakeReadCause, Mutability};
+use rustc_middle::ty::{self, BorrowKind};
use rustc_span::sym;
use super::ITER_OVEREAGER_CLONED;
use crate::redundant_clone::REDUNDANT_CLONE;
+use crate::rustc_trait_selection::infer::TyCtxtInferExt;
#[derive(Clone, Copy)]
pub(super) enum Op<'a> {
@@ -16,6 +20,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 +59,46 @@ pub(super) fn check<'tcx>(
return;
}
+ if let Op::NeedlessMove(_, expr) = op {
+ let rustc_hir::ExprKind::Closure(closure) = expr.kind else { return } ;
+ let body @ Body { params: [p], .. } = cx.tcx.hir().body(closure.body) else { return };
+ let mut delegate = MoveDelegate {used_move : HirIdSet::default()};
+ let infcx = cx.tcx.infer_ctxt().build();
+
+ ExprUseVisitor::new(
+ &mut delegate,
+ &infcx,
+ closure.body.hir_id.owner.def_id,
+ cx.param_env,
+ cx.typeck_results(),
+ )
+ .consume_body(body);
+
+ let mut to_be_discarded = false;
+
+ p.pat.walk(|it| {
+ if delegate.used_move.contains(&it.hir_id){
+ to_be_discarded = true;
+ return false;
+ }
+
+ match it.kind {
+ PatKind::Binding(BindingAnnotation(_, Mutability::Mut), _, _, _)
+ | PatKind::Ref(_, Mutability::Mut) => {
+ to_be_discarded = true;
+ false
+ }
+ _ => { true }
+ }
+ });
+
+ if to_be_discarded {
+ 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 +129,33 @@ 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());
+ diag.span_suggestion(replace_span, "try", snip, Applicability::MaybeIncorrect);
+ }
+ }
}
}
);
}
}
+
+struct MoveDelegate {
+ used_move: HirIdSet,
+}
+
+impl<'tcx> Delegate<'tcx> for MoveDelegate {
+ fn consume(&mut self, place_with_id: &PlaceWithHirId<'tcx>, _: HirId) {
+ if let PlaceBase::Local(l) = place_with_id.place.base {
+ self.used_move.insert(l);
+ }
+ }
+
+ fn borrow(&mut self, _: &PlaceWithHirId<'tcx>, _: HirId, _: BorrowKind) {}
+
+ fn mutate(&mut self, _: &PlaceWithHirId<'tcx>, _: HirId) {}
+
+ fn fake_read(&mut self, _: &PlaceWithHirId<'tcx>, _: FakeReadCause, _: HirId) {}
+}
diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs
index 3a8af0248d8..5075137caa0 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 9605b449b40..9dd046fec1f 100644
--- a/tests/ui/iter_overeager_cloned.fixed
+++ b/tests/ui/iter_overeager_cloned.fixed
@@ -56,14 +56,12 @@ fn main() {
}
}
- // 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 8d75f039d44..3cab3669554 100644
--- a/tests/ui/iter_overeager_cloned.rs
+++ b/tests/ui/iter_overeager_cloned.rs
@@ -57,13 +57,11 @@ fn main() {
}
}
- // 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 bdbb04d7f08..4417a40a63b 100644
--- a/tests/ui/iter_overeager_cloned.stderr
+++ b/tests/ui/iter_overeager_cloned.stderr
@@ -130,5 +130,21 @@ 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
+error: unneeded cloning of iterator items
+ --> $DIR/iter_overeager_cloned.rs:60: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:65: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 17 previous errors