Skip to content

Commit 1c9b31d

Browse files
pmnoxxpmnoxx
committed
New line: cloned_next
1 parent 0d27bd8 commit 1c9b31d

9 files changed

+292
-15
lines changed

‎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

‎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),

‎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,

‎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),

‎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+
}

‎clippy_lints/src/methods/mod.rs

+76-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 @ []) => {
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),
@@ -2320,6 +2367,13 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio
23202367
unnecessary_lazy_eval::check(cx, expr, recv, arg, "or");
23212368
}
23222369
},
2370+
("skip", args) => {
2371+
if let Some((name2, [recv2, args2 @ ..], _span2)) = method_call(recv) {
2372+
if let ("cloned", []) = (name2, args2) {
2373+
iter_overeager_cloned::check(cx, expr, recv2, name, args);
2374+
}
2375+
}
2376+
},
23232377
("splitn" | "rsplitn", [count_arg, pat_arg]) => {
23242378
if let Some((Constant::Int(count), _)) = constant(cx, cx.typeck_results(), count_arg) {
23252379
suspicious_splitn::check(cx, name, expr, recv, count);
@@ -2337,6 +2391,13 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio
23372391
}
23382392
},
23392393
("step_by", [arg]) => iterator_step_by_zero::check(cx, expr, arg),
2394+
("take", args @ [_arg]) => {
2395+
if let Some((name2, [recv2, args2 @ ..], _span2)) = method_call(recv) {
2396+
if let ("cloned", []) = (name2, args2) {
2397+
iter_overeager_cloned::check(cx, expr, recv2, name, args);
2398+
}
2399+
}
2400+
},
23402401
("to_os_string" | "to_owned" | "to_path_buf" | "to_vec", []) => {
23412402
implicit_clone::check(cx, name, expr, recv);
23422403
},

‎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+
}

‎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+
}

0 commit comments

Comments
 (0)