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

MIR-borrowck: emit "`foo` does not live long enough" instead of borrow errors #45989

Merged
merged 5 commits into from Nov 18, 2017

Conversation

Projects
None yet
4 participants
@davidtwco
Member

davidtwco commented Nov 14, 2017

Fixes #45360. As of writing, contains deduplication of existing errors.

r? @nikomatsakis

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Nov 14, 2017

Contributor

@davidtwco Awesome!

I didn't get a chance to look closely, but I wanted to point you at this gist:

https://gist.github.com/nikomatsakis/a0d5dee0bd3c4d4f0442914a9abdf74c

It describes how to update the existing AST borrow check tests so that we can simultaneously test: (a) the current behavior on stable and (b) the behavior with MIR-borrowck enabled and compare the two. It'd be helpful if you update a test or two to show how the current PR behaves.

Contributor

nikomatsakis commented Nov 14, 2017

@davidtwco Awesome!

I didn't get a chance to look closely, but I wanted to point you at this gist:

https://gist.github.com/nikomatsakis/a0d5dee0bd3c4d4f0442914a9abdf74c

It describes how to update the existing AST borrow check tests so that we can simultaneously test: (a) the current behavior on stable and (b) the behavior with MIR-borrowck enabled and compare the two. It'd be helpful if you update a test or two to show how the current PR behaves.

@davidtwco

This comment has been minimized.

Show comment
Hide comment
@davidtwco

davidtwco Nov 14, 2017

Member

@nikomatsakis That's great. I tested it locally running the same example from the issue - I'll update some tests tomorrow based on that gist.

Member

davidtwco commented Nov 14, 2017

@nikomatsakis That's great. I tested it locally running the same example from the issue - I'll update some tests tomorrow based on that gist.

@davidtwco

This comment has been minimized.

Show comment
Hide comment
@davidtwco

davidtwco Nov 15, 2017

Member

@nikomatsakis I updated the test given as an example in the issue to reflect the expected result once this PR is fully implemented. For now it demonstrates the deduplication. This is the updated test.

I'd be happy to update any other tests that this PR would fix.

Member

davidtwco commented Nov 15, 2017

@nikomatsakis I updated the test given as an example in the issue to reflect the expected result once this PR is fully implemented. For now it demonstrates the deduplication. This is the updated test.

I'd be happy to update any other tests that this PR would fix.

Show outdated Hide outdated src/test/compile-fail/issue-36082.rs Outdated
Show outdated Hide outdated src/test/compile-fail/issue-36082.rs Outdated
Show outdated Hide outdated src/librustc_mir/borrow_check.rs Outdated
@nikomatsakis

Looks great! Left a few nits.

Show outdated Hide outdated src/librustc_mir/borrow_check.rs Outdated
Show outdated Hide outdated src/librustc_mir/borrow_check.rs Outdated
@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Nov 16, 2017

Contributor

So, in terms of how to change the error message. The current error message arises from this call here:

https://github.com/davidtwco/rust/blob/d906a6e94dfaac3a1344db96237d59bb9285bf5b/src/librustc_mir/borrow_check.rs#L637-L638

Note that we pass Write(WriteKind::Move). That seems bogus. Per @arielb1's suggestion, we should rename WriteKind::StorageDead to WriteKind::StorageDeadOrDrop and pass that here, I think. (Note that we sometimes want Move, we'll have to look at the consume_via_drop argument to decide.)

That will then flow down to this case:

https://github.com/davidtwco/rust/blob/d906a6e94dfaac3a1344db96237d59bb9285bf5b/src/librustc_mir/borrow_check.rs#L492-L514

You can see that WriteKind::StorageDead (which we will have renamed) is treated the same as Mutate. We want to give it its own case. Then we want to invoke this helper function:

https://github.com/davidtwco/rust/blob/d906a6e94dfaac3a1344db96237d59bb9285bf5b/src/librustc_mir/util/borrowck_errors.rs#L375-L384

The other cases invoke similar helpers, but they use shallow wrappers like report_move_out_while_borrowed so we probably want to write one of those too.

Contributor

nikomatsakis commented Nov 16, 2017

So, in terms of how to change the error message. The current error message arises from this call here:

https://github.com/davidtwco/rust/blob/d906a6e94dfaac3a1344db96237d59bb9285bf5b/src/librustc_mir/borrow_check.rs#L637-L638

Note that we pass Write(WriteKind::Move). That seems bogus. Per @arielb1's suggestion, we should rename WriteKind::StorageDead to WriteKind::StorageDeadOrDrop and pass that here, I think. (Note that we sometimes want Move, we'll have to look at the consume_via_drop argument to decide.)

