Rollup merge of rust-lang#5837 - JarredAllen:needless_collect, r=phansch

flip1995 · web-flow · commit ca2a25d966de · 2020-08-04T12:06:38.000+02:00
needless_collect: catch x: Vec<_> = iter.collect(); x.into_iter() ... changelog: Expand the needless_collect lint as suggested in rust-lang#5627 (WIP). This PR is WIP because I can't figure out how to make the multi-part suggestion include its changes in the source code (the fixed is identical to the source, despite the lint making suggestions). Aside from that one issue, I think this should be good.
diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs
@@ -1,14 +1,14 @@
 use crate::consts::constant;
 use crate::reexport::Name;
 use crate::utils::paths;
+use crate::utils::sugg::Sugg;
 use crate::utils::usage::{is_unused, mutated_variables};
 use crate::utils::{
     get_enclosing_block, get_parent_expr, get_trait_def_id, has_iter_method, higher, implements_trait,
-    is_integer_const, is_no_std_crate, is_refutable, last_path_segment, match_trait_method, match_type, match_var,
-    multispan_sugg, snippet, snippet_opt, snippet_with_applicability, span_lint, span_lint_and_help,
-    span_lint_and_sugg, span_lint_and_then, SpanlessEq,
+    is_integer_const, is_no_std_crate, is_refutable, is_type_diagnostic_item, last_path_segment, match_trait_method,
+    match_type, match_var, multispan_sugg, qpath_res, snippet, snippet_opt, snippet_with_applicability, span_lint,
+    span_lint_and_help, span_lint_and_sugg, span_lint_and_then, sugg, SpanlessEq,
 };
-use crate::utils::{is_type_diagnostic_item, qpath_res, sugg};
 use if_chain::if_chain;
 use rustc_ast::ast;
 use rustc_data_structures::fx::{FxHashMap, FxHashSet};
@@ -17,7 +17,7 @@ use rustc_hir::def::{DefKind, Res};
 use rustc_hir::intravisit::{walk_block, walk_expr, walk_pat, walk_stmt, NestedVisitorMap, Visitor};
 use rustc_hir::{
     def_id, BinOpKind, BindingAnnotation, Block, BorrowKind, Expr, ExprKind, GenericArg, HirId, InlineAsmOperand,
-    LoopSource, MatchSource, Mutability, Node, Pat, PatKind, QPath, Stmt, StmtKind,
+    Local, LoopSource, MatchSource, Mutability, Node, Pat, PatKind, QPath, Stmt, StmtKind,
 };
 use rustc_infer::infer::TyCtxtInferExt;
 use rustc_lint::{LateContext, LateLintPass, LintContext};
@@ -27,7 +27,7 @@ use rustc_middle::middle::region;
 use rustc_middle::ty::{self, Ty, TyS};
 use rustc_session::{declare_lint_pass, declare_tool_lint};
 use rustc_span::source_map::Span;
-use rustc_span::symbol::Symbol;
+use rustc_span::symbol::{sym, Ident, Symbol};
 use rustc_typeck::expr_use_visitor::{ConsumeMode, Delegate, ExprUseVisitor, PlaceBase, PlaceWithHirId};
 use std::iter::{once, Iterator};
 use std::mem;
@@ -2358,6 +2358,10 @@ impl<'a, 'tcx> Visitor<'tcx> for VarCollectorVisitor<'a, 'tcx> {
 const NEEDLESS_COLLECT_MSG: &str = "avoid using `collect()` when not needed";
 
 fn check_needless_collect<'tcx>(expr: &'tcx Expr<'_>, cx: &LateContext<'tcx>) {
+    check_needless_collect_direct_usage(expr, cx);
+    check_needless_collect_indirect_usage(expr, cx);
+}
+fn check_needless_collect_direct_usage<'tcx>(expr: &'tcx Expr<'_>, cx: &LateContext<'tcx>) {
     if_chain! {
         if let ExprKind::MethodCall(ref method, _, ref args, _) = expr.kind;
         if let ExprKind::MethodCall(ref chain_method, _, _, _) = args[0].kind;
@@ -2425,6 +2429,157 @@ fn check_needless_collect<'tcx>(expr: &'tcx Expr<'_>, cx: &LateContext<'tcx>) {
     }
 }
 
