NLL: rewrites should include actual source input, not solely the hypothesized rewrites

[pnkfelix]

While working on #52663, I noticed something I had not seen before: the .nll.stderr files for borrowck-move-out-of-vec-tail.nll.stderr and borrowck-vec-pattern-nesting.nll.stderr have an odd characteristic: they print out highlighted spans that do not actually correspond to the actual input source code, but rather correspond to the hypothetical rewritten code that we are asking the user to type in.

---

Let me explain with actual pointers to lines in the files:

This is what the .nll.stderr files say we expect to see from the compiler (and what the compiler is actually printing under NLL mode, modulo details like the specific line numbers in the left-hand column):

rust/src/test/ui/borrowck/borrowck-move-out-of-vec-tail.nll.stderr

Lines 6 to 13 in fefe816

	help: to prevent move, use ref or ref mut
	|
	LL | &[Foo { string: ref a },
	| ^^^^^
	help: to prevent move, use ref or ref mut
	|
	LL | Foo { string: ref b }] => {
	| ^^^^^

rust/src/test/ui/borrowck/borrowck-vec-pattern-nesting.nll.stderr

Lines 65 to 76 in fefe816

	help: to prevent move, use ref or ref mut
	|
	LL | &mut [ref _a, _b, _c] => {} //~ ERROR cannot move out
	| ^^^^^^
	help: to prevent move, use ref or ref mut
	|
	LL | &mut [_a, ref _b, _c] => {} //~ ERROR cannot move out
	| ^^^^^^
	help: to prevent move, use ref or ref mut
	|
	LL | &mut [_a, _b, ref _c] => {} //~ ERROR cannot move out
	| ^^^^^^

In the .nll.stderr files that I linked above, the highlighted spans correspond to these lines:

rust/src/test/ui/borrowck/borrowck-move-out-of-vec-tail.rs

Lines 29 to 34 in fefe816

	match tail {
	&[Foo { string: a },
	//~^ ERROR cannot move out of type `[Foo]`
	//~| cannot move out
	//~| to prevent move
	Foo { string: b }] => {

rust/src/test/ui/borrowck/borrowck-vec-pattern-nesting.rs

Lines 77 to 79 in fefe816

	match vec {
	&mut [_a, _b, _c] => {} //~ ERROR cannot move out
	//~| cannot move out

(Do you see the problem? If not, read on...)

---

The problem I have with this is that when I see output from rustc like:

here be some code
     ^^

I expect to actually find the actual text here is some code in my source file. If the diagnostic wants to include a suggestion that we replace "be" with "is", then I expect that to appear as help text that says something like "write is instead of be" or even just "write is here" (with the "be" highlighted, as above)

In short, the user experience (at least for me) from the current NLL is a bit frustrating, because when I see the error message, it seems like it is saying "insert a ref here", but then from my point of view it is printing out the source code and my reaction is "there already is a ref there" (because I'm staring at the print-out in the user terminal and not the source code in my text editor).

---

The fix for this should be simple: identify the cases that are creating these strange diagnostics, and replace them with something more traditional. Note in particular that in both the cases above, the AST-borrowck generated error is more normal:

rust/src/test/ui/borrowck/borrowck-move-out-of-vec-tail.stderr

Lines 4 to 14 in fefe816

	LL | &[Foo { string: a },
	| ^ - hint: to prevent move, use `ref a` or `ref mut a`
	| __________________|
	| |
	LL | | //~^ ERROR cannot move out of type `[Foo]`
	LL | | //~| cannot move out
	LL | | //~| to prevent move
	LL | | Foo { string: b }] => {
	| |_________________________________-__^ cannot move out of here
	| |
	| ...and here (use `ref b` or `ref mut b`)

rust/src/test/ui/borrowck/borrowck-vec-pattern-nesting.stderr

Lines 64 to 69 in fefe816

	LL | &mut [_a, _b, _c] => {} //~ ERROR cannot move out
	| ^--^^--^^--^
	| || | |
	| || | ...and here (use `ref _c` or `ref mut _c`)
	| || ...and here (use `ref _b` or `ref mut _b`)
	| |hint: to prevent move, use `ref _a` or `ref mut _a`

However, if someone can point me at at an established precedent for this pattern (lets say at least three examples) in the current rustc, then I would also be willing to accept that this is something we already do and that I am just an old fogie who is unaware of how the UX tide is shifting.

[pnkfelix]

One thing that bothers me about this ticket: the rustc code generating the diagnostic in question, AFAICT, does not appear to be doing anything out of the ordinary:

rust/src/librustc_mir/borrow_check/move_errors.rs

Lines 354 to 360 in 8961132

	Some(name) => {
	err.span_suggestion(
	binding_span,
	"to prevent move, use ref or ref mut",
	format!("{} {:?}", ref_kind, name),
	);
	}

But if that code isn't doing anything weird to try to construct this diagnostic, then is this in fact a kind of diagnostic that we expect to see in practice?

(Note: I am not saying that I dislike printing out lines of hypothetical code. My whole problem here is just about the row of "^^^^^^" being printed underneath the hypothesized lines, because I interpret those as highlighting existing code. If those were not there, I probably would never have noticed this in the first place)

[pnkfelix]

Note also that in comparing with the AST-borrowck code:

rust/src/librustc_borrowck/borrowck/gather_loans/move_error.rs

Lines 172 to 188 in 8961132

	fn note_move_destination(mut err: DiagnosticBuilder,
	move_to_span: syntax_pos::Span,
	pat_name: ast::Name,
	is_first_note: bool) -> DiagnosticBuilder {
	if is_first_note {
	err.span_label(
	move_to_span,
	format!("hint: to prevent move, use `ref {0}` or `ref mut {0}`",
	pat_name));
	err
	} else {
	err.span_label(move_to_span,
	format!("...and here (use `ref {0}` or `ref mut {0}`)",
	pat_name));
	err
	}
	}

it should indeed be really easy to try to recreate the same combined set of diagnostics here that AST-borrowck is able to produce. if we want that (e.g. by switching to span_label instead of span_diagnostic...)

[pnkfelix]

Okay, after quite a bit of review, I tracked down code in the compiler that decides whether to emit the ^^^^^ for a suggestion:

rust/src/librustc_errors/emitter.rs

Lines 1217 to 1222 in ed8d14d

	// Only show underline if the suggestion spans a single line and doesn't cover the
	// entirety of the code output. If you have multiple replacements in the same line
	// of code, show the underline.
	let show_underline = !(parts.len() == 1
	&& parts[0].snippet.trim() == complete.trim())
	&& complete.lines().count() == 1;

And after I found that line, I was able to find its associated PR, and thus find an example of a test that is making use of this ... "feature" ...:

rust/src/test/ui/impl-trait/impl-generic-mismatch.stderr

Lines 20 to 25 in ed8d14d

	LL | fn bar(&self, _: &impl Debug) { }
	| ^^^^^^^^^^ expected generic parameter, found `impl Trait`
	help: try changing the `impl Trait` argument to a generic parameter
	|
	LL | fn bar<U: Debug>(&self, _: &U) { }
	| ^^^^^^^^^^ ^

However, that example has a crucial difference. Yes, it prints a rewrite of the code (and highlights the rewritten portion). But, it only does so after it has printed an error that highlights the problem in the original source. And that seems like one potential way to resolve this problem in our case.