Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

needless_collect enhancements #7188

Merged
merged 4 commits into from
May 13, 2021
Merged

needless_collect enhancements #7188

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a21607d
needless_collect: For `BTreeMap` and `HashMap` lint only `is_empty`
mgacek8 May 5, 2021
59ccc1e
needless_collect: replace paths with diag items
mgacek8 May 5, 2021
171789e
needless_collect: Lint `LinkedList` and `BinaryHeap` in direct usage.
mgacek8 May 5, 2021
b249290
needless_collect: use snippet_with_applicability
mgacek8 May 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 43 additions & 29 deletions clippy_lints/src/loops/needless_collect.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,14 @@ use super::NEEDLESS_COLLECT;
use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then};
use clippy_utils::source::snippet;
use clippy_utils::sugg::Sugg;
use clippy_utils::ty::{is_type_diagnostic_item, match_type};
use clippy_utils::{is_trait_method, path_to_local_id, paths};
use clippy_utils::ty::is_type_diagnostic_item;
use clippy_utils::{is_trait_method, path_to_local_id};
use if_chain::if_chain;
use rustc_errors::Applicability;
use rustc_hir::intravisit::{walk_block, walk_expr, NestedVisitorMap, Visitor};
use rustc_hir::{Block, Expr, ExprKind, GenericArg, GenericArgs, HirId, Local, Pat, PatKind, QPath, StmtKind, Ty};
use rustc_lint::LateContext;
use rustc_middle::hir::map::Map;

use rustc_span::symbol::{sym, Ident};
use rustc_span::{MultiSpan, Span};

Expand All @@ -28,32 +27,47 @@ fn check_needless_collect_direct_usage<'tcx>(expr: &'tcx Expr<'_>, cx: &LateCont
if let Some(generic_args) = chain_method.args;
if let Some(GenericArg::Type(ref ty)) = generic_args.args.get(0);
if let Some(ty) = cx.typeck_results().node_type_opt(ty.hir_id);
if is_type_diagnostic_item(cx, ty, sym::vec_type)
|| is_type_diagnostic_item(cx, ty, sym::vecdeque_type)
|| match_type(cx, ty, &paths::BTREEMAP)
|| is_type_diagnostic_item(cx, ty, sym::hashmap_type);
if let Some(sugg) = match &*method.ident.name.as_str() {
"len" => Some("count()".to_string()),
"is_empty" => Some("next().is_none()".to_string()),
"contains" => {
let contains_arg = snippet(cx, args[1].span, "??");
let (arg, pred) = contains_arg
.strip_prefix('&')
.map_or(("&x", &*contains_arg), |s| ("x", s));
Some(format!("any(|{}| x == {})", arg, pred))
}
_ => None,
};
then {
span_lint_and_sugg(
cx,
NEEDLESS_COLLECT,
method0_span.with_hi(expr.span.hi()),
NEEDLESS_COLLECT_MSG,
"replace with",
sugg,
Applicability::MachineApplicable,
);
let is_empty_sugg = Some("next().is_none()".to_string());
let method_name = &*method.ident.name.as_str();
let sugg = if is_type_diagnostic_item(cx, ty, sym::vec_type) ||
is_type_diagnostic_item(cx, ty, sym::vecdeque_type) ||
is_type_diagnostic_item(cx, ty, sym::LinkedList) ||
is_type_diagnostic_item(cx, ty, sym::BinaryHeap) {
match method_name {
"len" => Some("count()".to_string()),
"is_empty" => is_empty_sugg,
"contains" => {
let contains_arg = snippet(cx, args[1].span, "??");
flip1995 marked this conversation as resolved.
Show resolved Hide resolved
let (arg, pred) = contains_arg
.strip_prefix('&')
.map_or(("&x", &*contains_arg), |s| ("x", s));
Some(format!("any(|{}| x == {})", arg, pred))
}
_ => None,
flip1995 marked this conversation as resolved.
Show resolved Hide resolved
}
}
else if is_type_diagnostic_item(cx, ty, sym::BTreeMap) ||
is_type_diagnostic_item(cx, ty, sym::hashmap_type) {
match method_name {
"is_empty" => is_empty_sugg,
_ => None,
}
}
else {
None
};
if let Some(sugg) = sugg {
span_lint_and_sugg(
cx,
NEEDLESS_COLLECT,
method0_span.with_hi(expr.span.hi()),
NEEDLESS_COLLECT_MSG,
"replace with",
sugg,
Applicability::MachineApplicable,
);
}
}
}
}
Expand Down Expand Up @@ -86,7 +100,7 @@ fn check_needless_collect_indirect_usage<'tcx>(expr: &'tcx Expr<'_>, cx: &LateCo
if is_type_diagnostic_item(cx, ty, sym::vec_type) ||
is_type_diagnostic_item(cx, ty, sym::vecdeque_type) ||
is_type_diagnostic_item(cx, ty, sym::BinaryHeap) ||
match_type(cx, ty, &paths::LINKED_LIST);
is_type_diagnostic_item(cx, ty, sym::LinkedList);
if let Some(iter_calls) = detect_iter_and_into_iters(block, *ident);
if let [iter_call] = &*iter_calls;
then {
Expand Down
19 changes: 17 additions & 2 deletions tests/ui/needless_collect.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

#![allow(unused, clippy::suspicious_map, clippy::iter_count)]

use std::collections::{BTreeSet, HashMap, HashSet};
use std::collections::{BTreeMap, BTreeSet, BinaryHeap, HashMap, HashSet, LinkedList};

#[warn(clippy::needless_collect)]
#[allow(unused_variables, clippy::iter_cloned_collect, clippy::iter_next_slice)]
Expand All @@ -13,9 +13,24 @@ fn main() {
// Empty
}
sample.iter().cloned().any(|x| x == 1);
sample.iter().map(|x| (x, x)).count();
// #7164 HashMap's and BTreeMap's `len` usage should not be linted
sample.iter().map(|x| (x, x)).collect::<HashMap<_, _>>().len();
sample.iter().map(|x| (x, x)).collect::<BTreeMap<_, _>>().len();

sample.iter().map(|x| (x, x)).next().is_none();
sample.iter().map(|x| (x, x)).next().is_none();

// Notice the `HashSet`--this should not be linted
sample.iter().collect::<HashSet<_>>().len();
// Neither should this
sample.iter().collect::<BTreeSet<_>>().len();

sample.iter().count();
sample.iter().next().is_none();
sample.iter().cloned().any(|x| x == 1);
sample.iter().any(|x| x == &1);

// `BinaryHeap` doesn't have `contains` method
sample.iter().count();
sample.iter().next().is_none();
}
17 changes: 16 additions & 1 deletion tests/ui/needless_collect.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