+fn check_needless_collect_indirect_usage<'tcx>(expr: &'tcx Expr<'_>, cx: &LateContext<'tcx>) {
+    if let ExprKind::Block(ref block, _) = expr.kind {
+        for ref stmt in block.stmts {
+            if_chain! {
+                if let StmtKind::Local(
+                    Local { pat: Pat { kind: PatKind::Binding(_, _, ident, .. ), .. },
+                    init: Some(ref init_expr), .. }
+                ) = stmt.kind;
+                if let ExprKind::MethodCall(ref method_name, _, &[ref iter_source], ..) = init_expr.kind;
+                if method_name.ident.name == sym!(collect) && match_trait_method(cx, &init_expr, &paths::ITERATOR);
+                if let Some(ref generic_args) = method_name.args;
+                if let Some(GenericArg::Type(ref ty)) = generic_args.args.get(0);
+                if let ty = cx.typeck_results().node_type(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::LINKED_LIST);
+                if let Some(iter_calls) = detect_iter_and_into_iters(block, *ident);
+                if iter_calls.len() == 1;
+                then {
+                    // Suggest replacing iter_call with iter_replacement, and removing stmt
+                    let iter_call = &iter_calls[0];
+                    span_lint_and_then(
+                        cx,
+                        NEEDLESS_COLLECT,
+                        stmt.span.until(iter_call.span),
+                        NEEDLESS_COLLECT_MSG,
+                        |diag| {
+                            let iter_replacement = format!("{}{}", Sugg::hir(cx, iter_source, ".."), iter_call.get_iter_method(cx));
+                            diag.multipart_suggestion(
+                                iter_call.get_suggestion_text(),
+                                vec![
+                                    (stmt.span, String::new()),
+                                    (iter_call.span, iter_replacement)
+                                ],
+                                Applicability::MachineApplicable,// MaybeIncorrect,
+                            ).emit();
+                        },
+                    );
+                }
+            }
+        }
+    }
+}
+
+struct IterFunction {
+    func: IterFunctionKind,
+    span: Span,
+}
+impl IterFunction {
+    fn get_iter_method(&self, cx: &LateContext<'_>) -> String {
+        match &self.func {
+            IterFunctionKind::IntoIter => String::new(),
+            IterFunctionKind::Len => String::from(".count()"),
+            IterFunctionKind::IsEmpty => String::from(".next().is_none()"),
+            IterFunctionKind::Contains(span) => format!(".any(|x| x == {})", snippet(cx, *span, "..")),
+        }
+    }
+    fn get_suggestion_text(&self) -> &'static str {
+        match &self.func {
+            IterFunctionKind::IntoIter => {
+                "Use the original Iterator instead of collecting it and then producing a new one"
+            },
+            IterFunctionKind::Len => {
+                "Take the original Iterator's count instead of collecting it and finding the length"
+            },
+            IterFunctionKind::IsEmpty => {
+                "Check if the original Iterator has anything instead of collecting it and seeing if it's empty"
+            },
+            IterFunctionKind::Contains(_) => {
+                "Check if the original Iterator contains an element instead of collecting then checking"
+            },
+        }
+    }
+}
+enum IterFunctionKind {
+    IntoIter,
+    Len,
+    IsEmpty,
+    Contains(Span),
+}
+
+struct IterFunctionVisitor {
+    uses: Vec<IterFunction>,
+    seen_other: bool,
+    target: Ident,
+}
+impl<'tcx> Visitor<'tcx> for IterFunctionVisitor {
+    fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) {
+        // Check function calls on our collection
+        if_chain! {
+            if let ExprKind::MethodCall(method_name, _, ref args, _) = &expr.kind;
+            if let Some(Expr { kind: ExprKind::Path(QPath::Resolved(_, ref path)), .. }) = args.get(0);
+            if let &[name] = &path.segments;
+            if name.ident == self.target;
+            then {
+                let len = sym!(len);
+                let is_empty = sym!(is_empty);
+                let contains = sym!(contains);
+                match method_name.ident.name {
+                    sym::into_iter => self.uses.push(
+                        IterFunction { func: IterFunctionKind::IntoIter, span: expr.span }
+                    ),
+                    name if name == len => self.uses.push(
+                        IterFunction { func: IterFunctionKind::Len, span: expr.span }
+                    ),
+                    name if name == is_empty => self.uses.push(
+                        IterFunction { func: IterFunctionKind::IsEmpty, span: expr.span }
+                    ),
+                    name if name == contains => self.uses.push(
+                        IterFunction { func: IterFunctionKind::Contains(args[1].span), span: expr.span }
+                    ),
+                    _ => self.seen_other = true,
+                }
+                return
+            }
+        }
+        // Check if the collection is used for anything else
+        if_chain! {
+            if let Expr { kind: ExprKind::Path(QPath::Resolved(_, ref path)), .. } = expr;
+            if let &[name] = &path.segments;
+            if name.ident == self.target;
+            then {
+                self.seen_other = true;
+            } else {
+                walk_expr(self, expr);
+            }
+        }
+    }
+
+    type Map = Map<'tcx>;
+    fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> {
+        NestedVisitorMap::None
+    }
+}
+
+/// Detect the occurences of calls to `iter` or `into_iter` for the
+/// given identifier
+fn detect_iter_and_into_iters<'tcx>(block: &'tcx Block<'tcx>, identifier: Ident) -> Option<Vec<IterFunction>> {
+    let mut visitor = IterFunctionVisitor {
+        uses: Vec::new(),
+        target: identifier,
+        seen_other: false,
+    };
+    visitor.visit_block(block);
+    if visitor.seen_other {
+        None
+    } else {
+        Some(visitor.uses)
+    }
+}
+
 fn shorten_span(expr: &Expr<'_>, target_fn_name: Symbol) -> Span {
     let mut current_expr = expr;
     while let ExprKind::MethodCall(ref path, ref span, ref args, _) = current_expr.kind {
diff --git a/tests/ui/needless_collect_indirect.rs b/tests/ui/needless_collect_indirect.rs
@@ -0,0 +1,19 @@
+use std::collections::{HashMap, VecDeque};
+
+fn main() {
+    let sample = [1; 5];
+    let indirect_iter = sample.iter().collect::<Vec<_>>();
+    indirect_iter.into_iter().map(|x| (x, x + 1)).collect::<HashMap<_, _>>();
+    let indirect_len = sample.iter().collect::<VecDeque<_>>();
+    indirect_len.len();
+    let indirect_empty = sample.iter().collect::<VecDeque<_>>();
+    indirect_empty.is_empty();
+    let indirect_contains = sample.iter().collect::<VecDeque<_>>();
+    indirect_contains.contains(&&5);
+    let indirect_negative = sample.iter().collect::<Vec<_>>();
+    indirect_negative.len();
+    indirect_negative
+        .into_iter()
+        .map(|x| (*x, *x + 1))
+        .collect::<HashMap<_, _>>();
+}
diff --git a/tests/ui/needless_collect_indirect.stderr b/tests/ui/needless_collect_indirect.stderr
@@ -0,0 +1,55 @@
+error: avoid using `collect()` when not needed
+  --> $DIR/needless_collect_indirect.rs:5:5
+   |
+LL | /     let indirect_iter = sample.iter().collect::<Vec<_>>();
+LL | |     indirect_iter.into_iter().map(|x| (x, x + 1)).collect::<HashMap<_, _>>();
+   | |____^
+   |
+   = note: `-D clippy::needless-collect` implied by `-D warnings`
+help: Use the original Iterator instead of collecting it and then producing a new one
+   |
+LL |     
+LL |     sample.iter().map(|x| (x, x + 1)).collect::<HashMap<_, _>>();
+   |
+
+error: avoid using `collect()` when not needed
+  --> $DIR/needless_collect_indirect.rs:7:5
+   |
+LL | /     let indirect_len = sample.iter().collect::<VecDeque<_>>();
+LL | |     indirect_len.len();
+   | |____^
+   |
+help: Take the original Iterator's count instead of collecting it and finding the length
+   |
+LL |     
+LL |     sample.iter().count();
+   |
+
+error: avoid using `collect()` when not needed
+  --> $DIR/needless_collect_indirect.rs:9:5
+   |
+LL | /     let indirect_empty = sample.iter().collect::<VecDeque<_>>();
+LL | |     indirect_empty.is_empty();
+   | |____^
+   |
+help: Check if the original Iterator has anything instead of collecting it and seeing if it's empty
+   |
+LL |     
+LL |     sample.iter().next().is_none();
+   |
+
+error: avoid using `collect()` when not needed
+  --> $DIR/needless_collect_indirect.rs:11:5
+   |
+LL | /     let indirect_contains = sample.iter().collect::<VecDeque<_>>();
+LL | |     indirect_contains.contains(&&5);
+   | |____^
+   |
+help: Check if the original Iterator contains an element instead of collecting then checking
+   |
+LL |     
+LL |     sample.iter().any(|x| x == &&5);
+   |
+
+error: aborting due to 4 previous errors
+
