From 44a8a8d0ca407a08612b87eaff1211dda1528633 Mon Sep 17 00:00:00 2001
From: yukang <moorekang@gmail.com>
Date: Fri, 30 Jun 2023 00:38:56 +0800
Subject: [PATCH 1/2] Better messages for next in a iterator inside for loops

---
 .../src/diagnostics/conflict_errors.rs        | 79 ++++++++++++++++++-
 .../rustc_borrowck/src/diagnostics/mod.rs     |  6 ++
 tests/ui/suggestions/issue-102972.rs          | 16 ++++
 tests/ui/suggestions/issue-102972.stderr      | 33 ++++++++
 4 files changed, 132 insertions(+), 2 deletions(-)
 create mode 100644 tests/ui/suggestions/issue-102972.rs
 create mode 100644 tests/ui/suggestions/issue-102972.stderr

diff --git a/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs b/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs
index a068f2d69268b..8a6bdb72968e8 100644
--- a/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs
+++ b/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs
@@ -1,6 +1,5 @@
-use std::iter;
-
 use either::Either;
+use hir::PatField;
 use rustc_data_structures::captures::Captures;
 use rustc_data_structures::fx::FxIndexSet;
 use rustc_errors::{
@@ -28,6 +27,8 @@ use rustc_span::symbol::{kw, sym, Ident};
 use rustc_span::{BytePos, Span, Symbol};
 use rustc_trait_selection::infer::InferCtxtExt;
 use rustc_trait_selection::traits::ObligationCtxt;
+use std::iter;
+use std::marker::PhantomData;
 
 use crate::borrow_set::TwoPhaseActivation;
 use crate::borrowck_errors;
@@ -992,6 +993,11 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
                     issued_borrow.borrowed_place,
                     &issued_spans,
                 );
+                self.explain_iterator_advancement_in_for_loop_if_applicable(
+                    &mut err,
+                    span,
+                    &issued_spans,
+                );
                 err
             }
 
@@ -1278,6 +1284,75 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
         }
     }
 
+    /// Suggest using `while let` for call `next` on an iterator in a for loop.
+    ///
+    /// For example:
+    /// ```ignore (illustrative)
+    ///
+    /// for x in iter {
+    ///     ...
+    ///     iter.next()
+    /// }
+    /// ```
+    pub(crate) fn explain_iterator_advancement_in_for_loop_if_applicable(
+        &self,
+        err: &mut Diagnostic,
+        span: Span,
+        issued_spans: &UseSpans<'tcx>,
+    ) {
+        let issue_span = issued_spans.args_or_use();
+        let tcx = self.infcx.tcx;
+        let hir = tcx.hir();
+
+        let Some(body_id) = hir.get(self.mir_hir_id()).body_id() else { return };
+
+        struct ExprFinder<'hir> {
+            phantom: PhantomData<&'hir hir::Expr<'hir>>,
+            issue_span: Span,
+            expr_span: Span,
+            found_body_expr: bool,
+            loop_bind: Option<Symbol>,
+        }
+        impl<'hir> Visitor<'hir> for ExprFinder<'hir> {
+            fn visit_expr(&mut self, ex: &'hir hir::Expr<'hir>) {
+                if let hir::ExprKind::Loop(hir::Block{ stmts: [stmt, ..], ..}, _, hir::LoopSource::ForLoop, _) = ex.kind &&
+                    let hir::StmtKind::Expr(hir::Expr{ kind: hir::ExprKind::Match(call, [_, bind, ..], _), ..}) = stmt.kind &&
+                    let hir::ExprKind::Call(path, _args) = call.kind &&
+                    let hir::ExprKind::Path(hir::QPath::LangItem(LangItem::IteratorNext, _, _, )) = path.kind &&
+                    let hir::PatKind::Struct(path, [field, ..], _) = bind.pat.kind &&
+                    let hir::QPath::LangItem(LangItem::OptionSome, _, _) = path &&
+                    let PatField { pat: hir::Pat{ kind: hir::PatKind::Binding(_, _, ident, ..), .. }, ..} = field &&
+                    self.issue_span.source_equal(call.span) {
+                        self.loop_bind = Some(ident.name);
+                    }
+
+                if let hir::ExprKind::MethodCall(body_call, ..) = ex.kind &&
+                    body_call.ident.name == sym::next &&
+                    ex.span.source_equal(self.expr_span) {
+                        self.found_body_expr = true;
+                }
+
+                hir::intravisit::walk_expr(self, ex);
+            }
+        }
+        let mut finder = ExprFinder {
+            phantom: PhantomData,
+            expr_span: span,
+            issue_span,
+            loop_bind: None,
+            found_body_expr: false,
+        };
+        finder.visit_expr(hir.body(body_id).value);
+        if let Some(loop_bind) = finder.loop_bind &&
+            finder.found_body_expr {
+            err.note(format!(
+                "a for loop advances the iterator for you, the result is stored in `{}`.",
+                loop_bind
+            ));
+            err.help("if you want to call `next` on a iterator within the loop, consider using `while let`.");
+        }
+    }
+
     /// Suggest using closure argument instead of capture.
     ///
     /// For example:
