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..dfe1a3ccb1f8 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -223,6 +223,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 +421,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 +545,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 +2265,67 @@ impl<'a, 'tcx> Visitor<'tcx> for VarCollectorVisitor<'a, 'tcx> { NestedVisitorMap::None } } + +fn check_needless_collect<'a, 'tcx>(expr: &'tcx Expr, cx: &LateContext<'a, 'tcx>) { + 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 method.ident.name == "len" { + span_lint_and_sugg( + cx, + NEEDLESS_COLLECT, + expr.span, + "you are collecting an iterator to check its length", + "consider replacing with", + generate_needless_collect_len_sugg(&args[0], cx), + ); + } + if method.ident.name == "is_empty" { + span_lint_and_sugg( + cx, + NEEDLESS_COLLECT, + expr.span, + "you are collecting an iterator to check if it is empty", + "consider replacing with", + generate_needless_collect_is_empty_sugg(&args[0], cx), + ); + } + if method.ident.name == "contains" { + span_lint_and_sugg( + cx, + NEEDLESS_COLLECT, + expr.span, + "you are collecting an iterator to check if contains an element", + "consider replacing with", + generate_needless_collect_contains_sugg(&args[0], &args[1], cx), + ); + } + } + } + } +} + +fn generate_needless_collect_len_sugg<'a, 'tcx>(collect_expr: &'tcx Expr, cx: &LateContext<'a, 'tcx>) -> String { + if let ExprKind::MethodCall(_, _, ref args) = collect_expr.node { + let iter = snippet(cx, args[0].span, "??"); + return format!("{}.count()", iter); + } + unreachable!(); +} + +fn generate_needless_collect_is_empty_sugg<'a, 'tcx>(collect_expr: &'tcx Expr, cx: &LateContext<'a, 'tcx>) -> String { + if let ExprKind::MethodCall(_, _, ref args) = collect_expr.node { + let iter = snippet(cx, args[0].span, "??"); + return format!("{}.any(|_| true)", iter); + } + unreachable!(); +} + +fn generate_needless_collect_contains_sugg<'a, 'tcx>(collect_expr: &'tcx Expr, contains_arg: &'tcx Expr, cx: &LateContext<'a, 'tcx>) -> String { + if let ExprKind::MethodCall(_, _, ref args) = collect_expr.node { + let iter = snippet(cx, args[0].span, "??"); + let arg = snippet(cx, contains_arg.span, "??"); + return format!("{}.any(|&x| x == {})", iter, if arg.starts_with('&') { &arg[1..] } else { &arg }); + } + unreachable!(); +} diff --git a/needless_collect b/needless_collect new file mode 100755 index 000000000000..89054ecc66f6 Binary files /dev/null and b/needless_collect differ diff --git a/tests/ui/needless_collect.rs b/tests/ui/needless_collect.rs new file mode 100644 index 000000000000..5da77755d391 --- /dev/null +++ b/tests/ui/needless_collect.rs @@ -0,0 +1,10 @@ +#[warn(clippy, needless_collect)] +#[allow(unused_variables, 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); +} diff --git a/tests/ui/needless_collect.stderr b/tests/ui/needless_collect.stderr new file mode 100644 index 000000000000..b4e478c7eeca --- /dev/null +++ b/tests/ui/needless_collect.stderr @@ -0,0 +1,22 @@ +error: you are collecting an iterator to check its length + --> $DIR/needless_collect.rs:5:15 + | +5 | let len = sample.iter().collect::>().len(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider replacing with: `sample.iter().count()` + | + = note: `-D needless-collect` implied by `-D warnings` + +error: you are collecting an iterator to check if it is empty + --> $DIR/needless_collect.rs:6:8 + | +6 | if sample.iter().collect::>().is_empty() { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider replacing with: `sample.iter().any(|_| true)` + +error: you are collecting an iterator to check if contains an element + --> $DIR/needless_collect.rs:9:5 + | +9 | sample.iter().cloned().collect::>().contains(&1); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider replacing with: `sample.iter().cloned().any(|&x| x == 1)` + +error: aborting due to 3 previous errors +