Skip to content

Commit b5bfd11

Browse files
borsbors
committed
Auto merge of #11289 - lengyijun:filter_find, r=blyxyas
 [iter_overeager_cloned]: detect `.cloned().filter()` and `.cloned().find()` changelog: [`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 // notice `iter` must be `Iterator<Item= &T>` (callee of `cloned()`) // so the parameter in the closure of `filter` must be `&&T` // so the deref is safe iter.filter(|&x| foo(x) ).cloned() ```
2 parents 344ae11 + fc06189 commit b5bfd11

File tree

5 files changed

+193
-28
lines changed

5 files changed

+193
-28
lines changed

‎clippy_lints/src/methods/iter_overeager_cloned.rs‎

Lines changed: 42 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2,20 +2,36 @@ use clippy_utils::diagnostics::span_lint_and_then;
22
use clippy_utils::source::snippet_opt;
33
use clippy_utils::ty::{implements_trait, is_copy};
44
use rustc_errors::Applicability;
5-
use rustc_hir::Expr;
5+
use rustc_hir::{Expr, ExprKind};
66
use rustc_lint::LateContext;
77
use rustc_middle::ty;
88
use rustc_span::sym;
99

1010
use super::ITER_OVEREAGER_CLONED;
1111
use crate::redundant_clone::REDUNDANT_CLONE;
1212

13+
#[derive(Clone, Copy)]
14+
pub(super) enum Op<'a> {
15+
// rm `.cloned()`
16+
// e.g. `count`
17+
RmCloned,
18+
19+
// later `.cloned()`
20+
// and add `&` to the parameter of closure parameter
21+
// e.g. `find` `filter`
22+
FixClosure(&'a str, &'a Expr<'a>),
23+
24+
// later `.cloned()`
25+
// e.g. `skip` `take`
26+
LaterCloned,
27+
}
28+
1329
pub(super) fn check<'tcx>(
1430
cx: &LateContext<'tcx>,
1531
expr: &'tcx Expr<'_>,
1632
cloned_call: &'tcx Expr<'_>,
1733
cloned_recv: &'tcx Expr<'_>,
18-
is_count: bool,
34+
op: Op<'tcx>,
1935
needs_into_iter: bool,
2036
) {
2137
let typeck = cx.typeck_results();
@@ -35,10 +51,9 @@ pub(super) fn check<'tcx>(
3551
return;
3652
}
3753

