Lint against needless uses of collect()

Handles cases of `.collect().len()`, `.collect().is_empty()`, and
`.collect().contains()`. This lint is intended to be generic enough to
be added to at a later time with other similar patterns that could be
optimized.

Closes #3034

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

@@ -223,6 +223,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 +421,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 +545,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 +2265,67 @@ impl<'a, 'tcx> Visitor<'tcx> for VarCollectorVisitor<'a, 'tcx> {
         NestedVisitorMap::None
     }
 }
+
+fn check_needless_collect<'a, 'tcx>(expr: &'tcx Expr, cx: &LateContext<'a, 'tcx>) {
+    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 method.ident.name == "len" {
+                    span_lint_and_sugg(
+                        cx,
+                        NEEDLESS_COLLECT,
+                        expr.span,
+                        "you are collecting an iterator to check its length",
+                        "consider replacing with",
+                        generate_needless_collect_len_sugg(&args[0], cx),
+                    );
+                }
+                if method.ident.name == "is_empty" {
+                    span_lint_and_sugg(
+                        cx,
+                        NEEDLESS_COLLECT,
+                        expr.span,
+                        "you are collecting an iterator to check if it is empty",
+                        "consider replacing with",
+                        generate_needless_collect_is_empty_sugg(&args[0], cx),
+                    );
+                }
+                if method.ident.name == "contains" {
+                    span_lint_and_sugg(
+                        cx,
+                        NEEDLESS_COLLECT,
+                        expr.span,
+                        "you are collecting an iterator to check if contains an element",
+                        "consider replacing with",
+                        generate_needless_collect_contains_sugg(&args[0], &args[1], cx),
+                    );
+                }
+            }
+        }
+    }
+}
+
+fn generate_needless_collect_len_sugg<'a, 'tcx>(collect_expr: &'tcx Expr, cx: &LateContext<'a, 'tcx>) -> String {
+    if let ExprKind::MethodCall(_, _, ref args) = collect_expr.node {
+        let iter = snippet(cx, args[0].span, "??");
+        return format!("{}.count()", iter);
+    }
+    unreachable!();
+}
+
+fn generate_needless_collect_is_empty_sugg<'a, 'tcx>(collect_expr: &'tcx Expr, cx: &LateContext<'a, 'tcx>) -> String {
+    if let ExprKind::MethodCall(_, _, ref args) = collect_expr.node {
+        let iter = snippet(cx, args[0].span, "??");
+        return format!("{}.any(|_| true)", iter);
+    }
+    unreachable!();
+}
+
+fn generate_needless_collect_contains_sugg<'a, 'tcx>(collect_expr: &'tcx Expr, contains_arg: &'tcx Expr, cx: &LateContext<'a, 'tcx>) -> String {
+    if let ExprKind::MethodCall(_, _, ref args) = collect_expr.node {
+        let iter = snippet(cx, args[0].span, "??");
+        let arg = snippet(cx, contains_arg.span, "??");
+        return format!("{}.any(|&x| x == {})", iter, if arg.starts_with('&') { &arg[1..] } else { &arg });
+    }
+    unreachable!();
+}

tests/ui/needless_collect.rs

@@ -0,0 +1,10 @@
+#[warn(clippy, needless_collect)]
+#[allow(unused_variables, 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);
+}

tests/ui/needless_collect.stderr

@@ -0,0 +1,22 @@
+error: you are collecting an iterator to check its length
+ --> $DIR/needless_collect.rs:5:15
+  |
+5 |     let len = sample.iter().collect::<Vec<_>>().len();
+  |               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider replacing with: `sample.iter().count()`
+  |
+  = note: `-D needless-collect` implied by `-D warnings`
+
+error: you are collecting an iterator to check if it is empty
+ --> $DIR/needless_collect.rs:6:8
+  |
+6 |     if sample.iter().collect::<Vec<_>>().is_empty() {
+  |        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider replacing with: `sample.iter().any(|_| true)`
+
+error: you are collecting an iterator to check if contains an element
+ --> $DIR/needless_collect.rs:9:5
+  |
+9 |     sample.iter().cloned().collect::<Vec<_>>().contains(&1);
+  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider replacing with: `sample.iter().cloned().any(|&x| x == 1)`
+
+error: aborting due to 3 previous errors
+