Skip to content

Commit 86fb0e8

Browse files
borsbors
committed
Auto merge of #7020 - camsteffen:needless-collect, r=Manishearth
Improve needless_collect output changelog: Improve needless_collect output Fixes #6908 Partially addresses #6164
2 parents 9ce2373 + 33798bb commit 86fb0e8

File tree

3 files changed

+57
-87
lines changed

3 files changed

+57
-87
lines changed

‎clippy_lints/src/loops/needless_collect.rs‎

Lines changed: 28 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,8 @@ use rustc_hir::intravisit::{walk_block, walk_expr, NestedVisitorMap, Visitor};
1010
use rustc_hir::{Block, Expr, ExprKind, GenericArg, HirId, Local, Pat, PatKind, QPath, StmtKind};
1111
use rustc_lint::LateContext;
1212
use rustc_middle::hir::map::Map;
13-
use rustc_span::source_map::Span;
1413
use rustc_span::symbol::{sym, Ident};
14+
use rustc_span::{MultiSpan, Span};
1515

1616
const NEEDLESS_COLLECT_MSG: &str = "avoid using `collect()` when not needed";
1717

@@ -22,7 +22,7 @@ pub(super) fn check<'tcx>(expr: &'tcx Expr<'_>, cx: &LateContext<'tcx>) {
2222
fn check_needless_collect_direct_usage<'tcx>(expr: &'tcx Expr<'_>, cx: &LateContext<'tcx>) {
2323
if_chain! {
2424
if let ExprKind::MethodCall(ref method, _, ref args, _) = expr.kind;
25-
if let ExprKind::MethodCall(ref chain_method, _, _, _) = args[0].kind;
25+
if let ExprKind::MethodCall(ref chain_method, method0_span, _, _) = args[0].kind;
2626
if chain_method.ident.name == sym!(collect) && is_trait_method(cx, &args[0], sym::Iterator);
2727
if let Some(ref generic_args) = chain_method.args;
2828
if let Some(GenericArg::Type(ref ty)) = generic_args.args.get(0);
@@ -31,55 +31,28 @@ fn check_needless_collect_direct_usage<'tcx>(expr: &'tcx Expr<'_>, cx: &LateCont
3131
|| is_type_diagnostic_item(cx, ty, sym::vecdeque_type)
3232
|| match_type(cx, ty, &paths::BTREEMAP)
3333
|| is_type_diagnostic_item(cx, ty, sym::hashmap_type);
34-
then {
35-
if method.ident.name == sym!(len) {
36-
let span = shorten_needless_collect_span(expr);
37-
span_lint_and_sugg(
38-
cx,
39-
NEEDLESS_COLLECT,
40-
span,
41-
NEEDLESS_COLLECT_MSG,
42-
"replace with",
43-
"count()".to_string(),
44-
Applicability::MachineApplicable,
45-
);
46-
}
47-
if method.ident.name == sym!(is_empty) {
48-
let span = shorten_needless_collect_span(expr);
49-
span_lint_and_sugg(
50-
cx,
51-
NEEDLESS_COLLECT,
52-
span,
53-
NEEDLESS_COLLECT_MSG,
54-
"replace with",
55-
"next().is_none()".to_string(),
56-
Applicability::MachineApplicable,
57-
);
58-
}
59-
if method.ident.name == sym!(contains) {
34+
if let Some(sugg) = match &*method.ident.name.as_str() {
35+
"len" => Some("count()".to_string()),
36+
"is_empty" => Some("next().is_none()".to_string()),
37+
"contains" => {
6038
let contains_arg = snippet(cx, args[1].span, "??");
61-
let span = shorten_needless_collect_span(expr);
62-
span_lint_and_then(
63-
cx,
64-
NEEDLESS_COLLECT,
65-
span,
66-
NEEDLESS_COLLECT_MSG,
67-
|diag| {
68-
let (arg, pred) = contains_arg
69-
.strip_prefix('&')
70-
.map_or(("&x", &*contains_arg), |s| ("x", s));
71-
diag.span_suggestion(
72-
span,
73-
"replace with",
74-
format!(
75-
"any(|{}| x == {})",
76-
arg, pred
77-
),
78-
Applicability::MachineApplicable,
79-
);
80-
}
81-
);
39+
let (arg, pred) = contains_arg
40+
.strip_prefix('&')
41+
.map_or(("&x", &*contains_arg), |s| ("x", s));
42+
Some(format!("any(|{}| x == {})", arg, pred))
8243
}
44+
_ => None,
45+
};
46+
then {
47+
span_lint_and_sugg(
48+
cx,
49+
NEEDLESS_COLLECT,
50+
method0_span.with_hi(expr.span.hi()),
51+
NEEDLESS_COLLECT_MSG,
52+
"replace with",
53+
sugg,
54+
Applicability::MachineApplicable,
55+
);
8356
}
8457
}
8558
}
@@ -92,7 +65,7 @@ fn check_needless_collect_indirect_usage<'tcx>(expr: &'tcx Expr<'_>, cx: &LateCo
9265
Local { pat: Pat { hir_id: pat_id, kind: PatKind::Binding(_, _, ident, .. ), .. },
9366
init: Some(ref init_expr), .. }
9467
) = stmt.kind;
95-
if let ExprKind::MethodCall(ref method_name, _, &[ref iter_source], ..) = init_expr.kind;
68+
if let ExprKind::MethodCall(ref method_name, collect_span, &[ref iter_source], ..) = init_expr.kind;
9669
if method_name.ident.name == sym!(collect) && is_trait_method(cx, &init_expr, sym::Iterator);
9770
if let Some(ref generic_args) = method_name.args;
9871
if let Some(GenericArg::Type(ref ty)) = generic_args.args.get(0);
@@ -101,7 +74,7 @@ fn check_needless_collect_indirect_usage<'tcx>(expr: &'tcx Expr<'_>, cx: &LateCo
10174
is_type_diagnostic_item(cx, ty, sym::vecdeque_type) ||
10275
match_type(cx, ty, &paths::LINKED_LIST);
10376
if let Some(iter_calls) = detect_iter_and_into_iters(block, *ident);
104-
if iter_calls.len() == 1;
77+
if let [iter_call] = &*iter_calls;
10578
then {
10679
let mut used_count_visitor = UsedCountVisitor {
10780
cx,
@@ -114,11 +87,12 @@ fn check_needless_collect_indirect_usage<'tcx>(expr: &'tcx Expr<'_>, cx: &LateCo
11487
}
11588

11689
// Suggest replacing iter_call with iter_replacement, and removing stmt
117-
let iter_call = &iter_calls[0];
90+
let mut span = MultiSpan::from_span(collect_span);
91+
span.push_span_label(iter_call.span, "the iterator could be used here instead".into());
11892
span_lint_and_then(
11993
cx,
12094
super::NEEDLESS_COLLECT,
121-
stmt.span.until(iter_call.span),
95+
span,
12296
NEEDLESS_COLLECT_MSG,
12397
|diag| {
12498
let iter_replacement = format!("{}{}", Sugg::hir(cx, iter_source, ".."), iter_call.get_iter_method(cx));
@@ -129,7 +103,7 @@ fn check_needless_collect_indirect_usage<'tcx>(expr: &'tcx Expr<'_>, cx: &LateCo
129103
(iter_call.span, iter_replacement)
130104
],
131105
Applicability::MachineApplicable,// MaybeIncorrect,
132-
).emit();
106+
);
133107
},
134108
);
135109
}
@@ -269,14 +243,3 @@ fn detect_iter_and_into_iters<'tcx>(block: &'tcx Block<'tcx>, identifier: Ident)
269243
visitor.visit_block(block);
270244
if visitor.seen_other { None } else { Some(visitor.uses) }
271245
}
272-
273-
fn shorten_needless_collect_span(expr: &Expr<'_>) -> Span {
274-
if_chain! {
275-
if let ExprKind::MethodCall(.., args, _) = &expr.kind;
276-
if let ExprKind::MethodCall(_, span, ..) = &args[0].kind;
277-
then {
278-
return expr.span.with_lo(span.lo());
279-
}
280-
}
281-
unreachable!();
282-
}

