Skip to content

Commit f69721f

Browse files
borsbors
committed
Auto merge of rust-lang#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: rust-lang#7920
2 parents c94d62b + 6809234 commit f69721f

File tree

5 files changed

+145
-35
lines changed

5 files changed

+145
-35
lines changed

‎clippy_lints/src/loops/explicit_counter_loop.rs‎

Lines changed: 47 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,15 @@
11
use super::{
22
get_span_of_entire_for_loop, make_iterator_snippet, IncrementVisitor, InitializeVisitor, EXPLICIT_COUNTER_LOOP,
33
};
4-
use clippy_utils::diagnostics::span_lint_and_sugg;
4+
use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then};
55
use clippy_utils::source::snippet_with_applicability;
66
use clippy_utils::{get_enclosing_block, is_integer_const};
77
use if_chain::if_chain;
88
use rustc_errors::Applicability;
99
use rustc_hir::intravisit::{walk_block, walk_expr};
1010
use rustc_hir::{Expr, Pat};
1111
use rustc_lint::LateContext;
12+
use rustc_middle::ty::{self, UintTy};
1213

1314
// To trigger the EXPLICIT_COUNTER_LOOP lint, a variable must be
1415
// incremented exactly once in the loop body, and initialized to zero
@@ -32,26 +33,61 @@ pub(super) fn check<'tcx>(
3233
walk_block(&mut initialize_visitor, block);
3334

