diff options
| -rw-r--r-- | clippy_lints/src/loops/for_loop_explicit_counter.rs | 56 | ||||
| -rw-r--r-- | clippy_lints/src/loops/mod.rs | 65 | ||||
| -rw-r--r-- | clippy_lints/src/loops/utils.rs | 12 |
3 files changed, 71 insertions, 62 deletions
diff --git a/clippy_lints/src/loops/for_loop_explicit_counter.rs b/clippy_lints/src/loops/for_loop_explicit_counter.rs new file mode 100644 index 00000000000..68fee269eb1 --- /dev/null +++ b/clippy_lints/src/loops/for_loop_explicit_counter.rs @@ -0,0 +1,56 @@ +use super::{get_span_of_entire_for_loop, make_iterator_snippet, IncrementVisitor, InitializeVisitor}; +use crate::utils::{get_enclosing_block, is_integer_const, snippet_with_applicability, span_lint_and_sugg}; +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; + +// 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. +pub(super) fn check_for_loop_explicit_counter<'tcx>( + cx: &LateContext<'tcx>, + pat: &'tcx Pat<'_>, + arg: &'tcx Expr<'_>, + body: &'tcx Expr<'_>, + expr: &'tcx Expr<'_>, +) { + // 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() { + let mut initialize_visitor = InitializeVisitor::new(cx, expr, id); + walk_block(&mut initialize_visitor, block); + + if_chain! { + if let Some((name, 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( + cx, + super::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, + ); + } + } + } + } +} diff --git a/clippy_lints/src/loops/mod.rs b/clippy_lints/src/loops/mod.rs index 204f9f9b255..f06b3ae4c9f 100644 --- a/clippy_lints/src/loops/mod.rs +++ b/clippy_lints/src/loops/mod.rs @@ -1,4 +1,5 @@ mod for_loop_arg; +mod for_loop_explicit_counter; mod for_loop_over_map_kv; mod for_loop_range; mod for_mut_range_bound; @@ -32,7 +33,7 @@ use rustc_session::{declare_lint_pass, declare_tool_lint}; use rustc_span::source_map::Span; use rustc_span::symbol::{sym, Ident, Symbol}; use std::iter::{once, Iterator}; -use utils::make_iterator_snippet; +use utils::{get_span_of_entire_for_loop, make_iterator_snippet}; declare_clippy_lint! { /// **What it does:** Checks for for-loops that manually copy items between @@ -857,7 +858,7 @@ fn check_for_loop<'tcx>( let is_manual_memcpy_triggered = detect_manual_memcpy(cx, pat, arg, body, expr); if !is_manual_memcpy_triggered { for_loop_range::check_for_loop_range(cx, pat, arg, body, expr); - check_for_loop_explicit_counter(cx, pat, arg, body, expr); + for_loop_explicit_counter::check_for_loop_explicit_counter(cx, pat, arg, body, expr); } for_loop_arg::check_for_loop_arg(cx, pat, arg, expr); for_loop_over_map_kv::check_for_loop_over_map_kv(cx, pat, arg, body, expr); @@ -867,17 +868,6 @@ fn check_for_loop<'tcx>( manual_flatten::check_manual_flatten(cx, pat, arg, body, span); } -// this function assumes the given expression is a `for` loop. -fn get_span_of_entire_for_loop(expr: &Expr<'_>) -> Span { - // for some reason this is the only way to get the `Span` - // of the entire `for` loop - if let ExprKind::Match(_, arms, _) = &expr.kind { - arms[0].body.span - } else { - unreachable!() - } -} - /// a wrapper of `Sugg`. Besides what `Sugg` do, this removes unnecessary `0`; /// and also, it avoids subtracting a variable from the same one by replacing it with `0`. /// it exists for the convenience of the overloaded operators while normal functions can do the @@ -1474,55 +1464,6 @@ fn detect_same_item_push<'tcx>( } } -// 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. -fn check_for_loop_explicit_counter<'tcx>( - cx: &LateContext<'tcx>, - pat: &'tcx Pat<'_>, - arg: &'tcx Expr<'_>, - body: &'tcx Expr<'_>, - expr: &'tcx Expr<'_>, -) { - // 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() { - let mut initialize_visitor = InitializeVisitor::new(cx, expr, id); - walk_block(&mut initialize_visitor, block); - - if_chain! { - if let Some((name, 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( - 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, - ); - } - } - } - } -} - fn check_for_single_element_loop<'tcx>( cx: &LateContext<'tcx>, pat: &'tcx Pat<'_>, diff --git a/clippy_lints/src/loops/utils.rs b/clippy_lints/src/loops/utils.rs index b06df2d75f3..b9c934d5e54 100644 --- a/clippy_lints/src/loops/utils.rs +++ b/clippy_lints/src/loops/utils.rs @@ -2,6 +2,18 @@ use crate::utils::{get_trait_def_id, has_iter_method, implements_trait, paths, s use rustc_errors::Applicability; use rustc_hir::{BorrowKind, Expr, ExprKind, Mutability}; use rustc_lint::LateContext; +use rustc_span::source_map::Span; + +// this function assumes the given expression is a `for` loop. +pub(super) fn get_span_of_entire_for_loop(expr: &Expr<'_>) -> Span { + // for some reason this is the only way to get the `Span` + // of the entire `for` loop + if let ExprKind::Match(_, arms, _) = &expr.kind { + arms[0].body.span + } else { + unreachable!() + } +} /// If `arg` was the argument to a `for` loop, return the "cleanest" way of writing the /// actual `Iterator` that the loop uses. |