Skip to content

Commit

Permalink
Auto merge of rust-lang#7950 - Serial-ATA:issue-7920, r=llogiq
Browse files Browse the repository at this point in the history 
Fix `explicit_counter_loop` suggestion for non-usize types

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

closes: rust-lang#7920
  • Loading branch information
@bors
bors committed Nov 9, 2021
2 parents c94d62b + 6809234 commit f69721f
Show file tree
Hide file tree
Showing 5 changed files with 145 additions and 35 deletions.
58 changes: 47 additions & 11 deletions clippy_lints/src/loops/explicit_counter_loop.rs
View file
Original file line number Diff line number Diff line change
@@ -1,14 +1,15 @@
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;
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
Expand All @@ -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
));
},
);
}
}
Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/loops/manual_memcpy.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 },
})
Expand Down
74 changes: 52 additions & 22 deletions clippy_lints/src/loops/utils.rs
View file
Original file line number Diff line number Diff line change
@@ -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)]
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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
}
Expand All @@ -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<'_>) {
Expand Down Expand Up @@ -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 => {
Expand Down
30 changes: 30 additions & 0 deletions tests/ui/explicit_counter_loop.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
}
16 changes: 15 additions & 1 deletion tests/ui/explicit_counter_loop.stderr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

0 comments on commit f69721f

Please sign in to comment.