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, + loop_bind: Option<&'hir Ident>, + loop_span: Option, + head_span: Option, + pat_span: Option, + 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() { + // mut iter => { + // [opt_ident]: loop { + // match Iterator::next(&mut iter) { + // None => break, + // Some() => , + // }; + // } + // } + // }; + // corresponding to the desugaring of a for loop `for in { }`. + 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()` + 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 `` 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 = ;`, we need to change where the + // already borrowed value was accessed from `.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`.