From 9c8205846aef0896047f35e05286e13459164e83 Mon Sep 17 00:00:00 2001 From: Marcus Klaas Date: Thu, 26 Oct 2017 12:49:53 +0200 Subject: [PATCH 1/9] Add missed shortcircuit lint for collect calls --- clippy_lints/src/collect.rs | 194 ++++++++++++++++++++++++++++++++++++ clippy_lints/src/lib.rs | 3 + tests/ui/collect.rs | 33 ++++++ tests/ui/collect.stderr | 34 +++++++ tests/ui/filter_methods.rs | 2 +- 5 files changed, 265 insertions(+), 1 deletion(-) create mode 100644 clippy_lints/src/collect.rs create mode 100644 tests/ui/collect.rs create mode 100644 tests/ui/collect.stderr diff --git a/clippy_lints/src/collect.rs b/clippy_lints/src/collect.rs new file mode 100644 index 000000000000..626a4cd98098 --- /dev/null +++ b/clippy_lints/src/collect.rs @@ -0,0 +1,194 @@ +use itertools::{repeat_n, Itertools}; +use rustc::hir::*; +use rustc::lint::*; +use rustc::ty::TypeVariants; +use syntax::ast::NodeId; + +use std::collections::HashSet; + +use crate::utils::{match_trait_method, match_type, span_lint_and_sugg}; +use crate::utils::paths; + +/// **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)] +pub struct Pass { + // To ensure that we do not lint the same expression more than once + seen_expr_nodes: HashSet, +} + +impl Pass { + pub fn new() -> Self { + Self { seen_expr_nodes: HashSet::new() } + } +} + +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: TypeVariants, + is_option: bool, +) -> String { + let collection_pat = match collection_ty { + TypeVariants::TyAdt(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 + }, + TypeVariants::TyParam(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 ExprMethodCall(ref method, _, ref args) = expr.node { + if args.len() == 1 && method.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 method_call = cx.tables.type_dependent_defs()[args[0].hir_id]; + let trt_id = cx.tcx.trait_of_item(method_call.def_id()).unwrap(); + let assoc_item_id = cx.tcx.associated_items(trt_id).next().unwrap().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.sty.clone(), 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.sty.clone(), 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 ExprMethodCall(_, 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), + ); + } + } + + fn check_stmt(&mut self, cx: &LateContext<'a, 'tcx>, stmt: &'tcx Stmt) { + if_chain! { + if let StmtDecl(ref decl, _) = stmt.node; + if let DeclLocal(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 + ); + } + } + } + } +} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 8ce5861a939b..167a1ae24735 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -97,6 +97,7 @@ pub mod bytecount; pub mod cargo_common_metadata; pub mod collapsible_if; pub mod const_static_lifetime; +pub mod collect; pub mod copies; pub mod copy_iterator; pub mod cyclomatic_complexity; @@ -483,6 +484,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_lint_group("clippy::restriction", Some("clippy_restriction"), vec![ arithmetic::FLOAT_ARITHMETIC, @@ -1021,6 +1023,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, mutex_atomic::MUTEX_INTEGER, needless_borrow::NEEDLESS_BORROW, diff --git a/tests/ui/collect.rs b/tests/ui/collect.rs new file mode 100644 index 000000000000..332dfd6fa9d1 --- /dev/null +++ b/tests/ui/collect.rs @@ -0,0 +1,33 @@ +#![warn(possible_shortcircuiting_collect)] + +use std::iter::FromIterator; + +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) +} + +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..8f474b0a48aa --- /dev/null +++ b/tests/ui/collect.stderr @@ -0,0 +1,34 @@ +error: you are creating a collection of `Result`s + --> $DIR/collect.rs:6:21 + | +6 | let option_vec: Vec<_> = b.into_iter() + | ^^^^^^ + | + = note: `-D possible-shortcircuiting-collect` implied by `-D warnings` +help: if you are only interested in the case where all values are `Ok`, try + | +6 | let option_vec: Result, _> = b.into_iter() + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: you are creating a collection of `Option`s + --> $DIR/collect.rs:23:18 + | +23 | let _result: Vec<_> = a.iter().map(Some).collect(); + | ^^^^^^ +help: if you are only interested in the case where all values are `Some`, try + | +23 | let _result: Option> = a.iter().map(Some).collect(); + | ^^^^^^^^^^^^^^^^^^^^^^^^ + +error: you are creating a collection of `Option`s + --> $DIR/collect.rs:27:34 + | +27 | Some(Some(elem)).into_iter().collect() + | ^^^^^^^^^ +help: if you are only interested in the case where all values are `Some`, try + | +27 | 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 7ca74fd4b995..8daec7b91aad 100644 --- a/tests/ui/filter_methods.rs +++ b/tests/ui/filter_methods.rs @@ -8,7 +8,7 @@ // except according to those terms. #![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(); From 60f4767109d966acfcb7383f0a378d98d8b957f7 Mon Sep 17 00:00:00 2001 From: flip1995 <9744647+flip1995@users.noreply.github.com> Date: Fri, 8 Jun 2018 13:08:08 +0200 Subject: [PATCH 2/9] Fix bugs which produced ICEs --- clippy_lints/src/collect.rs | 55 +++++++++++++++++++++---------------- 1 file changed, 31 insertions(+), 24 deletions(-) diff --git a/clippy_lints/src/collect.rs b/clippy_lints/src/collect.rs index 626a4cd98098..c19dd9fb2d10 100644 --- a/clippy_lints/src/collect.rs +++ b/clippy_lints/src/collect.rs @@ -1,7 +1,7 @@ use itertools::{repeat_n, Itertools}; use rustc::hir::*; use rustc::lint::*; -use rustc::ty::TypeVariants; +use rustc::ty::{AssociatedKind, TypeVariants}; use syntax::ast::NodeId; use std::collections::HashSet; @@ -108,30 +108,37 @@ fn check_expr_for_collect<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr // 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 method_call = cx.tables.type_dependent_defs()[args[0].hir_id]; - let trt_id = cx.tcx.trait_of_item(method_call.def_id()).unwrap(); - let assoc_item_id = cx.tcx.associated_items(trt_id).next().unwrap().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, - ); + 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.sty.clone(), 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.sty.clone(), false), - type_colloquial: "Result", - success_variant: "Ok", - }) - } else { - None + return if match_type(cx, normal_ty, &paths::OPTION) { + Some(Suggestion { + pattern: format_suggestion_pattern(cx, collect_ty.sty.clone(), 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.sty.clone(), false), + type_colloquial: "Result", + success_variant: "Ok", + }) + } else { + None + }; + } }; } } From 38dab0c8010a50d60092fd25bf75737fce9ada62 Mon Sep 17 00:00:00 2001 From: flip1995 <9744647+flip1995@users.noreply.github.com> Date: Fri, 8 Jun 2018 13:33:03 +0200 Subject: [PATCH 3/9] Fix dogfood errors --- clippy_lints/src/collect.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/collect.rs b/clippy_lints/src/collect.rs index c19dd9fb2d10..995256dd39fd 100644 --- a/clippy_lints/src/collect.rs +++ b/clippy_lints/src/collect.rs @@ -43,7 +43,7 @@ declare_clippy_lint! { "missed shortcircuit opportunity on collect" } -#[derive(Clone)] +#[derive(Clone, Default)] pub struct Pass { // To ensure that we do not lint the same expression more than once seen_expr_nodes: HashSet, @@ -69,7 +69,7 @@ struct Suggestion { fn format_suggestion_pattern<'a, 'tcx>( cx: &LateContext<'a, 'tcx>, - collection_ty: TypeVariants, + collection_ty: &TypeVariants, is_option: bool, ) -> String { let collection_pat = match collection_ty { @@ -125,13 +125,13 @@ fn check_expr_for_collect<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr return if match_type(cx, normal_ty, &paths::OPTION) { Some(Suggestion { - pattern: format_suggestion_pattern(cx, collect_ty.sty.clone(), true), + pattern: format_suggestion_pattern(cx, &collect_ty.sty.clone(), 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.sty.clone(), false), + pattern: format_suggestion_pattern(cx, &collect_ty.sty.clone(), false), type_colloquial: "Result", success_variant: "Ok", }) From 682dc65542ee3cb07739501f5d131e6c82e0be7e Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Tue, 4 Dec 2018 08:14:48 +0100 Subject: [PATCH 4/9] Make it compile and make ui/collect.rs tests pass again --- clippy_lints/src/collect.rs | 30 +++++++++++++++++++----------- tests/ui/collect.rs | 4 ++-- 2 files changed, 21 insertions(+), 13 deletions(-) diff --git a/clippy_lints/src/collect.rs b/clippy_lints/src/collect.rs index 995256dd39fd..303812841e54 100644 --- a/clippy_lints/src/collect.rs +++ b/clippy_lints/src/collect.rs @@ -1,14 +1,20 @@ use itertools::{repeat_n, Itertools}; use rustc::hir::*; -use rustc::lint::*; -use rustc::ty::{AssociatedKind, TypeVariants}; +use rustc::ty::{AssociatedKind, TyKind}; use syntax::ast::NodeId; use std::collections::HashSet; +use crate::rustc_errors::Applicability; +use crate::rustc::lint::{ + LateContext, LateLintPass, LintArray, LintPass, +}; +use crate::rustc::{declare_tool_lint, lint_array}; 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. @@ -69,11 +75,11 @@ struct Suggestion { fn format_suggestion_pattern<'a, 'tcx>( cx: &LateContext<'a, 'tcx>, - collection_ty: &TypeVariants, + collection_ty: &TyKind<'_>, is_option: bool, ) -> String { let collection_pat = match collection_ty { - TypeVariants::TyAdt(def, subs) => { + TyKind::Adt(def, subs) => { let mut buf = cx.tcx.item_path_str(def.did); if !subs.is_empty() { @@ -84,7 +90,7 @@ fn format_suggestion_pattern<'a, 'tcx>( buf }, - TypeVariants::TyParam(p) => p.to_string(), + TyKind::Param(p) => p.to_string(), _ => "_".into(), }; @@ -96,8 +102,8 @@ fn format_suggestion_pattern<'a, 'tcx>( } fn check_expr_for_collect<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) -> Option { - if let ExprMethodCall(ref method, _, ref args) = expr.node { - if args.len() == 1 && method.name == "collect" && match_trait_method(cx, expr, &paths::ITERATOR) { + 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) { @@ -153,7 +159,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { } if let Some(suggestion) = check_expr_for_collect(cx, expr) { - let sugg_span = if let ExprMethodCall(_, call_span, _) = expr.node { + let sugg_span = if let ExprKind::MethodCall(_, call_span, _) = expr.node { expr.span.between(call_span) } else { unreachable!() @@ -169,14 +175,15 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { 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 StmtDecl(ref decl, _) = stmt.node; - if let DeclLocal(ref local) = decl.node; + 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 { @@ -192,7 +199,8 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { "if you are only interested in the case where all values are `{}`, try", suggestion.success_variant ), - suggestion.pattern + suggestion.pattern, + Applicability::MaybeIncorrect ); } } diff --git a/tests/ui/collect.rs b/tests/ui/collect.rs index 332dfd6fa9d1..c4cb21af6ffc 100644 --- a/tests/ui/collect.rs +++ b/tests/ui/collect.rs @@ -1,9 +1,9 @@ -#![warn(possible_shortcircuiting_collect)] +#![warn(clippy::possible_shortcircuiting_collect)] use std::iter::FromIterator; pub fn div(a: i32, b: &[i32]) -> Result, String> { - let option_vec: Vec<_> = b.into_iter() + let option_vec: Vec<_> = b.iter() .cloned() .map(|i| if i != 0 { Ok(a / i) From 0cbecd8e52bf558361ca7a16f966feefd881a77d Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Wed, 5 Dec 2018 07:24:35 +0100 Subject: [PATCH 5/9] Don't use TyKind directly, use ty instead --- clippy_lints/src/collect.rs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/clippy_lints/src/collect.rs b/clippy_lints/src/collect.rs index 303812841e54..a5df8a0d9425 100644 --- a/clippy_lints/src/collect.rs +++ b/clippy_lints/src/collect.rs @@ -1,6 +1,6 @@ use itertools::{repeat_n, Itertools}; -use rustc::hir::*; -use rustc::ty::{AssociatedKind, TyKind}; +use rustc::hir::{Expr, Stmt, DeclKind, StmtKind, ExprKind}; +use rustc::ty::{AssociatedKind}; use syntax::ast::NodeId; use std::collections::HashSet; @@ -9,7 +9,7 @@ use crate::rustc_errors::Applicability; use crate::rustc::lint::{ LateContext, LateLintPass, LintArray, LintPass, }; -use crate::rustc::{declare_tool_lint, lint_array}; +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; @@ -75,11 +75,11 @@ struct Suggestion { fn format_suggestion_pattern<'a, 'tcx>( cx: &LateContext<'a, 'tcx>, - collection_ty: &TyKind<'_>, + collection_ty: &ty::Ty<'_>, is_option: bool, ) -> String { - let collection_pat = match collection_ty { - TyKind::Adt(def, subs) => { + 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() { @@ -90,7 +90,7 @@ fn format_suggestion_pattern<'a, 'tcx>( buf }, - TyKind::Param(p) => p.to_string(), + ty::Param(p) => p.to_string(), _ => "_".into(), }; @@ -131,13 +131,13 @@ fn check_expr_for_collect<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr return if match_type(cx, normal_ty, &paths::OPTION) { Some(Suggestion { - pattern: format_suggestion_pattern(cx, &collect_ty.sty.clone(), true), + 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.sty.clone(), false), + pattern: format_suggestion_pattern(cx, &collect_ty, false), type_colloquial: "Result", success_variant: "Ok", }) From 4266e9d9f93a3f83d0b18e760ca37ac1a95224b6 Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Wed, 5 Dec 2018 07:39:30 +0100 Subject: [PATCH 6/9] Update UI test .stderr files --- tests/ui/collect.stderr | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/ui/collect.stderr b/tests/ui/collect.stderr index 8f474b0a48aa..7d890638a3a3 100644 --- a/tests/ui/collect.stderr +++ b/tests/ui/collect.stderr @@ -1,13 +1,13 @@ error: you are creating a collection of `Result`s --> $DIR/collect.rs:6:21 | -6 | let option_vec: Vec<_> = b.into_iter() +6 | let option_vec: Vec<_> = b.iter() | ^^^^^^ | - = note: `-D possible-shortcircuiting-collect` implied by `-D warnings` + = 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 | -6 | let option_vec: Result, _> = b.into_iter() +6 | let option_vec: Result, _> = b.iter() | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: you are creating a collection of `Option`s From 2bd90a41f1442a18e2c43c0d23ac810c226433dc Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Wed, 5 Dec 2018 20:40:46 +0100 Subject: [PATCH 7/9] Add missing licenses --- clippy_lints/src/collect.rs | 10 ++++++++++ tests/ui/collect.rs | 10 ++++++++++ 2 files changed, 20 insertions(+) diff --git a/clippy_lints/src/collect.rs b/clippy_lints/src/collect.rs index a5df8a0d9425..36058c54da10 100644 --- a/clippy_lints/src/collect.rs +++ b/clippy_lints/src/collect.rs @@ -1,3 +1,13 @@ +// 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}; diff --git a/tests/ui/collect.rs b/tests/ui/collect.rs index c4cb21af6ffc..448cccb14fa5 100644 --- a/tests/ui/collect.rs +++ b/tests/ui/collect.rs @@ -1,3 +1,13 @@ +// 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; From 0b09d356b2eaa1d6e3aab689b028d5a51dc1b12b Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Fri, 28 Dec 2018 21:33:02 +0100 Subject: [PATCH 8/9] Fix dogfood errors (FxHashSet and pass-by-value) --- clippy_lints/src/collect.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/collect.rs b/clippy_lints/src/collect.rs index 36058c54da10..47d08be0d34f 100644 --- a/clippy_lints/src/collect.rs +++ b/clippy_lints/src/collect.rs @@ -13,7 +13,7 @@ use rustc::hir::{Expr, Stmt, DeclKind, StmtKind, ExprKind}; use rustc::ty::{AssociatedKind}; use syntax::ast::NodeId; -use std::collections::HashSet; +use crate::rustc_data_structures::fx::FxHashSet; use crate::rustc_errors::Applicability; use crate::rustc::lint::{ @@ -62,12 +62,12 @@ declare_clippy_lint! { #[derive(Clone, Default)] pub struct Pass { // To ensure that we do not lint the same expression more than once - seen_expr_nodes: HashSet, + seen_expr_nodes: FxHashSet, } impl Pass { pub fn new() -> Self { - Self { seen_expr_nodes: HashSet::new() } + Self { seen_expr_nodes: FxHashSet::default() } } } @@ -85,7 +85,7 @@ struct Suggestion { fn format_suggestion_pattern<'a, 'tcx>( cx: &LateContext<'a, 'tcx>, - collection_ty: &ty::Ty<'_>, + collection_ty: ty::Ty<'_>, is_option: bool, ) -> String { let collection_pat = match collection_ty.sty { From 0a999afbdb55dcd9d889b374affc30c550bc8827 Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Fri, 28 Dec 2018 21:33:33 +0100 Subject: [PATCH 9/9] Update UI test output --- tests/ui/collect.stderr | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/tests/ui/collect.stderr b/tests/ui/collect.stderr index 7d890638a3a3..93f5cf0d425e 100644 --- a/tests/ui/collect.stderr +++ b/tests/ui/collect.stderr @@ -1,33 +1,33 @@ error: you are creating a collection of `Result`s - --> $DIR/collect.rs:6:21 - | -6 | let option_vec: Vec<_> = b.iter() - | ^^^^^^ - | - = note: `-D clippy::possible-shortcircuiting-collect` implied by `-D warnings` + --> $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 - | -6 | let option_vec: Result, _> = b.iter() - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +LL | let option_vec: Result, _> = b.iter() + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: you are creating a collection of `Option`s - --> $DIR/collect.rs:23:18 + --> $DIR/collect.rs:33:18 | -23 | let _result: Vec<_> = a.iter().map(Some).collect(); +LL | let _result: Vec<_> = a.iter().map(Some).collect(); | ^^^^^^ help: if you are only interested in the case where all values are `Some`, try | -23 | let _result: Option> = a.iter().map(Some).collect(); +LL | let _result: Option> = a.iter().map(Some).collect(); | ^^^^^^^^^^^^^^^^^^^^^^^^ error: you are creating a collection of `Option`s - --> $DIR/collect.rs:27:34 + --> $DIR/collect.rs:37:34 | -27 | Some(Some(elem)).into_iter().collect() +LL | Some(Some(elem)).into_iter().collect() | ^^^^^^^^^ help: if you are only interested in the case where all values are `Some`, try | -27 | Some(Some(elem)).into_iter().collect::>() +LL | Some(Some(elem)).into_iter().collect::>() | ^^^^^^^^^^^^^^^^^^^^^^ error: aborting due to 3 previous errors