Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use structured suggestion for #113174 #113487

Merged
merged 1 commit into from
Oct 12, 2023
Merged

Use structured suggestion for #113174 #113487

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
172 changes: 149 additions & 23 deletions compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
}

Expand Down
41 changes: 41 additions & 0 deletions tests/ui/suggestions/issue-102972.fixed
View file
Open in desktop
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
Open in desktop
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();
}
56 changes: 53 additions & 3 deletions tests/ui/suggestions/issue-102972.stderr
View file
Open in desktop
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 @@ -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
Expand All @@ -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`.
Loading