NLL fails to remind users that locals in same scope are dropped in reverse declaration order

[pnkfelix]

Looking at a case like E0597.rs, we can see that the AST-borrowck includes the note:

rust/src/test/ui/error-codes/E0597.stderr

Line 10 in 2e104a7

= note: values in a scope are dropped in the opposite order they are created

while NLL has no reminder about the drop order for a pair of locals declared in the same lexical scope.

• (I personally am not a fan of the use of the word "created" in the error message there, since I attach that word to the dynamic semantics, while drop order is entirely defined by the declaration order, which is part of the static semantics (or really, part of the source syntax itself). The only thing that's potentially dynamic is whether or not a local variable actually holds a value at the moment that we go out of that local variable's scope.)

But anyway, the point is that NLL is failing to say anything at all on the matter, which is probably not ideal for Rust's target audience.

[pnkfelix]

The importance of doing this (and doing it well) may grow in the face of the potential change to the static semantics described in #51036

[estebank]

I personally am not a fan of the use of the word "created" in the error message there

@pnkfelix %s/created/declared/g?

[pnkfelix]

@estebank yes, s/created/declared/ would be better here, I think.

[nikomatsakis]

So, @spastorino and I have been talking a bit about this in Zulip. The actual NLL error message is in some ways better than the AST one. Notably, it includes the crucial bit of info that the final use of the variable is in the destructor for x:

error[E0597]: `y` does not live long enough
  --> src/test/ui/error-codes/E0597.rs:18:16
   |
18 |     x.x = Some(&y);
   |                ^^ borrowed value does not live long enough
19 |     //~^ `y` does not live long enough [E0597]
20 | }
   | -
   | |
   | borrowed value only lives until here
   | borrow later used here, when `x` is dropped

that said, it would be that much better if it also included the note about the relative order of variables being significant. The question is when to print such a thing. As I wrote on Zulip, I think we want to trigger the message under two conditions:

(1) the last use is when another local variable is dropped
(2) a local variable is borrowed

We find about Condition (1) in the function explain_why_borrow_contains_point:

rust/src/librustc_mir/borrow_check/nll/explain_borrow/mod.rs

Lines 56 to 89 in 2a00629

	Cause::DropVar(local, location) => {
	match find_drop_use(mir, regioncx, borrow, location, local) {
	Some(p) => match &mir.local_decls[local].name {
	Some(local_name) => {
	err.span_label(
	mir.source_info(p).span,
	format!(
	"borrow later used here, when `{}` is dropped",
	local_name
	),
	);
	}
	None => {
	err.span_label(
	mir.local_decls[local].source_info.span,
	"borrow may end up in a temporary, created here",
	);
	err.span_label(
	mir.source_info(p).span,
	"temporary later dropped here, \
	potentially using the reference",
	);
	}
	},
	None => {
	span_bug!(
	mir.source_info(context.loc).span,
	"Cause should end in a DropVar"
	);
	}
	}
	}

In that same function, we have access to some information about the borrow in the borrow_data variable:

rust/src/librustc_mir/borrow_check/nll/explain_borrow/mod.rs

Line 29 in 2a00629

borrow: &BorrowData<'tcx>,

The borrow_data struct includes in particular the place that was borrowed:

rust/src/librustc_mir/borrow_check/borrow_set.rs

Line 80 in 2a00629

crate borrowed_place: mir::Place<'tcx>,

so we can check that this Place is a local variable (or a field of a local variable). I would probably not issue the warning if the Place includes a Deref, I don't think, although we should maybe try to craft some examples where that is the case and make sure I'm right that the message doesn't seem relevant then.

[nikomatsakis]

Actually I was probably thinking about this wrong. Instead of looking at what path was borrowed, I think we want to consider the point of the error:

In particular, if the point of the error is a StorageDead or a Drop, then that means that we are dropping one variable (in this case, y) when some reference to it will be still be used in the destructor of another (x). That seems like the perfect time to give this note.

[nikomatsakis]

That means detecting when we are reporting an error due to this call (or the one for Drop):

rust/src/librustc_mir/borrow_check/mod.rs

Lines 501 to 509 in 2a00629

	StatementKind::StorageDead(local) => {
	self.access_place(
	ContextKind::StorageDead.new(location),
	(&Place::Local(local), span),
	(Shallow(None), Write(WriteKind::StorageDeadOrDrop)),
	LocalMutationIsAllowed::Yes,
	flow_state,
	);
	}

So specifically looking for WriteKind::StorageDeadOrDrop:

rust/src/librustc_mir/borrow_check/mod.rs

Line 505 in 2a00629

(Shallow(None), Write(WriteKind::StorageDeadOrDrop)),

[nikomatsakis]

Comment from Zulip, recording for posterity:

Recall that the basic structure of the "error reporting" phase of the borrow check is like this:

• Walk over the MIR
• For each "action" of various kinds, check if there is a conflicting loan:
  • In this particular case, if there is a StorageDead(y) or Drop(y), that means that the stack space for a local variable is being freed
  • Therefore, if there is a loan of that variable y, that's a problem and we should report an error

Currently, if there is a loan of that variable y, we report an error, and then we invoke explain_why_borrow_contains_point. This tries to report why the variable y is still considered borrowed. In the case of our example, we find that the reason is because that reference is going to be used in the Drop(x) for some other variable x.

What I am saying is that in precisely these conditions, where the current statement is a Drop/StorageDead and the final use is another Drop, we should report the note about the relative ordering of variables. This is because that means that

• we are dropping one variable (y)
• but a reference to that variable is going to be used in the destructor of another variable (x)

It seems like precisely here is where the relative order of variables is relevant.