#![allow(unused, clippy::suspicious_map, clippy::iter_count)]

use std::collections::{BTreeSet, HashMap, HashSet};
use std::collections::{BTreeMap, BTreeSet, BinaryHeap, HashMap, HashSet, LinkedList};

#[warn(clippy::needless_collect)]
#[allow(unused_variables, clippy::iter_cloned_collect, clippy::iter_next_slice)]
Expand All @@ -13,9 +13,24 @@ fn main() {
// Empty
}
sample.iter().cloned().collect::<Vec<_>>().contains(&1);
// #7164 HashMap's and BTreeMap's `len` usage should not be linted
sample.iter().map(|x| (x, x)).collect::<HashMap<_, _>>().len();
sample.iter().map(|x| (x, x)).collect::<BTreeMap<_, _>>().len();

sample.iter().map(|x| (x, x)).collect::<HashMap<_, _>>().is_empty();
sample.iter().map(|x| (x, x)).collect::<BTreeMap<_, _>>().is_empty();

// Notice the `HashSet`--this should not be linted
sample.iter().collect::<HashSet<_>>().len();
// Neither should this
sample.iter().collect::<BTreeSet<_>>().len();

sample.iter().collect::<LinkedList<_>>().len();
sample.iter().collect::<LinkedList<_>>().is_empty();
sample.iter().cloned().collect::<LinkedList<_>>().contains(&1);
sample.iter().collect::<LinkedList<_>>().contains(&&1);

// `BinaryHeap` doesn't have `contains` method
sample.iter().collect::<BinaryHeap<_>>().len();
sample.iter().collect::<BinaryHeap<_>>().is_empty();
}
50 changes: 46 additions & 4 deletions tests/ui/needless_collect.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,52 @@ LL | sample.iter().cloned().collect::<Vec<_>>().contains(&1);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `any(|x| x == 1)`

error: avoid using `collect()` when not needed
--> $DIR/needless_collect.rs:16:35
--> $DIR/needless_collect.rs:20:35
|
LL | sample.iter().map(|x| (x, x)).collect::<HashMap<_, _>>().len();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `count()`
LL | sample.iter().map(|x| (x, x)).collect::<HashMap<_, _>>().is_empty();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `next().is_none()`

error: aborting due to 4 previous errors
error: avoid using `collect()` when not needed
--> $DIR/needless_collect.rs:21:35
|
LL | sample.iter().map(|x| (x, x)).collect::<BTreeMap<_, _>>().is_empty();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `next().is_none()`

error: avoid using `collect()` when not needed
--> $DIR/needless_collect.rs:28:19
|
LL | sample.iter().collect::<LinkedList<_>>().len();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `count()`

error: avoid using `collect()` when not needed
--> $DIR/needless_collect.rs:29:19
|
LL | sample.iter().collect::<LinkedList<_>>().is_empty();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `next().is_none()`

error: avoid using `collect()` when not needed
--> $DIR/needless_collect.rs:30:28
|
LL | sample.iter().cloned().collect::<LinkedList<_>>().contains(&1);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `any(|x| x == 1)`

error: avoid using `collect()` when not needed
--> $DIR/needless_collect.rs:31:19
|
LL | sample.iter().collect::<LinkedList<_>>().contains(&&1);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `any(|x| x == &1)`

error: avoid using `collect()` when not needed
--> $DIR/needless_collect.rs:34:19
|
LL | sample.iter().collect::<BinaryHeap<_>>().len();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `count()`

error: avoid using `collect()` when not needed
--> $DIR/needless_collect.rs:35:19
|
LL | sample.iter().collect::<BinaryHeap<_>>().is_empty();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `next().is_none()`

error: aborting due to 11 previous errors