38-
let (lint, msg, trailing_clone) = if is_count {
39-
(REDUNDANT_CLONE, "unneeded cloning of iterator items", "")
40-
} else {
41-
(ITER_OVEREAGER_CLONED, "unnecessarily eager cloning of iterator items", ".cloned()")
54+
let (lint, msg, trailing_clone) = match op {
55+
Op::RmCloned => (REDUNDANT_CLONE, "unneeded cloning of iterator items", ""),
56+
Op::LaterCloned | Op::FixClosure(_, _) => (ITER_OVEREAGER_CLONED, "unnecessarily eager cloning of iterator items", ".cloned()"),
4257
};
4358

4459
span_lint_and_then(
@@ -47,11 +62,27 @@ pub(super) fn check<'tcx>(
4762
expr.span,
4863
msg,
4964
|diag| {
50-
let method_span = expr.span.with_lo(cloned_call.span.hi());
51-
if let Some(mut snip) = snippet_opt(cx, method_span) {
52-
snip.push_str(trailing_clone);
53-
let replace_span = expr.span.with_lo(cloned_recv.span.hi());
54-
diag.span_suggestion(replace_span, "try", snip, Applicability::MachineApplicable);
65+
match op {
66+
Op::RmCloned | Op::LaterCloned => {
67+
let method_span = expr.span.with_lo(cloned_call.span.hi());
68+
if let Some(mut snip) = snippet_opt(cx, method_span) {
69+
snip.push_str(trailing_clone);
70+
let replace_span = expr.span.with_lo(cloned_recv.span.hi());
71+
diag.span_suggestion(replace_span, "try", snip, Applicability::MachineApplicable);
72+
}
73+
}
74+
Op::FixClosure(name, predicate_expr) => {
75+
if let Some(predicate) = snippet_opt(cx, predicate_expr.span) {
76+
let new_closure = if let ExprKind::Closure(_) = predicate_expr.kind {
77+
predicate.replacen('|', "|&", 1)
78+
} else {
79+
format!("|&x| {predicate}(x)")
80+
};
81+
let snip = format!(".{name}({new_closure}).cloned()" );
82+
let replace_span = expr.span.with_lo(cloned_recv.span.hi());
83+
diag.span_suggestion(replace_span, "try", snip, Applicability::MachineApplicable);
84+
}
85+
}
5586
}
5687
}
5788
);

‎clippy_lints/src/methods/mod.rs‎

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3919,7 +3919,7 @@ impl Methods {
39193919
}
39203920
},
39213921
("count", []) if is_trait_method(cx, expr, sym::Iterator) => match method_call(recv) {
3922-
Some(("cloned", recv2, [], _, _)) => iter_overeager_cloned::check(cx, expr, recv, recv2, true, false),
3922+
Some(("cloned", recv2, [], _, _)) => iter_overeager_cloned::check(cx, expr, recv, recv2, iter_overeager_cloned::Op::RmCloned , false),
39233923
Some((name2 @ ("into_iter" | "iter" | "iter_mut"), recv2, [], _, _)) => {
39243924
iter_count::check(cx, expr, recv2, name2);
39253925
},
@@ -3973,6 +3973,13 @@ impl Methods {
39733973
string_extend_chars::check(cx, expr, recv, arg);
39743974
extend_with_drain::check(cx, expr, recv, arg);
39753975
},
3976+
(name @ ( "filter" | "find" ) , [arg]) => {
3977+
if let Some(("cloned", recv2, [], _span2, _)) = method_call(recv) {
3978+
// if `arg` has side-effect, the semantic will change
3979+
iter_overeager_cloned::check(cx, expr, recv, recv2,
3980+
iter_overeager_cloned::Op::FixClosure(name, arg), false);
3981+
}
3982+
}
39763983
("filter_map", [arg]) => {
39773984
unnecessary_filter_map::check(cx, expr, arg, name);
39783985
filter_map_bool_then::check(cx, expr, arg, call_span);
@@ -3987,7 +3994,7 @@ impl Methods {
39873994
},
39883995
("flatten", []) => match method_call(recv) {
39893996
Some(("map", recv, [map_arg], map_span, _)) => map_flatten::check(cx, expr, recv, map_arg, map_span),
3990-
Some(("cloned", recv2, [], _, _)) => iter_overeager_cloned::check(cx, expr, recv, recv2, false, true),
3997+
Some(("cloned", recv2, [], _, _)) => iter_overeager_cloned::check(cx, expr, recv, recv2, iter_overeager_cloned::Op::LaterCloned , true),
39913998
_ => {},
39923999
},
39934000
("fold", [init, acc]) => {
@@ -4021,7 +4028,8 @@ impl Methods {
40214028
},
40224029
("last", []) => {
40234030
if let Some(("cloned", recv2, [], _span2, _)) = method_call(recv) {
4024-
iter_overeager_cloned::check(cx, expr, recv, recv2, false, false);
4031+
iter_overeager_cloned::check(cx, expr, recv, recv2,
4032+
iter_overeager_cloned::Op::LaterCloned , false);
40254033
}
40264034
},
40274035
("lock", []) => {
@@ -4058,7 +4066,7 @@ impl Methods {
40584066
("next", []) => {
40594067
if let Some((name2, recv2, args2, _, _)) = method_call(recv) {
40604068
match (name2, args2) {
4061-
("cloned", []) => iter_overeager_cloned::check(cx, expr, recv, recv2, false, false),
4069+
("cloned", []) => iter_overeager_cloned::check(cx, expr, recv, recv2, iter_overeager_cloned::Op::LaterCloned, false),
40624070
("filter", [arg]) => filter_next::check(cx, expr, recv2, arg),
40634071
("filter_map", [arg]) => filter_map_next::check(cx, expr, recv2, arg, &self.msrv),
40644072
("iter", []) => iter_next_slice::check(cx, expr, recv2),
@@ -4071,7 +4079,7 @@ impl Methods {
40714079
},
40724080
("nth", [n_arg]) => match method_call(recv) {
40734081
Some(("bytes", recv2, [], _, _)) => bytes_nth::check(cx, expr, recv2, n_arg),
4074-
Some(("cloned", recv2, [], _, _)) => iter_overeager_cloned::check(cx, expr, recv, recv2, false, false),
4082+
Some(("cloned", recv2, [], _, _)) => iter_overeager_cloned::check(cx, expr, recv, recv2, iter_overeager_cloned::Op::LaterCloned , false),
40754083
Some(("iter", recv2, [], _, _)) => iter_nth::check(cx, expr, recv2, recv, n_arg, false),
40764084
Some(("iter_mut", recv2, [], _, _)) => iter_nth::check(cx, expr, recv2, recv, n_arg, true),
40774085
_ => iter_nth_zero::check(cx, expr, recv, n_arg),
@@ -4126,7 +4134,8 @@ impl Methods {
41264134
iter_skip_zero::check(cx, expr, arg);
41274135

41284136
if let Some(("cloned", recv2, [], _span2, _)) = method_call(recv) {
4129-
iter_overeager_cloned::check(cx, expr, recv, recv2, false, false);
4137+
iter_overeager_cloned::check(cx, expr, recv, recv2,
4138+
iter_overeager_cloned::Op::LaterCloned , false);
41304139
}
41314140
}
41324141
("sort", []) => {
@@ -4152,7 +4161,8 @@ impl Methods {
41524161
("step_by", [arg]) => iterator_step_by_zero::check(cx, expr, arg),
41534162
("take", [_arg]) => {
41544163
if let Some(("cloned", recv2, [], _span2, _)) = method_call(recv) {
4155-
iter_overeager_cloned::check(cx, expr, recv, recv2, false, false);
4164+
iter_overeager_cloned::check(cx, expr, recv, recv2,
4165+
iter_overeager_cloned::Op::LaterCloned, false);
41564166
}
41574167
},
41584168
("take", []) => needless_option_take::check(cx, expr, recv),

‎tests/ui/iter_overeager_cloned.fixed‎

Lines changed: 35 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -20,18 +20,48 @@ fn main() {
2020
.iter()
2121
.flatten().cloned();
2222

23-
// Not implemented yet
24-
let _ = vec.iter().cloned().filter(|x| x.starts_with('2'));
23+
let _ = vec.iter().filter(|&x| x.starts_with('2')).cloned();
24+
25+
let _ = vec.iter().find(|&x| x == "2").cloned();
26+
27+
{
28+
let f = |x: &String| x.starts_with('2');
29+
let _ = vec.iter().filter(|&x| f(x)).cloned();
30+
let _ = vec.iter().find(|&x| f(x)).cloned();
31+
}
32+
33+
{
34+
let vec: Vec<(String, String)> = vec![];
35+
let f = move |x: &(String, String)| x.0.starts_with('2');
36+
let _ = vec.iter().filter(|&x| f(x)).cloned();
37+
let _ = vec.iter().find(|&x| f(x)).cloned();
38+
}
39+
40+
fn test_move<'a>(
41+
iter: impl Iterator<Item = &'a (&'a u32, String)> + 'a,
42+
target: String,
43+
) -> impl Iterator<Item = (&'a u32, String)> + 'a {
44+
iter.filter(move |&(&a, b)| a == 1 && b == &target).cloned()
45+
}
46+
47+
{
48+
#[derive(Clone)]
49+
struct S<'a> {
50+
a: &'a u32,
51+
b: String,
52+
}
53+
54+
fn bar<'a>(iter: impl Iterator<Item = &'a S<'a>> + 'a, target: String) -> impl Iterator<Item = S<'a>> + 'a {
55+
iter.filter(move |&S { a, b }| **a == 1 && b == &target).cloned()
56+
}
57+
}
2558

2659
// Not implemented yet
2760
let _ = vec.iter().cloned().map(|x| x.len());
2861

2962
// This would fail if changed.
3063
let _ = vec.iter().cloned().map(|x| x + "2");
3164

32-
// Not implemented yet
33-
let _ = vec.iter().cloned().find(|x| x == "2");
34-
3565
// Not implemented yet
3666
let _ = vec.iter().cloned().for_each(|x| assert!(!x.is_empty()));
3767

‎tests/ui/iter_overeager_cloned.rs‎

Lines changed: 34 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -21,18 +21,48 @@ fn main() {
2121
.cloned()
2222
.flatten();
2323

24-
// Not implemented yet
2524
let _ = vec.iter().cloned().filter(|x| x.starts_with('2'));
2625

26+
let _ = vec.iter().cloned().find(|x| x == "2");
27+
28+
{
29+
let f = |x: &String| x.starts_with('2');
30+
let _ = vec.iter().cloned().filter(f);
31+
let _ = vec.iter().cloned().find(f);
32+
}
33+
34+
{
35+
let vec: Vec<(String, String)> = vec![];
36+
let f = move |x: &(String, String)| x.0.starts_with('2');
37+
let _ = vec.iter().cloned().filter(f);
38+
let _ = vec.iter().cloned().find(f);
39+
}
40+
41+
fn test_move<'a>(
42+
iter: impl Iterator<Item = &'a (&'a u32, String)> + 'a,
43+
target: String,
44+
) -> impl Iterator<Item = (&'a u32, String)> + 'a {
45+
iter.cloned().filter(move |(&a, b)| a == 1 && b == &target)
46+
}
47+
48+
{
49+
#[derive(Clone)]
50+
struct S<'a> {
51+
a: &'a u32,
52+
b: String,
53+
}
54+
55+
fn bar<'a>(iter: impl Iterator<Item = &'a S<'a>> + 'a, target: String) -> impl Iterator<Item = S<'a>> + 'a {
56+
iter.cloned().filter(move |S { a, b }| **a == 1 && b == &target)
57+
}
58+
}
59+
2760
// Not implemented yet
2861
let _ = vec.iter().cloned().map(|x| x.len());
2962

3063
// This would fail if changed.
3164
let _ = vec.iter().cloned().map(|x| x + "2");
3265

33-
// Not implemented yet
34-
let _ = vec.iter().cloned().find(|x| x == "2");
35-
3666
// Not implemented yet
3767
let _ = vec.iter().cloned().for_each(|x| assert!(!x.is_empty()));
3868

‎tests/ui/iter_overeager_cloned.stderr‎

Lines changed: 65 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,5 +66,69 @@ LL ~ .iter()
6666
LL ~ .flatten().cloned();
6767
|
6868

69-
error: aborting due to 7 previous errors
69+
error: unnecessarily eager cloning of iterator items
70+
--> $DIR/iter_overeager_cloned.rs:24:13
71+
|
72+
LL | let _ = vec.iter().cloned().filter(|x| x.starts_with('2'));
73+
| ^^^^^^^^^^----------------------------------------
74+
| |
75+
| help: try: `.filter(|&x| x.starts_with('2')).cloned()`
76+
77+
error: unnecessarily eager cloning of iterator items
78+
--> $DIR/iter_overeager_cloned.rs:26:13
79+
|
80+
LL | let _ = vec.iter().cloned().find(|x| x == "2");
81+
| ^^^^^^^^^^----------------------------
82+
| |
83+
| help: try: `.find(|&x| x == "2").cloned()`
84+
85+
error: unnecessarily eager cloning of iterator items
86+
--> $DIR/iter_overeager_cloned.rs:30:17
87+
|
88+
LL | let _ = vec.iter().cloned().filter(f);
89+
| ^^^^^^^^^^-------------------
90+
| |
91+
| help: try: `.filter(|&x| f(x)).cloned()`
92+
93+
error: unnecessarily eager cloning of iterator items
94+
--> $DIR/iter_overeager_cloned.rs:31:17
95+
|
96+
LL | let _ = vec.iter().cloned().find(f);
97+
| ^^^^^^^^^^-----------------
98+
| |
99+
| help: try: `.find(|&x| f(x)).cloned()`
100+
101+
error: unnecessarily eager cloning of iterator items
102+
--> $DIR/iter_overeager_cloned.rs:37:17
103+
|
104+
LL | let _ = vec.iter().cloned().filter(f);
105+
| ^^^^^^^^^^-------------------
106+
| |
107+
| help: try: `.filter(|&x| f(x)).cloned()`
108+
109+
error: unnecessarily eager cloning of iterator items
110+
--> $DIR/iter_overeager_cloned.rs:38:17
111+
|
112+
LL | let _ = vec.iter().cloned().find(f);
113+
| ^^^^^^^^^^-----------------
114+
| |
115+
| help: try: `.find(|&x| f(x)).cloned()`
116+
117+
error: unnecessarily eager cloning of iterator items
118+
--> $DIR/iter_overeager_cloned.rs:45:9
119+
|
120+
LL | iter.cloned().filter(move |(&a, b)| a == 1 && b == &target)
121+
| ^^^^-------------------------------------------------------
122+
| |
123+
| help: try: `.filter(move |&(&a, b)| a == 1 && b == &target).cloned()`
124+
125+
error: unnecessarily eager cloning of iterator items
126+
--> $DIR/iter_overeager_cloned.rs:56:13
127+
|
128+
LL | iter.cloned().filter(move |S { a, b }| **a == 1 && b == &target)
129+
| ^^^^------------------------------------------------------------
130+
| |
131+
| help: try: `.filter(move |&S { a, b }| **a == 1 && b == &target).cloned()`
132+
133+
error: aborting due to 15 previous errors
70134

0 commit comments

Comments
 (0)