Skip to content

Commit 7408f1d

Browse files
borsbors
committed
Auto merge of #11319 - lengyijun:map_foreach, r=blyxyas
[iter_overeager_cloned]: detect .cloned().map() and .cloned().for_each() changelog: [`iter_overeager_cloned`] key idea: for `f` in `.map(f)` and `.for_each(f)`: 1. `f` must be closure 2. don't lint if mutable paramter in clsure `f`: `|mut x| ...` 3. don't lint if parameter is moved 4. maybe incorrect
2 parents b7cad50 + e440065 commit 7408f1d

File tree

5 files changed

+102
-15
lines changed

5 files changed

+102
-15
lines changed

‎clippy_lints/src/methods/iter_overeager_cloned.rs

Lines changed: 74 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,21 +1,29 @@
11
use clippy_utils::diagnostics::span_lint_and_then;
22
use clippy_utils::source::snippet_opt;
33
use clippy_utils::ty::{implements_trait, is_copy};
4+
use rustc_ast::BindingAnnotation;
45
use rustc_errors::Applicability;
5-
use rustc_hir::{Expr, ExprKind};
6+
use rustc_hir::{Body, Expr, ExprKind, HirId, HirIdSet, PatKind};
7+
use rustc_hir_typeck::expr_use_visitor::{Delegate, ExprUseVisitor, PlaceBase, PlaceWithHirId};
68
use rustc_lint::LateContext;
7-
use rustc_middle::ty;
9+
use rustc_middle::mir::{FakeReadCause, Mutability};
10+
use rustc_middle::ty::{self, BorrowKind};
811
use rustc_span::sym;
912

1013
use super::ITER_OVEREAGER_CLONED;
1114
use crate::redundant_clone::REDUNDANT_CLONE;
15+
use crate::rustc_trait_selection::infer::TyCtxtInferExt;
1216

1317
#[derive(Clone, Copy)]
1418
pub(super) enum Op<'a> {
1519
// rm `.cloned()`
1620
// e.g. `count`
1721
RmCloned,
1822

23+
// rm `.cloned()`
24+
// e.g. `map` `for_each`
25+
NeedlessMove(&'a str, &'a Expr<'a>),
26+
1927
// later `.cloned()`
2028
// and add `&` to the parameter of closure parameter
2129
// e.g. `find` `filter`
@@ -51,8 +59,46 @@ pub(super) fn check<'tcx>(
5159
return;
5260
}
5361

62+
if let Op::NeedlessMove(_, expr) = op {
63+
let rustc_hir::ExprKind::Closure(closure) = expr.kind else { return } ;
64+
let body @ Body { params: [p], .. } = cx.tcx.hir().body(closure.body) else { return };
65+
let mut delegate = MoveDelegate {used_move : HirIdSet::default()};
66+
let infcx = cx.tcx.infer_ctxt().build();
67+
68+
ExprUseVisitor::new(
69+
&mut delegate,
70+
&infcx,
71+
closure.body.hir_id.owner.def_id,
72+
cx.param_env,
73+
cx.typeck_results(),
74+
)
75+
.consume_body(body);
76+
77+
let mut to_be_discarded = false;
78+
79+
p.pat.walk(|it| {
80+
if delegate.used_move.contains(&it.hir_id){
81+
to_be_discarded = true;
82+
return false;
83+
}
84+
85+
match it.kind {
86+
PatKind::Binding(BindingAnnotation(_, Mutability::Mut), _, _, _)
87+
| PatKind::Ref(_, Mutability::Mut) => {
88+
to_be_discarded = true;
89+
false
90+
}
91+
_ => { true }
92+
}
93+
});
94+
95+
if to_be_discarded {
96+
return;
97+
}
98+
}
99+
54100
let (lint, msg, trailing_clone) = match op {
55-
Op::RmCloned => (REDUNDANT_CLONE, "unneeded cloning of iterator items", ""),
101+
Op::RmCloned | Op::NeedlessMove(_, _) => (REDUNDANT_CLONE, "unneeded cloning of iterator items", ""),
56102
Op::LaterCloned | Op::FixClosure(_, _) => (ITER_OVEREAGER_CLONED, "unnecessarily eager cloning of iterator items", ".cloned()"),
57103
};
58104

