diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index 8d2a2f8fa..f1483cd7e 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -925,14 +925,16 @@ fn check_for_loop_range<'a, 'tcx>( cx: cx, var: canonical_id, indexed: HashMap::new(), + indexed_directly: HashMap::new(), referenced: HashSet::new(), nonindex: false, }; walk_expr(&mut visitor, body); - // linting condition: we only indexed one variable - if visitor.indexed.len() == 1 { - let (indexed, indexed_extent) = visitor.indexed.into_iter().next().expect( + // linting condition: we only indexed one variable, and indexed it directly + // (`indexed_directly` is subset of `indexed`) + if visitor.indexed.len() == 1 && visitor.indexed_directly.len() == 1 { + let (indexed, indexed_extent) = visitor.indexed_directly.into_iter().next().expect( "already checked that we have exactly 1 element", ); @@ -1481,6 +1483,9 @@ struct VarVisitor<'a, 'tcx: 'a> { var: ast::NodeId, /// indexed variables, the extend is `None` for global indexed: HashMap>, + /// 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: HashMap>, /// Any names that are used outside an index operation. /// Used to detect things like `&mut vec` used together with `vec[i]` referenced: HashSet, @@ -1499,7 +1504,8 @@ impl<'a, 'tcx> Visitor<'tcx> for VarVisitor<'a, 'tcx> { let QPath::Resolved(None, ref seqvar) = *seqpath, seqvar.segments.len() == 1, ], { - let index_used = same_var(self.cx, idx, self.var) || { + let index_used_directly = same_var(self.cx, idx, self.var); + let index_used = index_used_directly || { let mut used_visitor = LocalUsedVisitor { cx: self.cx, local: self.var, @@ -1519,10 +1525,16 @@ impl<'a, 'tcx> Visitor<'tcx> for VarVisitor<'a, 'tcx> { let parent_def_id = self.cx.tcx.hir.local_def_id(parent_id); let extent = self.cx.tcx.region_scope_tree(parent_def_id).var_scope(hir_id.local_id); self.indexed.insert(seqvar.segments[0].name, Some(extent)); + if index_used_directly { + self.indexed_directly.insert(seqvar.segments[0].name, Some(extent)); + } return; // no need to walk further *on the variable* } Def::Static(..) | Def::Const(..) => { self.indexed.insert(seqvar.segments[0].name, None); + if index_used_directly { + self.indexed_directly.insert(seqvar.segments[0].name, None); + } return; // no need to walk further *on the variable* } _ => (), diff --git a/tests/ui/needless_range_loop.rs b/tests/ui/needless_range_loop.rs new file mode 100644 index 000000000..b960e3990 --- /dev/null +++ b/tests/ui/needless_range_loop.rs @@ -0,0 +1,27 @@ +fn calc_idx(i: usize) -> usize { + (i + i + 20) % 4 +} + +fn main() { + let ns = [2, 3, 5, 7]; + + for i in 3..10 { + println!("{}", ns[i]); + } + + for i in 3..10 { + println!("{}", ns[i % 4]); + } + + for i in 3..10 { + println!("{}", ns[i % ns.len()]); + } + + for i in 3..10 { + println!("{}", ns[calc_idx(i)]); + } + + for i in 3..10 { + println!("{}", ns[calc_idx(i) % 4]); + } +} diff --git a/tests/ui/needless_range_loop.stderr b/tests/ui/needless_range_loop.stderr new file mode 100644 index 000000000..e2c3e18e8 --- /dev/null +++ b/tests/ui/needless_range_loop.stderr @@ -0,0 +1,14 @@ +error: the loop variable `i` is only used to index `ns`. + --> $DIR/needless_range_loop.rs:8:5 + | +8 | / for i in 3..10 { +9 | | println!("{}", ns[i]); +10 | | } + | |_____^ + | + = note: `-D needless-range-loop` implied by `-D warnings` +help: consider using an iterator + | +8 | for in ns.iter().take(10).skip(3) { + | ^^^^^^ +