MIR-borrowck: emit "foo does not live long enough" instead of borrow errors

[arielb1]

e.g. in the test issue-36082.rs:

rust/src/test/compile-fail/issue-36082.rs

Lines 1 to 25 in f6d7514

	// Copyright 2016 The Rust Project Developers. See the COPYRIGHT
	// file at the top-level directory of this distribution and at
	// http://rust-lang.org/COPYRIGHT.
	//
	// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
	// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
	// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
	// option. This file may not be copied, modified, or distributed
	// except according to those terms.
	use std::cell::RefCell;
	fn main() {
	let mut r = 0;
	let s = 0;
	let x = RefCell::new((&mut r,s));
	let val: &_ = x.borrow().0;
	//~^ ERROR borrowed value does not live long enough
	//~| temporary value dropped here while still borrowed
	//~| temporary value created here
	//~| consider using a `let` binding to increase its lifetime
	println!("{}", val);
	}
	//~^ temporary value needs to live until here

MIR borrowck currently emits the following errors:

$ ./build/x86_64-unknown-linux-gnu/stage1/bin/rustc ../src/test/compile-fail/issue-36082.rs -Z borrowck-mir
error[E0597]: borrowed value does not live long enough (Ast)
  --> ../src/test/compile-fail/issue-36082.rs:18:31
   |
18 |     let val: &_ = x.borrow().0;
   |                   ----------  ^ temporary value dropped here while still borrowed
   |                   |
   |                   temporary value created here
...
24 | }
   | - temporary value needs to live until here
   |
   = note: consider using a `let` binding to increase its lifetime

error[E0505]: cannot move out of `_` because it is borrowed (Mir)
  --> ../src/test/compile-fail/issue-36082.rs:18:32
   |
18 |     let val: &_ = x.borrow().0;
   |                   ----------   ^ move out of `_` occurs here
   |                   |
   |                   borrow of `_` occurs here

error[E0505]: cannot move out of `_` because it is borrowed (Mir)
  --> ../src/test/compile-fail/issue-36082.rs:18:32
   |
18 |     let val: &_ = x.borrow().0;
   |                   ----------   ^ move out of `_` occurs here
   |                   |
   |                   borrow of `_` occurs here

error[E0506]: cannot assign to `_` because it is borrowed (Mir)
  --> ../src/test/compile-fail/issue-36082.rs:18:32
   |
18 |     let val: &_ = x.borrow().0;
   |                   ----------   ^ assignment to borrowed `_` occurs here
   |                   |
   |                   borrow of `_` occurs here

The MIR borrowck errors are correctly detected when the MIR storagedead and drop conflict with the pre-existing borrow.

That is the exact definition of a "does not live long enough" error, so we need to detect this case and instead of showing multiple borrow errors, show a single "does not live long enough" error.

[nikomatsakis]