diff --git a/compiler/rustc_borrowck/src/diagnostics/mod.rs b/compiler/rustc_borrowck/src/diagnostics/mod.rs
index 2f8c970f806d3..42e50fd0fad07 100644
--- a/compiler/rustc_borrowck/src/diagnostics/mod.rs
+++ b/compiler/rustc_borrowck/src/diagnostics/mod.rs
@@ -1146,6 +1146,12 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
                     // Avoid pointing to the same function in multiple different
                     // error messages.
                     if span != DUMMY_SP && self.fn_self_span_reported.insert(self_arg.span) {
+                        self.explain_iterator_advancement_in_for_loop_if_applicable(
+                            err,
+                            span,
+                            &move_spans,
+                        );
+
                         let func = tcx.def_path_str(method_did);
                         err.subdiagnostic(CaptureReasonNote::FuncTakeSelf {
                             func,
diff --git a/tests/ui/suggestions/issue-102972.rs b/tests/ui/suggestions/issue-102972.rs
new file mode 100644
index 0000000000000..106288b054d5d
--- /dev/null
+++ b/tests/ui/suggestions/issue-102972.rs
@@ -0,0 +1,16 @@
+fn test1() {
+    let mut chars = "Hello".chars();
+    for _c in chars.by_ref() {
+        chars.next(); //~ ERROR cannot borrow `chars` as mutable more than once at a time
+    }
+}
+
+fn test2() {
+    let v = vec![1, 2, 3];
+    let mut iter = v.iter();
+    for _i in iter {
+        iter.next(); //~ ERROR borrow of moved value: `iter`
+    }
+}
+
+fn main() { }
diff --git a/tests/ui/suggestions/issue-102972.stderr b/tests/ui/suggestions/issue-102972.stderr
new file mode 100644
index 0000000000000..03f9dbb6c895a
--- /dev/null
+++ b/tests/ui/suggestions/issue-102972.stderr
@@ -0,0 +1,33 @@
+error[E0499]: cannot borrow `chars` as mutable more than once at a time
+  --> $DIR/issue-102972.rs:4:9
+   |
+LL |     for _c in chars.by_ref() {
+   |               --------------
+   |               |
+   |               first mutable borrow occurs here
+   |               first borrow later used here
+LL |         chars.next();
+   |         ^^^^^^^^^^^^ second mutable borrow occurs here
+   |
+   = note: a for loop advances the iterator for you, the result is stored in `_c`.
+   = help: if you want to call `next` on a iterator within the loop, consider using `while let`.
+
+error[E0382]: borrow of moved value: `iter`
+  --> $DIR/issue-102972.rs:12:9
+   |
+LL |     let mut iter = v.iter();
+   |         -------- move occurs because `iter` has type `std::slice::Iter<'_, i32>`, which does not implement the `Copy` trait
+LL |     for _i in iter {
+   |               ---- `iter` moved due to this implicit call to `.into_iter()`
+LL |         iter.next();
+   |         ^^^^^^^^^^^ value borrowed here after move
+   |
+   = note: a for loop advances the iterator for you, the result is stored in `_i`.
+   = help: if you want to call `next` on a iterator within the loop, consider using `while let`.
+note: `into_iter` takes ownership of the receiver `self`, which moves `iter`
+  --> $SRC_DIR/core/src/iter/traits/collect.rs:LL:COL
+
+error: aborting due to 2 previous errors
+
+Some errors have detailed explanations: E0382, E0499.
+For more information about an error, try `rustc --explain E0382`.

From 4189faa21edc123e2ccfd3b444b0ad3c030aba0a Mon Sep 17 00:00:00 2001
From: yukang <moorekang@gmail.com>
Date: Fri, 30 Jun 2023 14:50:27 +0800
Subject: [PATCH 2/2] add typecheck for iterator

---
 .../src/diagnostics/conflict_errors.rs        | 37 +++++++++----------
 1 file changed, 17 insertions(+), 20 deletions(-)

diff --git a/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs b/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs
index 8a6bdb72968e8..93eef4d5966b4 100644
--- a/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs
+++ b/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs
@@ -28,7 +28,6 @@ use rustc_span::{BytePos, Span, Symbol};
 use rustc_trait_selection::infer::InferCtxtExt;
 use rustc_trait_selection::traits::ObligationCtxt;
 use std::iter;
-use std::marker::PhantomData;
 
 use crate::borrow_set::TwoPhaseActivation;
 use crate::borrowck_errors;
@@ -1305,12 +1304,12 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
         let hir = tcx.hir();
 
         let Some(body_id) = hir.get(self.mir_hir_id()).body_id() else { return };
+        let typeck_results = tcx.typeck(self.mir_def_id());
 
         struct ExprFinder<'hir> {
-            phantom: PhantomData<&'hir hir::Expr<'hir>>,
             issue_span: Span,
             expr_span: Span,
-            found_body_expr: bool,
+            body_expr: Option<&'hir hir::Expr<'hir>>,
             loop_bind: Option<Symbol>,
         }
         impl<'hir> Visitor<'hir> for ExprFinder<'hir> {
@@ -1326,30 +1325,28 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
                         self.loop_bind = Some(ident.name);
                     }
 
-                if let hir::ExprKind::MethodCall(body_call, ..) = ex.kind &&
-                    body_call.ident.name == sym::next &&
-                    ex.span.source_equal(self.expr_span) {
-                        self.found_body_expr = true;
+                if let hir::ExprKind::MethodCall(body_call, _recv, ..) = ex.kind &&
+                    body_call.ident.name == sym::next && ex.span.source_equal(self.expr_span) {
+                        self.body_expr = Some(ex);
                 }
 
                 hir::intravisit::walk_expr(self, ex);
             }
         }
-        let mut finder = ExprFinder {
-            phantom: PhantomData,
-            expr_span: span,
-            issue_span,
-            loop_bind: None,
-            found_body_expr: false,
-        };
+        let mut finder =
+            ExprFinder { expr_span: span, issue_span, loop_bind: None, body_expr: None };
         finder.visit_expr(hir.body(body_id).value);
+
         if let Some(loop_bind) = finder.loop_bind &&
-            finder.found_body_expr {
-            err.note(format!(
-                "a for loop advances the iterator for you, the result is stored in `{}`.",
-                loop_bind
-            ));
-            err.help("if you want to call `next` on a iterator within the loop, consider using `while let`.");
+            let Some(body_expr) = finder.body_expr &&
+                let Some(def_id) = typeck_results.type_dependent_def_id(body_expr.hir_id) &&
+                let Some(trait_did) = tcx.trait_of_item(def_id) &&
+                tcx.is_diagnostic_item(sym::Iterator, trait_did) {
+                    err.note(format!(
+                        "a for loop advances the iterator for you, the result is stored in `{}`.",
+                        loop_bind
+                    ));
+                    err.help("if you want to call `next` on a iterator within the loop, consider using `while let`.");
         }
     }