Auto merge of #7950 - Serial-ATA:issue-7920, r=llogiq

Fix `explicit_counter_loop` suggestion for non-usize types

changelog: Add a new suggestion for non-usize types in [`explicit_counter_loop`]

closes: #7920

5 files changed, 145 insertions, 35 deletions

diff --git a/clippy_lints/src/loops/explicit_counter_loop.rs b/clippy_lints/src/loops/explicit_counter_loop.rs
index 98e60f7ed85..bf9326b2901 100644
--- a/clippy_lints/src/loops/explicit_counter_loop.rs
+++ b/clippy_lints/src/loops/explicit_counter_loop.rs
@@ -1,7 +1,7 @@
 use super::{
     get_span_of_entire_for_loop, make_iterator_snippet, IncrementVisitor, InitializeVisitor, EXPLICIT_COUNTER_LOOP,
 };
-use clippy_utils::diagnostics::span_lint_and_sugg;
+use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then};
 use clippy_utils::source::snippet_with_applicability;
 use clippy_utils::{get_enclosing_block, is_integer_const};
 use if_chain::if_chain;
@@ -9,6 +9,7 @@ use rustc_errors::Applicability;
 use rustc_hir::intravisit::{walk_block, walk_expr};
 use rustc_hir::{Expr, Pat};
 use rustc_lint::LateContext;
+use rustc_middle::ty::{self, UintTy};
 
 // To trigger the EXPLICIT_COUNTER_LOOP lint, a variable must be
 // incremented exactly once in the loop body, and initialized to zero
@@ -32,26 +33,61 @@ pub(super) fn check<'tcx>(
             walk_block(&mut initialize_visitor, block);
 
             if_chain! {
-                if let Some((name, initializer)) = initialize_visitor.get_result();
+                if let Some((name, ty, initializer)) = initialize_visitor.get_result();
                 if is_integer_const(cx, initializer, 0);
                 then {
                     let mut applicability = Applicability::MachineApplicable;
 
                     let for_span = get_span_of_entire_for_loop(expr);
 
-                    span_lint_and_sugg(
+                    let int_name = match ty.map(ty::TyS::kind) {
+                        // usize or inferred
+                        Some(ty::Uint(UintTy::Usize)) | None => {
+                            span_lint_and_sugg(
+                                cx,
+                                EXPLICIT_COUNTER_LOOP,
+                                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),
+                                    make_iterator_snippet(cx, arg, &mut applicability),
+                                ),
+                                applicability,
+                            );
+                            return;
+                        }
+                        Some(ty::Int(int_ty)) => int_ty.name_str(),
+                        Some(ty::Uint(uint_ty)) => uint_ty.name_str(),
+                        _ => return,
+                    };
+
+                    span_lint_and_then(
                         cx,
                         EXPLICIT_COUNTER_LOOP,
                         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),
-                            make_iterator_snippet(cx, arg, &mut applicability),
-                        ),
-                        applicability,
+                        |diag| {
+                            diag.span_suggestion(
+                                for_span.with_hi(arg.span.hi()),
+                                "consider using",
+                                format!(
+                                    "for ({}, {}) in (0_{}..).zip({})",
+                                    name,
+                                    snippet_with_applicability(cx, pat.span, "item", &mut applicability),
+                                    int_name,
+                                    make_iterator_snippet(cx, arg, &mut applicability),
+                                ),
+                                applicability,
+                            );
+
+                            diag.note(&format!(
+                                "`{}` is of type `{}`, making it ineligible for `Iterator::enumerate`",
+                                name, int_name
+                            ));
+                        },
                     );
                 }
             }
