Initial implementation of possible shortcircuiting collect lint

clippy_lints/src/collect.rs

@@ -0,0 +1,219 @@
+// Copyright 2014-2018 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+use itertools::{repeat_n, Itertools};
+use rustc::hir::{Expr, Stmt, DeclKind, StmtKind, ExprKind};
+use rustc::ty::{AssociatedKind};
+use syntax::ast::NodeId;
+
+use crate::rustc_data_structures::fx::FxHashSet;
+
+use crate::rustc_errors::Applicability;
+use crate::rustc::lint::{
+    LateContext, LateLintPass, LintArray, LintPass,
+};
+use crate::rustc::{declare_tool_lint, lint_array, ty};
+use crate::utils::{match_trait_method, match_type, span_lint_and_sugg};
+use crate::utils::paths;
+
+use if_chain::if_chain;
+
+/// **What it does:** Detects collect calls on iterators to collections
+/// of either `Result<_, E>` or `Option<_>` inside functions that also
+/// have such a return type.
+///
+/// **Why is this bad?** It is possible to short-circuit these collect
+/// calls and return early whenever a `None` or `Err(E)` is encountered.
+///
+/// **Known problems:** It may be possible that a collection of options
+/// or results is intended. This would then generate a false positive.
+///
+/// **Example:**
+/// ```rust
+/// pub fn div(a: i32, b: &[i32]) -> Result<Vec<i32>, String> {
+///     let option_vec: Vec<_> = b.into_iter()
+///         .cloned()
+///         .map(|i| if i != 0 {
+///             Ok(a / i)
+///         } else {
+///             Err("Division by zero!".to_owned())
+///         })
+///         .collect();
+///     let mut int_vec = Vec::new();
+///     for opt in option_vec {
+///         int_vec.push(opt?);
+///     }
+///     Ok(int_vec)
+/// }
+/// ```
+declare_clippy_lint! {
+    pub POSSIBLE_SHORTCIRCUITING_COLLECT,
+    nursery,
+    "missed shortcircuit opportunity on collect"
+}
+
+#[derive(Clone, Default)]
+pub struct Pass {
+    // To ensure that we do not lint the same expression more than once
+    seen_expr_nodes: FxHashSet<NodeId>,
+}
+
+impl Pass {
+    pub fn new() -> Self {
+        Self { seen_expr_nodes: FxHashSet::default() }
+    }
+}
+
+impl LintPass for Pass {
+    fn get_lints(&self) -> LintArray {
+        lint_array!(POSSIBLE_SHORTCIRCUITING_COLLECT)
+    }
+}
+
+struct Suggestion {
+    pattern: String,
+    type_colloquial: &'static str,
+    success_variant: &'static str,
+}
+
+fn format_suggestion_pattern<'a, 'tcx>(
+    cx: &LateContext<'a, 'tcx>,
+    collection_ty: ty::Ty<'_>,
+    is_option: bool,
+) -> String {
+    let collection_pat = match collection_ty.sty {
+        ty::Adt(def, subs) => {
+            let mut buf = cx.tcx.item_path_str(def.did);
+
+            if !subs.is_empty() {
+                buf.push('<');
+                buf.push_str(&repeat_n('_', subs.len()).join(", "));
+                buf.push('>');
+            }
+
+            buf
+        },
+        ty::Param(p) => p.to_string(),
+        _ => "_".into(),
+    };
+
+    if is_option {
+        format!("Option<{}>", collection_pat)
+    } else {
+        format!("Result<{}, _>", collection_pat)
+    }
+}
+
+fn check_expr_for_collect<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) -> Option<Suggestion> {
+    if let ExprKind::MethodCall(ref method, _, ref args) = expr.node {
+        if args.len() == 1 && method.ident.name == "collect" && match_trait_method(cx, expr, &paths::ITERATOR) {
+            let collect_ty = cx.tables.expr_ty(expr);
+
+            if match_type(cx, collect_ty, &paths::OPTION) || match_type(cx, collect_ty, &paths::RESULT) {
+                // Already collecting into an Option or Result - good!
+                return None;
+            }
+
+            // Get the type of the Item associated to the Iterator on which collect() is
+            // called.
+            let arg_ty = cx.tables.expr_ty(&args[0]);
+            let ty_defs = cx.tables.type_dependent_defs();
+            if_chain! {
+                if let Some(method_call) = ty_defs.get(args[0].hir_id);
+                if let Some(trt_id) = cx.tcx.trait_of_item(method_call.def_id());
+                if let Some(assoc_item) = cx.tcx.associated_items(trt_id).next();
+                if assoc_item.kind == AssociatedKind::Type;
+                then {
+                    let assoc_item_id = assoc_item.def_id;
+                    let substitutions = cx.tcx.mk_substs_trait(arg_ty, &[]);
+                    let projection = cx.tcx.mk_projection(assoc_item_id, substitutions);
+                    let normal_ty = cx.tcx.normalize_erasing_regions(
+                        cx.param_env,
+                        projection,
+                    );
+
+                    return if match_type(cx, normal_ty, &paths::OPTION) {
+                        Some(Suggestion {
+                            pattern: format_suggestion_pattern(cx, &collect_ty, true),
+                            type_colloquial: "Option",
+                            success_variant: "Some",
+                        })
+                    } else if match_type(cx, normal_ty, &paths::RESULT) {
+                        Some(Suggestion {
+                            pattern: format_suggestion_pattern(cx, &collect_ty, false),
+                            type_colloquial: "Result",
+                            success_variant: "Ok",
+                        })
+                    } else {
+                        None
+                    };
+                }
+            };
+        }
+    }
+
+    None
+}
+
+impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass {
+    fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) {
+        if self.seen_expr_nodes.contains(&expr.id) {
+            return;
+        }
+
+        if let Some(suggestion) = check_expr_for_collect(cx, expr) {
+            let sugg_span = if let ExprKind::MethodCall(_, call_span, _) = expr.node {
+                expr.span.between(call_span)
+            } else {
+                unreachable!()
+            };
+
+            span_lint_and_sugg(
+                cx,
+                POSSIBLE_SHORTCIRCUITING_COLLECT,
+                sugg_span,
+                &format!("you are creating a collection of `{}`s", suggestion.type_colloquial),
+                &format!(
+                    "if you are only interested in the case where all values are `{}`, try",
+                    suggestion.success_variant
+                ),
+                format!("collect::<{}>()", suggestion.pattern),
+                Applicability::MaybeIncorrect
+            );
+        }
+    }
+
+    fn check_stmt(&mut self, cx: &LateContext<'a, 'tcx>, stmt: &'tcx Stmt) {
+        if_chain! {
+            if let StmtKind::Decl(ref decl, _) = stmt.node;
+            if let DeclKind::Local(ref local) = decl.node;
+            if let Some(ref ty) = local.ty;
+            if let Some(ref expr) = local.init;
+            then {
+                self.seen_expr_nodes.insert(expr.id);
+
+                if let Some(suggestion) = check_expr_for_collect(cx, expr) {
+                    span_lint_and_sugg(
+                        cx,
+                        POSSIBLE_SHORTCIRCUITING_COLLECT,
+                        ty.span,
+                        &format!("you are creating a collection of `{}`s", suggestion.type_colloquial),
+                        &format!(
+                            "if you are only interested in the case where all values are `{}`, try",
+                            suggestion.success_variant
+                        ),
+                        suggestion.pattern,
+                        Applicability::MaybeIncorrect
+                    );
+                }
+            }
+        }
+    }
+}

