Skip to content
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.

Sign up for GitHub

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

Jump to bottom

MIR borrowck doesn't accept the example of iterating and updating a mutable reference #56649

Merged
merged 3 commits into from
Dec 20, 2018
Merged

MIR borrowck doesn't accept the example of iterating and updating a mutable reference #56649

merged 3 commits into from
Dec 20, 2018

Conversation

davidtwco
Copy link
Member

@davidtwco
Add test for rust-lang#46589.
1149c58 
This commit adds the test for writing into a projection of a local to
confirm there are no remaining borrows.
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 9, 2018
@davidtwco

This comment has been minimized.

Sign in to view
@davidtwco

This comment has been minimized.

Sign in to view
@davidtwco davidtwco added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 11, 2018
@davidtwco davidtwco changed the title WIP: MIR borrowck doesn't accept the example of iterating and updating a mutable reference MIR borrowck doesn't accept the example of iterating and updating a mutable reference Dec 17, 2018
@davidtwco davidtwco added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 17, 2018
@davidtwco
Copy link
Member Author

This PR now takes a more reasonable approach, I expect it'll require some changes but it should be good to review now.

pnkfelix
pnkfelix reviewed Dec 17, 2018
View reviewed changes
src/librustc_mir/dataflow/impls/borrows.rs Outdated
// a projection (`foo.bar`).
self.kill_borrows_on_local(sets, local);
}
self.kill_borrows_on_place(sets, location, lhs);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a NOTE in a comment below this line, and I always wonder whether the directive in this comment has been (or is being) followed...

The local assigned_place has presumably been alpha-renamed, probably to lhs here)... but in any case, do we need to make modifications to propagate_call_return to make the logic here match up with the logic there? Are the comments still relevant in the first place?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, after thinking the matter through, I think the PR as it stands is sound; it is just more conservative than it could be, because I think we could apply the same place-overwritten borrow-killing logic to propagate_call_return, since that represents an overwrite of the dest_place that is passed to that method.

It would be best to first come up with a separate example illustrating this, rather than blindly adding the code to the PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Experimented with adding similar logic to the propagate_call_return function for a test case that might have been right. Not sure it is possible to get the MIR building to generate a call terminator that assigns into a destination like (_1.0) which would be required for this.

@davidtwco
Kill borrows from a projection after assignment.
db635fc 
This commit extends previous work to kill borrows from a local after
assignment into that local to kill borrows from a projection after
assignment into a prefix of that place.
@davidtwco
Add required lifetime parameter to BitDenotation.
7b628e1 
This avoids all sorts of confusing issues with using both `dest_place`
and `self` in the `propagate_call_return` function in the
`BitDenotation` implementation for `Borrows`.
@pnkfelix
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Dec 18, 2018

📌 Commit 7b628e1 has been approved by pnkfelix

@bors 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 Dec 18, 2018
@bors
Copy link
Contributor

bors commented Dec 20, 2018

⌛ Testing commit 7b628e1 with merge 817dda7...

bors added a commit that referenced this pull request Dec 20, 2018
@bors
Auto merge of #56649 - davidtwco:issue-46589, r=pnkfelix
817dda7 
MIR borrowck doesn't accept the example of iterating and updating a mutable reference

Fixes #46589.

r? @pnkfelix or @nikomatsakis
@bors
Copy link
Contributor

bors commented Dec 20, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: pnkfelix
Pushing 817dda7 to master...

@bors bors merged commit 7b628e1 into rust-lang:master Dec 20, 2018
@bors bors mentioned this pull request Dec 20, 2018
Closed
@davidtwco davidtwco deleted the issue-46589 branch December 20, 2018 10:00
@pnkfelix pnkfelix mentioned this pull request Jun 20, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pnkfelix pnkfelix pnkfelix left review comments

Assignees

@pnkfelix pnkfelix

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@davidtwco @pnkfelix @bors @rust-highfive