Skip to content

Commit ab8c8c6

Browse files
borsbors
committed
Auto merge of rust-lang#8960 - Jarcho:iter_cloned, r=giraffate
Changes to `iter_overeager_cloned` fixes: rust-lang#8494 changelog: Don't lint `iter_overeager_cloned` on `.cloned().flatten()` when `T::Item` doesn't implement `IntoIterator`
2 parents 72f5ff6 + bf3ab59 commit ab8c8c6

File tree

6 files changed

+92
-84
lines changed

6 files changed

+92
-84
lines changed

‎clippy_lints/src/methods/iter_overeager_cloned.rs

Lines changed: 43 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -1,70 +1,59 @@
1-
use clippy_utils::diagnostics::span_lint_and_sugg;
2-
use clippy_utils::source::snippet;
3-
use clippy_utils::ty::{get_iterator_item_ty, implements_trait, is_copy};
4-
use itertools::Itertools;
1+
use clippy_utils::diagnostics::span_lint_and_then;
2+
use clippy_utils::source::snippet_opt;
3+
use clippy_utils::ty::{get_associated_type, implements_trait, is_copy};
54
use rustc_errors::Applicability;
6-
use rustc_hir as hir;
5+
use rustc_hir::Expr;
76
use rustc_lint::LateContext;
87
use rustc_middle::ty;
98
use rustc_span::sym;
10-
use std::ops::Not;
119

1210
use super::ITER_OVEREAGER_CLONED;
1311
use crate::redundant_clone::REDUNDANT_CLONE;
1412

15-
/// lint overeager use of `cloned()` for `Iterator`s
1613
pub(super) fn check<'tcx>(
1714
cx: &LateContext<'tcx>,
18-
expr: &'tcx hir::Expr<'_>,
19-
recv: &'tcx hir::Expr<'_>,
20-
name: &str,
21-
map_arg: &[hir::Expr<'_>],
15+
expr: &'tcx Expr<'_>,
16+
cloned_call: &'tcx Expr<'_>,
17+
cloned_recv: &'tcx Expr<'_>,
18+
is_count: bool,
19+
needs_into_iter: bool,
2220
) {
23-
// Check if it's iterator and get type associated with `Item`.
24-
let inner_ty = if_chain! {
25-
if let Some(iterator_trait_id) = cx.tcx.get_diagnostic_item(sym::Iterator);
26-
let recv_ty = cx.typeck_results().expr_ty(recv);
27-
if implements_trait(cx, recv_ty, iterator_trait_id, &[]);
28-
if let Some(inner_ty) = get_iterator_item_ty(cx, cx.typeck_results().expr_ty_adjusted(recv));
29-
then {
30-
inner_ty
31-
} else {
21+
let typeck = cx.typeck_results();
22+
if let Some(iter_id) = cx.tcx.get_diagnostic_item(sym::Iterator)
23+
&& let Some(method_id) = typeck.type_dependent_def_id(expr.hir_id)
24+
&& cx.tcx.trait_of_item(method_id) == Some(iter_id)
25+
&& let Some(method_id) = typeck.type_dependent_def_id(cloned_call.hir_id)
26+
&& cx.tcx.trait_of_item(method_id) == Some(iter_id)
27+
&& let cloned_recv_ty = typeck.expr_ty_adjusted(cloned_recv)
28+
&& let Some(iter_assoc_ty) = get_associated_type(cx, cloned_recv_ty, iter_id, "Item")
29+
&& matches!(*iter_assoc_ty.kind(), ty::Ref(_, ty, _) if !is_copy(cx, ty))
30+
{
31+
if needs_into_iter
32+
&& let Some(into_iter_id) = cx.tcx.get_diagnostic_item(sym::IntoIterator)
33+
&& !implements_trait(cx, iter_assoc_ty, into_iter_id, &[])
34+
{
3235
return;
3336
}
34-
};
35-
36-
match inner_ty.kind() {
37-
ty::Ref(_, ty, _) if !is_copy(cx, *ty) => {},
38-
_ => return,
39-
};
4037

41-
let (lint, preserve_cloned) = match name {
42-
"count" => (REDUNDANT_CLONE, false),
43-
_ => (ITER_OVEREAGER_CLONED, true),
44-
};
45-
let wildcard_params = map_arg.is_empty().not().then(|| "...").unwrap_or_default();
46-
let msg = format!(
47-
"called `cloned().{}({})` on an `Iterator`. It may be more efficient to call `{}({}){}` instead",
48-
name,
49-
wildcard_params,
50-
name,
51-
wildcard_params,
52-
preserve_cloned.then(|| ".cloned()").unwrap_or_default(),
53-
);
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()")
42+
};
5443

55-
span_lint_and_sugg(
56-
cx,
57-
lint,
58-
expr.span,
59-
&msg,
60-
"try this",
61-
format!(
62-
"{}.{}({}){}",
63-
snippet(cx, recv.span, ".."),
64-
name,
65-
map_arg.iter().map(|a| snippet(cx, a.span, "..")).join(", "),
66-
preserve_cloned.then(|| ".cloned()").unwrap_or_default(),
67-
),
68-
Applicability::MachineApplicable,
69-
);
44+
span_lint_and_then(
45+
cx,
46+
lint,
47+
expr.span,
48+
msg,
49+
|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 this", snip, Applicability::MachineApplicable);
55+
}
56+
}
57+
);
58+
}
7059
}

