From 217965e85527f1caf99e86153f11f676f778a360 Mon Sep 17 00:00:00 2001
From: Michael Wright <mikerite@lavabit.com>
Date: Mon, 11 Feb 2019 07:03:12 +0200
Subject: [PATCH] Fix `needless_range_loop` bad suggestion

Detect if the index variable is used inside a closure.

Fixes #2542
---
 clippy_lints/src/loops.rs           | 30 ++++++++++++++++++++++-------
 tests/ui/needless_range_loop.rs     |  5 +++++
 tests/ui/needless_range_loop.stderr | 12 +++++++++++-
 3 files changed, 39 insertions(+), 8 deletions(-)

diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs
index dcd1a4e0a..99ed9fc86 100644
--- a/clippy_lints/src/loops.rs
+++ b/clippy_lints/src/loops.rs
@@ -1830,17 +1830,29 @@ impl<'a, 'tcx> Visitor<'tcx> for VarVisitor<'a, 'tcx> {
             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 @@ impl<'a, 'tcx> Visitor<'tcx> for VarVisitor<'a, 'tcx> {
                     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 a073cc0cf..5f22e2645 100644
--- 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 7044fc176..d1cc9b3ce 100644
--- 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