diff --git a/clippy_lints/src/loops/manual_memcpy.rs b/clippy_lints/src/loops/manual_memcpy.rs
index 6bdc560efd7..f6a673f7135 100644
--- a/clippy_lints/src/loops/manual_memcpy.rs
+++ b/clippy_lints/src/loops/manual_memcpy.rs
@@ -442,7 +442,7 @@ fn get_loop_counters<'a, 'tcx>(
                 let mut initialize_visitor = InitializeVisitor::new(cx, expr, var_id);
                 walk_block(&mut initialize_visitor, block);
 
-                initialize_visitor.get_result().map(|(_, initializer)| Start {
+                initialize_visitor.get_result().map(|(_, _, initializer)| Start {
                     id: var_id,
                     kind: StartKind::Counter { initializer },
                 })
diff --git a/clippy_lints/src/loops/utils.rs b/clippy_lints/src/loops/utils.rs
index f9f515cc40a..c1e367b344a 100644
--- a/clippy_lints/src/loops/utils.rs
+++ b/clippy_lints/src/loops/utils.rs
@@ -1,14 +1,17 @@
 use clippy_utils::ty::{has_iter_method, implements_trait};
 use clippy_utils::{get_parent_expr, is_integer_const, path_to_local, path_to_local_id, sugg};
 use if_chain::if_chain;
+use rustc_ast::ast::{LitIntType, LitKind};
 use rustc_errors::Applicability;
-use rustc_hir::intravisit::{walk_expr, walk_pat, walk_stmt, NestedVisitorMap, Visitor};
-use rustc_hir::HirIdMap;
-use rustc_hir::{BinOpKind, BorrowKind, Expr, ExprKind, HirId, Mutability, Pat, PatKind, Stmt, StmtKind};
+use rustc_hir::intravisit::{walk_expr, walk_local, walk_pat, walk_stmt, NestedVisitorMap, Visitor};
+use rustc_hir::{BinOpKind, BorrowKind, Expr, ExprKind, HirId, HirIdMap, Local, Mutability, Pat, PatKind, Stmt};
 use rustc_lint::LateContext;
 use rustc_middle::hir::map::Map;
+use rustc_middle::ty::Ty;
 use rustc_span::source_map::Span;
+use rustc_span::source_map::Spanned;
 use rustc_span::symbol::{sym, Symbol};
+use rustc_typeck::hir_ty_to_ty;
 use std::iter::Iterator;
 
 #[derive(Debug, PartialEq)]
@@ -106,10 +109,11 @@ impl<'a, 'tcx> Visitor<'tcx> for IncrementVisitor<'a, 'tcx> {
 }
 
 enum InitializeVisitorState<'hir> {
-    Initial,          // Not examined yet
-    Declared(Symbol), // Declared but not (yet) initialized
+    Initial,                            // Not examined yet
+    Declared(Symbol, Option<Ty<'hir>>), // Declared but not (yet) initialized
     Initialized {
         name: Symbol,
+        ty: Option<Ty<'hir>>,
         initializer: &'hir Expr<'hir>,
     },
     DontWarn,
@@ -138,9 +142,9 @@ impl<'a, 'tcx> InitializeVisitor<'a, 'tcx> {
         }
     }
 
-    pub(super) fn get_result(&self) -> Option<(Symbol, &'tcx Expr<'tcx>)> {
-        if let InitializeVisitorState::Initialized { name, initializer } = self.state {
-            Some((name, initializer))
+    pub(super) fn get_result(&self) -> Option<(Symbol, Option<Ty<'tcx>>, &'tcx Expr<'tcx>)> {
+        if let InitializeVisitorState::Initialized { name, ty, initializer } = self.state {
+            Some((name, ty, initializer))
         } else {
             None
         }
@@ -150,22 +154,25 @@ impl<'a, 'tcx> InitializeVisitor<'a, 'tcx> {
 impl<'a, 'tcx> Visitor<'tcx> for InitializeVisitor<'a, 'tcx> {
     type Map = Map<'tcx>;
 
-    fn visit_stmt(&mut self, stmt: &'tcx Stmt<'_>) {
+    fn visit_local(&mut self, l: &'tcx Local<'_>) {
         // Look for declarations of the variable
         if_chain! {
-            if let StmtKind::Local(local) = stmt.kind;
-            if local.pat.hir_id == self.var_id;
-            if let PatKind::Binding(.., ident, _) = local.pat.kind;
+            if l.pat.hir_id == self.var_id;
+            if let PatKind::Binding(.., ident, _) = l.pat.kind;
             then {
-                self.state = local.init.map_or(InitializeVisitorState::Declared(ident.name), |init| {
+                let ty = l.ty.map(|ty| hir_ty_to_ty(self.cx.tcx, ty));
+
+                self.state = l.init.map_or(InitializeVisitorState::Declared(ident.name, ty), |init| {
                     InitializeVisitorState::Initialized {
                         initializer: init,
+                        ty,
                         name: ident.name,
                     }
                 })
             }
         }
-        walk_stmt(self, stmt);
+
+        walk_local(self, l);
     }
 
     fn visit_expr(&mut self, expr: &'tcx Expr<'_>) {
@@ -195,15 +202,38 @@ impl<'a, 'tcx> Visitor<'tcx> for InitializeVisitor<'a, 'tcx> {
                         self.state = InitializeVisitorState::DontWarn;
                     },
                     ExprKind::Assign(lhs, rhs, _) if lhs.hir_id == expr.hir_id => {
-                        self.state = if_chain! {
-                            if self.depth == 0;
-                            if let InitializeVisitorState::Declared(name)
-                                | InitializeVisitorState::Initialized { name, ..} = self.state;
-                            then {
-                                InitializeVisitorState::Initialized { initializer: rhs, name }
-                            } else {
-                                InitializeVisitorState::DontWarn
+                        self.state = if self.depth == 0 {
+                            match self.state {
+                                InitializeVisitorState::Declared(name, mut ty) => {
+                                    if ty.is_none() {
+                                        if let ExprKind::Lit(Spanned {
+                                            node: LitKind::Int(_, LitIntType::Unsuffixed),
+                                            ..
+                                        }) = rhs.kind
+                                        {
+                                            ty = None;
+                                        } else {
+                                            ty = self.cx.typeck_results().expr_ty_opt(rhs);
+                                        }
+                                    }
+
+                                    InitializeVisitorState::Initialized {
+                                        initializer: rhs,
+                                        ty,
+                                        name,
+                                    }
+                                },
+                                InitializeVisitorState::Initialized { ty, name, .. } => {
+                                    InitializeVisitorState::Initialized {
+                                        initializer: rhs,
+                                        ty,
+                                        name,
+                                    }
+                                },
+                                _ => InitializeVisitorState::DontWarn,
                             }
+                        } else {
+                            InitializeVisitorState::DontWarn
                         }
                     },
                     ExprKind::AddrOf(BorrowKind::Ref, mutability, _) if mutability == Mutability::Mut => {
diff --git a/tests/ui/explicit_counter_loop.rs b/tests/ui/explicit_counter_loop.rs
index 81d8221bd13..aa966761feb 100644
--- a/tests/ui/explicit_counter_loop.rs
+++ b/tests/ui/explicit_counter_loop.rs
@@ -158,3 +158,33 @@ mod issue_4677 {
         }
     }
 }
+
+mod issue_7920 {
+    pub fn test() {
+        let slice = &[1, 2, 3];
+
+        let index_usize: usize = 0;
+        let mut idx_usize: usize = 0;
+
+        // should suggest `enumerate`
+        for _item in slice {
+            if idx_usize == index_usize {
+                break;
+            }
+
+            idx_usize += 1;
+        }
+
+        let index_u32: u32 = 0;
+        let mut idx_u32: u32 = 0;
+
+        // should suggest `zip`
+        for _item in slice {
+            if idx_u32 == index_u32 {
+                break;
+            }
+
+            idx_u32 += 1;
+        }
+    }
+}
diff --git a/tests/ui/explicit_counter_loop.stderr b/tests/ui/explicit_counter_loop.stderr
index 4cbacffe87b..9edddea651c 100644
--- a/tests/ui/explicit_counter_loop.stderr
+++ b/tests/ui/explicit_counter_loop.stderr
@@ -42,5 +42,19 @@ error: the variable `count` is used as a loop counter
 LL |         for _i in 3..10 {
    |         ^^^^^^^^^^^^^^^ help: consider using: `for (count, _i) in (3..10).enumerate()`
 
-error: aborting due to 7 previous errors
+error: the variable `idx_usize` is used as a loop counter
+  --> $DIR/explicit_counter_loop.rs:170:9
+   |
+LL |         for _item in slice {
+   |         ^^^^^^^^^^^^^^^^^^ help: consider using: `for (idx_usize, _item) in slice.into_iter().enumerate()`
+
+error: the variable `idx_u32` is used as a loop counter
+  --> $DIR/explicit_counter_loop.rs:182:9
+   |
+LL |         for _item in slice {
+   |         ^^^^^^^^^^^^^^^^^^ help: consider using: `for (idx_u32, _item) in (0_u32..).zip(slice.into_iter())`
+   |
+   = note: `idx_u32` is of type `u32`, making it ineligible for `Iterator::enumerate`
+
+error: aborting due to 9 previous errors