Merge pull request #2118 from chyvonomys/relax-needless-loop

relax `needless_range_loop` so that it reports only direct indexing

Author: oli-obk

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<Name, Option<region::Scope>>,
+    /// 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<Name, Option<region::Scope>>,
     /// Any names that are used outside an index operation.
     /// Used to detect things like `&mut vec` used together with `vec[i]`
     referenced: HashSet<Name>,
@@ -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*
                     }
                     _ => (),

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]);
+    }
+}

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 <item> in ns.iter().take(10).skip(3) {
+   |         ^^^^^^
+