Kill conflicting borrows of places with projections. by ecstatic-morse · Pull Request #62010 · rust-lang/rust

ecstatic-morse · 2019-06-20T23:35:17Z

Resolves#62007.

Due to a bug, the previous version of this check did not actually kill all conflicting borrows unless the borrowed place had no projections. Specifically, sets.on_entry will always be empty when statement_effect is called. It does not contain the set of borrows which are live at this point in the program.

@pnkfelix describes why this was not caught before in #62007, and created an example where the current borrow checker failed unnecessarily. This PR adds their example as a test, but they will likely want to add some additional ones.

r? @pnkfelix

Resolvesrust-lang#62007. Due to a bug, the previous version of this check did not actually kill any conflicting borrows unless the borrowed place had no projections. Specifically, `entry_set` will always be empty when `statement_effect` is called. It does not contain the set of borrows which are live at this point in the program.

ecstatic-morse · 2019-06-20T23:48:32Z

This still needs some docs in the test case pointing to #46589, as well as a comment explaining why it's sound not to kill any borrows, but I'll wait to see if the tests pass and for @pnkfelix to weigh in here.

pnkfelix · 2019-06-21T09:04:33Z

The test passed, so that's good.

It would be good to add tests of each of the other kinds of projections, and check how they are handled.

The test you've added here handles ProjectionElem::Field.

At the very least, I'd like to see:

Deref by passing in Box<&mut List<T>> instead of (&mut List<T>,) (and adjusting the test by adding *value where necessary). I expect this one to behave just like the tuple.
ConstantIndex by passing in [&mut List<T>; 2] (and adjusting the test by adding value[0] where necessary). I expect this one to behave just like the tuple ...
- ... but its possible that it doesn't currently pass (and that would be fine).
- (yeah, I added the above sub-bullet after I confirmed that it doesn't pass. But it was what I was thinking when I wrote my expectation above!)
Index by passing in [&mut List<T]>; 2] and adjusting the test by adding value[i] where i is a local variable. I am not expecting this one to be accepted; it will probably need to be a ~~compile-fail~~ ui test.

It would of course be good to cover the other ProjectionElem variants: Downcast, Subslice; but they would require a little bit more adjustment to the test I think, so I cannot dash them off at the top of my head.

pnkfelix · 2019-06-21T09:09:53Z

