From 7db0e4f791cf8baf3fe8e978a9056d0d8464a1bd Mon Sep 17 00:00:00 2001
From: Jason Newcomb <jsnewcomb@pm.me>
Date: Fri, 21 May 2021 12:27:40 -0400
Subject: [PATCH] Suggest `&mut iter` inside a closure for
 `while_let_on_iterator`

---
 clippy_lints/src/loops/while_let_on_iterator.rs | 11 +++++++----
 clippy_utils/src/lib.rs                         | 10 ++++++----
 tests/ui/while_let_on_iterator.fixed            | 14 ++++++++++++++
 tests/ui/while_let_on_iterator.rs               | 14 ++++++++++++++
 tests/ui/while_let_on_iterator.stderr           | 10 ++++++++--
 5 files changed, 49 insertions(+), 10 deletions(-)

diff --git a/clippy_lints/src/loops/while_let_on_iterator.rs b/clippy_lints/src/loops/while_let_on_iterator.rs
index 63560047578a1..d57588716a5bf 100644
--- a/clippy_lints/src/loops/while_let_on_iterator.rs
+++ b/clippy_lints/src/loops/while_let_on_iterator.rs
@@ -1,7 +1,9 @@
 use super::WHILE_LET_ON_ITERATOR;
 use clippy_utils::diagnostics::span_lint_and_sugg;
 use clippy_utils::source::snippet_with_applicability;
-use clippy_utils::{get_enclosing_loop, is_refutable, is_trait_method, match_def_path, paths, visitors::is_res_used};
+use clippy_utils::{
+    get_enclosing_loop_or_closure, is_refutable, is_trait_method, match_def_path, paths, visitors::is_res_used,
+};
 use if_chain::if_chain;
 use rustc_errors::Applicability;
 use rustc_hir::intravisit::{walk_expr, ErasedMap, NestedVisitorMap, Visitor};
@@ -315,9 +317,10 @@ fn needs_mutable_borrow(cx: &LateContext<'tcx>, iter_expr: &IterExpr, loop_expr:
         }
     }
 
-    if let Some(e) = get_enclosing_loop(cx.tcx, loop_expr) {
-        // The iterator expression will be used on the next iteration unless it is declared within the outer
-        // loop.
+    if let Some(e) = get_enclosing_loop_or_closure(cx.tcx, loop_expr) {
+        // The iterator expression will be used on the next iteration (for loops), or on the next call (for
+        // closures) unless it is declared within the enclosing expression. TODO: Check for closures
+        // used where an `FnOnce` type is expected.
         let local_id = match iter_expr.path {
             Res::Local(id) => id,
             _ => return true,
diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs
index 2b9b214daa7f9..d32c3ec929a4a 100644
--- a/clippy_utils/src/lib.rs
+++ b/clippy_utils/src/lib.rs
@@ -861,14 +861,16 @@ pub fn get_enclosing_block<'tcx>(cx: &LateContext<'tcx>, hir_id: HirId) -> Optio
     })
 }
 
-/// Gets the loop enclosing the given expression, if any.
-pub fn get_enclosing_loop(tcx: TyCtxt<'tcx>, expr: &Expr<'_>) -> Option<&'tcx Expr<'tcx>> {
+/// Gets the loop or closure enclosing the given expression, if any.
+pub fn get_enclosing_loop_or_closure(tcx: TyCtxt<'tcx>, expr: &Expr<'_>) -> Option<&'tcx Expr<'tcx>> {
     let map = tcx.hir();
     for (_, node) in map.parent_iter(expr.hir_id) {
         match node {
             Node::Expr(
-                e @ Expr {
-                    kind: ExprKind::Loop(..),
+                e
+                @
+                Expr {
+                    kind: ExprKind::Loop(..) | ExprKind::Closure(..),
                     ..
                 },
             ) => return Some(e),
diff --git a/tests/ui/while_let_on_iterator.fixed b/tests/ui/while_let_on_iterator.fixed
index c3e2cf0c4a4bd..52e80ceee83cf 100644
--- a/tests/ui/while_let_on_iterator.fixed
+++ b/tests/ui/while_let_on_iterator.fixed
@@ -320,6 +320,20 @@ fn issue1924() {
     println!("iterator field {}", it.1);
 }
 
+fn issue7249() {
+    let mut it = 0..10;
+    let mut x = || {
+        // Needs &mut, the closure can be called multiple times
+        for x in &mut it {
+            if x % 2 == 0 {
+                break;
+            }
+        }
+    };
+    x();
+    x();
+}
+
 fn main() {
     let mut it = 0..20;
     for _ in it {
diff --git a/tests/ui/while_let_on_iterator.rs b/tests/ui/while_let_on_iterator.rs
index 1717006a4490e..5078a3c9028c4 100644
--- a/tests/ui/while_let_on_iterator.rs
+++ b/tests/ui/while_let_on_iterator.rs
@@ -320,6 +320,20 @@ fn issue1924() {
     println!("iterator field {}", it.1);
 }
 
+fn issue7249() {
+    let mut it = 0..10;
+    let mut x = || {
+        // Needs &mut, the closure can be called multiple times
+        while let Some(x) = it.next() {
+            if x % 2 == 0 {
+                break;
+            }
+        }
+    };
+    x();
+    x();
+}
+
 fn main() {
     let mut it = 0..20;
     while let Some(..) = it.next() {
diff --git a/tests/ui/while_let_on_iterator.stderr b/tests/ui/while_let_on_iterator.stderr
index eff559bef7e3b..cb0afeae15ee0 100644
--- a/tests/ui/while_let_on_iterator.stderr
+++ b/tests/ui/while_let_on_iterator.stderr
@@ -105,10 +105,16 @@ LL |     while let Some(n) = it.next() {
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `for n in &mut it`
 
 error: this loop could be written as a `for` loop
-  --> $DIR/while_let_on_iterator.rs:325:5
+  --> $DIR/while_let_on_iterator.rs:327:9
+   |
+LL |         while let Some(x) = it.next() {
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `for x in &mut it`
+
+error: this loop could be written as a `for` loop
+  --> $DIR/while_let_on_iterator.rs:339:5
    |
 LL |     while let Some(..) = it.next() {
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `for _ in it`
 
-error: aborting due to 18 previous errors
+error: aborting due to 19 previous errors