@@ -83,8 +129,33 @@ pub(super) fn check<'tcx>(
83129
diag.span_suggestion(replace_span, "try", snip, Applicability::MachineApplicable);
84130
}
85131
}
132+
Op::NeedlessMove(_, _) => {
133+
let method_span = expr.span.with_lo(cloned_call.span.hi());
134+
if let Some(snip) = snippet_opt(cx, method_span) {
135+
let replace_span = expr.span.with_lo(cloned_recv.span.hi());
136+
diag.span_suggestion(replace_span, "try", snip, Applicability::MaybeIncorrect);
137+
}
138+
}
86139
}
87140
}
88141
);
89142
}
90143
}
144+
145+
struct MoveDelegate {
146+
used_move: HirIdSet,
147+
}
148+
149+
impl<'tcx> Delegate<'tcx> for MoveDelegate {
150+
fn consume(&mut self, place_with_id: &PlaceWithHirId<'tcx>, _: HirId) {
151+
if let PlaceBase::Local(l) = place_with_id.place.base {
152+
self.used_move.insert(l);
153+
}
154+
}
155+
156+
fn borrow(&mut self, _: &PlaceWithHirId<'tcx>, _: HirId, _: BorrowKind) {}
157+
158+
fn mutate(&mut self, _: &PlaceWithHirId<'tcx>, _: HirId) {}
159+
160+
fn fake_read(&mut self, _: &PlaceWithHirId<'tcx>, _: FakeReadCause, _: HirId) {}
161+
}

‎clippy_lints/src/methods/mod.rs

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4001,9 +4001,11 @@ impl Methods {
40014001
manual_try_fold::check(cx, expr, init, acc, call_span, &self.msrv);
40024002
unnecessary_fold::check(cx, expr, init, acc, span);
40034003
},
4004-
("for_each", [_]) => {
4005-
if let Some(("inspect", _, [_], span2, _)) = method_call(recv) {
4006-
inspect_for_each::check(cx, expr, span2);
4004+
("for_each", [arg]) => {
4005+
match method_call(recv) {
4006+
Some(("inspect", _, [_], span2, _)) => inspect_for_each::check(cx, expr, span2),
4007+
Some(("cloned", recv2, [], _, _)) => iter_overeager_cloned::check(cx, expr, recv, recv2, iter_overeager_cloned::Op::NeedlessMove(name, arg), false),
4008+
_ => {}
40074009
}
40084010
},
40094011
("get", [arg]) => {
@@ -4038,8 +4040,10 @@ impl Methods {
40384040
(name @ ("map" | "map_err"), [m_arg]) => {
40394041
if name == "map" {
40404042
map_clone::check(cx, expr, recv, m_arg, &self.msrv);
4041-
if let Some((map_name @ ("iter" | "into_iter"), recv2, _, _, _)) = method_call(recv) {
4042-
iter_kv_map::check(cx, map_name, expr, recv2, m_arg);
4043+
match method_call(recv) {
4044+
Some((map_name @ ("iter" | "into_iter"), recv2, _, _, _)) => iter_kv_map::check(cx, map_name, expr, recv2, m_arg),
4045+
Some(("cloned", recv2, [], _, _)) => iter_overeager_cloned::check(cx, expr, recv, recv2, iter_overeager_cloned::Op::NeedlessMove(name, m_arg), false),
4046+
_ => {}
40434047
}
40444048
} else {
40454049
map_err_ignore::check(cx, expr, m_arg);

‎tests/ui/iter_overeager_cloned.fixed

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -56,14 +56,12 @@ fn main() {
5656
}
5757
}
5858

59-
// Not implemented yet
60-
let _ = vec.iter().cloned().map(|x| x.len());
59+
let _ = vec.iter().map(|x| x.len());
6160

6261
// This would fail if changed.
6362
let _ = vec.iter().cloned().map(|x| x + "2");
6463

65-
// Not implemented yet
66-
let _ = vec.iter().cloned().for_each(|x| assert!(!x.is_empty()));
64+
let _ = vec.iter().for_each(|x| assert!(!x.is_empty()));
6765

6866
// Not implemented yet
6967
let _ = vec.iter().cloned().all(|x| x.len() == 1);

‎tests/ui/iter_overeager_cloned.rs

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -57,13 +57,11 @@ fn main() {
5757
}
5858
}
5959

60-
// Not implemented yet
6160
let _ = vec.iter().cloned().map(|x| x.len());
6261

6362
// This would fail if changed.
6463
let _ = vec.iter().cloned().map(|x| x + "2");
6564

66-
// Not implemented yet
6765
let _ = vec.iter().cloned().for_each(|x| assert!(!x.is_empty()));
6866

6967
// Not implemented yet

‎tests/ui/iter_overeager_cloned.stderr

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -130,5 +130,21 @@ LL | iter.cloned().filter(move |S { a, b }| **a == 1 && b == &target
130130
| |
131131
| help: try: `.filter(move |&S { a, b }| **a == 1 && b == &target).cloned()`
132132

133-
error: aborting due to 15 previous errors
133+
error: unneeded cloning of iterator items
134+
--> $DIR/iter_overeager_cloned.rs:60:13
135+
|
136+
LL | let _ = vec.iter().cloned().map(|x| x.len());
137+
| ^^^^^^^^^^--------------------------
138+
| |
139+
| help: try: `.map(|x| x.len())`
140+
141+
error: unneeded cloning of iterator items
142+
--> $DIR/iter_overeager_cloned.rs:65:13
143+
|
144+
LL | let _ = vec.iter().cloned().for_each(|x| assert!(!x.is_empty()));
145+
| ^^^^^^^^^^----------------------------------------------
146+
| |
147+
| help: try: `.for_each(|x| assert!(!x.is_empty()))`
148+
149+
error: aborting due to 17 previous errors
134150

0 commit comments

Comments
 (0)