Skip to content

Commit 9d2eb66

Browse files
estebankestebank
committed
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()`.
1 parent cdddcd3 commit 9d2eb66

File tree

4 files changed

+267
-27
lines changed

4 files changed

+267
-27
lines changed

‎compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs‎

Lines changed: 149 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1329,42 +1329,168 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
13291329
issue_span: Span,
13301330
expr_span: Span,
13311331
body_expr: Option<&'hir hir::Expr<'hir>>,
1332-
loop_bind: Option<Symbol>,
1332+
loop_bind: Option<&'hir Ident>,
1333+
loop_span: Option<Span>,
1334+
head_span: Option<Span>,
1335+
pat_span: Option<Span>,
1336+
head: Option<&'hir hir::Expr<'hir>>,
13331337
}
13341338
impl<'hir> Visitor<'hir> for ExprFinder<'hir> {
13351339
fn visit_expr(&mut self, ex: &'hir hir::Expr<'hir>) {
1336-
if let hir::ExprKind::Loop(hir::Block{ stmts: [stmt, ..], ..}, _, hir::LoopSource::ForLoop, _) = ex.kind &&
1337-
let hir::StmtKind::Expr(hir::Expr{ kind: hir::ExprKind::Match(call, [_, bind, ..], _), ..}) = stmt.kind &&
1338-
let hir::ExprKind::Call(path, _args) = call.kind &&
1339-
let hir::ExprKind::Path(hir::QPath::LangItem(LangItem::IteratorNext, _, _, )) = path.kind &&
1340-
let hir::PatKind::Struct(path, [field, ..], _) = bind.pat.kind &&
1341-
let hir::QPath::LangItem(LangItem::OptionSome, _, _) = path &&
1342-
let PatField { pat: hir::Pat{ kind: hir::PatKind::Binding(_, _, ident, ..), .. }, ..} = field &&
1343-
self.issue_span.source_equal(call.span) {
1344-
self.loop_bind = Some(ident.name);
1340+
// Try to find
1341+
// let result = match IntoIterator::into_iter(<head>) {
1342+
// mut iter => {
1343+
// [opt_ident]: loop {
1344+
// match Iterator::next(&mut iter) {
1345+
// None => break,
1346+
// Some(<pat>) => <body>,
1347+
// };
1348+
// }
1349+
// }
1350+
// };
1351+
// corresponding to the desugaring of a for loop `for <pat> in <head> { <body> }`.
1352+
if let hir::ExprKind::Call(path, [arg]) = ex.kind
1353+
&& let hir::ExprKind::Path(
1354+
hir::QPath::LangItem(LangItem::IntoIterIntoIter, _, _),
1355+
) = path.kind
1356+
&& arg.span.contains(self.issue_span)
1357+
{
1358+
// Find `IntoIterator::into_iter(<head>)`
1359+
self.head = Some(arg);
1360+
}
1361+
if let hir::ExprKind::Loop(
1362+
hir::Block { stmts: [stmt, ..], .. },
1363+
_,
1364+
hir::LoopSource::ForLoop,
1365+
_,
1366+
) = ex.kind
1367+
&& let hir::StmtKind::Expr(hir::Expr {
1368+
kind: hir::ExprKind::Match(call, [_, bind, ..], _),
1369+
span: head_span,
1370+
..
1371+
}) = stmt.kind
1372+
&& let hir::ExprKind::Call(path, _args) = call.kind
1373+
&& let hir::ExprKind::Path(
1374+
hir::QPath::LangItem(LangItem::IteratorNext, _, _),
1375+
) = path.kind
1376+
&& let hir::PatKind::Struct(path, [field, ..], _) = bind.pat.kind
1377+
&& let hir::QPath::LangItem(LangItem::OptionSome, pat_span, _) = path
1378+
&& call.span.contains(self.issue_span)
1379+
{
1380+
// Find `<pat>` and the span for the whole `for` loop.
1381+
if let PatField { pat: hir::Pat {
1382+
kind: hir::PatKind::Binding(_, _, ident, ..),
1383+
..
1384+
}, ..} = field {
1385+
self.loop_bind = Some(ident);
13451386
}
1387+
self.head_span = Some(*head_span);
1388+
self.pat_span = Some(pat_span);
1389+
self.loop_span = Some(stmt.span);
1390+
}
13461391

1347-
if let hir::ExprKind::MethodCall(body_call, _recv, ..) = ex.kind &&
1348-
body_call.ident.name == sym::next && ex.span.source_equal(self.expr_span) {
1349-
self.body_expr = Some(ex);
1392+
if let hir::ExprKind::MethodCall(body_call, recv, ..) = ex.kind
1393+
&& body_call.ident.name == sym::next
1394+
&& recv.span.source_equal(self.expr_span)
1395+
{
1396+
self.body_expr = Some(ex);
13501397
}
13511398

13521399
hir::intravisit::walk_expr(self, ex);
13531400
}
13541401
}
1355-
let mut finder =
1356-
ExprFinder { expr_span: span, issue_span, loop_bind: None, body_expr: None };
1402+
let mut finder = ExprFinder {
1403+
expr_span: span,
1404+
issue_span,
1405+
loop_bind: None,
1406+
body_expr: None,
1407+
head_span: None,
1408+
loop_span: None,
1409+
pat_span: None,
1410+
head: None,
1411+
};
13571412
finder.visit_expr(hir.body(body_id).value);
13581413

1359-
if let Some(loop_bind) = finder.loop_bind &&
1360-
let Some(body_expr) = finder.body_expr &&
1361-
let Some(def_id) = typeck_results.type_dependent_def_id(body_expr.hir_id) &&
1362-
let Some(trait_did) = tcx.trait_of_item(def_id) &&
1363-
tcx.is_diagnostic_item(sym::Iterator, trait_did) {
1364-
err.note(format!(
1365-
"a for loop advances the iterator for you, the result is stored in `{loop_bind}`."
1414+
if let Some(body_expr) = finder.body_expr
1415+
&& let Some(loop_span) = finder.loop_span
1416+
&& let Some(def_id) = typeck_results.type_dependent_def_id(body_expr.hir_id)
1417+
&& let Some(trait_did) = tcx.trait_of_item(def_id)
1418+
&& tcx.is_diagnostic_item(sym::Iterator, trait_did)
1419+
{
1420+
if let Some(loop_bind) = finder.loop_bind {
1421+
err.note(format!(
1422+
"a for loop advances the iterator for you, the result is stored in `{}`",
1423+
loop_bind.name,
1424+
));
1425+
} else {
1426+
err.note(
1427+
"a for loop advances the iterator for you, the result is stored in its pattern",
1428+
);
1429+
}
1430+
let msg = "if you want to call `next` on a iterator within the loop, consider using \
1431+
`while let`";
1432+
if let Some(head) = finder.head
1433+
&& let Some(pat_span) = finder.pat_span
1434+
&& loop_span.contains(body_expr.span)
1435+
&& loop_span.contains(head.span)
1436+
{
1437+
let sm = self.infcx.tcx.sess.source_map();
1438+
1439+
let mut sugg = vec![];
1440+
if let hir::ExprKind::Path(hir::QPath::Resolved(None, _)) = head.kind {
1441+
// A bare path doesn't need a `let` assignment, it's already a simple
1442+
// binding access.
1443+
// As a new binding wasn't added, we don't need to modify the advancing call.
1444+
sugg.push((
1445+
loop_span.with_hi(pat_span.lo()),
1446+
format!("while let Some("),
1447+
));
1448+
sugg.push((
1449+
pat_span.shrink_to_hi().with_hi(head.span.lo()),
1450+
") = ".to_string(),
1451+
));
1452+
sugg.push((
1453+
head.span.shrink_to_hi(),
1454+
".next()".to_string(),
1455+
));
1456+
} else {
1457+
// Needs a new a `let` binding.
1458+
let indent = if let Some(indent) = sm.indentation_before(loop_span) {
1459+
format!("\n{indent}")
1460+
} else {
1461+
" ".to_string()
1462+
};
1463+
let Ok(head_str) = sm.span_to_snippet(head.span) else {
1464+
err.help(msg);
1465+
return;
1466+
};
1467+
sugg.push((
1468+
loop_span.with_hi(pat_span.lo()),
1469+
format!("let iter = {head_str};{indent}while let Some("),
13661470
));
1367-
err.help("if you want to call `next` on an iterator within the loop, consider using `while let`.");
1471+
sugg.push((
1472+
pat_span.shrink_to_hi().with_hi(head.span.hi()),
1473+
") = iter.next()".to_string(),
1474+
));
1475+
// As a new binding was added, we should change how the iterator is advanced to
1476+
// use the newly introduced binding.
1477+
if let hir::ExprKind::MethodCall(_, recv, ..) = body_expr.kind
1478+
&& let hir::ExprKind::Path(hir::QPath::Resolved(None, ..)) = recv.kind
1479+
{
1480+
// As we introduced a `let iter = <head>;`, we need to change where the
1481+
// already borrowed value was accessed from `<recv>.next()` to
1482+
// `iter.next()`.
1483+
sugg.push((recv.span, "iter".to_string()));
1484+
}
1485+
}
1486+
err.multipart_suggestion(
1487+
msg,
1488+
sugg,
1489+
Applicability::MaybeIncorrect,
1490+
);
1491+
} else {
1492+
err.help(msg);
1493+
}
13681494
}
13691495
}
13701496

‎tests/ui/suggestions/issue-102972.fixed‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
// run-rustfix
2+
3+
fn test1() {
4+
let mut chars = "Hello".chars();
5+
let iter = chars.by_ref();
6+
while let Some(_c) = iter.next() {
7+
iter.next(); //~ ERROR cannot borrow `chars` as mutable more than once at a time
8+
}
9+
}
10+
11+
fn test2() {
12+
let v = vec![1, 2, 3];
13+
let mut iter = v.iter();
14+
while let Some(_i) = iter.next() {
15+
iter.next(); //~ ERROR borrow of moved value: `iter`
16+
}
17+
}
18+
19+
fn test3() {
20+
let v = vec![(), (), ()];
21+
let mut i = v.iter();
22+
let iter = i.by_ref();
23+
while let Some(()) = iter.next() {
24+
iter.next(); //~ ERROR cannot borrow `i`
25+
}
26+
}
27+
28+
fn test4() {
29+
let v = vec![(), (), ()];
30+
let mut iter = v.iter();
31+
while let Some(()) = iter.next() {
32+
iter.next(); //~ ERROR borrow of moved value: `iter`
33+
}
34+
}
35+
36+
fn main() {
37+
test1();
38+
test2();
39+
test3();
40+
test4();
41+
}

‎tests/ui/suggestions/issue-102972.rs‎

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
// run-rustfix
2+
13
fn test1() {
24
let mut chars = "Hello".chars();
35
for _c in chars.by_ref() {
@@ -13,4 +15,25 @@ fn test2() {
1315
}
1416
}
1517

16-
fn main() { }
18+
fn test3() {
19+
let v = vec![(), (), ()];
20+
let mut i = v.iter();
21+
for () in i.by_ref() {
22+
i.next(); //~ ERROR cannot borrow `i`
23+
}
24+
}
25+
26+
fn test4() {
27+
let v = vec![(), (), ()];
28+
let mut iter = v.iter();
29+
for () in iter {
30+
iter.next(); //~ ERROR borrow of moved value: `iter`
31+
}
32+
}
33+
34+
fn main() {
35+
test1();
36+
test2();
37+
test3();
38+
test4();
39+
}

‎tests/ui/suggestions/issue-102972.stderr‎

Lines changed: 53 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
error[E0499]: cannot borrow `chars` as mutable more than once at a time
2-
--> $DIR/issue-102972.rs:4:9
2+
--> $DIR/issue-102972.rs:6:9
33
|
44
LL | for _c in chars.by_ref() {
55
| --------------
@@ -8,9 +8,17 @@ LL | for _c in chars.by_ref() {
88
| first borrow later used here
99
LL | chars.next();
1010
| ^^^^^ second mutable borrow occurs here
11+
|
12+
= note: a for loop advances the iterator for you, the result is stored in `_c`
13+
help: if you want to call `next` on a iterator within the loop, consider using `while let`
14+
|
15+
LL ~ let iter = chars.by_ref();
16+
LL ~ while let Some(_c) = iter.next() {
17+
LL ~ iter.next();
18+
|
1119

1220
error[E0382]: borrow of moved value: `iter`
13-
--> $DIR/issue-102972.rs:12:9
21+
--> $DIR/issue-102972.rs:14:9
1422
|
1523
LL | let mut iter = v.iter();
1624
| -------- 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 {
1927
LL | iter.next();
2028
| ^^^^ value borrowed here after move
2129
|
30+
= note: a for loop advances the iterator for you, the result is stored in `_i`
2231
note: `into_iter` takes ownership of the receiver `self`, which moves `iter`
2332
--> $SRC_DIR/core/src/iter/traits/collect.rs:LL:COL
33+
help: if you want to call `next` on a iterator within the loop, consider using `while let`
34+
|
35+
LL | while let Some(_i) = iter.next() {
36+
| ~~~~~~~~~~~~~~~ ~~~ +++++++
37+
38+
error[E0499]: cannot borrow `i` as mutable more than once at a time
39+
--> $DIR/issue-102972.rs:22:9
40+
|
41+
LL | for () in i.by_ref() {
42+
| ----------
43+
| |
44+
| first mutable borrow occurs here
45+
| first borrow later used here
46+
LL | i.next();
47+
| ^ second mutable borrow occurs here
48+
|
49+
= note: a for loop advances the iterator for you, the result is stored in its pattern
50+
help: if you want to call `next` on a iterator within the loop, consider using `while let`
51+
|
52+
LL ~ let iter = i.by_ref();
53+
LL ~ while let Some(()) = iter.next() {
54+
LL ~ iter.next();
55+
|
56+
57+
error[E0382]: borrow of moved value: `iter`
58+
--> $DIR/issue-102972.rs:30:9
59+
|
60+
LL | let mut iter = v.iter();
61+
| -------- move occurs because `iter` has type `std::slice::Iter<'_, ()>`, which does not implement the `Copy` trait
62+
LL | for () in iter {
63+
| ---- `iter` moved due to this implicit call to `.into_iter()`
64+
LL | iter.next();
65+
| ^^^^ value borrowed here after move
66+
|
67+
= note: a for loop advances the iterator for you, the result is stored in its pattern
68+
note: `into_iter` takes ownership of the receiver `self`, which moves `iter`
69+
--> $SRC_DIR/core/src/iter/traits/collect.rs:LL:COL
70+
help: if you want to call `next` on a iterator within the loop, consider using `while let`
71+
|
72+
LL | while let Some(()) = iter.next() {
73+
| ~~~~~~~~~~~~~~~ ~~~ +++++++
2474

25-
error: aborting due to 2 previous errors
75+
error: aborting due to 4 previous errors
2676

2777
Some errors have detailed explanations: E0382, E0499.
2878
For more information about an error, try `rustc --explain E0382`.

0 commit comments

Comments
 (0)