diff --git a/clippy_lints/src/loops/explicit_counter_loop.rs b/clippy_lints/src/loops/explicit_counter_loop.rs index 98e60f7ed85cf..bf9326b2901dc 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 6bdc560efd79d..f6a673f71355a 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 f9f515cc40a0f..c1e367b344ade 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>), // Declared but not (yet) initialized Initialized { name: Symbol, + ty: Option>, 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>, &'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 81d8221bd13e0..aa966761febdf 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 4cbacffe87bf4..9edddea651c26 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