Skip to content

loops: Fix multi-part counter detection in explicit_counter_loop and … #16203

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
shreya-d2604 wants to merge 1 commit into
base: master
Choose a base branch
from
+249 −125
Open

loops: Fix multi-part counter detection in explicit_counter_loop and … #16203

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
90 changes: 58 additions & 32 deletions clippy_lints/src/loops/explicit_counter_loop.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use super::{EXPLICIT_COUNTER_LOOP, IncrementVisitor, InitializeVisitor, make_iterator_snippet};
use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then};
use clippy_utils::diagnostics::span_lint_and_then;
use clippy_utils::source::snippet_with_applicability;
use clippy_utils::{get_enclosing_block, is_integer_const};
use rustc_ast::Label;
Expand All @@ -12,6 +12,8 @@ use rustc_middle::ty::{self, Ty, UintTy};
// To trigger the EXPLICIT_COUNTER_LOOP lint, a variable must be
// incremented exactly once in the loop body, and initialized to zero
// at the start of the loop.
// ... (imports and prelude)

pub(super) fn check<'tcx>(
cx: &LateContext<'tcx>,
pat: &'tcx Pat<'_>,
Expand All @@ -20,66 +22,90 @@ pub(super) fn check<'tcx>(
expr: &'tcx Expr<'_>,
label: Option<Label>,
) {
// Look for variables that are incremented once per loop iteration.
let mut increment_visitor = IncrementVisitor::new(cx);
walk_expr(&mut increment_visitor, body);

// For each candidate, check the parent block to see if
// it's initialized to zero at the start of the loop.
if let Some(block) = get_enclosing_block(cx, expr.hir_id) {
for id in increment_visitor.into_results() {
for (id, increment_span) in increment_visitor.into_results() {
let mut initialize_visitor = InitializeVisitor::new(cx, expr, id);
walk_block(&mut initialize_visitor, block);

if let Some((name, ty, initializer)) = initialize_visitor.get_result()
if let Some((name, ty, initializer, init_span)) = initialize_visitor.get_result()
&& is_integer_const(cx, initializer, 0)
{
let mut applicability = Applicability::MaybeIncorrect;
let span = expr.span.with_hi(arg.span.hi());
let loop_label = label.map_or(String::new(), |l| format!("{}: ", l.ident.name));
let int_name = match ty.map(Ty::kind) {
// usize or inferred

// Determine the suggestion and optional note based on the variable's type
let suggestion_info = match ty.map(Ty::kind) {
Some(ty::Uint(UintTy::Usize)) | None => {
span_lint_and_sugg(
cx,
EXPLICIT_COUNTER_LOOP,
span,
format!("the variable `{name}` is used as a loop counter"),
"consider using",
// usize or inferred (uses enumerate)
Some((
format!(
"{loop_label}for ({name}, {}) in {}.enumerate()",
snippet_with_applicability(cx, pat.span, "item", &mut applicability),
make_iterator_snippet(cx, arg, &mut applicability),
),
applicability,
);
return;
None,
))
},
Some(ty::Int(int_ty)) => {
// Signed integer types (uses (0_type..).zip)
let int_name = int_ty.name_str();
Some((
format!(
"{loop_label}for ({name}, {}) in (0_{int_name}..).zip({})",
snippet_with_applicability(cx, pat.span, "item", &mut applicability),
make_iterator_snippet(cx, arg, &mut applicability),
),
Some(format!(
"`{name}` is of type `{int_name}`, making it ineligible for `Iterator::enumerate`"
)),
))
},
Some(ty::Uint(uint_ty)) => {
// Other unsigned integer types (uses (0_type..).zip)
let uint_name = uint_ty.name_str();
Some((
format!(
"{loop_label}for ({name}, {}) in (0_{uint_name}..).zip({})",
snippet_with_applicability(cx, pat.span, "item", &mut applicability),
make_iterator_snippet(cx, arg, &mut applicability),
),
Some(format!(
"`{name}` is of type `{uint_name}`, making it ineligible for `Iterator::enumerate`"
)),
))
},
Some(ty::Int(int_ty)) => int_ty.name_str(),
Some(ty::Uint(uint_ty)) => uint_ty.name_str(),
_ => return,
// Anything else (e.g., f32, struct) is ineligible
_ => None,
};

// If ineligible, stop processing this counter variable
let Some((suggestion, note)) = suggestion_info else {
continue;
};

span_lint_and_then(
cx,
EXPLICIT_COUNTER_LOOP,
span,
expr.span,
format!("the variable `{name}` is used as a loop counter"),
|diag| {
diag.span_suggestion(
span,
"consider using",
format!(
"{loop_label}for ({name}, {}) in (0_{int_name}..).zip({})",
snippet_with_applicability(cx, pat.span, "item", &mut applicability),
make_iterator_snippet(cx, arg, &mut applicability),
),
diag.multipart_suggestion(
"consider rewriting the loop to use an iterator adapter",
vec![
(span, suggestion),
(init_span, String::new()),
(increment_span, String::new()),
],
applicability,
);

diag.note(format!(
"`{name}` is of type `{int_name}`, making it ineligible for `Iterator::enumerate`"
));
if let Some(note_text) = note {
diag.note(note_text);
}
},
);
}
Expand Down
18 changes: 8 additions & 10 deletions clippy_lints/src/loops/manual_memcpy.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -442,30 +442,28 @@ fn get_assignments<'a, 'tcx>(
.map(get_assignment)
}

fn get_loop_counters<'a, 'tcx>(
cx: &'a LateContext<'tcx>,
fn get_loop_counters<'tcx>(
cx: &LateContext<'tcx>,
body: &'tcx Block<'tcx>,
expr: &'tcx Expr<'_>,
) -> Option<impl Iterator<Item = Start<'tcx>> + 'a> {
// Look for variables that are incremented once per loop iteration.
) -> Option<Vec<Start<'tcx>>> {
let mut increment_visitor = IncrementVisitor::new(cx);
walk_block(&mut increment_visitor, body);

// For each candidate, check the parent block to see if
// it's initialized to zero at the start of the loop.
get_enclosing_block(cx, expr.hir_id).and_then(|block| {
get_enclosing_block(cx, expr.hir_id).map(move |block| {
increment_visitor
.into_results()
.filter_map(move |var_id| {
.filter_map(move |(var_id, _)| {
// var_id is now the HirId struct
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 },
})
})
.into()
.collect()
})
}

Expand Down
76 changes: 28 additions & 48 deletions clippy_lints/src/loops/utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,20 +1,21 @@
use clippy_utils::res::MaybeResPath;
use clippy_utils::ty::{has_iter_method, implements_trait};
use clippy_utils::{get_parent_expr, is_integer_const, sugg};
use rustc_ast::ast::{LitIntType, LitKind};

use rustc_errors::Applicability;
use rustc_hir::intravisit::{Visitor, walk_expr, walk_local};
use rustc_hir::{AssignOpKind, BorrowKind, Expr, ExprKind, HirId, HirIdMap, LetStmt, Mutability, PatKind};
use rustc_lint::LateContext;
use rustc_middle::hir::nested_filter;
use rustc_middle::ty::{self, Ty};
use rustc_span::source_map::Spanned;

use rustc_span::Span;
use rustc_span::symbol::{Symbol, sym};

#[derive(Debug, PartialEq, Eq)]
enum IncrementVisitorVarState {
Initial, // Not examined yet
IncrOnce, // Incremented exactly once, may be a loop counter
Initial, // Not examined yet
IncrOnce(Span), // Incremented exactly once, may be a loop counter
DontWarn,
}

Expand All @@ -34,10 +35,10 @@ impl<'a, 'tcx> IncrementVisitor<'a, 'tcx> {
}
}

pub(super) fn into_results(self) -> impl Iterator<Item = HirId> {
pub(super) fn into_results(self) -> impl Iterator<Item = (HirId, Span)> {
self.states.into_iter().filter_map(|(id, state)| {
if state == IncrementVisitorVarState::IncrOnce {
Some(id)
if let IncrementVisitorVarState::IncrOnce(span) = state {
Some((id, span))
} else {
None
}
Expand All @@ -51,7 +52,7 @@ impl<'tcx> Visitor<'tcx> for IncrementVisitor<'_, 'tcx> {
if let Some(def_id) = expr.res_local_id() {
if let Some(parent) = get_parent_expr(self.cx, expr) {
let state = self.states.entry(def_id).or_insert(IncrementVisitorVarState::Initial);
if *state == IncrementVisitorVarState::IncrOnce {
if matches!(*state, IncrementVisitorVarState::IncrOnce(_)) {
*state = IncrementVisitorVarState::DontWarn;
return;
}
Expand All @@ -61,10 +62,10 @@ impl<'tcx> Visitor<'tcx> for IncrementVisitor<'_, 'tcx> {
if lhs.hir_id == expr.hir_id {
*state = if op.node == AssignOpKind::AddAssign
&& is_integer_const(self.cx, rhs, 1)
&& *state == IncrementVisitorVarState::Initial
&& matches!(*state, IncrementVisitorVarState::Initial)
&& self.depth == 0
{
IncrementVisitorVarState::IncrOnce
IncrementVisitorVarState::IncrOnce(parent.span)
} else {
// Assigned some other value or assigned multiple times
IncrementVisitorVarState::DontWarn
Expand Down Expand Up @@ -97,12 +98,14 @@ impl<'tcx> Visitor<'tcx> for IncrementVisitor<'_, 'tcx> {
}

enum InitializeVisitorState<'hir> {
Initial, // Not examined yet
Initial, // Not examined yet
#[allow(dead_code)]
Declared(Symbol, Option<Ty<'hir>>), // Declared but not (yet) initialized
Initialized {
name: Symbol,
ty: Option<Ty<'hir>>,
initializer: &'hir Expr<'hir>,
stmt_span: Span,
},
DontWarn,
}
Expand Down Expand Up @@ -130,9 +133,15 @@ impl<'a, 'tcx> InitializeVisitor<'a, 'tcx> {
}
}

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))
pub(super) fn get_result(&self) -> Option<(Symbol, Option<Ty<'tcx>>, &'tcx Expr<'tcx>, Span)> {
if let InitializeVisitorState::Initialized {
name,
ty,
initializer,
stmt_span,
} = self.state
{
Some((name, ty, initializer, stmt_span))
} else {
None
}
Expand All @@ -154,6 +163,7 @@ impl<'tcx> Visitor<'tcx> for InitializeVisitor<'_, 'tcx> {
initializer: init,
ty,
name: ident.name,
stmt_span: l.span,
}
});
}
Expand All @@ -175,6 +185,7 @@ impl<'tcx> Visitor<'tcx> for InitializeVisitor<'_, 'tcx> {
return;
}

// If node is the desired variable, see how it's used
// If node is the desired variable, see how it's used
if expr.res_local_id() == Some(self.var_id) {
if self.past_loop {
Expand All @@ -187,40 +198,9 @@ impl<'tcx> Visitor<'tcx> for InitializeVisitor<'_, 'tcx> {
ExprKind::AssignOp(_, lhs, _) if lhs.hir_id == expr.hir_id => {
self.state = InitializeVisitorState::DontWarn;
},
ExprKind::Assign(lhs, rhs, _) if lhs.hir_id == expr.hir_id => {
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
}
// If the variable is reassigned (`i = value;`), it invalidates the simple counter pattern.
ExprKind::Assign(lhs, _, _) if lhs.hir_id == expr.hir_id => {
self.state = InitializeVisitorState::DontWarn;
},
ExprKind::AddrOf(BorrowKind::Ref, Mutability::Mut, _) => {
self.state = InitializeVisitorState::DontWarn;
Expand Down
3 changes: 0 additions & 3 deletions tests/ui/explicit_counter_loop.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,8 @@ fn main() {
let mut _index = 1;
_index = 0;
for _v in &vec {
//~^ explicit_counter_loop

_index += 1
}

let mut _index = 0;
for _v in &mut vec {
//~^ explicit_counter_loop
Expand Down
Loading