‎clippy_utils/src/diagnostics.rs‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -133,9 +133,11 @@ pub fn span_lint_and_note<'a, T: LintContext>(
133133
///
134134
/// If you need to customize your lint output a lot, use this function.
135135
/// If you change the signature, remember to update the internal lint `CollapsibleCalls`
136-
pub fn span_lint_and_then<'a, T: LintContext, F>(cx: &'a T, lint: &'static Lint, sp: Span, msg: &str, f: F)
136+
pub fn span_lint_and_then<C, S, F>(cx: &C, lint: &'static Lint, sp: S, msg: &str, f: F)
137137
where
138-
F: for<'b> FnOnce(&mut DiagnosticBuilder<'b>),
138+
C: LintContext,
139+
S: Into<MultiSpan>,
140+
F: FnOnce(&mut DiagnosticBuilder<'_>),
139141
{
140142
cx.struct_span_lint(lint, sp, |diag| {
141143
let mut diag = diag.build(msg);

‎tests/ui/needless_collect_indirect.stderr‎

Lines changed: 25 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,10 @@
11
error: avoid using `collect()` when not needed
2-
--> $DIR/needless_collect_indirect.rs:5:5
2+
--> $DIR/needless_collect_indirect.rs:5:39
33
|
4-
LL | / let indirect_iter = sample.iter().collect::<Vec<_>>();
5-
LL | | indirect_iter.into_iter().map(|x| (x, x + 1)).collect::<HashMap<_, _>>();
6-
| |____^
4+
LL | let indirect_iter = sample.iter().collect::<Vec<_>>();
5+
| ^^^^^^^
6+
LL | indirect_iter.into_iter().map(|x| (x, x + 1)).collect::<HashMap<_, _>>();
7+
| ------------------------- the iterator could be used here instead
78
|
89
= note: `-D clippy::needless-collect` implied by `-D warnings`
910
help: use the original Iterator instead of collecting it and then producing a new one
@@ -13,11 +14,12 @@ LL | sample.iter().map(|x| (x, x + 1)).collect::<HashMap<_, _>>();
1314
|
1415

1516
error: avoid using `collect()` when not needed
16-
--> $DIR/needless_collect_indirect.rs:7:5
17+
--> $DIR/needless_collect_indirect.rs:7:38
1718
|
18-
LL | / let indirect_len = sample.iter().collect::<VecDeque<_>>();
19-
LL | | indirect_len.len();
20-
| |____^
19+
LL | let indirect_len = sample.iter().collect::<VecDeque<_>>();
20+
| ^^^^^^^
21+
LL | indirect_len.len();
22+
| ------------------ the iterator could be used here instead
2123
|
2224
help: take the original Iterator's count instead of collecting it and finding the length
2325
|
@@ -26,11 +28,12 @@ LL | sample.iter().count();
2628
|
2729

2830
error: avoid using `collect()` when not needed
29-
--> $DIR/needless_collect_indirect.rs:9:5
31+
--> $DIR/needless_collect_indirect.rs:9:40
3032
|
31-
LL | / let indirect_empty = sample.iter().collect::<VecDeque<_>>();
32-
LL | | indirect_empty.is_empty();
33-
| |____^
33+
LL | let indirect_empty = sample.iter().collect::<VecDeque<_>>();
34+
| ^^^^^^^
35+
LL | indirect_empty.is_empty();
36+
| ------------------------- the iterator could be used here instead
3437
|
3538
help: check if the original Iterator has anything instead of collecting it and seeing if it's empty
3639
|
@@ -39,11 +42,12 @@ LL | sample.iter().next().is_none();
3942
|
4043

4144
error: avoid using `collect()` when not needed
42-
--> $DIR/needless_collect_indirect.rs:11:5
45+
--> $DIR/needless_collect_indirect.rs:11:43
4346
|
44-
LL | / let indirect_contains = sample.iter().collect::<VecDeque<_>>();
45-
LL | | indirect_contains.contains(&&5);
46-
| |____^
47+
LL | let indirect_contains = sample.iter().collect::<VecDeque<_>>();
48+
| ^^^^^^^
49+
LL | indirect_contains.contains(&&5);
50+
| ------------------------------- the iterator could be used here instead
4751
|
4852
help: check if the original Iterator contains an element instead of collecting then checking
4953
|
@@ -52,11 +56,12 @@ LL | sample.iter().any(|x| x == &5);
5256
|
5357

5458
error: avoid using `collect()` when not needed
55-
--> $DIR/needless_collect_indirect.rs:23:5
59+
--> $DIR/needless_collect_indirect.rs:23:48
5660
|
57-
LL | / let non_copy_contains = sample.into_iter().collect::<Vec<_>>();
58-
LL | | non_copy_contains.contains(&a);
59-
| |____^
61+
LL | let non_copy_contains = sample.into_iter().collect::<Vec<_>>();
62+
| ^^^^^^^
63+
LL | non_copy_contains.contains(&a);
64+
| ------------------------------ the iterator could be used here instead
6065
|
6166
help: check if the original Iterator contains an element instead of collecting then checking
6267
|

0 commit comments

Comments
 (0)