That will then flow down to this case:

https://github.com/davidtwco/rust/blob/d906a6e94dfaac3a1344db96237d59bb9285bf5b/src/librustc_mir/borrow_check.rs#L492-L514

You can see that WriteKind::StorageDead (which we will have renamed) is treated the same as Mutate. We want to give it its own case. Then we want to invoke this helper function:

https://github.com/davidtwco/rust/blob/d906a6e94dfaac3a1344db96237d59bb9285bf5b/src/librustc_mir/util/borrowck_errors.rs#L375-L384

The other cases invoke similar helpers, but they use shallow wrappers like report_move_out_while_borrowed so we probably want to write one of those too.

@davidtwco

This comment has been minimized.

Show comment
Hide comment
@davidtwco

davidtwco Nov 16, 2017

Member

Rebased and producing the correct error message now.

Member

davidtwco commented Nov 16, 2017

Rebased and producing the correct error message now.

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Nov 16, 2017

Contributor

☔️ The latest upstream changes (presumably #45825) made this pull request unmergeable. Please resolve the merge conflicts.

Contributor

bors commented Nov 16, 2017

☔️ The latest upstream changes (presumably #45825) made this pull request unmergeable. Please resolve the merge conflicts.

davidtwco added some commits Nov 14, 2017

@davidtwco

This comment has been minimized.

Show comment
Hide comment
@davidtwco

davidtwco Nov 17, 2017

Member

Rebased on top of the conflicting changes and to squash my own commits. Error message now looks more or less what we're seeing with the Ast version, only difference is the symbols used and I can't see a way around that. Other than that, I think this is done.

@bors r=nikomatsakis

Edit: I have no idea how bors works apparently.

Member

davidtwco commented Nov 17, 2017

Rebased on top of the conflicting changes and to squash my own commits. Error message now looks more or less what we're seeing with the Ast version, only difference is the symbols used and I can't see a way around that. Other than that, I think this is done.

@bors r=nikomatsakis

Edit: I have no idea how bors works apparently.

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Nov 17, 2017

Contributor

@davidtwco: 🔑 Insufficient privileges: Not in reviewers

Contributor

bors commented Nov 17, 2017

@davidtwco: 🔑 Insufficient privileges: Not in reviewers

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Nov 17, 2017

Contributor

@davidtwco: 🔑 Insufficient privileges: Not in reviewers

Contributor

bors commented Nov 17, 2017

@davidtwco: 🔑 Insufficient privileges: Not in reviewers

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis
Contributor

nikomatsakis commented Nov 17, 2017

@bors r+

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Nov 17, 2017

Contributor

📋 Looks like this PR is still in progress, ignoring approval

Contributor

bors commented Nov 17, 2017

📋 Looks like this PR is still in progress, ignoring approval

@davidtwco davidtwco changed the title from WIP: MIR-borrowck: emit "`foo` does not live long enough" instead of borrow errors to MIR-borrowck: emit "`foo` does not live long enough" instead of borrow errors Nov 17, 2017

@davidtwco

This comment has been minimized.

Show comment
Hide comment
@davidtwco

davidtwco Nov 17, 2017

Member

Pushed a commit that fixes the error on the Travis build in an unrelated test. Should be good now, apologies.

Member

davidtwco commented Nov 17, 2017

Pushed a commit that fixes the error on the Travis build in an unrelated test. Should be good now, apologies.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis
Contributor

nikomatsakis commented Nov 17, 2017

@bors r+

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Nov 17, 2017

Contributor

📌 Commit a1d55be has been approved by nikomatsakis

Contributor

bors commented Nov 17, 2017

📌 Commit a1d55be has been approved by nikomatsakis

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Nov 18, 2017

Contributor

⌛️ Testing commit a1d55be with merge 79a1385...

Contributor

bors commented Nov 18, 2017

⌛️ Testing commit a1d55be with merge 79a1385...

bors added a commit that referenced this pull request Nov 18, 2017

Auto merge of #45989 - davidtwco:issue-45360, r=nikomatsakis
MIR-borrowck: emit "`foo` does not live long enough" instead of borrow errors

Fixes #45360. As of writing, contains deduplication of existing errors.

r? @nikomatsakis
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Nov 18, 2017

Contributor

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

Contributor

bors commented Nov 18, 2017

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

@bors bors merged commit a1d55be into rust-lang:master Nov 18, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@davidtwco davidtwco deleted the davidtwco:issue-45360 branch Dec 1, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment