Stream: t-compiler/wg-nll

Topic: #53569 even more dangly paths


Matthew Jasper (Aug 28 2018 at 19:43, on Zulip):

@nikomatsakis @pnkfelix Creating a separate thread for this.

Matthew Jasper (Aug 28 2018 at 21:44, on Zulip):

So my idea on how to check this in places_conflict

Matthew Jasper (Aug 28 2018 at 21:45, on Zulip):

Check as normal until the access place runs out of projections and the two places are DisjointOrEqual at this point (this is the same place that shallow and deep accesses begin to differ)

Matthew Jasper (Aug 28 2018 at 21:47, on Zulip):

After here we go through the remaining projections on the borrow place, if we find a field projection from an ADT which implements Drop we stop and return that the places conflict.

Matthew Jasper (Aug 28 2018 at 21:48, on Zulip):

If we reach a deference of a built in reference then we stop and return that the places don't conflict.

Matthew Jasper (Aug 28 2018 at 21:48, on Zulip):

If we run out of projections then we return that the places conflict.

Matthew Jasper (Aug 28 2018 at 21:51, on Zulip):

Dereferencing a Box is special cased here (although it's special inherently). I think we could extend this using #[may_dangle] annotations, but other #[may_dangle] standard library types don't contain references and don't have any public fields.

Matthew Jasper (Aug 28 2018 at 21:55, on Zulip):

We only consider ADTs because all of the other types with arbitrary Drop glue can't be the type of the base of a projection.

pnkfelix (Sep 11 2018 at 13:48, on Zulip):

Hey @Matthew Jasper , so @nikomatsakis and I just noticed that we failed to followup here. (I'm posting here largely to bump up this topic in the list under the NLL stream)

pnkfelix (Sep 18 2018 at 11:45, on Zulip):

@Matthew Jasper so I'm finally taking some time to try to understand the idea you have sketched here.

pnkfelix (Sep 18 2018 at 11:49, on Zulip):

Am I correct in inferring that your outline is responding to the scenario laid out by @nikomatsakis here: https://github.com/rust-lang/rust/issues/53569#issuecomment-414821527

pnkfelix (Sep 18 2018 at 11:50, on Zulip):

namely, when you talk about changing the behavior of places_conflict, I infer that is to revise our behavior in the case where today we are erroring out when we hit the Drop(v) that conflicts with the outstanding loan of v.as<Ok>.0 ?

pnkfelix (Sep 18 2018 at 11:57, on Zulip):

I do think I'm starting to see how this works. Namely, we know that drop glue follows a structural pattern. We already do some recursive structural analysis when we look at the access itself (the Drop(v)), but this is extending that thinking to also apply to the outstanding borrows. If no destructors occur on the whole path (starting from and including the accessed place, but continuing on into the place described by the borrow), then we know the drop itself doesn't actually touch the borrow. Is that the basic idea?

pnkfelix (Sep 18 2018 at 11:58, on Zulip):

For this to work we'd need to feed the WriteKind for the access down into places_conflict, right?

pnkfelix (Sep 18 2018 at 11:58, on Zulip):

(or at least a boolean "is_drop"; but if you're going to do that you might as well do the whole WriteKind)

Matthew Jasper (Sep 18 2018 at 12:03, on Zulip):

(or at least a boolean "is_drop"; but if you're going to do that you might as well do the whole WriteKind)

This (a new variant to either WriteKind, or to ShallowOrDeep) seemed like the best idea when I tried implementing it.

pnkfelix (Sep 18 2018 at 12:07, on Zulip):

Yes I just started looking at hacking this up, and realized that a boolean flag is_drop on ShallowOrDeep::Deep might be best.

pnkfelix (Sep 18 2018 at 12:09, on Zulip):

How far did you get implementing it?

Matthew Jasper (Sep 18 2018 at 12:39, on Zulip):

It was pretty much done I think.

pnkfelix (Sep 18 2018 at 12:44, on Zulip):

one problem with the idea as you have laid it out, depending on whether we are still tracking borrows of derefs of immutable-references ... it may signal false conflicts when you have a Drop(v) and a reborrow of &*v.a where v.a: &State and v: SomeDropType

pnkfelix (Sep 18 2018 at 12:44, on Zulip):

i.e. it is okay to such borrows to be created and last beyond the destruction of v

pnkfelix (Sep 18 2018 at 12:45, on Zulip):

But one easy way around that is to not signal a conflict eagerly upon seeing a type that needs_drop.

pnkfelix (Sep 18 2018 at 12:45, on Zulip):

instead just remember that you saw it and keep descending

pnkfelix (Sep 18 2018 at 14:43, on Zulip):

okay so I've put up PR #54324

pnkfelix (Sep 18 2018 at 14:43, on Zulip):

mostly for @Matthew Jasper to take a look at

pnkfelix (Sep 18 2018 at 14:44, on Zulip):

But I infer from the discussion that @Matthew Jasper had a much broader set of revisions in mind that covered more cases?

nikomatsakis (Sep 18 2018 at 16:40, on Zulip):

@Matthew Jasper do you have a PR with your approach?

nikomatsakis (Sep 18 2018 at 16:41, on Zulip):

I think it might honestly be easier for me to see some code than to follow the writing here :)

Matthew Jasper (Sep 18 2018 at 18:18, on Zulip):

branch: https://github.com/rust-lang/rust/compare/master...matthewjasper:better-drop-access

nikomatsakis (Sep 18 2018 at 18:24, on Zulip):

Huh, I see. That's...quite clever.

nikomatsakis (Sep 18 2018 at 18:25, on Zulip):

93 additions and 366 deletions.

nikomatsakis (Sep 18 2018 at 18:25, on Zulip):

always nice

nikomatsakis (Sep 18 2018 at 18:25, on Zulip):

as an aside, I think that the name places_conflict is really bad. It seemed good, but it keeps leading me to wrong intuitions

nikomatsakis (Sep 18 2018 at 18:25, on Zulip):

I think borrow_conflicts_with_access would be much better

nikomatsakis (Sep 18 2018 at 18:26, on Zulip):

and the idea of adding a new kind of access — a Drop access — makes total sense

nikomatsakis (Sep 18 2018 at 18:26, on Zulip):

+1 from me

nikomatsakis (Sep 18 2018 at 18:26, on Zulip):

as an aside, I think that the name places_conflict is really bad. It seemed good, but it keeps leading me to wrong intuitions

specifically, it sounds like it is testing a property of two "places in isolation"...but that's not really true

pnkfelix (Sep 18 2018 at 18:55, on Zulip):

+1 from me too. That branch seems like the right direction to go in.

nikomatsakis (Sep 18 2018 at 18:57, on Zulip):

@pnkfelix do you want to close https://github.com/rust-lang/rust/pull/54324 then?

pnkfelix (Sep 18 2018 at 18:57, on Zulip):

yeah I was just putting a commnet up on it

Matthew Jasper (Sep 23 2018 at 15:45, on Zulip):

PR #54509

Last update: Nov 21 2019 at 13:15UTC