Skip to content

Commit

Permalink
Auto merge of rust-lang#7188 - mgacek8:issue7164_needless_collect_FP,…
Browse files Browse the repository at this point in the history 
… r=xFrednet,flip1995

`needless_collect` enhancements

fixes rust-lang#7164
changelog: `needless_collect`: For `BTreeMap` and `HashMap` lint only `is_empty`, as `len` might produce different results than iter's `count`
changelog: `needless_collect`: Lint `LinkedList` and `BinaryHeap` in direct usage case as well
  • Loading branch information
@bors
bors committed May 13, 2021
2 parents 08ce8bb + b249290 commit 10db5a6
Show file tree
Hide file tree
Showing 4 changed files with 114 additions and 29 deletions.
57 changes: 35 additions & 22 deletions clippy_lints/src/loops/needless_collect.rs
View file
Original file line number Diff line number Diff line change
@@ -1,16 +1,15 @@
use super::NEEDLESS_COLLECT;
use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then};
use clippy_utils::source::snippet;
use clippy_utils::source::{snippet, snippet_with_applicability};
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,31 +27,45 @@ 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 {
let mut applicability = Applicability::MachineApplicable;
let is_empty_sugg = "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" => "count()".to_string(),
"is_empty" => is_empty_sugg,
"contains" => {
let contains_arg = snippet_with_applicability(cx, args[1].span, "??", &mut applicability);
let (arg, pred) = contains_arg
.strip_prefix('&')
.map_or(("&x", &*contains_arg), |s| ("x", s));
format!("any(|{}| x == {})", arg, pred)
}
_ => return,
}
}
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,
_ => return,
}
}
else {
return;
};
span_lint_and_sugg(
cx,
NEEDLESS_COLLECT,
method0_span.with_hi(expr.span.hi()),
NEEDLESS_COLLECT_MSG,
"replace with",
sugg,
Applicability::MachineApplicable,
applicability,
);
}
}
Expand Down Expand Up @@ -86,7 +99,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
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
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
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

0 comments on commit 10db5a6

Please sign in to comment.