From 9d2eb66e1a9639b35f428fa478de179f9cfbe466 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Esteban=20K=C3=BCber?= <esteban@kuber.com.ar>
Date: Sat, 8 Jul 2023 15:04:53 +0000
Subject: [PATCH] Use structured suggestion for #113174
When encountering a for loop that is rejected by the borrow checker
because it is being advanced within its body, provide a structured
suggestion for `while let Some(pat) = iter.next()`.
---
.../src/diagnostics/conflict_errors.rs | 172 +++++++++++++++---
tests/ui/suggestions/issue-102972.fixed | 41 +++++
tests/ui/suggestions/issue-102972.rs | 25 ++-
tests/ui/suggestions/issue-102972.stderr | 56 +++++-
4 files changed, 267 insertions(+), 27 deletions(-)
create mode 100644 tests/ui/suggestions/issue-102972.fixed
diff --git a/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs b/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs
index 4488276e0e7a6..ec9bb215f3f36 100644
--- a/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs
+++ b/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs
@@ -1329,42 +1329,168 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
issue_span: Span,
expr_span: Span,
body_expr: Option<&'hir hir::Expr<'hir>>,
- loop_bind: Option<Symbol>,
+ loop_bind: Option<&'hir Ident>,
+ loop_span: Option<Span>,
+ head_span: Option<Span>,
+ pat_span: Option<Span>,
+ head: Option<&'hir hir::Expr<'hir>>,
}
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);
+ // Try to find
+ // let result = match IntoIterator::into_iter(<head>) {
+ // mut iter => {
+ // [opt_ident]: loop {
+ // match Iterator::next(&mut iter) {
+ // None => break,
+ // Some(<pat>) => <body>,
+ // };
+ // }
+ // }
+ // };
+ // corresponding to the desugaring of a for loop `for <pat> in <head> { <body> }`.
+ if let hir::ExprKind::Call(path, [arg]) = ex.kind
+ && let hir::ExprKind::Path(
+ hir::QPath::LangItem(LangItem::IntoIterIntoIter, _, _),
+ ) = path.kind
+ && arg.span.contains(self.issue_span)
+ {
+ // Find `IntoIterator::into_iter(<head>)`
+ self.head = Some(arg);
+ }
+ 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, ..], _),
+ span: head_span,
+ ..
+ }) = 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, pat_span, _) = path
+ && call.span.contains(self.issue_span)
+ {
+ // Find `<pat>` and the span for the whole `for` loop.
+ if let PatField { pat: hir::Pat {
+ kind: hir::PatKind::Binding(_, _, ident, ..),
+ ..
+ }, ..} = field {
+ self.loop_bind = Some(ident);
}
+ self.head_span = Some(*head_span);
+ self.pat_span = Some(pat_span);
+ self.loop_span = Some(stmt.span);
+ }
- 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);
+ if let hir::ExprKind::MethodCall(body_call, recv, ..) = ex.kind
+ && body_call.ident.name == sym::next
+ && recv.span.source_equal(self.expr_span)
+ {
+ self.body_expr = Some(ex);
}
hir::intravisit::walk_expr(self, ex);
}
}
- let mut finder =
- ExprFinder { expr_span: span, issue_span, loop_bind: None, body_expr: None };
+ let mut finder = ExprFinder {
+ expr_span: span,
+ issue_span,
+ loop_bind: None,
+ body_expr: None,
+ head_span: None,
+ loop_span: None,
+ pat_span: None,
+ head: None,
+ };
finder.visit_expr(hir.body(body_id).value);
- if let Some(loop_bind) = finder.loop_bind &&
- 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}`."
+ if let Some(body_expr) = finder.body_expr
+ && let Some(loop_span) = finder.loop_span
+ && 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)
+ {
+ if let Some(loop_bind) = finder.loop_bind {
+ err.note(format!(
+ "a for loop advances the iterator for you, the result is stored in `{}`",
+ loop_bind.name,
+ ));
+ } else {
+ err.note(
+ "a for loop advances the iterator for you, the result is stored in its pattern",
+ );
+ }
+ let msg = "if you want to call `next` on a iterator within the loop, consider using \
+ `while let`";
+ if let Some(head) = finder.head
+ && let Some(pat_span) = finder.pat_span
+ && loop_span.contains(body_expr.span)
+ && loop_span.contains(head.span)
+ {
+ let sm = self.infcx.tcx.sess.source_map();
+
+ let mut sugg = vec![];
+ if let hir::ExprKind::Path(hir::QPath::Resolved(None, _)) = head.kind {
+ // A bare path doesn't need a `let` assignment, it's already a simple
+ // binding access.
+ // As a new binding wasn't added, we don't need to modify the advancing call.
+ sugg.push((
+ loop_span.with_hi(pat_span.lo()),
+ format!("while let Some("),
+ ));
+ sugg.push((
+ pat_span.shrink_to_hi().with_hi(head.span.lo()),
+ ") = ".to_string(),
+ ));
+ sugg.push((
+ head.span.shrink_to_hi(),
+ ".next()".to_string(),
+ ));
+ } else {
+ // Needs a new a `let` binding.
+ let indent = if let Some(indent) = sm.indentation_before(loop_span) {
+ format!("\n{indent}")
+ } else {
+ " ".to_string()
+ };
+ let Ok(head_str) = sm.span_to_snippet(head.span) else {
+ err.help(msg);
+ return;
+ };
+ sugg.push((
+ loop_span.with_hi(pat_span.lo()),
+ format!("let iter = {head_str};{indent}while let Some("),
));
- err.help("if you want to call `next` on an iterator within the loop, consider using `while let`.");
+ sugg.push((
+ pat_span.shrink_to_hi().with_hi(head.span.hi()),
+ ") = iter.next()".to_string(),
+ ));
+ // As a new binding was added, we should change how the iterator is advanced to
+ // use the newly introduced binding.
+ if let hir::ExprKind::MethodCall(_, recv, ..) = body_expr.kind
+ && let hir::ExprKind::Path(hir::QPath::Resolved(None, ..)) = recv.kind
+ {
+ // As we introduced a `let iter = <head>;`, we need to change where the
+ // already borrowed value was accessed from `<recv>.next()` to
+ // `iter.next()`.
+ sugg.push((recv.span, "iter".to_string()));
+ }
+ }
+ err.multipart_suggestion(
+ msg,
+ sugg,
+ Applicability::MaybeIncorrect,
+ );
+ } else {
+ err.help(msg);
+ }
}
}
diff --git a/tests/ui/suggestions/issue-102972.fixed b/tests/ui/suggestions/issue-102972.fixed
new file mode 100644
index 0000000000000..ebd73b2dc1499
--- /dev/null
+++ b/tests/ui/suggestions/issue-102972.fixed
@@ -0,0 +1,41 @@
+// run-rustfix
+
+fn test1() {
+ let mut chars = "Hello".chars();
+ let iter = chars.by_ref();
+ while let Some(_c) = iter.next() {
+ iter.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();
+ while let Some(_i) = iter.next() {
+ iter.next(); //~ ERROR borrow of moved value: `iter`
+ }
+}
+
+fn test3() {
+ let v = vec![(), (), ()];
+ let mut i = v.iter();
+ let iter = i.by_ref();
+ while let Some(()) = iter.next() {
+ iter.next(); //~ ERROR cannot borrow `i`
+ }
+}
+
+fn test4() {
+ let v = vec![(), (), ()];
+ let mut iter = v.iter();
+ while let Some(()) = iter.next() {
+ iter.next(); //~ ERROR borrow of moved value: `iter`
+ }
+}
+
+fn main() {
+ test1();
+ test2();
+ test3();
+ test4();
+}
diff --git a/tests/ui/suggestions/issue-102972.rs b/tests/ui/suggestions/issue-102972.rs
index 106288b054d5d..1f8e9776759ad 100644
--- a/tests/ui/suggestions/issue-102972.rs
+++ b/tests/ui/suggestions/issue-102972.rs
@@ -1,3 +1,5 @@
+// run-rustfix
+
fn test1() {
let mut chars = "Hello".chars();
for _c in chars.by_ref() {
@@ -13,4 +15,25 @@ fn test2() {
}
}
-fn main() { }
+fn test3() {
+ let v = vec![(), (), ()];
+ let mut i = v.iter();
+ for () in i.by_ref() {
+ i.next(); //~ ERROR cannot borrow `i`
+ }
+}
+
+fn test4() {
+ let v = vec![(), (), ()];
+ let mut iter = v.iter();
+ for () in iter {
+ iter.next(); //~ ERROR borrow of moved value: `iter`
+ }
+}
+
+fn main() {
+ test1();
+ test2();
+ test3();
+ test4();
+}
diff --git a/tests/ui/suggestions/issue-102972.stderr b/tests/ui/suggestions/issue-102972.stderr
index 3303d6bbc3fc8..4b0d3b96f8551 100644
--- a/tests/ui/suggestions/issue-102972.stderr
+++ b/tests/ui/suggestions/issue-102972.stderr
@@ -1,5 +1,5 @@
error[E0499]: cannot borrow `chars` as mutable more than once at a time
- --> $DIR/issue-102972.rs:4:9
+ --> $DIR/issue-102972.rs:6:9
|
LL | for _c in chars.by_ref() {
| --------------
@@ -8,9 +8,17 @@ LL | for _c in chars.by_ref() {
| 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`
+ |
+LL ~ let iter = chars.by_ref();
+LL ~ while let Some(_c) = iter.next() {
+LL ~ iter.next();
+ |
error[E0382]: borrow of moved value: `iter`
- --> $DIR/issue-102972.rs:12:9
+ --> $DIR/issue-102972.rs:14:9
|
LL | let mut iter = v.iter();
| -------- move occurs because `iter` has type `std::slice::Iter<'_, i32>`, which does not implement the `Copy` trait
@@ -19,10 +27,52 @@ LL | for _i in iter {
LL | iter.next();
| ^^^^ value borrowed here after move
|
+ = note: a for loop advances the iterator for you, the result is stored in `_i`
note: `into_iter` takes ownership of the receiver `self`, which moves `iter`
--> $SRC_DIR/core/src/iter/traits/collect.rs:LL:COL
+help: if you want to call `next` on a iterator within the loop, consider using `while let`
+ |
+LL | while let Some(_i) = iter.next() {
+ | ~~~~~~~~~~~~~~~ ~~~ +++++++
+
+error[E0499]: cannot borrow `i` as mutable more than once at a time
+ --> $DIR/issue-102972.rs:22:9
+ |
+LL | for () in i.by_ref() {
+ | ----------
+ | |
+ | first mutable borrow occurs here
+ | first borrow later used here
+LL | i.next();
+ | ^ second mutable borrow occurs here
+ |
+ = note: a for loop advances the iterator for you, the result is stored in its pattern
+help: if you want to call `next` on a iterator within the loop, consider using `while let`
+ |
+LL ~ let iter = i.by_ref();
+LL ~ while let Some(()) = iter.next() {
+LL ~ iter.next();
+ |
+
+error[E0382]: borrow of moved value: `iter`
+ --> $DIR/issue-102972.rs:30:9
+ |
+LL | let mut iter = v.iter();
+ | -------- move occurs because `iter` has type `std::slice::Iter<'_, ()>`, which does not implement the `Copy` trait
+LL | for () 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 its pattern
+note: `into_iter` takes ownership of the receiver `self`, which moves `iter`
+ --> $SRC_DIR/core/src/iter/traits/collect.rs:LL:COL
+help: if you want to call `next` on a iterator within the loop, consider using `while let`
+ |
+LL | while let Some(()) = iter.next() {
+ | ~~~~~~~~~~~~~~~ ~~~ +++++++
-error: aborting due to 2 previous errors
+error: aborting due to 4 previous errors
Some errors have detailed explanations: E0382, E0499.
For more information about an error, try `rustc --explain E0382`.