From 44a8a8d0ca407a08612b87eaff1211dda1528633 Mon Sep 17 00:00:00 2001 From: yukang 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, + } + 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 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, } 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`."); } }