diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index c49a70afa6fa..23fdbf648e30 100644 --- a/clippy_lints/src/lib.rs +++ b/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, diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index d12b2619c62f..8a12530cb0d2 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -14,10 +14,12 @@ use rustc::middle::mem_categorization::Categorization; use rustc::middle::mem_categorization::cmt_; use rustc::ty::{self, Ty}; use rustc::ty::subst::Subst; +use rustc_errors::Applicability; use std::collections::{HashMap, HashSet}; use std::iter::{once, Iterator}; use syntax::ast; use syntax::source_map::Span; +use syntax_pos::BytePos; use crate::utils::{sugg, sext}; use crate::utils::usage::mutated_variables; use crate::consts::{constant, Constant}; @@ -223,6 +225,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::>().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 +423,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 +547,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 +2267,71 @@ impl<'a, 'tcx> Visitor<'tcx> for VarCollectorVisitor<'a, 'tcx> { NestedVisitorMap::None } } + +const NEEDLESS_COLLECT_MSG: &str = "avoid using `collect()` when not needed"; + +fn check_needless_collect<'a, 'tcx>(expr: &'tcx Expr, cx: &LateContext<'a, 'tcx>) { + if_chain! { + 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 let Some(ref generic_args) = chain_method.args; + if let Some(GenericArg::Type(ref ty)) = generic_args.args.get(0); + then { + let ty = cx.tables.node_id_to_type(ty.hir_id); + if match_type(cx, ty, &paths::VEC) || + match_type(cx, ty, &paths::VEC_DEQUE) || + match_type(cx, ty, &paths::BTREEMAP) || + match_type(cx, ty, &paths::HASHMAP) { + if method.ident.name == "len" { + let span = shorten_needless_collect_span(expr); + span_lint_and_then(cx, NEEDLESS_COLLECT, span, NEEDLESS_COLLECT_MSG, |db| { + db.span_suggestion_with_applicability( + span, + "replace with", + ".count()".to_string(), + Applicability::MachineApplicable, + ); + }); + } + if method.ident.name == "is_empty" { + let span = shorten_needless_collect_span(expr); + span_lint_and_then(cx, NEEDLESS_COLLECT, span, NEEDLESS_COLLECT_MSG, |db| { + db.span_suggestion_with_applicability( + span, + "replace with", + ".next().is_none()".to_string(), + Applicability::MachineApplicable, + ); + }); + } + if method.ident.name == "contains" { + let contains_arg = snippet(cx, args[1].span, "??"); + let span = shorten_needless_collect_span(expr); + span_lint_and_then(cx, NEEDLESS_COLLECT, span, NEEDLESS_COLLECT_MSG, |db| { + db.span_suggestion_with_applicability( + span, + "replace with", + format!( + ".any(|&x| x == {})", + if contains_arg.starts_with('&') { &contains_arg[1..] } else { &contains_arg } + ), + Applicability::MachineApplicable, + ); + }); + } + } + } + } +} + +fn shorten_needless_collect_span(expr: &Expr) -> Span { + if_chain! { + if let ExprKind::MethodCall(_, _, ref args) = expr.node; + if let ExprKind::MethodCall(_, ref span, _) = args[0].node; + then { + return expr.span.with_lo(span.lo() - BytePos(1)); + } + } + unreachable!() +} diff --git a/tests/ui/needless_collect.rs b/tests/ui/needless_collect.rs new file mode 100644 index 000000000000..b001f20d5279 --- /dev/null +++ b/tests/ui/needless_collect.rs @@ -0,0 +1,19 @@ +#![feature(tool_lints)] + +use std::collections::{HashMap, HashSet, BTreeSet}; + +#[warn(clippy::needless_collect)] +#[allow(unused_variables, clippy::iter_cloned_collect)] +fn main() { + let sample = [1; 5]; + let len = sample.iter().collect::>().len(); + if sample.iter().collect::>().is_empty() { + // Empty + } + sample.iter().cloned().collect::>().contains(&1); + sample.iter().map(|x| (x, x)).collect::>().len(); + // Notice the `HashSet`--this should not be linted + sample.iter().collect::>().len(); + // Neither should this + sample.iter().collect::>().len(); +} diff --git a/tests/ui/needless_collect.stderr b/tests/ui/needless_collect.stderr new file mode 100644 index 000000000000..0124db3b9758 --- /dev/null +++ b/tests/ui/needless_collect.stderr @@ -0,0 +1,28 @@ +error: avoid using `collect()` when not needed + --> $DIR/needless_collect.rs:9:28 + | +9 | let len = sample.iter().collect::>().len(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `.count()` + | + = note: `-D clippy::needless-collect` implied by `-D warnings` + +error: avoid using `collect()` when not needed + --> $DIR/needless_collect.rs:10:21 + | +10 | if sample.iter().collect::>().is_empty() { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `.next().is_none()` + +error: avoid using `collect()` when not needed + --> $DIR/needless_collect.rs:13:27 + | +13 | sample.iter().cloned().collect::>().contains(&1); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `.any(|&x| x == 1)` + +error: avoid using `collect()` when not needed + --> $DIR/needless_collect.rs:14:34 + | +14 | sample.iter().map(|x| (x, x)).collect::>().len(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `.count()` + +error: aborting due to 4 previous errors +