(the code changes here look good, too. I'll actually see about adding the tests I described so that we can get this into the pipeline to land sooner rather than later.)

overwriting one field should not allow reborrow of an unrelated field.

…ection.

pnkfelix · 2019-06-21T11:34:17Z

I tried to make a variant of the tests I added above that covered the case of a downcast-projection, but I'm not sure if I know a way to get a downcast projection onto the righthand side of an assignment in the form that we need here to observe the phenomenon being tested.

So I'm not going to worry about adding a test for that case at this point.

(LIkewise, I'm not going to worry about subslice projection either.)

pnkfelix · 2019-06-21T11:34:36Z

r=me assuming the tests I added all pass the CI.

pnkfelix · 2019-06-21T11:36:19Z

@bors r+

bors · 2019-06-21T11:36:20Z

📌 Commit f483269 has been approved by pnkfelix

pnkfelix · 2019-06-21T11:37:12Z

@bors rollup=never

(This PR may affect performance. I don't think it will, so I'm not blocking it on a perf run. But I also want it to be apparent in the graphs if this PR does affect performance.)

@pnkfelix

…, r=pnkfelix Kill conflicting borrows of places with projections. Resolvesrust-lang#62007. Due to a bug, the previous version of this check did not actually kill all conflicting borrows unless the borrowed place had no projections. Specifically, `sets.on_entry` will always be empty when `statement_effect` is called. It does not contain the set of borrows which are live at this point in the program. @pnkfelix describes why this was not caught before in rust-lang#62007, and created an example where the current borrow checker failed unnecessarily. This PR adds their example as a test, but they will likely want to add some additional ones. r? @pnkfelix

@pnkfelix

…, r=pnkfelix Kill conflicting borrows of places with projections. Resolvesrust-lang#62007. Due to a bug, the previous version of this check did not actually kill all conflicting borrows unless the borrowed place had no projections. Specifically, `sets.on_entry` will always be empty when `statement_effect` is called. It does not contain the set of borrows which are live at this point in the program. @pnkfelix describes why this was not caught before in rust-lang#62007, and created an example where the current borrow checker failed unnecessarily. This PR adds their example as a test, but they will likely want to add some additional ones. r? @pnkfelix

pnkfelix · 2019-06-21T23:38:37Z

@Centril did I mistype the command for indicating this PR should not be included in a rollup?

Centril · 2019-06-21T23:41:29Z

@pnkfelix No you did it right, I just didn't look closely enough.

It would be good if homu enforced this automatically and refused to make the rollup... cc @kennytm

bors · 2019-06-22T05:12:18Z

⌛ Testing commit f483269 with merge 305930c...

@pnkfelix

Kill conflicting borrows of places with projections. Resolves#62007. Due to a bug, the previous version of this check did not actually kill all conflicting borrows unless the borrowed place had no projections. Specifically, `sets.on_entry` will always be empty when `statement_effect` is called. It does not contain the set of borrows which are live at this point in the program. @pnkfelix describes why this was not caught before in #62007, and created an example where the current borrow checker failed unnecessarily. This PR adds their example as a test, but they will likely want to add some additional ones. r? @pnkfelix

kennytm · 2019-06-22T07:20:34Z

@Centril Please file a feature request on rust-lang/homu

bors · 2019-06-22T07:58:51Z

☀️ Test successful - checks-travis, status-appveyor
Approved by: pnkfelix
Pushing 305930c to master...

rust-highfive assigned pnkfelix Jun 20, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 20, 2019

Centril reviewed Jun 20, 2019
View reviewed changes

Comment threadsrc/test/run-pass/borrowck/borrowck-borrow-of-projection-kills-other-borrows-issue-62007.rs

pnkfelix mentioned this pull request Jun 21, 2019
NLL iterating and updating &mut fails for non-local Place. #62007
Closed

pnkfelix added 5 commits June 21, 2019 12:43

rename file because I found the old filename too long and unwieldy.
e78361a

add comment outlining test.
681af62

Added test for deref projection.
241ceed

Add test that our handling of projections hasn't gone too far:
3b8ef01
overwriting one field should not allow reborrow of an unrelated field.

Add test checking our behavior for assigning over a ConstIndex proj…
f483269
…ection.

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 21, 2019

pnkfelix mentioned this pull request Jun 21, 2019
rustc_mir: Hide initial block state when defining transfer functions #61787
Merged

emilyalbini mentioned this pull request Jun 21, 2019
Rollup of 4 pull requests #62029
Closed

Centril mentioned this pull request Jun 21, 2019
Rollup of 10 pull requests #62035
Closed

Centril mentioned this pull request Jun 22, 2019
Prevent rollups in the web UI from including rollup=never PRs rust-lang/homu#49
Closed

bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 22, 2019

bors merged commit f483269 into rust-lang:masterJun 22, 2019

bors mentioned this pull request Jun 22, 2019
Place 2.0 change from enum to struct #60913
Merged

ecstatic-morse deleted the kill-borrows-of-proj branch October 6, 2020 01:42

Uh oh!

Uh oh!

Conversation

ecstatic-morse commented Jun 20, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ecstatic-morse commented Jun 20, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pnkfelix commented Jun 21, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pnkfelix commented Jun 21, 2019

Uh oh!

pnkfelix commented Jun 21, 2019

Uh oh!

pnkfelix commented Jun 21, 2019

Uh oh!

pnkfelix commented Jun 21, 2019

Uh oh!

bors commented Jun 21, 2019

Uh oh!

pnkfelix commented Jun 21, 2019

Uh oh!

pnkfelix commented Jun 21, 2019

Uh oh!

Centril commented Jun 21, 2019

Uh oh!

bors commented Jun 22, 2019

Uh oh!

kennytm commented Jun 22, 2019

Uh oh!

bors commented Jun 22, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ecstatic-morse commented Jun 20, 2019•
edited
Loading

ecstatic-morse commented Jun 20, 2019•
edited
Loading

pnkfelix commented Jun 21, 2019•
edited
Loading