NLL: error from URL crate

[nikomatsakis]

In #47596, @SimonSapin reported several NLL errors in dependencies of the servo crate. @Aaron1011 later minimized one of those errors into this example:

#![feature(nll)]

struct MyStruct<'a> {
    field: &'a mut (),
    field2: WithDrop
}

struct WithDrop;

impl Drop for WithDrop {

    fn drop(&mut self) {}

}

impl<'a> MyStruct<'a> {

    fn consume(self) -> &'a mut () { self.field }
}

fn main() {}

which yields:

error[E0597]: `*self.field` does not live long enough
  --> src/main.rs:18:38
   |
18 |     fn consume(self) -> &'a mut () { self.field }
   |                                      ^^^^^^^^^^  - borrowed value only lives until here
   |                                      |
   |                                      borrowed value does not live long enough
   |
   = note: borrowed value must be valid for lifetime '_#3r...

Removing feature(nll) makes the code work, so probably this is an NLL bug.

[SimonSapin]

As noted in the other issue, changing the method to the one below works around the bug:

    fn consume(self) -> &'a mut () {
        let MyStruct { field, field2 } = self;
        field
    }

In other words, when self is owned self.field should move the field out rather than (re-)borrow it even if that field’s type happens to be &mut _. If other fields of self have destructors, they should be dropped individually as if the struct had been deconstructed with a pattern.

[nikomatsakis]

Another related case, from atomic_refcell, minimized by @lqd here:

https://play.rust-lang.org/?gist=5b3934b08dec4d034dc5b00fba8647c3&version=nightly

[davidtwco]

I'll take a look at this one.

[davidtwco]

@nikomatsakis I would appreciate some pointers on where I should be looking for this, couldn't spot anything when I took a look at what was happening in the areas of the code that I'm familiar with. Not worked with the NLL code much.

[nikomatsakis]

@davidtwco I don't know that this is an NLL problem per se, I think it has more to do with MIR borrowck. In particular, removing #![feature(nll)] and compiling with -Zborrowck=mir still gives roughly the same error. I tried running with -Zdump-mir=nll to dump out the MIR, and I see that the heart of the MIR in the relevant file (mir_dump/rustc.{{impl}}[1]-consume.-------.nll.0.mir) is:

        StorageLive(_2);
        _2 = &mut (*(_1.0: &mut ()));
        _0 = &mut (*_2);
        StorageDead(_2);

I think the error comes precisely at StorageDead(_2). I presume this is because of the borrow 0 = &mut *_2, which is still in effect. However, it's not supposed to be an error to kill the storage for _2 when *_2 is borrowed (because *_2 is a deref of a pointer; if _2.x were borrowed, that would be an error).

[nikomatsakis]

Tracing through, I expect that we start at a StorageDead item:

rust/src/librustc_mir/borrow_check/mod.rs

Lines 424 to 431 in 70f7d58

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

This invokes access_place:

rust/src/librustc_mir/borrow_check/mod.rs