I don't have time for detailed mentoring notes, but I suspect what we want to do is something like this. We'll add some sort of map in the borrowck context -- when we check either a storagedead or a drop (some things won't have drop), we enter the lvalue being affected into the map and check if we've already reported an error. If so, we do nothing (or, better, use span_delay_bug to report an error). Then we carry on. Otherwise, if map entry is not present, we need to report the nicer error.

A very first step might be to introduce the map and just use it to suppress some of the current errors, so that we get down to one instance of this message:

$ ./build/x86_64-unknown-linux-gnu/stage1/bin/rustc ../src/test/compile-fail/issue-36082.rs -Z borrowck-mir
error[E0505]: cannot move out of `_` because it is borrowed (Mir)
  --> ../src/test/compile-fail/issue-36082.rs:18:32
   |
18 |     let val: &_ = x.borrow().0;
   |                   ----------   ^ move out of `_` occurs here
   |                   |
   |                   borrow of `_` occurs here

instead of 3. (I'm actually not sure where all 3 come from.)

[arielb1]

@nikomatsakis

(I'm actually not sure where all 3 come from.)

There's a 1 storagedead, 1 drop on the happy path, and 1 drop on the unwind path. If there were "cache invalidations", there could be multiple drops on the happy/unwind path.

But sure, we should do the following:

1. When there's a conflict between a MIR Drop/StorageDead and a borrow, instead of emitting a "cannot move out of X because it is borrowed" error, emit a "borrow does not live long enough" error.
2. Keep a map, so that the "borrow does not live long enough" error isn't emitted multiple times (I'm not 100% sure this is needed, because the compiler already does identical error deduplication).
3. In the case of Drop and leaking &mut references #31567 (where a drop conflicts with a "deep" borrow), it's probably a good idea to have a new error message rather than "borrow does not live long enough".

[nikomatsakis]

So, @arielb1 and I chatted a bit on gitter. Seems like we were thinking in similar directions. Here are some more tips to help get started.

First off, the StorageDead MIR instruction is what removes stack space for a given local variable. It corresponds (roughly) to exiting from a scope. When we see a StorageDead(X) instruction in MIR, we invoke the access_lvalue subroutine:

rust/src/librustc_mir/borrow_check.rs

Lines 291 to 296 in b7ccb0a

	StatementKind::StorageDead(local) => {
	self.access_lvalue(ContextKind::StorageDead.new(location),
	(&Lvalue::Local(local), span),
	(Shallow(None), Write(WriteKind::StorageDead)),
	flow_state);
	}

access_lvalue checks whether a particular access is permitted. The borrow checker classifies accesses in various ways, but this case is called a "SHALLOW WRITE". The idea is that popping the stack space "writes" all the data in the value -- in this case, scribbling it with "uninitialized" values -- but it doesn't affect the data reached by any pointers (hence the "shallow" bit).

The other place where these errors arise is for the various kinds of DROP terminators (a "terminator" in MIR is an instruction that only runs at the end of a basic block; they are the places where branches in the control flow can occur):

rust/src/librustc_mir/borrow_check.rs

Lines 314 to 331 in b7ccb0a

	TerminatorKind::Drop { location: ref drop_lvalue, target: _, unwind: _ } => {
	self.consume_lvalue(ContextKind::Drop.new(loc),
	ConsumeKind::Drop,
	(drop_lvalue, span), flow_state);
	}
	TerminatorKind::DropAndReplace { location: ref drop_lvalue,
	value: ref new_value,
	target: _,
	unwind: _ } => {
	self.mutate_lvalue(ContextKind::DropAndReplace.new(loc),
	(drop_lvalue, span),
	Deep,
	JustWrite,
	flow_state);
	self.consume_operand(ContextKind::DropAndReplace.new(loc),
	ConsumeKind::Drop,
	(new_value, span), flow_state);
	}

DROP terminators correspond to running the destructor, so they also line up with exiting a scope. They precede the StorageDead for a value, but we only generate a DROP if there may be a destructor to run, so they are not always present. In any case, you can see that DROPs generate calls to consume_lvalue and some other routines. If you follow those to their definition, you will see that they too invoke access_lvalue (same as for shallow drop):

rust/src/librustc_mir/borrow_check.rs

Lines 592 to 607 in b7ccb0a

	fn consume_lvalue(&mut self,
	context: Context,
	consume_via_drop: ConsumeKind,
	lvalue_span: (&Lvalue<'tcx>, Span),
	flow_state: &InProgress<'b, 'gcx, 'tcx>) {
	let lvalue = lvalue_span.0;
	let ty = lvalue.ty(self.mir, self.tcx).to_ty(self.tcx);
	let moves_by_default =
	self.fake_infer_ctxt.type_moves_by_default(self.param_env, ty, DUMMY_SP);
	if moves_by_default {
	// move of lvalue: check if this is move of already borrowed path
	self.access_lvalue(context, lvalue_span, (Deep, Write(WriteKind::Move)), flow_state);
	} else {
	// copy of lvalue: check if this is "copy of frozen path" (FIXME: see check_loans.rs)
	self.access_lvalue(context, lvalue_span, (Deep, Read(ReadKind::Copy)), flow_state);
	}

Ultimately, a drop always generate a superset of the errors that storage dead do.

So, to suppress the duplicates, we can add a map to the MIR borrowck context struct:

rust/src/librustc_mir/borrow_check.rs

Lines 154 to 162 in b7ccb0a

	#[allow(dead_code)]
	pub struct MirBorrowckCtxt<'c, 'b, 'a: 'b+'c, 'gcx: 'a+'tcx, 'tcx: 'a> {
	tcx: TyCtxt<'a, 'gcx, 'tcx>,
	mir: &'b Mir<'tcx>,
	node_id: ast::NodeId,
	move_data: &'b MoveData<'tcx>,
	param_env: ParamEnv<'tcx>,
	fake_infer_ctxt: &'c InferCtxt<'c, 'gcx, 'tcx>,
	}

One question is what type should go into this set. The StorageDead instructions take a Local, which in MIR is just an index identifying a local variable (e.g., x). The DROP instructions take an Lvalue, which can represent any sort of path (e.g., x.y.z). However, at this stage in the MIR, we only ever have drop instructions that are operating on Local variables, I believe (later, during drop-elaboration, they get expanded to take more kinds of paths, but that hasn't happened at this stage). So we can convert everything into a mir::Local -- basically by matching on the Lvalue that comes in from a drop and assering that it is the Lvalue::Local variant.

Hence we would add a field sort of like storage_dead_or_drop_error_reported: FxHashSet<Local> with a suitable comment explaining its purpose, and then I imagine we could insert calls before we invoke any handling at all, so e.g. change the StorageDead match arm quoted first to something like:

        StatementKind::StorageDead(local) => {
            if self.storage_dead_or_drop_error_reported.insert(local) {
                self.access_lvalue(ContextKind::StorageDead.new(location),
                                   (&Lvalue::Local(local), span),
                                   (Shallow(None), Write(WriteKind::StorageDead)),
                                   flow_state);
            }
        }

(Similarly for the drop calls.)

That ought to at least suppress the duplicates!

[arielb1]

@nikomatsakis

Won't that cause a soundness issue if a local can be borrowed on the normal termination path and can't be borrowed on the unwind path?

I think we need to:

1. Use WriteKind::StorageDead for both TermiantorKind::Drop and StorageDead. We should also:
   1.a. probably add comments somewhere that ReadKind and WriteKind are only used for error message formatting.
   1.b. rename WriteKind::StorageDead to WriteKind::OutOfScope or something.

2. Change the error reporting code in access_lvalue to report an "foo does not live long enough" error instead of a borrow error for the newly-renamed WriteKind::OutOfScope.
   2.a. rustc deduplicates multiple occurrences of the same error message (as you can see by there being only 1 "cannot move out of" and 1 "cannot assign to" error message for every drop on a recent nightly, instead of 2 "cannot move out of" messages as in the OP), so making the error message the same for both StorageDead and Drop should ensure only 1 error message.
   But if that feels too fragile to you, you can add some deduplication on the borrowck side too. I'm not sure whether it's worth it.

3. Also, there's a case where the new error message would be confusing which you'll probably want to improve:
   The problem is that destructors can invalidate interior mutable references that would stay valid if there was only the StorageDead. For example, in this code, the destructor of VecWrapper invalidates the Box that is returned from get_dangling:

   struct VecWrapper<'a>(&'a mut S);
   
   struct S(Box<u32>);
   
   fn get_dangling<'a>(v: VecWrapper<'a>) -> &'a u32 {
       let s_inner: &'a S = &*v.0; //~ ERROR
       &s_inner.0
   }
   
   impl<'a> Drop for VecWrapper<'a> {
       fn drop(&mut self) {
           *self.0 = S(Box::new(0));
       }
   }

   This case is not handled correctly by AST borrowck, leading to issue Drop and leaking &mut references #31567 and unsoundness. However, we do not want to emit a "borrow does not live long enough" error - the problem isn't with the length of the borrow but with the destructor. So we might want to detect the case where a drop invalidates a "purely deep" borrow (i.e. a borrow that contains a dereference), and emit a "borrow might be invalidated by destructor" error. Not sure what is the best solution here TBH.

[nikomatsakis]

Won't that cause a soundness issue if a local can be borrowed on the normal termination path and can't be borrowed on the unwind path?

Hmm, yes, I originally intended to only use the map when an error is reported, but that's not what I wrote, is it?

Use WriteKind::StorageDead for both TermiantorKind::Drop and StorageDead

This seems ok to start. I do think we should at some point pay careful attention to the drop error messages: I think talking about "in the destructor" could help clarify to people why they are getting errors, and it's something we can't clearly say if we use StorageDead.

But if that feels too fragile to you, you can add some deduplication on the borrowck side too. I'm not sure whether it's worth it.

If we customize the error, then we can't lean on rustc's built-in deduplication, right?

[davidtwco]

I haven't had a chance to read the last two comments as apparently GitHub's loading of new comments w/out a refresh stopped working while I was implementing your previous comments. I have a WIP pull request with the previous instructions here: #45989.

I'll wait on implementing anything further until we're sure as to the approach we'd like to take.

[arielb1]

This seems ok to start. I do think we should at some point pay careful attention to the drop error messages: I think talking about "in the destructor" could help clarify to people why they are getting errors, and it's something we can't clearly say if we use StorageDead.

The distinction we want to make is between shallow and deep borrows. If there's a shallow borrow, we want to emit a "borrow does not live long enough" regardless of whether we first see a drop or storagedead (I'm quite sure the drop should always come first if it exists - after all, a storagedead without a drop is a leak - but that just strengthens the point), while if there's a deep borrow, we want a "destructor might invalidate borrow" error/future-compat-warning.