From a21607d9b504281a00325065955dd825334ad6ef Mon Sep 17 00:00:00 2001 From: Mateusz Gacek <96mateusz.gacek@gmail.com> Date: Wed, 5 May 2021 12:08:24 -0700 Subject: [PATCH 1/4] needless_collect: For `BTreeMap` and `HashMap` lint only `is_empty` - `len` might produce different results than `count` - they don't have `contain` but `contains_key` method --- clippy_lints/src/loops/needless_collect.rs | 64 +++++++++++++--------- tests/ui/needless_collect.fixed | 10 +++- tests/ui/needless_collect.rs | 8 ++- tests/ui/needless_collect.stderr | 14 +++-- 4 files changed, 63 insertions(+), 33 deletions(-) diff --git a/clippy_lints/src/loops/needless_collect.rs b/clippy_lints/src/loops/needless_collect.rs index 9662a0b22a3a..1ed58faa9252 100644 --- a/clippy_lints/src/loops/needless_collect.rs +++ b/clippy_lints/src/loops/needless_collect.rs @@ -10,7 +10,6 @@ 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,32 +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 { - 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) { + match method_name { + "len" => Some("count()".to_string()), + "is_empty" => is_empty_sugg, + "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, + } + } + else if match_type(cx, ty, &paths::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, + ); + } } } } diff --git a/tests/ui/needless_collect.fixed b/tests/ui/needless_collect.fixed index af6c7bf15ea6..d7595569681a 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, HashMap, HashSet}; #[warn(clippy::needless_collect)] #[allow(unused_variables, clippy::iter_cloned_collect, clippy::iter_next_slice)] @@ -13,7 +13,13 @@ 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 diff --git a/tests/ui/needless_collect.rs b/tests/ui/needless_collect.rs index 6ae14f370b14..9883c75b745f 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, HashMap, HashSet}; #[warn(clippy::needless_collect)] #[allow(unused_variables, clippy::iter_cloned_collect, clippy::iter_next_slice)] @@ -13,7 +13,13 @@ 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 diff --git a/tests/ui/needless_collect.stderr b/tests/ui/needless_collect.stderr index 2a9539d59759..3acdf66a42e1 100644 --- a/tests/ui/needless_collect.stderr +++ b/tests/ui/needless_collect.stderr @@ -19,10 +19,16 @@ 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: aborting due to 5 previous errors From 59ccc1efb368a9a3b036510f7b2d56519608da85 Mon Sep 17 00:00:00 2001 From: Mateusz Gacek <96mateusz.gacek@gmail.com> Date: Wed, 5 May 2021 12:16:57 -0700 Subject: [PATCH 2/4] needless_collect: replace paths with diag items Related to: #5393 --- clippy_lints/src/loops/needless_collect.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/clippy_lints/src/loops/needless_collect.rs b/clippy_lints/src/loops/needless_collect.rs index 1ed58faa9252..6a9aa08426c0 100644 --- a/clippy_lints/src/loops/needless_collect.rs +++ b/clippy_lints/src/loops/needless_collect.rs @@ -2,8 +2,8 @@ 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}; @@ -30,7 +30,7 @@ fn check_needless_collect_direct_usage<'tcx>(expr: &'tcx Expr<'_>, cx: &LateCont then { 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) || + let sugg = if is_type_diagnostic_item(cx, ty, sym::vec_type) || is_type_diagnostic_item(cx, ty, sym::vecdeque_type) { match method_name { "len" => Some("count()".to_string()), @@ -45,7 +45,7 @@ fn check_needless_collect_direct_usage<'tcx>(expr: &'tcx Expr<'_>, cx: &LateCont _ => None, } } - else if match_type(cx, ty, &paths::BTREEMAP) || + 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, @@ -98,7 +98,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 { From 171789eb4526fe4ef65514f26056ca4551915cca Mon Sep 17 00:00:00 2001 From: Mateusz Gacek <96mateusz.gacek@gmail.com> Date: Wed, 5 May 2021 12:17:49 -0700 Subject: [PATCH 3/4] needless_collect: Lint `LinkedList` and `BinaryHeap` in direct usage. Those two types are supported already when used indirectly. This commit adds support for direct usage as well. --- clippy_lints/src/loops/needless_collect.rs | 4 ++- tests/ui/needless_collect.fixed | 11 ++++++- tests/ui/needless_collect.rs | 11 ++++++- tests/ui/needless_collect.stderr | 38 +++++++++++++++++++++- 4 files changed, 60 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/loops/needless_collect.rs b/clippy_lints/src/loops/needless_collect.rs index 6a9aa08426c0..89c95f3d127d 100644 --- a/clippy_lints/src/loops/needless_collect.rs +++ b/clippy_lints/src/loops/needless_collect.rs @@ -31,7 +31,9 @@ fn check_needless_collect_direct_usage<'tcx>(expr: &'tcx Expr<'_>, cx: &LateCont 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::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, diff --git a/tests/ui/needless_collect.fixed b/tests/ui/needless_collect.fixed index d7595569681a..6ecbbcb62495 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::{BTreeMap, 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)] @@ -24,4 +24,13 @@ fn main() { 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 9883c75b745f..8dc69bcf5b38 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::{BTreeMap, 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)] @@ -24,4 +24,13 @@ fn main() { 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 3acdf66a42e1..039091627a8d 100644 --- a/tests/ui/needless_collect.stderr +++ b/tests/ui/needless_collect.stderr @@ -30,5 +30,41 @@ error: avoid using `collect()` when not needed LL | sample.iter().map(|x| (x, x)).collect::>().is_empty(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `next().is_none()` -error: aborting due to 5 previous errors +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 From b24929044877b0a3710f40b3cf27b61d79b5d5df Mon Sep 17 00:00:00 2001 From: Mateusz Gacek <96mateusz.gacek@gmail.com> Date: Thu, 13 May 2021 17:09:59 +0200 Subject: [PATCH 4/4] needless_collect: use snippet_with_applicability + small code refactor - using early returns. --- clippy_lints/src/loops/needless_collect.rs | 37 +++++++++++----------- 1 file changed, 18 insertions(+), 19 deletions(-) diff --git a/clippy_lints/src/loops/needless_collect.rs b/clippy_lints/src/loops/needless_collect.rs index 89c95f3d127d..d34067808889 100644 --- a/clippy_lints/src/loops/needless_collect.rs +++ b/clippy_lints/src/loops/needless_collect.rs @@ -1,6 +1,6 @@ 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; use clippy_utils::{is_trait_method, path_to_local_id}; @@ -28,46 +28,45 @@ fn check_needless_collect_direct_usage<'tcx>(expr: &'tcx Expr<'_>, cx: &LateCont 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); then { - let is_empty_sugg = Some("next().is_none()".to_string()); + 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" => Some("count()".to_string()), + "len" => "count()".to_string(), "is_empty" => is_empty_sugg, "contains" => { - let contains_arg = snippet(cx, args[1].span, "??"); + 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)); - Some(format!("any(|{}| x == {})", arg, pred)) + format!("any(|{}| x == {})", arg, pred) } - _ => None, + _ => 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, - _ => None, + _ => return, } } else { - None + return; }; - 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, - ); - } + span_lint_and_sugg( + cx, + NEEDLESS_COLLECT, + method0_span.with_hi(expr.span.hi()), + NEEDLESS_COLLECT_MSG, + "replace with", + sugg, + applicability, + ); } } }