Better messages for `next` inside `for` loops

[lylythechosenone]

Given the following code:
playground

fn main() {
    let mut chars = "HHeelllloo,,  wwoorrlldd!!".chars();
    for c in chars.by_ref() {
        print!("{}", c);
        chars.next(); // Skip next
    }
}

The current output is:

error[[E0499]](https://doc.rust-lang.org/stable/error-index.html#E0499): cannot borrow `chars` as mutable more than once at a time
 --> src/main.rs:5:9
  |
3 |     for c in chars.by_ref() {
  |              --------------
  |              |
  |              first mutable borrow occurs here
  |              first borrow later used here
4 |         print!("{}", c);
5 |         chars.next(); // Skip next
  |         ^^^^^^^^^^^^ second mutable borrow occurs here

Ideally the output should look like:

error[[E0499]](https://doc.rust-lang.org/stable/error-index.html#E0499): cannot borrow `chars` as mutable more than once at a time
 --> src/main.rs:5:9
  |
3 |     for c in chars.by_ref() {
  |              --------------
  |              |
  |              first mutable borrow occurs here
  |              first borrow later used here
4 |         print!("{}", c);
5 |         chars.next(); // Skip 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` within the loop, consider `while let`.

or similar. The note and/or help are both subject to changes, this is just a basic idea.
We could also conceivably rewrite the error message to expand on what the note says, like so:

error[[E0499]](https://doc.rust-lang.org/stable/error-index.html#E0499): cannot call `next` inside a for loop, as this results in two mutable borrows of `chars`.
 --> src/main.rs:5:9
  |
3 |     for c in chars.by_ref() {
  |              --------------
  |              |
  |              first mutable borrow occurs here
  |              first borrow later used here
4 |         print!("{}", c);
5 |         chars.next(); // Skip 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` within the loop, consider `while let`.

[lyming2007]

@rustbot claim

[lyming2007]

fn main() {
    let mut chars = "HHeelllloo,,  wwoorrlldd!!".chars();
    for c in chars.by_ref() {
        print!("{}", c);
        chars.next(); // Skip next
    }
}

It's hard to get 'c' in line 3 during the context where the compiler errors out the second mutable borrow message, because the mutable borrow happens between chars.by_ref() and chars.next()

[BGR360]

I don't think it's critical to mention c in the diagnostic. We shouldn't let the difficulty of doing that stop us from trying to improve the message.

@rustbot label +D-newcomer-roadblock

[lylythechosenone]

I'd love to try to fix this, but sadly I'm not familiar with the internals of rustc. If someone could point me towards the section where this error is, I could give it a try.

[BGR360]

Hi @lylythechosenone !

Whenever I try to dive into fixing a problem with rustc, I usually start with a search through the codebase. In this case, I'd search for strings that come from the error message: "mutable more than once at a time", "first mutable borrow occurs here", or "second mutable borrow occurs here":

And that leads me to here:

rust/compiler/rustc_borrowck/src/borrowck_errors.rs

Lines 36 to 84 in 7d85104

	pub(crate) fn cannot_mutably_borrow_multiply(
	&self,
	new_loan_span: Span,
	desc: &str,
	opt_via: &str,
	old_loan_span: Span,
	old_opt_via: &str,
	old_load_end_span: Option<Span>,
	) -> DiagnosticBuilder<'cx, ErrorGuaranteed> {
	let via =
	|msg: &str| if msg.is_empty() { "".to_string() } else { format!(" (via {})", msg) };
	let mut err = struct_span_err!(
	self,
	new_loan_span,
	E0499,
	"cannot borrow {}{} as mutable more than once at a time",
	desc,
	via(opt_via),
	);
	if old_loan_span == new_loan_span {
	// Both borrows are happening in the same place
	// Meaning the borrow is occurring in a loop
	err.span_label(
	new_loan_span,
	format!(
	"{}{} was mutably borrowed here in the previous iteration of the loop{}",
	desc,
	via(opt_via),
	opt_via,
	),
	);
	if let Some(old_load_end_span) = old_load_end_span {
	err.span_label(old_load_end_span, "mutable borrow ends here");
	}
	} else {
	err.span_label(
	old_loan_span,
	format!("first mutable borrow occurs here{}", via(old_opt_via)),
	);
	err.span_label(
	new_loan_span,
	format!("second mutable borrow occurs here{}", via(opt_via)),
	);
	if let Some(old_load_end_span) = old_load_end_span {
	err.span_label(old_load_end_span, "first borrow ends here");
	}
	}
	err
	}

So then I'd use rust-analyzer to try to find all the usages of that method. In this case, turns out there's only one:

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

Lines 814 to 830 in 7d85104

	(BorrowKind::Mut { .. }, BorrowKind::Mut { .. }) => {
	first_borrow_desc = "first ";
	let mut err = self.cannot_mutably_borrow_multiply(
	span,
	&desc_place,
	&msg_place,
	issued_span,
	&msg_borrow,
	None,
	);
	self.suggest_split_at_mut_if_applicable(
	&mut err,
	place,
	issued_borrow.borrowed_place,
	);
	err
	}

And there we see that there is already some conditional suggestion being made; it calls Self::suggest_split_at_mut_if_applicable. And it seems there's a family of those suggest_*_if_applicable. So I think it would make sense to try to write your own explain_iterator_advancement_in_for_loop_if_applicable or something like that.

You'll need to figure out how to sort of "back track" from the MIR back to the HIR or the AST so you can figure out whether you're inside a for loop. I would study the code in compiler/rustc_borrowck/src/diagnostics/ to try to figure out how to do that.

And don't forget to write a handful of regression tests in src/test/ui/ somewhere, maybe in src/test/ui/iterators/: https://rustc-dev-guide.rust-lang.org/tests/ui.html

It might also be possible to perform this check at a higher level, say, in rustc_hir_analysis. If you can figure out that you're in a for loop, and that the user is trying to call .next() on the iterator, then you could raise the error then.