Fix `needless_range_loop` bad suggestion

Detect if the index variable is used inside a closure.

Fixes #2542

diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs
index dcd1a4e0a61033e58537f5b207afc3b52bf3749c..99ed9fc86fdef312849767e9f1b200f1a14c4b2f 100644 (file)
--- a/clippy_lints/src/loops.rs
+++ b/clippy_lints/src/loops.rs
@@ -1830,17 +1830,29 @@ fn visit_expr(&mut self, expr: &'tcx Expr) {
             if let ExprKind::Path(ref qpath) = expr.node;
             if let QPath::Resolved(None, ref path) = *qpath;
             if path.segments.len() == 1;
-            if let Def::Local(local_id) = self.cx.tables.qpath_def(qpath, expr.hir_id);
             then {
-                if local_id == self.var {
-                    // we are not indexing anything, record that
-                    self.nonindex = true;
-                } else {
-                    // not the correct variable, but still a variable
-                    self.referenced.insert(path.segments[0].ident.name);
+                match self.cx.tables.qpath_def(qpath, expr.hir_id) {
+                    Def::Upvar(local_id, ..) => {
+                        if local_id == self.var {
+                            // we are not indexing anything, record that
+                            self.nonindex = true;
+                        }
+                    }
+                    Def::Local(local_id) =>
+                    {
+
+                        if local_id == self.var {
+                            self.nonindex = true;
+                        } else {
+                            // not the correct variable, but still a variable
+                            self.referenced.insert(path.segments[0].ident.name);
+                        }
+                    }
+                    _ => {}
                 }
             }
         }
+
         let old = self.prefer_mutable;
         match expr.node {
             ExprKind::AssignOp(_, ref lhs, ref rhs) | ExprKind::Assign(ref lhs, ref rhs) => {
@@ -1880,6 +1892,10 @@ fn visit_expr(&mut self, expr: &'tcx Expr) {
                     self.visit_expr(expr);
                 }
             },
+            ExprKind::Closure(_, _, body_id, ..) => {
+                let body = self.cx.tcx.hir().body(body_id);
+                self.visit_expr(&body.value);
+            },
             _ => walk_expr(self, expr),
         }
         self.prefer_mutable = old;

diff --git a/tests/ui/needless_range_loop.rs b/tests/ui/needless_range_loop.rs
index a073cc0cfa32859ba97db4bde7e6574104f6f22c..5f22e2645d12b4d17b5320679dd17e5c8327e3b1 100644 (file)
--- a/tests/ui/needless_range_loop.rs
+++ b/tests/ui/needless_range_loop.rs
@@ -80,4 +80,9 @@ fn main() {
     for i in 1..3 {
         println!("{}", arr[i]);
     }
+
+    // #2542
+    for i in 0..vec.len() {
+        vec[i] = Some(1).unwrap_or_else(|| panic!("error on {}", i));
+    }
 }

diff --git a/tests/ui/needless_range_loop.stderr b/tests/ui/needless_range_loop.stderr
index 7044fc17617ad62df41cdf815f0b4843da749cff..d1cc9b3ce727a6f0a796c54cf7d16581fea73efa 100644 (file)
--- a/tests/ui/needless_range_loop.stderr
+++ b/tests/ui/needless_range_loop.stderr
@@ -80,5 +80,15 @@ help: consider using an iterator
 LL |     for <item> in arr.iter().skip(1) {
    |         ^^^^^^    ^^^^^^^^^^^^^^^^^^
 
-error: aborting due to 8 previous errors
+error: the loop variable `i` is used to index `vec`
+  --> $DIR/needless_range_loop.rs:85:14
+   |
+LL |     for i in 0..vec.len() {
+   |              ^^^^^^^^^^^^
+help: consider using an iterator
+   |
+LL |     for (i, <item>) in vec.iter_mut().enumerate() {
+   |         ^^^^^^^^^^^    ^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: aborting due to 9 previous errors