Lines 711 to 724 in 70f7d58

	/// Checks an access to the given place to see if it is allowed. Examines the set of borrows
	/// that are in scope, as well as which paths have been initialized, to ensure that (a) the
	/// place is initialized and (b) it is not borrowed in some way that would prevent this
	/// access.
	///
	/// Returns true if an error is reported, false otherwise.
	fn access_place(
	&mut self,
	context: Context,
	place_span: (&Place<'tcx>, Span),
	kind: (ShallowOrDeep, ReadOrWrite),
	is_local_mutation_allowed: LocalMutationIsAllowed,
	flow_state: &Flows<'cx, 'gcx, 'tcx>,
	) -> AccessErrorsReported {

which then checks for conflicts:

rust/src/librustc_mir/borrow_check/mod.rs

Lines 744 to 745 in 70f7d58

	let conflict_error =
	self.check_access_for_conflict(context, place_span, sd, rw, flow_state);

Inside the check_access_for_conflict action:

rust/src/librustc_mir/borrow_check/mod.rs

Lines 753 to 760 in 70f7d58

	fn check_access_for_conflict(
	&mut self,
	context: Context,
	place_span: (&Place<'tcx>, Span),
	sd: ShallowOrDeep,
	rw: ReadOrWrite,
	flow_state: &Flows<'cx, 'gcx, 'tcx>,
	) -> bool {

I expect we head down this arm:

rust/src/librustc_mir/borrow_check/mod.rs

Line 823 in 70f7d58

| (Write(kind), _) => {

and then presumably here:

rust/src/librustc_mir/borrow_check/mod.rs

Lines 859 to 867 in 70f7d58

	WriteKind::StorageDeadOrDrop => {
	error_reported = true;
	this.report_borrowed_value_does_not_live_long_enough(
	context,
	borrow,
	place_span.1,
	flow_state.borrows.operator(),
	);
	}

Stepping back, I think the fundamental problem comes from the call to each_borrow_involving_path in check_access_for_conflict. In particular, I think it ought to be screening out a borrow of *foo, since the sd parameter is "shallow" (which is supposed to mean: do not trace through references):

rust/src/librustc_mir/borrow_check/mod.rs

Lines 762 to 766 in 70f7d58

	self.each_borrow_involving_path(
	context,
	(sd, place_span.0),
	flow_state,
	|this, index, borrow| match (rw, borrow.kind) {

Looking at the source of that function:

rust/src/librustc_mir/borrow_check/mod.rs

Lines 2054 to 2061 in 70f7d58

	fn each_borrow_involving_path<F>(
	&mut self,
	_context: Context,
	access_place: (ShallowOrDeep, &Place<'tcx>),
	flow_state: &Flows<'cx, 'gcx, 'tcx>,
	mut op: F,
	) where
	F: FnMut(&mut Self, ReserveOrActivateIndex, &BorrowData<'tcx>) -> Control,

I expect the problem must be in the places_conflict function invoked here:

rust/src/librustc_mir/borrow_check/mod.rs

Line 2076 in 70f7d58

if self.places_conflict(&borrowed.borrowed_place, place, access) {

But it seems like that function is returning false when we find a deref:

rust/src/librustc_mir/borrow_check/mod.rs

Lines 1967 to 1973 in 70f7d58

	(ProjectionElem::Deref, _, Shallow(None)) => {
	// e.g. a borrow of `*x.y` while we shallowly access `x.y` or some
	// prefix thereof - the shallow access can't touch anything behind
	// the pointer.
	debug!("places_conflict: shallow access behind ptr");
	return false;
	}

So I'm not sure where it goes wrong. Perhaps insert some debugs and see where my predictions go astray? =)

[nikomatsakis]

Ok, after some discussion with @davidtwco it has become clear where I went wrong. The error is actually being reported by a Drop(_1) terminator, where _1 is the self path. The problem here is that we currently assume that Drop(_1) may access any data reachable from _1 -- and that includes _1.field. If the type MyStruct had a destructor, that would be the right thing, and the error would be legit. But -- in this example -- that assumption is overly conservative.

I think what we have to do is to kind of "bake in" a bit of knowledge about the drop glue. For simplicity, I'll just describe what we need to do in terms of structs here as a kind of pseudo-code.

Right now we do this:

check_drop(X: Place):
    Check that deep access to X is legal

This is too conservative. We want to be doing something like:

check_drop(X: Place):
    let T = typeof(X);
    if T is a struct S and S does not have a destructor {
        // Since S does not have a destructor, the default drop glue
        // will just unroll its fields and drop them one by one. So let's
        // check that all of those accesses will be legal.
        for each field F defined in S {
            check_drop(X.F) // i.e., extend the Place X with an access to the field F
        }
    } else if T needs drop {
        check that deep access to X is legal
    } else {
        // no code executes when T is dropped, so no checks needed
    }
}  

If we did this, we would wind up unrolling the Drop(_1) into a drop of _1.field and _1.field2. the type of _1.field is an &mut, and those do not need drop, so nothing happens there. The type of _1.field2 is a struct with a dtor, so we'd check for conflicts on that path (and see none).