Auto merge of rust-lang#7020 - camsteffen:needless-collect, r=Manishearth

Improve needless_collect output

changelog: Improve needless_collect output

Fixes rust-lang#6908
Partially addresses rust-lang#6164

Author: bors

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!();
-}

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<C, S, F>(cx: &C, lint: &'static Lint, sp: S, msg: &str, f: F)
 where
-    F: for<'b> FnOnce(&mut DiagnosticBuilder<'b>),
+    C: LintContext,
+    S: Into<MultiSpan>,
+    F: FnOnce(&mut DiagnosticBuilder<'_>),
 {
     cx.struct_span_lint(lint, sp, |diag| {
         let mut diag = diag.build(msg);

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::<Vec<_>>();
-LL | |     indirect_iter.into_iter().map(|x| (x, x + 1)).collect::<HashMap<_, _>>();
-   | |____^
+LL |     let indirect_iter = sample.iter().collect::<Vec<_>>();
+   |                                       ^^^^^^^
+LL |     indirect_iter.into_iter().map(|x| (x, x + 1)).collect::<HashMap<_, _>>();
+   |     ------------------------- 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::<HashMap<_, _>>();
    |
 
 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::<VecDeque<_>>();
-LL | |     indirect_len.len();
-   | |____^
+LL |     let indirect_len = sample.iter().collect::<VecDeque<_>>();
+   |                                      ^^^^^^^
+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::<VecDeque<_>>();
-LL | |     indirect_empty.is_empty();
-   | |____^
+LL |     let indirect_empty = sample.iter().collect::<VecDeque<_>>();
+   |                                        ^^^^^^^
+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::<VecDeque<_>>();
-LL | |     indirect_contains.contains(&&5);
-   | |____^
+LL |     let indirect_contains = sample.iter().collect::<VecDeque<_>>();
+   |                                           ^^^^^^^
+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::<Vec<_>>();
-LL | |     non_copy_contains.contains(&a);
-   | |____^
+LL |     let non_copy_contains = sample.into_iter().collect::<Vec<_>>();
+   |                                                ^^^^^^^
+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
    |