NLL migration mode sometimes prints more diagnostics than either AST-borrowck or NLL

[pnkfelix]

While investigating #52979, I noticed some of the diagnostics from --edition 2018 are different than what either of the test.stderr and test.nll.stderr would lead one to expect to see.

This appears to arise from the implementation of -Z borrowck=migrate, from what I can tell.

For example, on ui/borrowck/borrowck-in-static.rs, we have from AST-borrowck:

rust/src/test/ui/borrowck/borrowck-in-static.stderr

Lines 1 to 9 in 40cb447

	error[E0507]: cannot move out of captured outer variable in an `Fn` closure
	--> $DIR/borrowck-in-static.rs:15:17
	|
	LL | let x = Box::new(0);
	| - captured outer variable
	LL | Box::new(|| x) //~ ERROR cannot move out of captured outer variable
	| ^ cannot move out of captured outer variable in an `Fn` closure
	error: aborting due to previous error

and from NLL we have:

rust/src/test/ui/borrowck/borrowck-in-static.nll.stderr

Lines 1 to 7 in 40cb447

	error[E0507]: cannot move out of captured variable in an `Fn` closure
	--> $DIR/borrowck-in-static.rs:15:17
	|
	LL | Box::new(|| x) //~ ERROR cannot move out of captured outer variable
	| ^ cannot move out of captured variable in an `Fn` closure
	error: aborting due to previous error

but I am seeing this from -Z borrowck=migrate:

error[E0507]: cannot move out of captured variable in an `Fn` closure
  --> ../src/test/ui/borrowck/borrowck-in-static.rs:15:17
   |
15 |     Box::new(|| x) //~ ERROR cannot move out of captured outer variable
   |                 ^ cannot move out of captured variable in an `Fn` closure

error[E0507]: cannot move out of `x`, as it is a captured variable in a `Fn` closure
  --> ../src/test/ui/borrowck/borrowck-in-static.rs:15:17
   |
15 |     Box::new(|| x) //~ ERROR cannot move out of captured outer variable
   |                 ^
   |                 |
   |                 cannot move out of `x`, as it is a captured variable in a `Fn` closure
   |                 cannot move
   |
help: consider changing this to accept closures that implement `FnMut`
  --> ../src/test/ui/borrowck/borrowck-in-static.rs:15:14
   |
15 |     Box::new(|| x) //~ ERROR cannot move out of captured outer variable
   |              ^^^^

error: aborting due to 2 previous errors

My guess that this is arising because I must have missed at least one case where AST-borrowck signals an error. Thus, when the -Z borrowck=migrate runs the AST-borrowck as a fallback in response to an error encounters during the MIR-borrowck, the user can sometimes see the both sets the errors from both NLL and AST-borrowck.

[pnkfelix]

Actually my hypothesis that this is the sum of the two sets of errors is clearly false given even just the simple example above. Where does -Z borrowck=migrate get the message:

error[E0507]: cannot move out of `x`, as it is a captured variable in a `Fn` closure

when that particular level of detail is not included in either of the other diagnostics presented in the other two files...?

[pnkfelix]

Ah wait, this was actually an expected side-effect of migrate mode, due to this part of #52681

rust/src/librustc_mir/borrow_check/mod.rs

Lines 1920 to 1958 in 4dae470

	Reservation(wk @ WriteKind::Move)
	| Write(wk @ WriteKind::Move)
	| Reservation(wk @ WriteKind::StorageDeadOrDrop)
	| Reservation(wk @ WriteKind::MutableBorrow(BorrowKind::Shared))
	| Write(wk @ WriteKind::StorageDeadOrDrop)
	| Write(wk @ WriteKind::MutableBorrow(BorrowKind::Shared)) => {
	if let Err(_place_err) = self.is_mutable(place, is_local_mutation_allowed) {
	if self.tcx.migrate_borrowck() {
	// rust-lang/rust#46908: In pure NLL mode this
	// code path should be unreachable (and thus
	// we signal an ICE in the else branch
	// here). But we can legitimately get here
	// under borrowck=migrate mode, so instead of
	// ICE'ing we instead report a legitimate
	// error (which will then be downgraded to a
	// warning by the migrate machinery).
	error_access = match wk {
	WriteKind::MutableBorrow(_) => AccessKind::MutableBorrow,
	WriteKind::Move => AccessKind::Move,
	WriteKind::StorageDeadOrDrop |
	WriteKind::Mutate => AccessKind::Mutate,
	};
	self.report_mutability_error(
	place,
	span,
	_place_err,
	error_access,
	location,
	);
	} else {
	self.tcx.sess.delay_span_bug(
	span,
	&format!(
	"Accessing `{:?}` with the kind `{:?}` shouldn't be possible",
	place, kind
	),
	);
	}
	}

[pnkfelix]

So there a some follow up questions I now have:

1. I do remember going through the control-flow for this case and figuring out why it "made sense" that this code path became reachable under migration mode. But I have since then forgotten the justification. It would be good to at least document the justification in a comment.
2. More upsetting (to me at least): the new error we get here under migration mode, presumably generated via self.report_mutability_error(..), looks slightly better to me than the errors we are getting from AST-borrowck and NLL: it names x, and (more importantly) it provides a hint that you may want to change the code so that it accepts FnMut closures...

[pnkfelix]

Having said that, neither of these follow up questions is sufficiently high priority for me to spend more time on them today. I'm going to unassign myself from this issue, but leave it open so that people can find it if they similarly encounter this arguably bizarre behavior from migration mode (which is turned on in --edition 2018 ...)

[pnkfelix]

although now that I think about it more ... I'm certain that in my original justification for the newly generated diagnostics from cases like borrowck-in-static.rs, I had thought it could only arise in cases where the NLL errors would be downgraded to warnings; i.e. situations where AST-borrowck did not error.

but clearly this is a case where AST-borrowck did error, or at least something in that pipeline errored.

So something is still funky here.

[nikomatsakis]

@pnkfelix I am doing a triage sweep and am not sure where to put this -- going to put in Edition Release for now I guess.