diff --git a/clippy_lints/src/loops/needless_collect.rs b/clippy_lints/src/loops/needless_collect.rs index d23d3c508fa..e0c5caf5136 100644 --- a/clippy_lints/src/loops/needless_collect.rs +++ b/clippy_lints/src/loops/needless_collect.rs @@ -10,8 +10,8 @@ use rustc_hir::intravisit::{walk_block, walk_expr, NestedVisitorMap, Visitor}; use rustc_hir::{Block, Expr, ExprKind, GenericArg, HirId, Local, Pat, PatKind, QPath, StmtKind}; use rustc_lint::LateContext; use rustc_middle::hir::map::Map; -use rustc_span::source_map::Span; use rustc_span::symbol::{sym, Ident}; +use rustc_span::{MultiSpan, Span}; const NEEDLESS_COLLECT_MSG: &str = "avoid using `collect()` when not needed"; @@ -22,7 +22,7 @@ pub(super) fn check<'tcx>(expr: &'tcx Expr<'_>, cx: &LateContext<'tcx>) { fn check_needless_collect_direct_usage<'tcx>(expr: &'tcx Expr<'_>, cx: &LateContext<'tcx>) { if_chain! { if let ExprKind::MethodCall(ref method, _, ref args, _) = expr.kind; - if let ExprKind::MethodCall(ref chain_method, _, _, _) = args[0].kind; + if let ExprKind::MethodCall(ref chain_method, method0_span, _, _) = args[0].kind; if chain_method.ident.name == sym!(collect) && is_trait_method(cx, &args[0], sym::Iterator); if let Some(ref generic_args) = chain_method.args; if let Some(GenericArg::Type(ref ty)) = generic_args.args.get(0); @@ -31,55 +31,28 @@ fn check_needless_collect_direct_usage<'tcx>(expr: &'tcx Expr<'_>, cx: &LateCont || is_type_diagnostic_item(cx, ty, sym::vecdeque_type) || match_type(cx, ty, &paths::BTREEMAP) || is_type_diagnostic_item(cx, ty, sym::hashmap_type); - then { - if method.ident.name == sym!(len) { - let span = shorten_needless_collect_span(expr); - span_lint_and_sugg( - cx, - NEEDLESS_COLLECT, - span, - NEEDLESS_COLLECT_MSG, - "replace with", - "count()".to_string(), - Applicability::MachineApplicable, - ); - } - if method.ident.name == sym!(is_empty) { - let span = shorten_needless_collect_span(expr); - span_lint_and_sugg( - cx, - NEEDLESS_COLLECT, - span, - NEEDLESS_COLLECT_MSG, - "replace with", - "next().is_none()".to_string(), - Applicability::MachineApplicable, - ); - } - if method.ident.name == sym!(contains) { + if let Some(sugg) = match &*method.ident.name.as_str() { + "len" => Some("count()".to_string()), + "is_empty" => Some("next().is_none()".to_string()), + "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, - |diag| { - let (arg, pred) = contains_arg - .strip_prefix('&') - .map_or(("&x", &*contains_arg), |s| ("x", s)); - diag.span_suggestion( - span, - "replace with", - format!( - "any(|{}| x == {})", - arg, pred - ), - Applicability::MachineApplicable, - ); - } - ); + let (arg, pred) = contains_arg + .strip_prefix('&') + .map_or(("&x", &*contains_arg), |s| ("x", s)); + Some(format!("any(|{}| x == {})", arg, pred)) } + _ => None, + }; + then { + span_lint_and_sugg( + cx, + NEEDLESS_COLLECT, + method0_span.with_hi(expr.span.hi()), + NEEDLESS_COLLECT_MSG, + "replace with", + sugg, + Applicability::MachineApplicable, + ); } } } @@ -92,7 +65,7 @@ fn check_needless_collect_indirect_usage<'tcx>(expr: &'tcx Expr<'_>, cx: &LateCo Local { pat: Pat { hir_id: pat_id, kind: PatKind::Binding(_, _, ident, .. ), .. }, init: Some(ref init_expr), .. } ) = stmt.kind; - if let ExprKind::MethodCall(ref method_name, _, &[ref iter_source], ..) = init_expr.kind; + if let ExprKind::MethodCall(ref method_name, collect_span, &[ref iter_source], ..) = init_expr.kind; if method_name.ident.name == sym!(collect) && is_trait_method(cx, &init_expr, sym::Iterator); if let Some(ref generic_args) = method_name.args; if let Some(GenericArg::Type(ref ty)) = generic_args.args.get(0); @@ -101,7 +74,7 @@ fn check_needless_collect_indirect_usage<'tcx>(expr: &'tcx Expr<'_>, cx: &LateCo is_type_diagnostic_item(cx, ty, sym::vecdeque_type) || match_type(cx, ty, &paths::LINKED_LIST); if let Some(iter_calls) = detect_iter_and_into_iters(block, *ident); - if iter_calls.len() == 1; + if let [iter_call] = &*iter_calls; then { let mut used_count_visitor = UsedCountVisitor { cx, @@ -114,11 +87,12 @@ fn check_needless_collect_indirect_usage<'tcx>(expr: &'tcx Expr<'_>, cx: &LateCo } // Suggest replacing iter_call with iter_replacement, and removing stmt - let iter_call = &iter_calls[0]; + let mut span = MultiSpan::from_span(collect_span); + span.push_span_label(iter_call.span, "the iterator could be used here instead".into()); span_lint_and_then( cx, super::NEEDLESS_COLLECT, - stmt.span.until(iter_call.span), + span, NEEDLESS_COLLECT_MSG, |diag| { let iter_replacement = format!("{}{}", Sugg::hir(cx, iter_source, ".."), iter_call.get_iter_method(cx)); @@ -129,7 +103,7 @@ fn check_needless_collect_indirect_usage<'tcx>(expr: &'tcx Expr<'_>, cx: &LateCo (iter_call.span, iter_replacement) ], Applicability::MachineApplicable,// MaybeIncorrect, - ).emit(); + ); }, ); } @@ -269,14 +243,3 @@ fn detect_iter_and_into_iters<'tcx>(block: &'tcx Block<'tcx>, identifier: Ident) visitor.visit_block(block); if visitor.seen_other { None } else { Some(visitor.uses) } } - -fn shorten_needless_collect_span(expr: &Expr<'_>) -> Span { - if_chain! { - if let ExprKind::MethodCall(.., args, _) = &expr.kind; - if let ExprKind::MethodCall(_, span, ..) = &args[0].kind; - then { - return expr.span.with_lo(span.lo()); - } - } - unreachable!(); -} diff --git a/clippy_utils/src/diagnostics.rs b/clippy_utils/src/diagnostics.rs index e9c9cb12b9d..73a2fa992b3 100644 --- a/clippy_utils/src/diagnostics.rs +++ b/clippy_utils/src/diagnostics.rs @@ -133,9 +133,11 @@ pub fn span_lint_and_note<'a, T: LintContext>( /// /// If you need to customize your lint output a lot, use this function. /// If you change the signature, remember to update the internal lint `CollapsibleCalls` -pub fn span_lint_and_then<'a, T: LintContext, F>(cx: &'a T, lint: &'static Lint, sp: Span, msg: &str, f: F) +pub fn span_lint_and_then(cx: &C, lint: &'static Lint, sp: S, msg: &str, f: F) where - F: for<'b> FnOnce(&mut DiagnosticBuilder<'b>), + C: LintContext, + S: Into, + F: FnOnce(&mut DiagnosticBuilder<'_>), { cx.struct_span_lint(lint, sp, |diag| { let mut diag = diag.build(msg); diff --git a/tests/ui/needless_collect_indirect.stderr b/tests/ui/needless_collect_indirect.stderr index 76e789d9052..c773b841f3b 100644 --- a/tests/ui/needless_collect_indirect.stderr +++ b/tests/ui/needless_collect_indirect.stderr @@ -1,9 +1,10 @@ error: avoid using `collect()` when not needed - --> $DIR/needless_collect_indirect.rs:5:5 + --> $DIR/needless_collect_indirect.rs:5:39 | -LL | / let indirect_iter = sample.iter().collect::>(); -LL | | indirect_iter.into_iter().map(|x| (x, x + 1)).collect::>(); - | |____^ +LL | let indirect_iter = sample.iter().collect::>(); + | ^^^^^^^ +LL | indirect_iter.into_iter().map(|x| (x, x + 1)).collect::>(); + | ------------------------- the iterator could be used here instead | = note: `-D clippy::needless-collect` implied by `-D warnings` help: use the original Iterator instead of collecting it and then producing a new one @@ -13,11 +14,12 @@ LL | sample.iter().map(|x| (x, x + 1)).collect::>(); | error: avoid using `collect()` when not needed - --> $DIR/needless_collect_indirect.rs:7:5 + --> $DIR/needless_collect_indirect.rs:7:38 | -LL | / let indirect_len = sample.iter().collect::>(); -LL | | indirect_len.len(); - | |____^ +LL | let indirect_len = sample.iter().collect::>(); + | ^^^^^^^ +LL | indirect_len.len(); + | ------------------ the iterator could be used here instead | help: take the original Iterator's count instead of collecting it and finding the length | @@ -26,11 +28,12 @@ LL | sample.iter().count(); | error: avoid using `collect()` when not needed - --> $DIR/needless_collect_indirect.rs:9:5 + --> $DIR/needless_collect_indirect.rs:9:40 | -LL | / let indirect_empty = sample.iter().collect::>(); -LL | | indirect_empty.is_empty(); - | |____^ +LL | let indirect_empty = sample.iter().collect::>(); + | ^^^^^^^ +LL | indirect_empty.is_empty(); + | ------------------------- the iterator could be used here instead | help: check if the original Iterator has anything instead of collecting it and seeing if it's empty | @@ -39,11 +42,12 @@ LL | sample.iter().next().is_none(); | error: avoid using `collect()` when not needed - --> $DIR/needless_collect_indirect.rs:11:5 + --> $DIR/needless_collect_indirect.rs:11:43 | -LL | / let indirect_contains = sample.iter().collect::>(); -LL | | indirect_contains.contains(&&5); - | |____^ +LL | let indirect_contains = sample.iter().collect::>(); + | ^^^^^^^ +LL | indirect_contains.contains(&&5); + | ------------------------------- the iterator could be used here instead | help: check if the original Iterator contains an element instead of collecting then checking | @@ -52,11 +56,12 @@ LL | sample.iter().any(|x| x == &5); | error: avoid using `collect()` when not needed - --> $DIR/needless_collect_indirect.rs:23:5 + --> $DIR/needless_collect_indirect.rs:23:48 | -LL | / let non_copy_contains = sample.into_iter().collect::>(); -LL | | non_copy_contains.contains(&a); - | |____^ +LL | let non_copy_contains = sample.into_iter().collect::>(); + | ^^^^^^^ +LL | non_copy_contains.contains(&a); + | ------------------------------ the iterator could be used here instead | help: check if the original Iterator contains an element instead of collecting then checking |