Auto merge of #4691 - HMPerson1:suggest_iter, r=phansch

Fix suggestion of `explicit_counter_loop`

changelog: In the suggestion of `explicit_counter_loop`, if the `for` loop argument doesn't implement `Iterator`, then we suggest `x.into_iter().enumerate()` (or `x.iter{_mut}()` as appropriate). Also, the span of the suggestion has been corrected.

Fixes #4678

4 files changed, 92 insertions, 29 deletions

diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs
index 22821e02fe2..731dd92c82a 100644
--- a/clippy_lints/src/loops.rs
+++ b/clippy_lints/src/loops.rs
@@ -27,9 +27,10 @@ use syntax_pos::{BytePos, Symbol};
 
 use crate::utils::paths;
 use crate::utils::{
-    get_enclosing_block, get_parent_expr, has_iter_method, higher, is_integer_const, is_refutable, last_path_segment,
-    match_trait_method, match_type, match_var, multispan_sugg, snippet, snippet_opt, snippet_with_applicability,
-    span_help_and_lint, span_lint, span_lint_and_sugg, span_lint_and_then, SpanlessEq,
+    get_enclosing_block, get_parent_expr, get_trait_def_id, has_iter_method, higher, implements_trait,
+    is_integer_const, is_refutable, last_path_segment, match_trait_method, match_type, match_var, multispan_sugg,
+    snippet, snippet_opt, snippet_with_applicability, span_help_and_lint, span_lint, span_lint_and_sugg,
+    span_lint_and_then, SpanlessEq,
 };
 
 declare_clippy_lint! {
@@ -1460,27 +1461,26 @@ fn check_for_loop_explicit_counter<'a, 'tcx>(
             if visitor2.state == VarState::Warn {
                 if let Some(name) = visitor2.name {
                     let mut applicability = Applicability::MachineApplicable;
+
+                    // for some reason this is the only way to get the `Span`
+                    // of the entire `for` loop
+                    let for_span = if let ExprKind::Match(_, arms, _) = &expr.kind {
+                        arms[0].body.span
+                    } else {
+                        unreachable!()
+                    };
+
                     span_lint_and_sugg(
                         cx,
                         EXPLICIT_COUNTER_LOOP,
-                        expr.span,
+                        for_span.with_hi(arg.span.hi()),
                         &format!("the variable `{}` is used as a loop counter.", name),
                         "consider using",
                         format!(
                             "for ({}, {}) in {}.enumerate()",
                             name,
                             snippet_with_applicability(cx, pat.span, "item", &mut applicability),
-                            if higher::range(cx, arg).is_some() {
-                                format!(
-                                    "({})",
-                                    snippet_with_applicability(cx, arg.span, "_", &mut applicability)
-                                )
-                            } else {
-                                format!(
-                                    "{}",
-                                    sugg::Sugg::hir_with_applicability(cx, arg, "_", &mut applicability).maybe_par()
-                                )
-                            }
+                            make_iterator_snippet(cx, arg, &mut applicability),
                         ),
                         applicability,
                     );
@@ -1490,6 +1490,39 @@ fn check_for_loop_explicit_counter<'a, 'tcx>(
     }
 }
 
+/// If `arg` was the argument to a `for` loop, return the "cleanest" way of writing the
+/// actual `Iterator` that the loop uses.
+fn make_iterator_snippet(cx: &LateContext<'_, '_>, arg: &Expr, applic_ref: &mut Applicability) -> String {
+    let impls_iterator = get_trait_def_id(cx, &paths::ITERATOR)
+        .map_or(false, |id| implements_trait(cx, cx.tables.expr_ty(arg), id, &[]));
+    if impls_iterator {
+        format!(
+            "{}",
+            sugg::Sugg::hir_with_applicability(cx, arg, "_", applic_ref).maybe_par()
+        )
+    } else {
+        // (&x).into_iter() ==> x.iter()
+        // (&mut x).into_iter() ==> x.iter_mut()
+        match &arg.kind {
+            ExprKind::AddrOf(mutability, arg_inner) if has_iter_method(cx, cx.tables.expr_ty(&arg_inner)).is_some() => {
+                let meth_name = match mutability {
+                    MutMutable => "iter_mut",
+                    MutImmutable => "iter",
+                };
+                format!(
+                    "{}.{}()",
+                    sugg::Sugg::hir_with_applicability(cx, &arg_inner, "_", applic_ref).maybe_par(),
+                    meth_name,
+                )
+            },
+            _ => format!(
+                "{}.into_iter()",
+                sugg::Sugg::hir_with_applicability(cx, arg, "_", applic_ref).maybe_par()
+            ),
+        }
+    }
+}
+
 /// Checks for the `FOR_KV_MAP` lint.
 fn check_for_loop_over_map_kv<'a, 'tcx>(
     cx: &LateContext<'a, 'tcx>,
diff --git a/clippy_lints/src/utils/sugg.rs b/clippy_lints/src/utils/sugg.rs
index 0675c603341..4fe41a880cc 100644
--- a/clippy_lints/src/utils/sugg.rs
+++ b/clippy_lints/src/utils/sugg.rs
@@ -46,7 +46,7 @@ impl<'a> Sugg<'a> {
     pub fn hir_opt(cx: &LateContext<'_, '_>, expr: &hir::Expr) -> Option<Self> {
         snippet_opt(cx, expr.span).map(|snippet| {
             let snippet = Cow::Owned(snippet);
-            Self::hir_from_snippet(expr, snippet)
+            Self::hir_from_snippet(cx, expr, snippet)
         })
     }
 
@@ -84,12 +84,20 @@ impl<'a> Sugg<'a> {
     pub fn hir_with_macro_callsite(cx: &LateContext<'_, '_>, expr: &hir::Expr, default: &'a str) -> Self {
         let snippet = snippet_with_macro_callsite(cx, expr.span, default);
 
-        Self::hir_from_snippet(expr, snippet)
+        Self::hir_from_snippet(cx, expr, snippet)
     }
 
     /// Generate a suggestion for an expression with the given snippet. This is used by the `hir_*`
     /// function variants of `Sugg`, since these use different snippet functions.
-    fn hir_from_snippet(expr: &hir::Expr, snippet: Cow<'a, str>) -> Self {
+    fn hir_from_snippet(cx: &LateContext<'_, '_>, expr: &hir::Expr, snippet: Cow<'a, str>) -> Self {
+        if let Some(range) = higher::range(cx, expr) {
+            let op = match range.limits {
+                ast::RangeLimits::HalfOpen => AssocOp::DotDot,
+                ast::RangeLimits::Closed => AssocOp::DotDotEq,
+            };
+            return Sugg::BinOp(op, snippet);
+        }
+
         match expr.kind {
             hir::ExprKind::AddrOf(..)
             | hir::ExprKind::Box(..)
diff --git a/tests/ui/explicit_counter_loop.rs b/tests/ui/explicit_counter_loop.rs
index 71ef1e8674a..e6fbf83a287 100644
--- a/tests/ui/explicit_counter_loop.rs
+++ b/tests/ui/explicit_counter_loop.rs
@@ -12,6 +12,16 @@ fn main() {
     for _v in &vec {
         _index += 1
     }
+
+    let mut _index = 0;
+    for _v in &mut vec {
+        _index += 1;
+    }
+
+    let mut _index = 0;
+    for _v in vec {
+        _index += 1;
+    }
 }
 
 mod issue_1219 {
diff --git a/tests/ui/explicit_counter_loop.stderr b/tests/ui/explicit_counter_loop.stderr
index 5efd51abf18..931af46efe6 100644
--- a/tests/ui/explicit_counter_loop.stderr
+++ b/tests/ui/explicit_counter_loop.stderr
@@ -1,34 +1,46 @@
 error: the variable `_index` is used as a loop counter.
-  --> $DIR/explicit_counter_loop.rs:6:15
+  --> $DIR/explicit_counter_loop.rs:6:5
    |
 LL |     for _v in &vec {
-   |               ^^^^ help: consider using: `for (_index, _v) in (&vec).enumerate()`
+   |     ^^^^^^^^^^^^^^ help: consider using: `for (_index, _v) in vec.iter().enumerate()`
    |
    = note: `-D clippy::explicit-counter-loop` implied by `-D warnings`
 
 error: the variable `_index` is used as a loop counter.
-  --> $DIR/explicit_counter_loop.rs:12:15
+  --> $DIR/explicit_counter_loop.rs:12:5
    |
 LL |     for _v in &vec {
-   |               ^^^^ help: consider using: `for (_index, _v) in (&vec).enumerate()`
+   |     ^^^^^^^^^^^^^^ help: consider using: `for (_index, _v) in vec.iter().enumerate()`
+
+error: the variable `_index` is used as a loop counter.
+  --> $DIR/explicit_counter_loop.rs:17:5
+   |
+LL |     for _v in &mut vec {
+   |     ^^^^^^^^^^^^^^^^^^ help: consider using: `for (_index, _v) in vec.iter_mut().enumerate()`
+
+error: the variable `_index` is used as a loop counter.
+  --> $DIR/explicit_counter_loop.rs:22:5
+   |
+LL |     for _v in vec {
+   |     ^^^^^^^^^^^^^ help: consider using: `for (_index, _v) in vec.into_iter().enumerate()`
 
 error: the variable `count` is used as a loop counter.
-  --> $DIR/explicit_counter_loop.rs:51:19
+  --> $DIR/explicit_counter_loop.rs:61:9
    |
 LL |         for ch in text.chars() {
-   |                   ^^^^^^^^^^^^ help: consider using: `for (count, ch) in text.chars().enumerate()`
+   |         ^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `for (count, ch) in text.chars().enumerate()`
 
 error: the variable `count` is used as a loop counter.
-  --> $DIR/explicit_counter_loop.rs:62:19
+  --> $DIR/explicit_counter_loop.rs:72:9
    |
 LL |         for ch in text.chars() {
-   |                   ^^^^^^^^^^^^ help: consider using: `for (count, ch) in text.chars().enumerate()`
+   |         ^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `for (count, ch) in text.chars().enumerate()`
 
 error: the variable `count` is used as a loop counter.
-  --> $DIR/explicit_counter_loop.rs:120:19
+  --> $DIR/explicit_counter_loop.rs:130:9
    |
 LL |         for _i in 3..10 {
-   |                   ^^^^^ help: consider using: `for (count, _i) in (3..10).enumerate()`
+   |         ^^^^^^^^^^^^^^^ help: consider using: `for (count, _i) in (3..10).enumerate()`
 
-error: aborting due to 5 previous errors
+error: aborting due to 7 previous errors