From fe75faa6eeafe22eb5e6ffae652c7514792334ab Mon Sep 17 00:00:00 2001
From: Jason Newcomb <jsnewcomb@pm.me>
Date: Fri, 30 Jul 2021 22:59:20 -0400
Subject: [PATCH] Fix `while_let_on_iterator` Reborrow mutable references
 rather then take a reference to them.

---
 .../src/loops/while_let_on_iterator.rs        | 13 +++++++++--
 tests/ui/while_let_on_iterator.fixed          | 23 +++++++++++++++++++
 tests/ui/while_let_on_iterator.rs             | 23 +++++++++++++++++++
 tests/ui/while_let_on_iterator.stderr         | 16 +++++++++++--
 4 files changed, 71 insertions(+), 4 deletions(-)

diff --git a/clippy_lints/src/loops/while_let_on_iterator.rs b/clippy_lints/src/loops/while_let_on_iterator.rs
index d57588716a5..6655e2e445c 100644
--- a/clippy_lints/src/loops/while_let_on_iterator.rs
+++ b/clippy_lints/src/loops/while_let_on_iterator.rs
@@ -7,7 +7,7 @@
 use if_chain::if_chain;
 use rustc_errors::Applicability;
 use rustc_hir::intravisit::{walk_expr, ErasedMap, NestedVisitorMap, Visitor};
-use rustc_hir::{def::Res, Expr, ExprKind, HirId, Local, MatchSource, Node, PatKind, QPath, UnOp};
+use rustc_hir::{def::Res, Expr, ExprKind, HirId, Local, MatchSource, Mutability, Node, PatKind, QPath, UnOp};
 use rustc_lint::LateContext;
 use rustc_span::{symbol::sym, Span, Symbol};
 
@@ -48,7 +48,12 @@ pub(super) fn check(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
     // borrowed mutably. TODO: If the struct can be partially moved from and the struct isn't used
     // afterwards a mutable borrow of a field isn't necessary.
     let ref_mut = if !iter_expr.fields.is_empty() || needs_mutable_borrow(cx, &iter_expr, loop_expr) {
-        "&mut "
+        if cx.typeck_results().node_type(iter_expr.hir_id).ref_mutability() == Some(Mutability::Mut) {
+            // Reborrow for mutable references. It may not be possible to get a mutable reference here.
+            "&mut *"
+        } else {
+            "&mut "
+        }
     } else {
         ""
     };
@@ -69,6 +74,8 @@ pub(super) fn check(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
 struct IterExpr {
     /// The span of the whole expression, not just the path and fields stored here.
     span: Span,
+    /// The HIR id of the whole expression, not just the path and fields stored here.
+    hir_id: HirId,
     /// The fields used, in order of child to parent.
     fields: Vec<Symbol>,
     /// The path being used.
@@ -78,12 +85,14 @@ struct IterExpr {
 /// the expression might have side effects.
 fn try_parse_iter_expr(cx: &LateContext<'_>, mut e: &Expr<'_>) -> Option<IterExpr> {
     let span = e.span;
+    let hir_id = e.hir_id;
     let mut fields = Vec::new();
     loop {
         match e.kind {
             ExprKind::Path(ref path) => {
                 break Some(IterExpr {
                     span,
+                    hir_id,
                     fields,
                     path: cx.qpath_res(path, e.hir_id),
                 });
diff --git a/tests/ui/while_let_on_iterator.fixed b/tests/ui/while_let_on_iterator.fixed
index 52e80ceee83..52c70ced2bd 100644
--- a/tests/ui/while_let_on_iterator.fixed
+++ b/tests/ui/while_let_on_iterator.fixed
@@ -334,6 +334,29 @@ fn issue7249() {
     x();
 }
 
+fn issue7510() {
+    let mut it = 0..10;
+    let it = &mut it;
+    // Needs to reborrow `it` as the binding isn't mutable
+    for x in &mut *it {
+        if x % 2 == 0 {
+            break;
+        }
+    }
+    println!("{}", it.next().unwrap());
+
+    struct S<T>(T);
+    let mut it = 0..10;
+    let it = S(&mut it);
+    // Needs to reborrow `it.0` as the binding isn't mutable
+    for x in &mut *it.0 {
+        if x % 2 == 0 {
+            break;
+        }
+    }
+    println!("{}", it.0.next().unwrap());
+}
+
 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 5078a3c9028..8434acaf582 100644
--- a/tests/ui/while_let_on_iterator.rs
+++ b/tests/ui/while_let_on_iterator.rs
@@ -334,6 +334,29 @@ fn issue7249() {
     x();
 }
 
+fn issue7510() {
+    let mut it = 0..10;
+    let it = &mut it;
+    // Needs to reborrow `it` as the binding isn't mutable
+    while let Some(x) = it.next() {
+        if x % 2 == 0 {
+            break;
+        }
+    }
+    println!("{}", it.next().unwrap());
+
+    struct S<T>(T);
+    let mut it = 0..10;
+    let it = S(&mut it);
+    // Needs to reborrow `it.0` as the binding isn't mutable
+    while let Some(x) = it.0.next() {
+        if x % 2 == 0 {
+            break;
+        }
+    }
+    println!("{}", it.0.next().unwrap());
+}
+
 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 cb0afeae15e..50bcc26cec2 100644
--- a/tests/ui/while_let_on_iterator.stderr
+++ b/tests/ui/while_let_on_iterator.stderr
@@ -111,10 +111,22 @@ 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
+  --> $DIR/while_let_on_iterator.rs:341:5
+   |
+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:352:5
+   |
+LL |     while let Some(x) = it.0.next() {
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `for x in &mut *it.0`
+
+error: this loop could be written as a `for` loop
+  --> $DIR/while_let_on_iterator.rs:362:5
    |
 LL |     while let Some(..) = it.next() {
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `for _ in it`
 
-error: aborting due to 19 previous errors
+error: aborting due to 21 previous errors
 
-- 
2.44.0