Skip to content

Commit 93cad4a

Browse files
borsbors
committed
Auto merge of #8203 - pmnoxx:piotr-next-lint, r=llogiq
New lint: `iter_overeager_cloned` Closes #8202 changelog: New lint: [`iter_overeager_cloned`]
2 parents 0d27bd8 + 36396c6 commit 93cad4a

9 files changed

+285
-15
lines changed

Diff for: ‎CHANGELOG.md

+1
Original file line numberDiff line numberDiff line change
@@ -3049,6 +3049,7 @@ Released 2018-09-13
30493049
[`iter_not_returning_iterator`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_not_returning_iterator
30503050
[`iter_nth`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_nth
30513051
[`iter_nth_zero`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_nth_zero
3052+
[`iter_overeager_cloned`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_overeager_cloned
30523053
[`iter_skip_next`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_skip_next
30533054
[`iterator_step_by_zero`]: https://rust-lang.github.io/rust-clippy/master/index.html#iterator_step_by_zero
30543055
[`just_underscores_and_digits`]: https://rust-lang.github.io/rust-clippy/master/index.html#just_underscores_and_digits

Diff for: ‎clippy_lints/src/lib.register_all.rs

+1
Original file line numberDiff line numberDiff line change
@@ -156,6 +156,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![
156156
LintId::of(methods::ITER_NEXT_SLICE),
157157
LintId::of(methods::ITER_NTH),
158158
LintId::of(methods::ITER_NTH_ZERO),
159+
LintId::of(methods::ITER_OVEREAGER_CLONED),
159160
LintId::of(methods::ITER_SKIP_NEXT),
160161
LintId::of(methods::MANUAL_FILTER_MAP),
161162
LintId::of(methods::MANUAL_FIND_MAP),

Diff for: ‎clippy_lints/src/lib.register_lints.rs

+1
Original file line numberDiff line numberDiff line change
@@ -288,6 +288,7 @@ store.register_lints(&[
288288
methods::ITER_NEXT_SLICE,
289289
methods::ITER_NTH,
290290
methods::ITER_NTH_ZERO,
291+
methods::ITER_OVEREAGER_CLONED,
291292
methods::ITER_SKIP_NEXT,
292293
methods::MANUAL_FILTER_MAP,
293294
methods::MANUAL_FIND_MAP,

Diff for: ‎clippy_lints/src/lib.register_perf.rs

+1
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ store.register_group(true, "clippy::perf", Some("clippy_perf"), vec![
1414
LintId::of(methods::EXPECT_FUN_CALL),
1515
LintId::of(methods::EXTEND_WITH_DRAIN),
1616
LintId::of(methods::ITER_NTH),
17+
LintId::of(methods::ITER_OVEREAGER_CLONED),
1718
LintId::of(methods::MANUAL_STR_REPEAT),
1819
LintId::of(methods::OR_FUN_CALL),
1920
LintId::of(methods::SINGLE_CHAR_PATTERN),

Diff for: ‎clippy_lints/src/methods/iter_overeager_cloned.rs

+62
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
use clippy_utils::diagnostics::span_lint_and_sugg;
2+
use clippy_utils::source::snippet;
3+
use clippy_utils::ty::{get_iterator_item_ty, is_copy};
4+
use itertools::Itertools;
5+
use rustc_errors::Applicability;
6+
use rustc_hir as hir;
7+
use rustc_lint::LateContext;
8+
use rustc_middle::ty;
9+
use std::ops::Not;
10+
11+
use super::ITER_OVEREAGER_CLONED;
12+
use crate::redundant_clone::REDUNDANT_CLONE;
13+
14+
/// lint overeager use of `cloned()` for `Iterator`s
15+
pub(super) fn check<'tcx>(
16+
cx: &LateContext<'tcx>,
17+
expr: &'tcx hir::Expr<'_>,
18+
recv: &'tcx hir::Expr<'_>,
19+
name: &str,
20+
map_arg: &[hir::Expr<'_>],
21+
) {
22+
// Check if it's iterator and get type associated with `Item`.
23+
let inner_ty = match get_iterator_item_ty(cx, cx.typeck_results().expr_ty_adjusted(recv)) {
24+
Some(ty) => ty,
25+
_ => return,
26+
};
27+
28+
match inner_ty.kind() {
29+
ty::Ref(_, ty, _) if !is_copy(cx, ty) => {},
30+
_ => return,
31+
};
32+
33+
let (lint, preserve_cloned) = match name {
34+
"count" => (REDUNDANT_CLONE, false),
35+
_ => (ITER_OVEREAGER_CLONED, true),
36+
};
37+
let wildcard_params = map_arg.is_empty().not().then(|| "...").unwrap_or_default();
38+
let msg = format!(
39+
"called `cloned().{}({})` on an `Iterator`. It may be more efficient to call `{}({}){}` instead",
40+
name,
41+
wildcard_params,
42+
name,
43+
wildcard_params,
44+
preserve_cloned.then(|| ".cloned()").unwrap_or_default(),
45+
);
46+
47+
span_lint_and_sugg(
48+
cx,
49+
lint,
50+
expr.span,
51+
&msg,
52+
"try this",
53+
format!(
54+
"{}.{}({}){}",
55+
snippet(cx, recv.span, ".."),
56+
name,
57+
map_arg.iter().map(|a| snippet(cx, a.span, "..")).join(", "),
58+
preserve_cloned.then(|| ".cloned()").unwrap_or_default(),
59+
),
60+
Applicability::MachineApplicable,
61+
);
62+
}

Diff for: ‎clippy_lints/src/methods/mod.rs

+69-15
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ mod iter_count;
3030
mod iter_next_slice;
3131
mod iter_nth;
3232
mod iter_nth_zero;
33+
mod iter_overeager_cloned;
3334
mod iter_skip_next;
3435
mod iterator_step_by_zero;
3536
mod manual_saturating_arithmetic;
@@ -106,6 +107,41 @@ declare_clippy_lint! {
106107
"used `cloned` where `copied` could be used instead"
107108
}
108109

110+
declare_clippy_lint! {
111+
/// ### What it does
112+
/// Checks for usage of `_.cloned().<func>()` where call to `.cloned()` can be postponed.
113+
///
114+
/// ### Why is this bad?
115+
/// It's often inefficient to clone all elements of an iterator, when eventually, only some
116+
/// of them will be consumed.
117+
///
118+
/// ### Examples
119+
/// ```rust
120+
/// # let vec = vec!["string".to_string()];
121+
///
122+
/// // Bad
123+
/// vec.iter().cloned().take(10);
124+
///
125+
/// // Good
126+
/// vec.iter().take(10).cloned();
127+
///
128+
/// // Bad
129+
/// vec.iter().cloned().last();
130+
///
131+
/// // Good
132+
/// vec.iter().last().cloned();
133+
///
134+
/// ```
135+
/// ### Known Problems
136+
/// This `lint` removes the side of effect of cloning items in the iterator.
137+
/// A code that relies on that side-effect could fail.
138+
///
139+
#[clippy::version = "1.59.0"]
140+
pub ITER_OVEREAGER_CLONED,
141+
perf,
142+
"using `cloned()` early with `Iterator::iter()` can lead to some performance inefficiencies"
143+
}
144+
109145
declare_clippy_lint! {
110146
/// ### What it does
111147
/// Checks for usages of `Iterator::flat_map()` where `filter_map()` could be
@@ -1950,6 +1986,7 @@ impl_lint_pass!(Methods => [
19501986
CLONE_ON_COPY,
19511987
CLONE_ON_REF_PTR,
19521988
CLONE_DOUBLE_REF,
1989+
ITER_OVEREAGER_CLONED,
19531990
CLONED_INSTEAD_OF_COPIED,
19541991
FLAT_MAP_OPTION,
19551992
INEFFICIENT_TO_STRING,
@@ -2243,9 +2280,10 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio
22432280
},
22442281
_ => {},
22452282
},
2246-
("count", []) => match method_call(recv) {
2247-
Some((name @ ("into_iter" | "iter" | "iter_mut"), [recv2], _)) => {
2248-
iter_count::check(cx, expr, recv2, name);
2283+
(name @ "count", args @ []) => match method_call(recv) {
2284+
Some(("cloned", [recv2], _)) => iter_overeager_cloned::check(cx, expr, recv2, name, args),
2285+
Some((name2 @ ("into_iter" | "iter" | "iter_mut"), [recv2], _)) => {
2286+
iter_count::check(cx, expr, recv2, name2);
22492287
},
22502288
Some(("map", [_, arg], _)) => suspicious_map::check(cx, expr, recv, arg),
22512289
_ => {},
@@ -2266,10 +2304,10 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio
22662304
flat_map_identity::check(cx, expr, arg, span);
22672305
flat_map_option::check(cx, expr, arg, span);
22682306
},
2269-
("flatten", []) => {
2270-
if let Some(("map", [recv, map_arg], _)) = method_call(recv) {
2271-
map_flatten::check(cx, expr, recv, map_arg);
2272-
}
2307+
(name @ "flatten", args @ []) => match method_call(recv) {
2308+
Some(("map", [recv, map_arg], _)) => map_flatten::check(cx, expr, recv, map_arg),
2309+
Some(("cloned", [recv2], _)) => iter_overeager_cloned::check(cx, expr, recv2, name, args),
2310+
_ => {},
22732311
},
22742312
("fold", [init, acc]) => unnecessary_fold::check(cx, expr, init, acc, span),
22752313
("for_each", [_]) => {
@@ -2281,6 +2319,13 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio
22812319
("is_file", []) => filetype_is_file::check(cx, expr, recv),
22822320
("is_none", []) => check_is_some_is_none(cx, expr, recv, false),
22832321
("is_some", []) => check_is_some_is_none(cx, expr, recv, true),
2322+
("last", args @ []) | ("skip", args @ [_]) => {
2323+
if let Some((name2, [recv2, args2 @ ..], _span2)) = method_call(recv) {
2324+
if let ("cloned", []) = (name2, args2) {
2325+
iter_overeager_cloned::check(cx, expr, recv2, name, args);
2326+
}
2327+
}
2328+
},
22842329
("map", [m_arg]) => {
22852330
if let Some((name, [recv2, args @ ..], span2)) = method_call(recv) {
22862331
match (name, args) {
@@ -2296,20 +2341,22 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio
22962341
map_identity::check(cx, expr, recv, m_arg, span);
22972342
},
22982343
("map_or", [def, map]) => option_map_or_none::check(cx, expr, recv, def, map),
2299-
("next", []) => {
2300-
if let Some((name, [recv, args @ ..], _)) = method_call(recv) {
2301-
match (name, args) {
2302-
("filter", [arg]) => filter_next::check(cx, expr, recv, arg),
2303-
("filter_map", [arg]) => filter_map_next::check(cx, expr, recv, arg, msrv),
2304-
("iter", []) => iter_next_slice::check(cx, expr, recv),
2305-
("skip", [arg]) => iter_skip_next::check(cx, expr, recv, arg),
2344+
(name @ "next", args @ []) => {
2345+
if let Some((name2, [recv2, args2 @ ..], _)) = method_call(recv) {
2346+
match (name2, args2) {
2347+
("cloned", []) => iter_overeager_cloned::check(cx, expr, recv2, name, args),
2348+
("filter", [arg]) => filter_next::check(cx, expr, recv2, arg),
2349+
("filter_map", [arg]) => filter_map_next::check(cx, expr, recv2, arg, msrv),
2350+
("iter", []) => iter_next_slice::check(cx, expr, recv2),
2351+
("skip", [arg]) => iter_skip_next::check(cx, expr, recv2, arg),
23062352
("skip_while", [_]) => skip_while_next::check(cx, expr),
23072353
_ => {},
23082354
}
23092355
}
23102356
},
2311-
("nth", [n_arg]) => match method_call(recv) {
2357+
("nth", args @ [n_arg]) => match method_call(recv) {
23122358
Some(("bytes", [recv2], _)) => bytes_nth::check(cx, expr, recv2, n_arg),
2359+
Some(("cloned", [recv2], _)) => iter_overeager_cloned::check(cx, expr, recv2, name, args),
23132360
Some(("iter", [recv2], _)) => iter_nth::check(cx, expr, recv2, recv, n_arg, false),
23142361
Some(("iter_mut", [recv2], _)) => iter_nth::check(cx, expr, recv2, recv, n_arg, true),
23152362
_ => iter_nth_zero::check(cx, expr, recv, n_arg),
@@ -2337,6 +2384,13 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio
23372384
}
23382385
},
23392386
("step_by", [arg]) => iterator_step_by_zero::check(cx, expr, arg),
2387+
("take", args @ [_arg]) => {
2388+
if let Some((name2, [recv2, args2 @ ..], _span2)) = method_call(recv) {
2389+
if let ("cloned", []) = (name2, args2) {
2390+
iter_overeager_cloned::check(cx, expr, recv2, name, args);
2391+
}
2392+
}
2393+
},
23402394
("to_os_string" | "to_owned" | "to_path_buf" | "to_vec", []) => {
23412395
implicit_clone::check(cx, name, expr, recv);
23422396
},

Diff for: ‎tests/ui/iter_overeager_cloned.fixed

+45
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
// run-rustfix
2+
#![warn(clippy::iter_overeager_cloned, clippy::redundant_clone, clippy::filter_next)]
3+
4+
fn main() {
5+
let vec = vec!["1".to_string(), "2".to_string(), "3".to_string()];
6+
7+
let _: Option<String> = vec.iter().last().cloned();
8+
9+
let _: Option<String> = vec.iter().chain(vec.iter()).next().cloned();
10+
11+
let _: usize = vec.iter().filter(|x| x == &"2").count();
12+
13+
let _: Vec<_> = vec.iter().take(2).cloned().collect();
14+
15+
let _: Vec<_> = vec.iter().skip(2).cloned().collect();
16+
17+
let _ = vec.iter().filter(|x| x == &"2").nth(2).cloned();
18+
19+
let _ = [Some(Some("str".to_string())), Some(Some("str".to_string()))]
20+
.iter().flatten().cloned();
21+
22+
// Not implemented yet
23+
let _ = vec.iter().cloned().filter(|x| x.starts_with('2'));
24+
25+
// Not implemented yet
26+
let _ = vec.iter().cloned().map(|x| x.len());
27+
28+
// This would fail if changed.
29+
let _ = vec.iter().cloned().map(|x| x + "2");
30+
31+
// Not implemented yet
32+
let _ = vec.iter().cloned().find(|x| x == "2");
33+
34+
// Not implemented yet
35+
let _ = vec.iter().cloned().for_each(|x| assert!(!x.is_empty()));
36+
37+
// Not implemented yet
38+
let _ = vec.iter().cloned().all(|x| x.len() == 1);
39+
40+
// Not implemented yet
41+
let _ = vec.iter().cloned().any(|x| x.len() == 1);
42+
43+
// Should probably stay as it is.
44+
let _ = [0, 1, 2, 3, 4].iter().cloned().take(10);
45+
}

Diff for: ‎tests/ui/iter_overeager_cloned.rs

+47
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
// run-rustfix
2+
#![warn(clippy::iter_overeager_cloned, clippy::redundant_clone, clippy::filter_next)]
3+
4+
fn main() {
5+
let vec = vec!["1".to_string(), "2".to_string(), "3".to_string()];
6+
7+
let _: Option<String> = vec.iter().cloned().last();
8+
9+
let _: Option<String> = vec.iter().chain(vec.iter()).cloned().next();
10+
11+
let _: usize = vec.iter().filter(|x| x == &"2").cloned().count();
12+
13+
let _: Vec<_> = vec.iter().cloned().take(2).collect();
14+
15+
let _: Vec<_> = vec.iter().cloned().skip(2).collect();
16+
17+
let _ = vec.iter().filter(|x| x == &"2").cloned().nth(2);
18+
19+
let _ = [Some(Some("str".to_string())), Some(Some("str".to_string()))]
20+
.iter()
21+
.cloned()
22+
.flatten();
23+
24+
// Not implemented yet
25+
let _ = vec.iter().cloned().filter(|x| x.starts_with('2'));
26+
27+
// Not implemented yet
28+
let _ = vec.iter().cloned().map(|x| x.len());
29+
30+
// This would fail if changed.
31+
let _ = vec.iter().cloned().map(|x| x + "2");
32+
33+
// Not implemented yet
34+
let _ = vec.iter().cloned().find(|x| x == "2");
35+
36+
// Not implemented yet
37+
let _ = vec.iter().cloned().for_each(|x| assert!(!x.is_empty()));
38+
39+
// Not implemented yet
40+
let _ = vec.iter().cloned().all(|x| x.len() == 1);
41+
42+
// Not implemented yet
43+
let _ = vec.iter().cloned().any(|x| x.len() == 1);
44+
45+
// Should probably stay as it is.
46+
let _ = [0, 1, 2, 3, 4].iter().cloned().take(10);
47+
}

Diff for: ‎tests/ui/iter_overeager_cloned.stderr

+58
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
error: called `cloned().last()` on an `Iterator`. It may be more efficient to call `last().cloned()` instead
2+
--> $DIR/iter_overeager_cloned.rs:7:29
3+
|
4+
LL | let _: Option<String> = vec.iter().cloned().last();
5+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `vec.iter().last().cloned()`
6+
|
7+
= note: `-D clippy::iter-overeager-cloned` implied by `-D warnings`
8+
9+
error: called `cloned().next()` on an `Iterator`. It may be more efficient to call `next().cloned()` instead
10+
--> $DIR/iter_overeager_cloned.rs:9:29
11+
|
12+
LL | let _: Option<String> = vec.iter().chain(vec.iter()).cloned().next();
13+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `vec.iter().chain(vec.iter()).next().cloned()`
14+
15+
error: called `cloned().count()` on an `Iterator`. It may be more efficient to call `count()` instead
16+
--> $DIR/iter_overeager_cloned.rs:11:20
17+
|
18+
LL | let _: usize = vec.iter().filter(|x| x == &"2").cloned().count();
19+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `vec.iter().filter(|x| x == &"2").count()`
20+
|
21+
= note: `-D clippy::redundant-clone` implied by `-D warnings`
22+
23+
error: called `cloned().take(...)` on an `Iterator`. It may be more efficient to call `take(...).cloned()` instead
24+
--> $DIR/iter_overeager_cloned.rs:13:21
25+
|
26+
LL | let _: Vec<_> = vec.iter().cloned().take(2).collect();
27+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `vec.iter().take(2).cloned()`
28+
29+
error: called `cloned().skip(...)` on an `Iterator`. It may be more efficient to call `skip(...).cloned()` instead
30+
--> $DIR/iter_overeager_cloned.rs:15:21
31+
|
32+
LL | let _: Vec<_> = vec.iter().cloned().skip(2).collect();
33+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `vec.iter().skip(2).cloned()`
34+
35+
error: called `cloned().nth(...)` on an `Iterator`. It may be more efficient to call `nth(...).cloned()` instead
36+
--> $DIR/iter_overeager_cloned.rs:17:13
37+
|
38+
LL | let _ = vec.iter().filter(|x| x == &"2").cloned().nth(2);
39+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `vec.iter().filter(|x| x == &"2").nth(2).cloned()`
40+
41+
error: called `cloned().flatten()` on an `Iterator`. It may be more efficient to call `flatten().cloned()` instead
42+
--> $DIR/iter_overeager_cloned.rs:19:13
43+
|
44+
LL | let _ = [Some(Some("str".to_string())), Some(Some("str".to_string()))]
45+
| _____________^
46+
LL | | .iter()
47+
LL | | .cloned()
48+
LL | | .flatten();
49+
| |__________________^
50+
|
51+
help: try this
52+
|
53+
LL ~ let _ = [Some(Some("str".to_string())), Some(Some("str".to_string()))]
54+
LL ~ .iter().flatten().cloned();
55+
|
56+
57+
error: aborting due to 7 previous errors
58+

0 commit comments

Comments
 (0)