From 8fcade651bea9a3f07615ed80f842dc95f2e7550 Mon Sep 17 00:00:00 2001 From: linshuy2 Date: Wed, 25 Feb 2026 00:50:49 +0000 Subject: [PATCH] fix: `needless_range_loop` suggests wrongly for nested index --- clippy_lints/src/loops/needless_range_loop.rs | 145 +++++++++++------ tests/ui/needless_range_loop.rs | 40 +++++ tests/ui/needless_range_loop.stderr | 154 +++++++++++++++++- tests/ui/needless_range_loop2.stderr | 40 +++++ 4 files changed, 329 insertions(+), 50 deletions(-) diff --git a/clippy_lints/src/loops/needless_range_loop.rs b/clippy_lints/src/loops/needless_range_loop.rs index a0de464ff7f8..d858ed3a42cc 100644 --- a/clippy_lints/src/loops/needless_range_loop.rs +++ b/clippy_lints/src/loops/needless_range_loop.rs @@ -1,9 +1,12 @@ +use std::hash::{Hash, Hasher}; +use std::{iter, mem}; + use super::NEEDLESS_RANGE_LOOP; use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::source::snippet; use clippy_utils::ty::has_iter_method; use clippy_utils::visitors::is_local_used; -use clippy_utils::{SpanlessEq, contains_name, higher, is_integer_const, peel_hir_expr_while, sugg}; +use clippy_utils::{SpanlessEq, SpanlessHash, contains_name, higher, is_integer_const, peel_hir_expr_while, sugg}; use rustc_ast::ast; use rustc_data_structures::fx::{FxHashMap, FxHashSet, FxIndexMap}; use rustc_errors::Applicability; @@ -13,8 +16,8 @@ use rustc_hir::{BinOpKind, BorrowKind, Closure, Expr, ExprKind, HirId, Mutabilit use rustc_lint::LateContext; use rustc_middle::middle::region; use rustc_middle::ty::{self, Ty}; +use rustc_span::Span; use rustc_span::symbol::{Symbol, sym}; -use std::{iter, mem}; /// Checks for looping over a range and then indexing a sequence with it. /// The iteratee must be a range literal. @@ -53,14 +56,10 @@ pub(super) fn check<'tcx>( if visitor.indexed_indirectly.is_empty() && !visitor.unnamed_indexed_indirectly && !visitor.unnamed_indexed_directly - && visitor.indexed_directly.len() == 1 + && let mut indexed_directly_iter = visitor.indexed_directly.into_iter() + && let Some(((indexed, indexed_extended), (indexed_extent, indexed_span))) = indexed_directly_iter.next() + && indexed_directly_iter.next().is_none() { - let (indexed, (indexed_extent, indexed_ty)) = visitor - .indexed_directly - .into_iter() - .next() - .expect("already checked that we have exactly 1 element"); - // ensure that the indexed variable was declared before the loop, see #601 if let Some(indexed_extent) = indexed_extent { let parent_def_id = cx.tcx.hir_get_parent_item(expr.hir_id); @@ -72,6 +71,7 @@ pub(super) fn check<'tcx>( } // don't lint if the container that is indexed does not have .iter() method + let indexed_ty = cx.typeck_results().expr_ty(indexed_extended.expr); let has_iter = has_iter_method(cx, indexed_ty); if has_iter.is_none() { return; @@ -146,44 +146,50 @@ pub(super) fn check<'tcx>( mem::swap(&mut method_1, &mut method_2); } - if visitor.nonindex { - span_lint_and_then( - cx, - NEEDLESS_RANGE_LOOP, - span, - format!("the loop variable `{}` is used to index `{indexed}`", ident.name), - |diag| { + let indexed_extended_snippet = snippet(cx, indexed_extended.expr.span, ".."); + span_lint_and_then( + cx, + NEEDLESS_RANGE_LOOP, + span, + if visitor.nonindex { + format!( + "the loop variable `{}` is used to index `{indexed_extended_snippet}`", + ident.name + ) + } else { + format!( + "the loop variable `{}` is only used to index `{indexed_extended_snippet}`", + ident.name + ) + }, + |diag| { + diag.span_note(indexed_span, "for this index operation"); + if visitor.nonindex { diag.multipart_suggestion( "consider using an iterator and enumerate()", vec![ (pat.span, format!("({}, )", ident.name)), - (span, format!("{indexed}.{method}().enumerate(){method_1}{method_2}")), + ( + span, + format!("{indexed_extended_snippet}.{method}().enumerate(){method_1}{method_2}"), + ), ], Applicability::HasPlaceholders, ); - }, - ); - } else { - let repl = if starts_at_zero && take_is_empty { - format!("&{ref_mut}{indexed}") - } else { - format!("{indexed}.{method}(){method_1}{method_2}") - }; - - span_lint_and_then( - cx, - NEEDLESS_RANGE_LOOP, - span, - format!("the loop variable `{}` is only used to index `{indexed}`", ident.name), - |diag| { + } else { + let repl = if starts_at_zero && take_is_empty { + format!("&{ref_mut}{indexed_extended_snippet}") + } else { + format!("{indexed_extended_snippet}.{method}(){method_1}{method_2}") + }; diag.multipart_suggestion( "consider using an iterator", vec![(pat.span, "".to_string()), (span, repl)], Applicability::HasPlaceholders, ); - }, - ); - } + } + }, + ); } } } @@ -235,7 +241,7 @@ struct VarVisitor<'a, 'tcx> { unnamed_indexed_indirectly: bool, /// subset of `indexed` of vars that are indexed directly: `v[i]` /// this will not contain cases like `v[calc_index(i)]` or `v[(i + 4) % N]` - indexed_directly: FxIndexMap, Ty<'tcx>)>, + indexed_directly: FxIndexMap<(Symbol, SpanlessExpr<'a, 'tcx>), (Option, Span)>, /// directly indexed literals, like `[1, 2, 3][i]` unnamed_indexed_directly: bool, /// Any names that are used outside an index operation. @@ -261,13 +267,24 @@ impl<'tcx> VarVisitor<'_, 'tcx> { index_used_directly &= matches!(idx.kind, ExprKind::Path(_)); } // Handle nested indices - let seqexpr = peel_hir_expr_while(seqexpr, |e| { - if let ExprKind::Index(e, idx, _) = e.kind { - if is_local_used(self.cx, idx, self.var) { - used_cnt += 1; - index_used_directly &= matches!(idx.kind, ExprKind::Path(_)); - } - Some(e) + // For example, in `a.b[0][i][i]`, we will have `nested_seqexpr` be `a.b[0]`, with the corresponding + // `nested_seqexpr_index_span` be the span of `a.b[0][i]`, and `seqexpr` be `a.b`. + let mut nested_seqexpr_index_span = expr.span; + let nested_seqexpr = peel_hir_expr_while(seqexpr, |e| { + if let ExprKind::Index(inner, idx, _) = e.kind + && is_local_used(self.cx, idx, self.var) + { + used_cnt += 1; + index_used_directly &= matches!(idx.kind, ExprKind::Path(_)); + nested_seqexpr_index_span = e.span; + Some(inner) + } else { + None + } + }); + let seqexpr = peel_hir_expr_while(nested_seqexpr, |e| { + if let ExprKind::Index(inner, _, _) | ExprKind::Field(inner, _) = e.kind { + Some(inner) } else { None } @@ -299,8 +316,14 @@ impl<'tcx> VarVisitor<'_, 'tcx> { .unwrap(); if index_used_directly { self.indexed_directly.insert( - seqvar.segments[0].ident.name, - (Some(extent), self.cx.typeck_results().node_type(seqexpr.hir_id)), + ( + seqvar.segments[0].ident.name, + SpanlessExpr { + cx: self.cx, + expr: nested_seqexpr, + }, + ), + (Some(extent), nested_seqexpr_index_span), ); } else { self.indexed_indirectly @@ -311,8 +334,14 @@ impl<'tcx> VarVisitor<'_, 'tcx> { Res::Def(DefKind::Static { .. } | DefKind::Const, ..) => { if index_used_directly { self.indexed_directly.insert( - seqvar.segments[0].ident.name, - (None, self.cx.typeck_results().node_type(seqexpr.hir_id)), + ( + seqvar.segments[0].ident.name, + SpanlessExpr { + cx: self.cx, + expr: nested_seqexpr, + }, + ), + (None, nested_seqexpr_index_span), ); } else { self.indexed_indirectly.insert(seqvar.segments[0].ident.name, None); @@ -418,3 +447,25 @@ impl<'tcx> Visitor<'tcx> for VarVisitor<'_, 'tcx> { self.prefer_mutable = old; } } + +struct SpanlessExpr<'cx, 'tcx> { + cx: &'cx LateContext<'tcx>, + expr: &'cx Expr<'tcx>, +} + +impl PartialEq for SpanlessExpr<'_, '_> { + fn eq(&self, other: &Self) -> bool { + let mut eq = SpanlessEq::new(self.cx); + eq.eq_expr(self.expr, other.expr) + } +} + +impl Eq for SpanlessExpr<'_, '_> {} + +impl Hash for SpanlessExpr<'_, '_> { + fn hash(&self, state: &mut H) { + let mut hash = SpanlessHash::new(self.cx); + hash.hash_expr(self.expr); + state.write_u64(hash.finish()); + } +} diff --git a/tests/ui/needless_range_loop.rs b/tests/ui/needless_range_loop.rs index ea4591d8b71a..2b98f139cc17 100644 --- a/tests/ui/needless_range_loop.rs +++ b/tests/ui/needless_range_loop.rs @@ -242,3 +242,43 @@ fn issue_15068() { let _ = a[0][i]; } } + +fn issue16631() { + let mut matrix: Vec> = Vec::new(); + for i in 0..=2 { + //~^ needless_range_loop + matrix[i][i] = true; + } + + let values = [[0; 4]; 4]; + let col = 2; + for i in 0..4 { + //~^ needless_range_loop + let _ = values[i][col]; + } + + let mut colors = [[0; 3]; 4]; + for i in 0..3 { + colors[2][i] = ((u16::from(colors[0][i]) * 2 + u16::from(colors[1][i]) + 1) / 3) as u8; + colors[3][i] = ((u16::from(colors[0][i]) + u16::from(colors[1][i]) * 2 + 1) / 3) as u8; + } + + let mut colors = [[0; 3]; 4]; + let i = 0; + for j in 0..3 { + colors[i][j] = colors[0][j]; + } + + let mut colors = [[0; 3]; 4]; + for j in 0..3 { + //~^ needless_range_loop + let _ = colors[0][j]; + } + + struct Wrapper(T); + let mut wrapper = Wrapper([0; 3]); + for i in 0..3 { + //~^ needless_range_loop + let _ = wrapper.0[i]; + } +} diff --git a/tests/ui/needless_range_loop.stderr b/tests/ui/needless_range_loop.stderr index 33a519d8a80d..1a6b2d122490 100644 --- a/tests/ui/needless_range_loop.stderr +++ b/tests/ui/needless_range_loop.stderr @@ -4,6 +4,11 @@ error: the loop variable `i` is only used to index `vec` LL | for i in 0..vec.len() { | ^^^^^^^^^^^^ | +note: for this index operation + --> tests/ui/needless_range_loop.rs:19:24 + | +LL | println!("{}", vec[i]); + | ^^^^^^ = note: `-D clippy::needless-range-loop` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::needless_range_loop)]` help: consider using an iterator @@ -18,6 +23,11 @@ error: the loop variable `i` is only used to index `vec` LL | for i in 0..vec.len() { | ^^^^^^^^^^^^ | +note: for this index operation + --> tests/ui/needless_range_loop.rs:30:17 + | +LL | let _ = vec[i]; + | ^^^^^^ help: consider using an iterator | LL - for i in 0..vec.len() { @@ -30,6 +40,11 @@ error: the loop variable `j` is only used to index `STATIC` LL | for j in 0..4 { | ^^^^ | +note: for this index operation + --> tests/ui/needless_range_loop.rs:37:26 + | +LL | println!("{:?}", STATIC[j]); + | ^^^^^^^^^ help: consider using an iterator | LL - for j in 0..4 { @@ -42,6 +57,11 @@ error: the loop variable `j` is only used to index `CONST` LL | for j in 0..4 { | ^^^^ | +note: for this index operation + --> tests/ui/needless_range_loop.rs:43:26 + | +LL | println!("{:?}", CONST[j]); + | ^^^^^^^^ help: consider using an iterator | LL - for j in 0..4 { @@ -54,6 +74,11 @@ error: the loop variable `i` is used to index `vec` LL | for i in 0..vec.len() { | ^^^^^^^^^^^^ | +note: for this index operation + --> tests/ui/needless_range_loop.rs:49:27 + | +LL | println!("{} {}", vec[i], i); + | ^^^^^^ help: consider using an iterator and enumerate() | LL - for i in 0..vec.len() { @@ -66,6 +91,11 @@ error: the loop variable `i` is only used to index `vec2` LL | for i in 0..vec.len() { | ^^^^^^^^^^^^ | +note: for this index operation + --> tests/ui/needless_range_loop.rs:59:24 + | +LL | println!("{}", vec2[i]); + | ^^^^^^^ help: consider using an iterator | LL - for i in 0..vec.len() { @@ -78,6 +108,11 @@ error: the loop variable `i` is only used to index `vec` LL | for i in 5..vec.len() { | ^^^^^^^^^^^^ | +note: for this index operation + --> tests/ui/needless_range_loop.rs:65:24 + | +LL | println!("{}", vec[i]); + | ^^^^^^ help: consider using an iterator | LL - for i in 5..vec.len() { @@ -90,6 +125,11 @@ error: the loop variable `i` is only used to index `vec` LL | for i in 0..MAX_LEN { | ^^^^^^^^^^ | +note: for this index operation + --> tests/ui/needless_range_loop.rs:71:24 + | +LL | println!("{}", vec[i]); + | ^^^^^^ help: consider using an iterator | LL - for i in 0..MAX_LEN { @@ -102,6 +142,11 @@ error: the loop variable `i` is only used to index `vec` LL | for i in 0..=MAX_LEN { | ^^^^^^^^^^^ | +note: for this index operation + --> tests/ui/needless_range_loop.rs:77:24 + | +LL | println!("{}", vec[i]); + | ^^^^^^ help: consider using an iterator | LL - for i in 0..=MAX_LEN { @@ -114,6 +159,11 @@ error: the loop variable `i` is only used to index `vec` LL | for i in 5..10 { | ^^^^^ | +note: for this index operation + --> tests/ui/needless_range_loop.rs:83:24 + | +LL | println!("{}", vec[i]); + | ^^^^^^ help: consider using an iterator | LL - for i in 5..10 { @@ -126,6 +176,11 @@ error: the loop variable `i` is only used to index `vec` LL | for i in 5..=10 { | ^^^^^^ | +note: for this index operation + --> tests/ui/needless_range_loop.rs:89:24 + | +LL | println!("{}", vec[i]); + | ^^^^^^ help: consider using an iterator | LL - for i in 5..=10 { @@ -138,6 +193,11 @@ error: the loop variable `i` is used to index `vec` LL | for i in 5..vec.len() { | ^^^^^^^^^^^^ | +note: for this index operation + --> tests/ui/needless_range_loop.rs:95:27 + | +LL | println!("{} {}", vec[i], i); + | ^^^^^^ help: consider using an iterator and enumerate() | LL - for i in 5..vec.len() { @@ -150,6 +210,11 @@ error: the loop variable `i` is used to index `vec` LL | for i in 5..10 { | ^^^^^ | +note: for this index operation + --> tests/ui/needless_range_loop.rs:101:27 + | +LL | println!("{} {}", vec[i], i); + | ^^^^^^ help: consider using an iterator and enumerate() | LL - for i in 5..10 { @@ -162,6 +227,11 @@ error: the loop variable `i` is used to index `vec` LL | for i in 0..vec.len() { | ^^^^^^^^^^^^ | +note: for this index operation + --> tests/ui/needless_range_loop.rs:108:9 + | +LL | vec[i] = Some(1).unwrap_or_else(|| panic!("error on {}", i)); + | ^^^^^^ help: consider using an iterator and enumerate() | LL - for i in 0..vec.len() { @@ -174,23 +244,101 @@ error: the loop variable `i` is used to index `a` LL | for i in 0..MAX_LEN { | ^^^^^^^^^^ | +note: for this index operation + --> tests/ui/needless_range_loop.rs:237:17 + | +LL | let _ = a[i][i]; + | ^^^^ help: consider using an iterator and enumerate() | LL - for i in 0..MAX_LEN { LL + for (i, ) in a.iter().enumerate().take(MAX_LEN) { | -error: the loop variable `i` is only used to index `a` +error: the loop variable `i` is only used to index `a[0]` --> tests/ui/needless_range_loop.rs:240:14 | LL | for i in 0..MAX_LEN { | ^^^^^^^^^^ | +note: for this index operation + --> tests/ui/needless_range_loop.rs:242:17 + | +LL | let _ = a[0][i]; + | ^^^^^^^ help: consider using an iterator | LL - for i in 0..MAX_LEN { -LL + for in a.iter().take(MAX_LEN) { +LL + for in a[0].iter().take(MAX_LEN) { + | + +error: the loop variable `i` is used to index `matrix` + --> tests/ui/needless_range_loop.rs:248:14 + | +LL | for i in 0..=2 { + | ^^^^^ + | +note: for this index operation + --> tests/ui/needless_range_loop.rs:250:9 + | +LL | matrix[i][i] = true; + | ^^^^^^^^^ +help: consider using an iterator and enumerate() + | +LL - for i in 0..=2 { +LL + for (i, ) in matrix.iter_mut().enumerate().take(2 + 1) { + | + +error: the loop variable `i` is only used to index `values` + --> tests/ui/needless_range_loop.rs:255:14 + | +LL | for i in 0..4 { + | ^^^^ + | +note: for this index operation + --> tests/ui/needless_range_loop.rs:257:17 + | +LL | let _ = values[i][col]; + | ^^^^^^^^^ +help: consider using an iterator + | +LL - for i in 0..4 { +LL + for in &values { + | + +error: the loop variable `j` is only used to index `colors[0]` + --> tests/ui/needless_range_loop.rs:273:14 + | +LL | for j in 0..3 { + | ^^^^ + | +note: for this index operation + --> tests/ui/needless_range_loop.rs:275:17 + | +LL | let _ = colors[0][j]; + | ^^^^^^^^^^^^ +help: consider using an iterator + | +LL - for j in 0..3 { +LL + for in &colors[0] { + | + +error: the loop variable `i` is only used to index `wrapper.0` + --> tests/ui/needless_range_loop.rs:280:14 + | +LL | for i in 0..3 { + | ^^^^ + | +note: for this index operation + --> tests/ui/needless_range_loop.rs:282:17 + | +LL | let _ = wrapper.0[i]; + | ^^^^^^^^^^^^ +help: consider using an iterator + | +LL - for i in 0..3 { +LL + for in &wrapper.0 { | -error: aborting due to 16 previous errors +error: aborting due to 20 previous errors diff --git a/tests/ui/needless_range_loop2.stderr b/tests/ui/needless_range_loop2.stderr index cb979b3f3c24..36d2f443ef52 100644 --- a/tests/ui/needless_range_loop2.stderr +++ b/tests/ui/needless_range_loop2.stderr @@ -4,6 +4,11 @@ error: the loop variable `i` is only used to index `ns` LL | for i in 3..10 { | ^^^^^ | +note: for this index operation + --> tests/ui/needless_range_loop2.rs:14:24 + | +LL | println!("{}", ns[i]); + | ^^^^^ = note: `-D clippy::needless-range-loop` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::needless_range_loop)]` help: consider using an iterator @@ -18,6 +23,11 @@ error: the loop variable `i` is only used to index `ms` LL | for i in 0..ms.len() { | ^^^^^^^^^^^ | +note: for this index operation + --> tests/ui/needless_range_loop2.rs:37:9 + | +LL | ms[i] *= 2; + | ^^^^^ help: consider using an iterator | LL - for i in 0..ms.len() { @@ -30,6 +40,11 @@ error: the loop variable `i` is only used to index `ms` LL | for i in 0..ms.len() { | ^^^^^^^^^^^ | +note: for this index operation + --> tests/ui/needless_range_loop2.rs:45:22 + | +LL | let x = &mut ms[i]; + | ^^^^^ help: consider using an iterator | LL - for i in 0..ms.len() { @@ -42,6 +57,11 @@ error: the loop variable `i` is only used to index `vec` LL | for i in x..x + 4 { | ^^^^^^^^ | +note: for this index operation + --> tests/ui/needless_range_loop2.rs:71:9 + | +LL | vec[i] += 1; + | ^^^^^^ help: consider using an iterator | LL - for i in x..x + 4 { @@ -54,6 +74,11 @@ error: the loop variable `i` is only used to index `vec` LL | for i in x..=x + 4 { | ^^^^^^^^^ | +note: for this index operation + --> tests/ui/needless_range_loop2.rs:80:9 + | +LL | vec[i] += 1; + | ^^^^^^ help: consider using an iterator | LL - for i in x..=x + 4 { @@ -66,6 +91,11 @@ error: the loop variable `i` is only used to index `arr` LL | for i in 0..3 { | ^^^^ | +note: for this index operation + --> tests/ui/needless_range_loop2.rs:88:24 + | +LL | println!("{}", arr[i]); + | ^^^^^^ help: consider using an iterator | LL - for i in 0..3 { @@ -78,6 +108,11 @@ error: the loop variable `i` is only used to index `arr` LL | for i in 0..2 { | ^^^^ | +note: for this index operation + --> tests/ui/needless_range_loop2.rs:94:24 + | +LL | println!("{}", arr[i]); + | ^^^^^^ help: consider using an iterator | LL - for i in 0..2 { @@ -90,6 +125,11 @@ error: the loop variable `i` is only used to index `arr` LL | for i in 1..3 { | ^^^^ | +note: for this index operation + --> tests/ui/needless_range_loop2.rs:100:24 + | +LL | println!("{}", arr[i]); + | ^^^^^^ help: consider using an iterator | LL - for i in 1..3 {