diff --git a/clippy_lints/src/collect.rs b/clippy_lints/src/collect.rs new file mode 100644 index 000000000000..47d08be0d34f --- /dev/null +++ b/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 or the MIT license +// , 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, 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, +} + +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 { + 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 + ); + } + } + } + } +} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 824058a08cb3..287e868ed32c 100644 --- a/clippy_lints/src/lib.rs +++ b/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, diff --git a/tests/ui/collect.rs b/tests/ui/collect.rs new file mode 100644 index 000000000000..448cccb14fa5 --- /dev/null +++ b/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 or the MIT license +// , 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, 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(a: &[T]) { + // Make sure that our lint also works for generic functions. + let _result: Vec<_> = a.iter().map(Some).collect(); +} + +pub fn generic_collection + FromIterator>>(elem: T) -> C { + Some(Some(elem)).into_iter().collect() +} + +fn main() { + // We're collecting into an `Option`. Do not trigger lint. + let _sup: Option> = (0..5).map(Some).collect(); +} diff --git a/tests/ui/collect.stderr b/tests/ui/collect.stderr new file mode 100644 index 000000000000..93f5cf0d425e --- /dev/null +++ b/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, _> = 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> = 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::>() + | ^^^^^^^^^^^^^^^^^^^^^^ + +error: aborting due to 3 previous errors + diff --git a/tests/ui/filter_methods.rs b/tests/ui/filter_methods.rs index ef434245fd70..b8f1e0d073a0 100644 --- a/tests/ui/filter_methods.rs +++ b/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();