Skip to content

Commit

Permalink
[iter_overeager_cloned]: detect .cloned().filter() and .cloned().find()
Browse files Browse the repository at this point in the history 
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()
```
  • Loading branch information
@lengyijun
lengyijun committed Aug 7, 2023
1 parent 84d2896 commit c1eff7f
Show file tree
Hide file tree
Showing 5 changed files with 156 additions and 27 deletions.
51 changes: 41 additions & 10 deletions clippy_lints/src/methods/iter_overeager_cloned.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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(
Expand All @@ -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);
}
}
}
}
);
Expand Down
24 changes: 17 additions & 7 deletions clippy_lints/src/methods/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
},
Expand Down Expand Up @@ -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);
Expand All @@ -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]) => {
Expand Down Expand Up @@ -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", []) => {
Expand Down Expand Up @@ -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),
Expand All @@ -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),
Expand Down Expand Up @@ -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", []) => {
Expand All @@ -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),
Expand Down
26 changes: 21 additions & 5 deletions tests/ui/iter_overeager_cloned.fixed
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,18 +21,34 @@ 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());

// 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()));

Expand Down
24 changes: 20 additions & 4 deletions tests/ui/iter_overeager_cloned.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()));

Expand Down
58 changes: 57 additions & 1 deletion tests/ui/iter_overeager_cloned.stderr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

0 comments on commit c1eff7f

Please sign in to comment.