Skip to content

Commit

Permalink
Use structured suggestion for #113174
Browse files Browse the repository at this point in the history 
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()`.
  • Loading branch information
@estebank
estebank committed Jul 8, 2023
1 parent 1a449dc commit 6c1ba4a
Show file tree
Hide file tree
Showing 4 changed files with 265 additions and 32 deletions.
172 changes: 148 additions & 24 deletions compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1311,43 +1311,167 @@ 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 == 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
&& self.issue_span.source_equal(call.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 && ex.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 a 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);
}
}
}

Expand Down
41 changes: 41 additions & 0 deletions tests/ui/suggestions/issue-102972.fixed
View file
Original file line number Diff line number Diff line change
@@ -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();
}
25 changes: 24 additions & 1 deletion tests/ui/suggestions/issue-102972.rs
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
// run-rustfix

fn test1() {
let mut chars = "Hello".chars();
for _c in chars.by_ref() {
Expand All @@ -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();
}
59 changes: 52 additions & 7 deletions tests/ui/suggestions/issue-102972.stderr
View file
Original file line number Diff line number Diff line change
@@ -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() {
| --------------
Expand All @@ -9,11 +9,16 @@ LL | for _c in chars.by_ref() {
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`.
= 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
Expand All @@ -22,12 +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`.
= help: if you want to call `next` on a iterator within the loop, consider using `while let`.
= 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`.

0 comments on commit 6c1ba4a

Please sign in to comment.