diff --git a/clippy_lints/src/loops/needless_collect.rs b/clippy_lints/src/loops/needless_collect.rs index 9662a0b22a3..d3406780888 100644 --- a/clippy_lints/src/loops/needless_collect.rs +++ b/clippy_lints/src/loops/needless_collect.rs @@ -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}; @@ -28,23 +27,37 @@ 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, @@ -52,7 +65,7 @@ fn check_needless_collect_direct_usage<'tcx>(expr: &'tcx Expr<'_>, cx: &LateCont NEEDLESS_COLLECT_MSG, "replace with", sugg, - Applicability::MachineApplicable, + applicability, ); } } @@ -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 { diff --git a/tests/ui/needless_collect.fixed b/tests/ui/needless_collect.fixed index af6c7bf15ea..6ecbbcb6249 100644 --- a/tests/ui/needless_collect.fixed +++ b/tests/ui/needless_collect.fixed @@ -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)] @@ -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::>().len(); + sample.iter().map(|x| (x, x)).collect::>().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::>().len(); // Neither should this sample.iter().collect::>().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(); } diff --git a/tests/ui/needless_collect.rs b/tests/ui/needless_collect.rs index 6ae14f370b1..8dc69bcf5b3 100644 --- a/tests/ui/needless_collect.rs +++ b/tests/ui/needless_collect.rs @@ -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)] @@ -13,9 +13,24 @@ fn main() { // Empty } sample.iter().cloned().collect::>().contains(&1); + // #7164 HashMap's and BTreeMap's `len` usage should not be linted sample.iter().map(|x| (x, x)).collect::>().len(); + sample.iter().map(|x| (x, x)).collect::>().len(); + + sample.iter().map(|x| (x, x)).collect::>().is_empty(); + sample.iter().map(|x| (x, x)).collect::>().is_empty(); + // Notice the `HashSet`--this should not be linted sample.iter().collect::>().len(); // Neither should this sample.iter().collect::>().len(); + + sample.iter().collect::>().len(); + sample.iter().collect::>().is_empty(); + sample.iter().cloned().collect::>().contains(&1); + sample.iter().collect::>().contains(&&1); + + // `BinaryHeap` doesn't have `contains` method + sample.iter().collect::>().len(); + sample.iter().collect::>().is_empty(); } diff --git a/tests/ui/needless_collect.stderr b/tests/ui/needless_collect.stderr index 2a9539d5975..039091627a8 100644 --- a/tests/ui/needless_collect.stderr +++ b/tests/ui/needless_collect.stderr @@ -19,10 +19,52 @@ LL | sample.iter().cloned().collect::>().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::>().len(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `count()` +LL | sample.iter().map(|x| (x, x)).collect::>().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::>().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::>().len(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `count()` + +error: avoid using `collect()` when not needed + --> $DIR/needless_collect.rs:29:19 + | +LL | sample.iter().collect::>().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::>().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::>().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::>().len(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `count()` + +error: avoid using `collect()` when not needed + --> $DIR/needless_collect.rs:35:19 + | +LL | sample.iter().collect::>().is_empty(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `next().is_none()` + +error: aborting due to 11 previous errors