Auto merge of #7188 - mgacek8:issue7164_needless_collect_FP, r=xFrednet,flip1995

`needless_collect` enhancements

fixes #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

4 files changed, 114 insertions, 29 deletions

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::<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();
 }
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::<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();
 }
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::<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