‎clippy_lints/src/methods/mod.rs

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -2583,8 +2583,8 @@ impl Methods {
25832583
},
25842584
_ => {},
25852585
},
2586-
(name @ "count", args @ []) => match method_call(recv) {
2587-
Some(("cloned", [recv2], _)) => iter_overeager_cloned::check(cx, expr, recv2, name, args),
2586+
("count", []) => match method_call(recv) {
2587+
Some(("cloned", [recv2], _)) => iter_overeager_cloned::check(cx, expr, recv, recv2, true, false),
25882588
Some((name2 @ ("into_iter" | "iter" | "iter_mut"), [recv2], _)) => {
25892589
iter_count::check(cx, expr, recv2, name2);
25902590
},
@@ -2614,9 +2614,9 @@ impl Methods {
26142614
flat_map_identity::check(cx, expr, arg, span);
26152615
flat_map_option::check(cx, expr, arg, span);
26162616
},
2617-
(name @ "flatten", args @ []) => match method_call(recv) {
2617+
("flatten", []) => match method_call(recv) {
26182618
Some(("map", [recv, map_arg], map_span)) => map_flatten::check(cx, expr, recv, map_arg, map_span),
2619-
Some(("cloned", [recv2], _)) => iter_overeager_cloned::check(cx, expr, recv2, name, args),
2619+
Some(("cloned", [recv2], _)) => iter_overeager_cloned::check(cx, expr, recv, recv2, false, true),
26202620
_ => {},
26212621
},
26222622
("fold", [init, acc]) => unnecessary_fold::check(cx, expr, init, acc, span),
@@ -2636,10 +2636,10 @@ impl Methods {
26362636
unnecessary_join::check(cx, expr, recv, join_arg, span);
26372637
}
26382638
},
2639-
("last", args @ []) | ("skip", args @ [_]) => {
2639+
("last", []) | ("skip", [_]) => {
26402640
if let Some((name2, [recv2, args2 @ ..], _span2)) = method_call(recv) {
26412641
if let ("cloned", []) = (name2, args2) {
2642-
iter_overeager_cloned::check(cx, expr, recv2, name, args);
2642+
iter_overeager_cloned::check(cx, expr, recv, recv2, false, false);
26432643
}
26442644
}
26452645
},
@@ -2660,10 +2660,10 @@ impl Methods {
26602660
map_identity::check(cx, expr, recv, m_arg, name, span);
26612661
},
26622662
("map_or", [def, map]) => option_map_or_none::check(cx, expr, recv, def, map),
2663-
(name @ "next", args @ []) => {
2663+
("next", []) => {
26642664
if let Some((name2, [recv2, args2 @ ..], _)) = method_call(recv) {
26652665
match (name2, args2) {
2666-
("cloned", []) => iter_overeager_cloned::check(cx, expr, recv2, name, args),
2666+
("cloned", []) => iter_overeager_cloned::check(cx, expr, recv, recv2, false, false),
26672667
("filter", [arg]) => filter_next::check(cx, expr, recv2, arg),
26682668
("filter_map", [arg]) => filter_map_next::check(cx, expr, recv2, arg, self.msrv),
26692669
("iter", []) => iter_next_slice::check(cx, expr, recv2),
@@ -2673,9 +2673,9 @@ impl Methods {
26732673
}
26742674
}
26752675
},
2676-
("nth", args @ [n_arg]) => match method_call(recv) {
2676+
("nth", [n_arg]) => match method_call(recv) {
26772677
Some(("bytes", [recv2], _)) => bytes_nth::check(cx, expr, recv2, n_arg),
2678-
Some(("cloned", [recv2], _)) => iter_overeager_cloned::check(cx, expr, recv2, name, args),
2678+
Some(("cloned", [recv2], _)) => iter_overeager_cloned::check(cx, expr, recv, recv2, false, false),
26792679
Some(("iter", [recv2], _)) => iter_nth::check(cx, expr, recv2, recv, n_arg, false),
26802680
Some(("iter_mut", [recv2], _)) => iter_nth::check(cx, expr, recv2, recv, n_arg, true),
26812681
_ => iter_nth_zero::check(cx, expr, recv, n_arg),
@@ -2698,10 +2698,10 @@ impl Methods {
26982698
}
26992699
},
27002700
("step_by", [arg]) => iterator_step_by_zero::check(cx, expr, arg),
2701-
("take", args @ [_arg]) => {
2701+
("take", [_arg]) => {
27022702
if let Some((name2, [recv2, args2 @ ..], _span2)) = method_call(recv) {
27032703
if let ("cloned", []) = (name2, args2) {
2704-
iter_overeager_cloned::check(cx, expr, recv2, name, args);
2704+
iter_overeager_cloned::check(cx, expr, recv, recv2, false, false);
27052705
}
27062706
}
27072707
},

‎clippy_utils/src/ty.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -78,9 +78,9 @@ pub fn get_associated_type<'tcx>(
7878
cx.tcx
7979
.associated_items(trait_id)
8080
.find_by_name_and_kind(cx.tcx, Ident::from_str(name), ty::AssocKind::Type, trait_id)
81-
.map(|assoc| {
81+
.and_then(|assoc| {
8282
let proj = cx.tcx.mk_projection(assoc.def_id, cx.tcx.mk_substs_trait(ty, &[]));
83-
cx.tcx.normalize_erasing_regions(cx.param_env, proj)
83+
cx.tcx.try_normalize_erasing_regions(cx.param_env, proj).ok()
8484
})
8585
}
8686

‎tests/ui/iter_overeager_cloned.fixed

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,8 @@ fn main() {
1818
let _ = vec.iter().filter(|x| x == &"2").nth(2).cloned();
1919

2020
let _ = [Some(Some("str".to_string())), Some(Some("str".to_string()))]
21-
.iter().flatten().cloned();
21+
.iter()
22+
.flatten().cloned();
2223

2324
// Not implemented yet
2425
let _ = vec.iter().cloned().filter(|x| x.starts_with('2'));
@@ -43,6 +44,9 @@ fn main() {
4344

4445
// Should probably stay as it is.
4546
let _ = [0, 1, 2, 3, 4].iter().cloned().take(10);
47+
48+
// `&Range<_>` doesn't implement `IntoIterator`
49+
let _ = [0..1, 2..5].iter().cloned().flatten();
4650
}
4751

4852
// #8527

‎tests/ui/iter_overeager_cloned.rs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,9 @@ fn main() {
4545

4646
// Should probably stay as it is.
4747
let _ = [0, 1, 2, 3, 4].iter().cloned().take(10);
48+
49+
// `&Range<_>` doesn't implement `IntoIterator`
50+
let _ = [0..1, 2..5].iter().cloned().flatten();
4851
}
4952

5053
// #8527

‎tests/ui/iter_overeager_cloned.stderr

Lines changed: 27 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,44 +1,56 @@
1-
error: called `cloned().last()` on an `Iterator`. It may be more efficient to call `last().cloned()` instead
1+
error: unnecessarily eager cloning of iterator items
22
--> $DIR/iter_overeager_cloned.rs:8:29
33
|
44
LL | let _: Option<String> = vec.iter().cloned().last();
5-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `vec.iter().last().cloned()`
5+
| ^^^^^^^^^^----------------
6+
| |
7+
| help: try this: `.last().cloned()`
68
|
79
= note: `-D clippy::iter-overeager-cloned` implied by `-D warnings`
810

9-
error: called `cloned().next()` on an `Iterator`. It may be more efficient to call `next().cloned()` instead
11+
error: unnecessarily eager cloning of iterator items
1012
--> $DIR/iter_overeager_cloned.rs:10:29
1113
|
1214
LL | let _: Option<String> = vec.iter().chain(vec.iter()).cloned().next();
13-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `vec.iter().chain(vec.iter()).next().cloned()`
15+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^----------------
16+
| |
17+
| help: try this: `.next().cloned()`
1418

15-
error: called `cloned().count()` on an `Iterator`. It may be more efficient to call `count()` instead
19+
error: unneeded cloning of iterator items
1620
--> $DIR/iter_overeager_cloned.rs:12:20
1721
|
1822
LL | let _: usize = vec.iter().filter(|x| x == &"2").cloned().count();
19-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `vec.iter().filter(|x| x == &"2").count()`
23+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^-----------------
24+
| |
25+
| help: try this: `.count()`
2026
|
2127
= note: `-D clippy::redundant-clone` implied by `-D warnings`
2228

23-
error: called `cloned().take(...)` on an `Iterator`. It may be more efficient to call `take(...).cloned()` instead
29+
error: unnecessarily eager cloning of iterator items
2430
--> $DIR/iter_overeager_cloned.rs:14:21
2531
|
2632
LL | let _: Vec<_> = vec.iter().cloned().take(2).collect();
27-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `vec.iter().take(2).cloned()`
33+
| ^^^^^^^^^^-----------------
34+
| |
35+
| help: try this: `.take(2).cloned()`
2836

29-
error: called `cloned().skip(...)` on an `Iterator`. It may be more efficient to call `skip(...).cloned()` instead
37+
error: unnecessarily eager cloning of iterator items
3038
--> $DIR/iter_overeager_cloned.rs:16:21
3139
|
3240
LL | let _: Vec<_> = vec.iter().cloned().skip(2).collect();
33-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `vec.iter().skip(2).cloned()`
41+
| ^^^^^^^^^^-----------------
42+
| |
43+
| help: try this: `.skip(2).cloned()`
3444

35-
error: called `cloned().nth(...)` on an `Iterator`. It may be more efficient to call `nth(...).cloned()` instead
45+
error: unnecessarily eager cloning of iterator items
3646
--> $DIR/iter_overeager_cloned.rs:18:13
3747
|
3848
LL | let _ = vec.iter().filter(|x| x == &"2").cloned().nth(2);
39-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `vec.iter().filter(|x| x == &"2").nth(2).cloned()`
49+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^----------------
50+
| |
51+
| help: try this: `.nth(2).cloned()`
4052

41-
error: called `cloned().flatten()` on an `Iterator`. It may be more efficient to call `flatten().cloned()` instead
53+
error: unnecessarily eager cloning of iterator items
4254
--> $DIR/iter_overeager_cloned.rs:20:13
4355
|
4456
LL | let _ = [Some(Some("str".to_string())), Some(Some("str".to_string()))]
@@ -50,8 +62,8 @@ LL | | .flatten();
5062
|
5163
help: try this
5264
|
53-
LL ~ let _ = [Some(Some("str".to_string())), Some(Some("str".to_string()))]
54-
LL ~ .iter().flatten().cloned();
65+
LL ~ .iter()
66+
LL ~ .flatten().cloned();
5567
|
5668

5769
error: aborting due to 7 previous errors

0 commit comments

Comments
 (0)