3435
if_chain! {
35-
if let Some((name, initializer)) = initialize_visitor.get_result();
36+
if let Some((name, ty, initializer)) = initialize_visitor.get_result();
3637
if is_integer_const(cx, initializer, 0);
3738
then {
3839
let mut applicability = Applicability::MachineApplicable;
3940

4041
let for_span = get_span_of_entire_for_loop(expr);
4142

42-
span_lint_and_sugg(
43+
let int_name = match ty.map(ty::TyS::kind) {
44+
// usize or inferred
45+
Some(ty::Uint(UintTy::Usize)) | None => {
46+
span_lint_and_sugg(
47+
cx,
48+
EXPLICIT_COUNTER_LOOP,
49+
for_span.with_hi(arg.span.hi()),
50+
&format!("the variable `{}` is used as a loop counter", name),
51+
"consider using",
52+
format!(
53+
"for ({}, {}) in {}.enumerate()",
54+
name,
55+
snippet_with_applicability(cx, pat.span, "item", &mut applicability),
56+
make_iterator_snippet(cx, arg, &mut applicability),
57+
),
58+
applicability,
59+
);
60+
return;
61+
}
62+
Some(ty::Int(int_ty)) => int_ty.name_str(),
63+
Some(ty::Uint(uint_ty)) => uint_ty.name_str(),
64+
_ => return,
65+
};
66+
67+
span_lint_and_then(
4368
cx,
4469
EXPLICIT_COUNTER_LOOP,
4570
for_span.with_hi(arg.span.hi()),
4671
&format!("the variable `{}` is used as a loop counter", name),
47-
"consider using",
48-
format!(
49-
"for ({}, {}) in {}.enumerate()",
50-
name,
51-
snippet_with_applicability(cx, pat.span, "item", &mut applicability),
52-
make_iterator_snippet(cx, arg, &mut applicability),
53-
),
54-
applicability,
72+
|diag| {
73+
diag.span_suggestion(
74+
for_span.with_hi(arg.span.hi()),
75+
"consider using",
76+
format!(
77+
"for ({}, {}) in (0_{}..).zip({})",
78+
name,
79+
snippet_with_applicability(cx, pat.span, "item", &mut applicability),
80+
int_name,
81+
make_iterator_snippet(cx, arg, &mut applicability),
82+
),
83+
applicability,
84+
);
85+
86+
diag.note(&format!(
87+
"`{}` is of type `{}`, making it ineligible for `Iterator::enumerate`",
88+
name, int_name
89+
));
90+
},
5591
);
5692
}
5793
}

‎clippy_lints/src/loops/manual_memcpy.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -442,7 +442,7 @@ fn get_loop_counters<'a, 'tcx>(
442442
let mut initialize_visitor = InitializeVisitor::new(cx, expr, var_id);
443443
walk_block(&mut initialize_visitor, block);
444444

445-
initialize_visitor.get_result().map(|(_, initializer)| Start {
445+
initialize_visitor.get_result().map(|(_, _, initializer)| Start {
446446
id: var_id,
447447
kind: StartKind::Counter { initializer },
448448
})

‎clippy_lints/src/loops/utils.rs‎

Lines changed: 52 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,17 @@
11
use clippy_utils::ty::{has_iter_method, implements_trait};
22
use clippy_utils::{get_parent_expr, is_integer_const, path_to_local, path_to_local_id, sugg};
33
use if_chain::if_chain;
4+
use rustc_ast::ast::{LitIntType, LitKind};
45
use rustc_errors::Applicability;
5-
use rustc_hir::intravisit::{walk_expr, walk_pat, walk_stmt, NestedVisitorMap, Visitor};
6-
use rustc_hir::HirIdMap;
7-
use rustc_hir::{BinOpKind, BorrowKind, Expr, ExprKind, HirId, Mutability, Pat, PatKind, Stmt, StmtKind};
6+
use rustc_hir::intravisit::{walk_expr, walk_local, walk_pat, walk_stmt, NestedVisitorMap, Visitor};
7+
use rustc_hir::{BinOpKind, BorrowKind, Expr, ExprKind, HirId, HirIdMap, Local, Mutability, Pat, PatKind, Stmt};
88
use rustc_lint::LateContext;
99
use rustc_middle::hir::map::Map;
10+
use rustc_middle::ty::Ty;
1011
use rustc_span::source_map::Span;
12+
use rustc_span::source_map::Spanned;
1113
use rustc_span::symbol::{sym, Symbol};
14+
use rustc_typeck::hir_ty_to_ty;
1215
use std::iter::Iterator;
1316

1417
#[derive(Debug, PartialEq)]
@@ -106,10 +109,11 @@ impl<'a, 'tcx> Visitor<'tcx> for IncrementVisitor<'a, 'tcx> {
106109
}
107110

108111
enum InitializeVisitorState<'hir> {
109-
Initial, // Not examined yet
110-
Declared(Symbol), // Declared but not (yet) initialized
112+
Initial, // Not examined yet
113+
Declared(Symbol, Option<Ty<'hir>>), // Declared but not (yet) initialized
111114
Initialized {
112115
name: Symbol,
116+
ty: Option<Ty<'hir>>,
113117
initializer: &'hir Expr<'hir>,
114118
},
115119
DontWarn,
@@ -138,9 +142,9 @@ impl<'a, 'tcx> InitializeVisitor<'a, 'tcx> {
138142
}
139143
}
140144

141-
pub(super) fn get_result(&self) -> Option<(Symbol, &'tcx Expr<'tcx>)> {
142-
if let InitializeVisitorState::Initialized { name, initializer } = self.state {
143-
Some((name, initializer))
145+
pub(super) fn get_result(&self) -> Option<(Symbol, Option<Ty<'tcx>>, &'tcx Expr<'tcx>)> {
146+
if let InitializeVisitorState::Initialized { name, ty, initializer } = self.state {
147+
Some((name, ty, initializer))
144148
} else {
145149
None
146150
}
@@ -150,22 +154,25 @@ impl<'a, 'tcx> InitializeVisitor<'a, 'tcx> {
150154
impl<'a, 'tcx> Visitor<'tcx> for InitializeVisitor<'a, 'tcx> {
151155
type Map = Map<'tcx>;
152156

153-
fn visit_stmt(&mut self, stmt: &'tcx Stmt<'_>) {
157+
fn visit_local(&mut self, l: &'tcx Local<'_>) {
154158
// Look for declarations of the variable
155159
if_chain! {
156-
if let StmtKind::Local(local) = stmt.kind;
157-
if local.pat.hir_id == self.var_id;
158-
if let PatKind::Binding(.., ident, _) = local.pat.kind;
160+
if l.pat.hir_id == self.var_id;
161+
if let PatKind::Binding(.., ident, _) = l.pat.kind;
159162
then {
160-
self.state = local.init.map_or(InitializeVisitorState::Declared(ident.name), |init| {
163+
let ty = l.ty.map(|ty| hir_ty_to_ty(self.cx.tcx, ty));
164+
165+
self.state = l.init.map_or(InitializeVisitorState::Declared(ident.name, ty), |init| {
161166
InitializeVisitorState::Initialized {
162167
initializer: init,
168+
ty,
163169
name: ident.name,
164170
}
165171
})
166172
}
167173
}
168-
walk_stmt(self, stmt);
174+
175+
walk_local(self, l);
169176
}
170177

171178
fn visit_expr(&mut self, expr: &'tcx Expr<'_>) {
@@ -195,15 +202,38 @@ impl<'a, 'tcx> Visitor<'tcx> for InitializeVisitor<'a, 'tcx> {
195202
self.state = InitializeVisitorState::DontWarn;
196203
},
197204
ExprKind::Assign(lhs, rhs, _) if lhs.hir_id == expr.hir_id => {
198-
self.state = if_chain! {
199-
if self.depth == 0;
200-
if let InitializeVisitorState::Declared(name)
201-
| InitializeVisitorState::Initialized { name, ..} = self.state;
202-
then {
203-
InitializeVisitorState::Initialized { initializer: rhs, name }
204-
} else {
205-
InitializeVisitorState::DontWarn
205+
self.state = if self.depth == 0 {
206+
match self.state {
207+
InitializeVisitorState::Declared(name, mut ty) => {
208+
if ty.is_none() {
209+
if let ExprKind::Lit(Spanned {
210+
node: LitKind::Int(_, LitIntType::Unsuffixed),
211+
..
212+
}) = rhs.kind
213+
{
214+
ty = None;
215+
} else {
216+
ty = self.cx.typeck_results().expr_ty_opt(rhs);
217+
}
218+
}
219+
220+
InitializeVisitorState::Initialized {
221+
initializer: rhs,
222+
ty,
223+
name,
224+
}
225+
},
226+
InitializeVisitorState::Initialized { ty, name, .. } => {
227+
InitializeVisitorState::Initialized {
228+
initializer: rhs,
229+
ty,
230+
name,
231+
}
232+
},
233+
_ => InitializeVisitorState::DontWarn,
206234
}
235+
} else {
236+
InitializeVisitorState::DontWarn
207237
}
208238
},
209239
ExprKind::AddrOf(BorrowKind::Ref, mutability, _) if mutability == Mutability::Mut => {

‎tests/ui/explicit_counter_loop.rs‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -158,3 +158,33 @@ mod issue_4677 {
158158
}
159159
}
160160
}
161+
162+
mod issue_7920 {
163+
pub fn test() {
164+
let slice = &[1, 2, 3];
165+
166+
let index_usize: usize = 0;
167+
let mut idx_usize: usize = 0;
168+
169+
// should suggest `enumerate`
170+
for _item in slice {
171+
if idx_usize == index_usize {
172+
break;
173+
}
174+
175+
idx_usize += 1;
176+
}
177+
178+
let index_u32: u32 = 0;
179+
let mut idx_u32: u32 = 0;
180+
181+
// should suggest `zip`
182+
for _item in slice {
183+
if idx_u32 == index_u32 {
184+
break;
185+
}
186+
187+
idx_u32 += 1;
188+
}
189+
}
190+
}

‎tests/ui/explicit_counter_loop.stderr‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,5 +42,19 @@ error: the variable `count` is used as a loop counter
4242
LL | for _i in 3..10 {
4343
| ^^^^^^^^^^^^^^^ help: consider using: `for (count, _i) in (3..10).enumerate()`
4444

45-
error: aborting due to 7 previous errors
45+
error: the variable `idx_usize` is used as a loop counter
46+
--> $DIR/explicit_counter_loop.rs:170:9
47+
|
48+
LL | for _item in slice {
49+
| ^^^^^^^^^^^^^^^^^^ help: consider using: `for (idx_usize, _item) in slice.into_iter().enumerate()`
50+
51+
error: the variable `idx_u32` is used as a loop counter
52+
--> $DIR/explicit_counter_loop.rs:182:9
53+
|
54+
LL | for _item in slice {
55+
| ^^^^^^^^^^^^^^^^^^ help: consider using: `for (idx_u32, _item) in (0_u32..).zip(slice.into_iter())`
56+
|
57+
= note: `idx_u32` is of type `u32`, making it ineligible for `Iterator::enumerate`
58+
59+
error: aborting due to 9 previous errors
4660

0 commit comments

Comments
 (0)