diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index dfe1a3ccb1f8..3b886875de73 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -18,6 +18,7 @@ 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}; @@ -2269,63 +2270,62 @@ impl<'a, 'tcx> Visitor<'tcx> for VarCollectorVisitor<'a, 'tcx> { 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), - ); + if chain_method.ident.name == "collect" && + match_trait_method(cx, &args[0], &paths::ITERATOR) && + chain_method.args.is_some() { + let generic_args = chain_method.args.as_ref().unwrap(); + if let Some(GenericArg::Type(ref ty)) = generic_args.args.get(0) { + 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" { + span_lint_and_sugg( + cx, + NEEDLESS_COLLECT, + shorten_needless_collect_span(expr), + "you are collecting an iterator to check its length", + "consider replacing with", + ".count()".to_string(), + ); + } + if method.ident.name == "is_empty" { + span_lint_and_sugg( + cx, + NEEDLESS_COLLECT, + shorten_needless_collect_span(expr), + "you are collecting an iterator to check if it is empty", + "consider replacing with", + ".next().is_none()".to_string(), + ); + } + if method.ident.name == "contains" { + let contains_arg = snippet(cx, args[1].span, "??"); + span_lint_and_sugg( + cx, + NEEDLESS_COLLECT, + shorten_needless_collect_span(expr), + "you are collecting an iterator to check if contains an element", + "consider replacing with", + format!( + ".any(|&x| x == {})", + if contains_arg.starts_with('&') { &contains_arg[1..] } else { &contains_arg } + ), + ); + } + } } } } } } -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); +fn shorten_needless_collect_span(expr: &Expr) -> Span { + if let ExprKind::MethodCall(_, _, ref args) = expr.node { + if let ExprKind::MethodCall(_, ref span, _) = args[0].node { + return expr.span.with_lo(span.lo() - BytePos(1)); + } } - 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!(); + unreachable!() } diff --git a/tests/ui/needless_collect.rs b/tests/ui/needless_collect.rs index 5da77755d391..1aff8ee95659 100644 --- a/tests/ui/needless_collect.rs +++ b/tests/ui/needless_collect.rs @@ -1,3 +1,5 @@ +use std::collections::{HashMap, HashSet, BTreeSet}; + #[warn(clippy, needless_collect)] #[allow(unused_variables, iter_cloned_collect)] fn main() { @@ -7,4 +9,9 @@ fn main() { // 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 index b4e478c7eeca..1eca733ea02f 100644 --- a/tests/ui/needless_collect.stderr +++ b/tests/ui/needless_collect.stderr @@ -1,22 +1,28 @@ error: you are collecting an iterator to check its length - --> $DIR/needless_collect.rs:5:15 + --> $DIR/needless_collect.rs:7:28 | -5 | let len = sample.iter().collect::>().len(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider replacing with: `sample.iter().count()` +7 | let len = sample.iter().collect::>().len(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider replacing with: `.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 + --> $DIR/needless_collect.rs:8:21 | -6 | if sample.iter().collect::>().is_empty() { - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider replacing with: `sample.iter().any(|_| true)` +8 | if sample.iter().collect::>().is_empty() { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider replacing with: `.next().is_none()` 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)` + --> $DIR/needless_collect.rs:11:27 + | +11 | sample.iter().cloned().collect::>().contains(&1); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider replacing with: `.any(|&x| x == 1)` -error: aborting due to 3 previous errors +error: you are collecting an iterator to check its length + --> $DIR/needless_collect.rs:12:34 + | +12 | sample.iter().map(|x| (x, x)).collect::>().len(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider replacing with: `.count()` + +error: aborting due to 4 previous errors