clippy_lints/src/lib.rs

@@ -156,6 +156,7 @@ pub mod cargo_common_metadata;
 pub mod cognitive_complexity;
 pub mod collapsible_if;
 pub mod const_static_lifetime;
+pub mod collect;
 pub mod copies;
 pub mod copy_iterator;
 pub mod dbg_macro;
@@ -568,6 +569,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) {
     reg.register_late_lint_pass(box ptr_offset_with_cast::Pass);
     reg.register_late_lint_pass(box redundant_clone::RedundantClone);
     reg.register_late_lint_pass(box slow_vector_initialization::Pass);
+    reg.register_late_lint_pass(box collect::Pass::new());
     reg.register_late_lint_pass(box types::RefToMut);
     reg.register_late_lint_pass(box assertions_on_constants::AssertionsOnConstants);
     reg.register_late_lint_pass(box missing_const_for_fn::MissingConstForFn);
@@ -1117,6 +1119,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) {
 
     reg.register_lint_group("clippy::nursery", Some("clippy_nursery"), vec![
         attrs::EMPTY_LINE_AFTER_OUTER_ATTR,
+        collect::POSSIBLE_SHORTCIRCUITING_COLLECT,
         fallible_impl_from::FALLIBLE_IMPL_FROM,
         missing_const_for_fn::MISSING_CONST_FOR_FN,
         mutex_atomic::MUTEX_INTEGER,

tests/ui/collect.rs

@@ -0,0 +1,43 @@
+// Copyright 2014-2018 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+#![warn(clippy::possible_shortcircuiting_collect)]
+
+use std::iter::FromIterator;
+
+pub fn div(a: i32, b: &[i32]) -> Result<Vec<i32>, String> {
+    let option_vec: Vec<_> = b.iter()
+        .cloned()
+        .map(|i| if i != 0 {
+            Ok(a / i)
+        } else {
+            Err("Division by zero!".to_owned())
+        })
+        .collect();
+    let mut int_vec = Vec::new();
+    for opt in option_vec {
+        int_vec.push(opt?);
+    }
+    Ok(int_vec)
+}
+
+pub fn generic<T>(a: &[T]) {
+    // Make sure that our lint also works for generic functions.
+    let _result: Vec<_> = a.iter().map(Some).collect();
+}
+
+pub fn generic_collection<T, C: FromIterator<T> + FromIterator<Option<T>>>(elem: T) -> C {
+    Some(Some(elem)).into_iter().collect()
+}
+
+fn main() {
+    // We're collecting into an `Option`. Do not trigger lint.
+    let _sup: Option<Vec<_>> = (0..5).map(Some).collect();
+}

tests/ui/collect.stderr

@@ -0,0 +1,34 @@
+error: you are creating a collection of `Result`s
+  --> $DIR/collect.rs:16:21
+   |
+LL |     let option_vec: Vec<_> = b.iter()
+   |                     ^^^^^^
+   |
+   = note: `-D clippy::possible-shortcircuiting-collect` implied by `-D warnings`
+help: if you are only interested in the case where all values are `Ok`, try
+   |
+LL |     let option_vec: Result<std::vec::Vec<_>, _> = b.iter()
+   |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: you are creating a collection of `Option`s
+  --> $DIR/collect.rs:33:18
+   |
+LL |     let _result: Vec<_> = a.iter().map(Some).collect();
+   |                  ^^^^^^
+help: if you are only interested in the case where all values are `Some`, try
+   |
+LL |     let _result: Option<std::vec::Vec<_>> = a.iter().map(Some).collect();
+   |                  ^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: you are creating a collection of `Option`s
+  --> $DIR/collect.rs:37:34
+   |
+LL |     Some(Some(elem)).into_iter().collect()
+   |                                  ^^^^^^^^^
+help: if you are only interested in the case where all values are `Some`, try
+   |
+LL |     Some(Some(elem)).into_iter().collect::<Option<C>>()
+   |                                  ^^^^^^^^^^^^^^^^^^^^^^
+
+error: aborting due to 3 previous errors
+

tests/ui/filter_methods.rs

@@ -1,5 +1,5 @@
 #![warn(clippy::all, clippy::pedantic)]
-#![allow(clippy::missing_docs_in_private_items)]
+#![allow(clippy::missing_docs_in_private_items, clippy::possible_shortcircuiting_collect)]
 
 fn main() {
     let _: Vec<_> = vec![5; 6].into_iter().filter(|&x| x == 0).map(|x| x * 2).collect();