Merge pull request #3109 from shssoichiro/3034-needless-collect

Lint against needless uses of `collect()`

Author: oli-obk

clippy_lints/src/lib.rs

@@ -554,6 +554,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) {
         loops::ITER_NEXT_LOOP,
         loops::MANUAL_MEMCPY,
         loops::MUT_RANGE_BOUND,
+        loops::NEEDLESS_COLLECT,
         loops::NEEDLESS_RANGE_LOOP,
         loops::NEVER_LOOP,
         loops::REVERSE_RANGE_LOOP,
@@ -904,6 +905,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) {
         escape::BOXED_LOCAL,
         large_enum_variant::LARGE_ENUM_VARIANT,
         loops::MANUAL_MEMCPY,
+        loops::NEEDLESS_COLLECT,
         loops::UNUSED_COLLECT,
         methods::EXPECT_FUN_CALL,
         methods::ITER_NTH,

clippy_lints/src/loops.rs

@@ -14,10 +14,12 @@ use rustc::middle::mem_categorization::Categorization;
 use rustc::middle::mem_categorization::cmt_;
 use rustc::ty::{self, Ty};
 use rustc::ty::subst::Subst;
+use rustc_errors::Applicability;
 use std::collections::{HashMap, HashSet};
 use std::iter::{once, Iterator};
 use syntax::ast;
 use syntax::source_map::Span;
+use syntax_pos::BytePos;
 use crate::utils::{sugg, sext};
 use crate::utils::usage::mutated_variables;
 use crate::consts::{constant, Constant};
@@ -223,6 +225,27 @@ declare_clippy_lint! {
      written as a for loop"
 }
 
+/// **What it does:** Checks for functions collecting an iterator when collect
+/// is not needed.
+///
+/// **Why is this bad?** `collect` causes the allocation of a new data structure,
+/// when this allocation may not be needed.
+///
+/// **Known problems:**
+/// None
+///
+/// **Example:**
+/// ```rust
+/// let len = iterator.collect::<Vec<_>>().len();
+/// // should be
+/// let len = iterator.count();
+/// ```
+declare_clippy_lint! {
+    pub NEEDLESS_COLLECT,
+    perf,
+    "collecting an iterator when collect is not needed"
+}
+
 /// **What it does:** Checks for loops over ranges `x..y` where both `x` and `y`
 /// are constant and `x` is greater or equal to `y`, unless the range is
 /// reversed or has a negative `.step_by(_)`.
@@ -400,6 +423,7 @@ impl LintPass for Pass {
             FOR_LOOP_OVER_OPTION,
             WHILE_LET_LOOP,
             UNUSED_COLLECT,
+            NEEDLESS_COLLECT,
             REVERSE_RANGE_LOOP,
             EXPLICIT_COUNTER_LOOP,
             EMPTY_LOOP,
@@ -523,6 +547,8 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass {
         if let ExprKind::While(ref cond, _, _) = expr.node {
             check_infinite_loop(cx, cond, expr);
         }
+
+        check_needless_collect(expr, cx);
     }
 
     fn check_stmt(&mut self, cx: &LateContext<'a, 'tcx>, stmt: &'tcx Stmt) {
@@ -2241,3 +2267,71 @@ impl<'a, 'tcx> Visitor<'tcx> for VarCollectorVisitor<'a, 'tcx> {
         NestedVisitorMap::None
     }
 }
+
+const NEEDLESS_COLLECT_MSG: &str = "avoid using `collect()` when not needed";
+
+fn check_needless_collect<'a, 'tcx>(expr: &'tcx Expr, cx: &LateContext<'a, 'tcx>) {
+    if_chain! {
+        if let ExprKind::MethodCall(ref method, _, ref args) = expr.node;
+        if let ExprKind::MethodCall(ref chain_method, _, _) = args[0].node;
+        if chain_method.ident.name == "collect" && match_trait_method(cx, &args[0], &paths::ITERATOR);
+        if let Some(ref generic_args) = chain_method.args;
+        if let Some(GenericArg::Type(ref ty)) = generic_args.args.get(0);
+        then {
+            let ty = cx.tables.node_id_to_type(ty.hir_id);
+            if match_type(cx, ty, &paths::VEC) ||
+                match_type(cx, ty, &paths::VEC_DEQUE) ||
+                match_type(cx, ty, &paths::BTREEMAP) ||
+                match_type(cx, ty, &paths::HASHMAP) {
+                if method.ident.name == "len" {
+                    let span = shorten_needless_collect_span(expr);
+                    span_lint_and_then(cx, NEEDLESS_COLLECT, span, NEEDLESS_COLLECT_MSG, |db| {
+                        db.span_suggestion_with_applicability(
+                            span,
+                            "replace with",
+                            ".count()".to_string(),
+                            Applicability::MachineApplicable,
+                        );
+                    });
+                }
+                if method.ident.name == "is_empty" {
+                    let span = shorten_needless_collect_span(expr);
+                    span_lint_and_then(cx, NEEDLESS_COLLECT, span, NEEDLESS_COLLECT_MSG, |db| {
+                        db.span_suggestion_with_applicability(
+                            span,
+                            "replace with",
+                            ".next().is_none()".to_string(),
+                            Applicability::MachineApplicable,
+                        );
+                    });
+                }
+                if method.ident.name == "contains" {
+                    let contains_arg = snippet(cx, args[1].span, "??");
+                    let span = shorten_needless_collect_span(expr);
+                    span_lint_and_then(cx, NEEDLESS_COLLECT, span, NEEDLESS_COLLECT_MSG, |db| {
+                        db.span_suggestion_with_applicability(
+                            span,
+                            "replace with",
+                            format!(
+                                ".any(|&x| x == {})",
+                                if contains_arg.starts_with('&') { &contains_arg[1..] } else { &contains_arg }
+                            ),
+                            Applicability::MachineApplicable,
+                        );
+                    });
+                }
+            }
+        }
+    }
+}
+
+fn shorten_needless_collect_span(expr: &Expr) -> Span {
+    if_chain! {
+        if let ExprKind::MethodCall(_, _, ref args) = expr.node;
+        if let ExprKind::MethodCall(_, ref span, _) = args[0].node;
+        then {
+            return expr.span.with_lo(span.lo() - BytePos(1));
+        }
+    }
+    unreachable!()
+}

tests/ui/needless_collect.rs

@@ -0,0 +1,19 @@
+#![feature(tool_lints)]
+
+use std::collections::{HashMap, HashSet, BTreeSet};
+
+#[warn(clippy::needless_collect)]
+#[allow(unused_variables, clippy::iter_cloned_collect)]
+fn main() {
+    let sample = [1; 5];
+    let len = sample.iter().collect::<Vec<_>>().len();
+    if sample.iter().collect::<Vec<_>>().is_empty() {
+        // Empty
+    }
+    sample.iter().cloned().collect::<Vec<_>>().contains(&1);
+    sample.iter().map(|x| (x, x)).collect::<HashMap<_, _>>().len();
+    // Notice the `HashSet`--this should not be linted
+    sample.iter().collect::<HashSet<_>>().len();
+    // Neither should this
+    sample.iter().collect::<BTreeSet<_>>().len();
+}

tests/ui/needless_collect.stderr

@@ -0,0 +1,28 @@
+error: avoid using `collect()` when not needed
+ --> $DIR/needless_collect.rs:9:28
+  |
+9 |     let len = sample.iter().collect::<Vec<_>>().len();
+  |                            ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `.count()`
+  |
+  = note: `-D clippy::needless-collect` implied by `-D warnings`
+
+error: avoid using `collect()` when not needed
+  --> $DIR/needless_collect.rs:10:21
+   |
+10 |     if sample.iter().collect::<Vec<_>>().is_empty() {
+   |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `.next().is_none()`
+
+error: avoid using `collect()` when not needed
+  --> $DIR/needless_collect.rs:13:27
+   |
+13 |     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:14:34
+   |
+14 |     sample.iter().map(|x| (x, x)).collect::<HashMap<_, _>>().len();
+   |                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `.count()`
+
+error: aborting due to 4 previous errors
+