New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NLL fixes #46984

Merged
merged 3 commits into from Jan 3, 2018

Conversation

Projects
None yet
5 participants
@arielb1
Contributor

arielb1 commented Dec 24, 2017

First, introduce pre-statement effects to dataflow to fix #46875. Edge dataflow effects might make that redundant, but I'm not sure of the best way to integrate them with liveness etc., and if this is a hack, this is one of the cleanest hacks I've seen.

And I want a small fix to avoid the torrent of bug reports.

Second, fix linking of projections to fix #46974

r? @pnkfelix

arielb1 added some commits Dec 23, 2017

fix linking of place projections
projections other than dereferences of `&mut` used to do no linking. Fix
that.

Fixes #46974.
.operator()
.before_statement_effect(&mut sets, loc);
}
self.apply_local_effect(loc);

This comment has been minimized.

@nikomatsakis

nikomatsakis Jan 1, 2018

Contributor

Hmm, so, I have the edge-effect implemented, but I'm not sure what I think about it. On the one hand, I think the work I did feels like a cleaner approach than the existing "special case" for "call-normal-return". On the other hand, adding edge effects is not quite sufficient to make everything work correctly: the problem has to do with how the "activation" checks in two-phase borrows work.

What I did on my branch is basically that the "statement effect" for some statement S kills all the borrows whose regions do not include the successor S'. In other words, when you check a statement like bb0[5], you kill borrows whose regions do not include bb0[6]. Then, on an edge targeting bb1, you would kill borrows that do not include bb1[0]. (No borrows are killed in the terminator effect.)

This works fine for most things, but it has a funny effect on the activation check. In particular, to determine at some statement S which borrows have been activated, we iterate through the gens that are present in the statement effect. But this isn't quite right: the statement effect here represents the state on entry to S', but we really want to see some point before that -- the statement before as we exit S, but before we enter S'. In particular, if you have some borrow that is activated by S, but goes out of scope in S', we want to see that activation as a "gen", but the setup I described above will gen the bit but then later kill it.

I can see two ways to fix this. The first is to allow gen/kill bits to both be set. In the specific case of activations, this would mean that statement S both gens and kills the same bit. This seems ok but only happens to work because in this case the kill comes temporally after the gen.

The other was adding a method like the one you wound up adding in this PR. And maybe that just suffices, then.

My only nit here is that I'd like to improve the comments on reconstruct_statement_effect -- it feels surprising to me that it "applies" an effect. I think the code is ultimately doing the right thing, though, we just need to be precise in terms of describing what is happening -- in particular, reconstruct_statement_effect is basically setting up the effects so that:

  • the state represents the point where we have "entered" S but not yet executed it
    • (which is why we do an apply here)
  • the gen/kill bits represent the net effect of executing S
    • (but not the effect of entering S')

My mental model then is that there are sort of two points in time for each statement, let's call them S-before and S-after. reconstruct_statement_effect is thus setting our state to S-before and setting up the gen/kill to transition to S-after.

It's probably worth noting somewhere (or maybe even with a debug-assertion) that there is an implicit cursor -- i.e., you can't reconstruct the statement effect for statement 1 if you have not done statement 0 first.

(In any case, we could take the edge effect changes just as a refactoring, but we don't have to. It makes the code more regular but perhaps a bit more complex. I can open the PR and @pnkfelix and I can discuss.)

@nikomatsakis

nikomatsakis Jan 1, 2018

Contributor

Hmm, so, I have the edge-effect implemented, but I'm not sure what I think about it. On the one hand, I think the work I did feels like a cleaner approach than the existing "special case" for "call-normal-return". On the other hand, adding edge effects is not quite sufficient to make everything work correctly: the problem has to do with how the "activation" checks in two-phase borrows work.

What I did on my branch is basically that the "statement effect" for some statement S kills all the borrows whose regions do not include the successor S'. In other words, when you check a statement like bb0[5], you kill borrows whose regions do not include bb0[6]. Then, on an edge targeting bb1, you would kill borrows that do not include bb1[0]. (No borrows are killed in the terminator effect.)

This works fine for most things, but it has a funny effect on the activation check. In particular, to determine at some statement S which borrows have been activated, we iterate through the gens that are present in the statement effect. But this isn't quite right: the statement effect here represents the state on entry to S', but we really want to see some point before that -- the statement before as we exit S, but before we enter S'. In particular, if you have some borrow that is activated by S, but goes out of scope in S', we want to see that activation as a "gen", but the setup I described above will gen the bit but then later kill it.

I can see two ways to fix this. The first is to allow gen/kill bits to both be set. In the specific case of activations, this would mean that statement S both gens and kills the same bit. This seems ok but only happens to work because in this case the kill comes temporally after the gen.

The other was adding a method like the one you wound up adding in this PR. And maybe that just suffices, then.

My only nit here is that I'd like to improve the comments on reconstruct_statement_effect -- it feels surprising to me that it "applies" an effect. I think the code is ultimately doing the right thing, though, we just need to be precise in terms of describing what is happening -- in particular, reconstruct_statement_effect is basically setting up the effects so that:

  • the state represents the point where we have "entered" S but not yet executed it
    • (which is why we do an apply here)
  • the gen/kill bits represent the net effect of executing S
    • (but not the effect of entering S')

My mental model then is that there are sort of two points in time for each statement, let's call them S-before and S-after. reconstruct_statement_effect is thus setting our state to S-before and setting up the gen/kill to transition to S-after.

It's probably worth noting somewhere (or maybe even with a debug-assertion) that there is an implicit cursor -- i.e., you can't reconstruct the statement effect for statement 1 if you have not done statement 0 first.

(In any case, we could take the edge effect changes just as a refactoring, but we don't have to. It makes the code more regular but perhaps a bit more complex. I can open the PR and @pnkfelix and I can discuss.)

}
ty::TyAdt(def, _) if def.is_box() => {

This comment has been minimized.

@nikomatsakis

nikomatsakis Jan 3, 2018

Contributor

is the change here the inclusion of Box? (i.e., the behavior for other cases is the same?)

@nikomatsakis

nikomatsakis Jan 3, 2018

Contributor

is the change here the inclusion of Box? (i.e., the behavior for other cases is the same?)

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Jan 3, 2018

Contributor

@bors r+

Contributor

nikomatsakis commented Jan 3, 2018

@bors r+

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jan 3, 2018

Contributor

📌 Commit bd1bd76 has been approved by nikomatsakis

Contributor

bors commented Jan 3, 2018

📌 Commit bd1bd76 has been approved by nikomatsakis

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis
Contributor

nikomatsakis commented Jan 3, 2018

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jan 3, 2018

Contributor

⌛️ Testing commit bd1bd76 with merge 0a3761e...

Contributor

bors commented Jan 3, 2018

⌛️ Testing commit bd1bd76 with merge 0a3761e...

bors added a commit that referenced this pull request Jan 3, 2018

Auto merge of #46984 - arielb1:pre-statement-effect, r=nikomatsakis
NLL fixes

First, introduce pre-statement effects to dataflow to fix #46875. Edge dataflow effects might make that redundant, but I'm not sure of the best way to integrate them with liveness etc., and if this is a hack, this is one of the cleanest hacks I've seen.

And I want a small fix to avoid the torrent of bug reports.

Second, fix linking of projections to fix #46974

r? @pnkfelix
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jan 3, 2018

Contributor

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 0a3761e to master...

Contributor

bors commented Jan 3, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 0a3761e to master...

@bors bors merged commit bd1bd76 into rust-lang:master Jan 3, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment