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: error message confuses locals and temporaries #46598

Merged
merged 7 commits into from Dec 12, 2017

Conversation

Projects
None yet
4 participants
@davidtwco
Member

davidtwco commented Dec 9, 2017

Fixes #46471 and fixes #46472 (see this Gitter comment).

r? @arielb1

@davidtwco

This comment has been minimized.

Show comment
Hide comment
@davidtwco

davidtwco Dec 9, 2017

Member

This isn't quite there yet, the error output is approximately correct, but the spans are a character to the left of what they should be to match exactly. I think this was a issue with tabs locally, not seeing it anymore so consider this resolved.

I'm also not sure that the condition I'm checking is what I should be using for this, but it works.

static lifetime is also hardcoded at the moment, I've seen the note_and_explain_region function and tried to use that but I struggled to find a ScopeTree to pass to it (flow_state.borrows.base_results.operator().scope_tree was closest I could find but was private).

I think it might also be worth splitting the report_borrowed_value_does_not_live_long_enough function into two smaller functions and moving the condition to the callers, it seems to be doing two different things. Interested to hear thoughts on this.

Member

davidtwco commented Dec 9, 2017

This isn't quite there yet, the error output is approximately correct, but the spans are a character to the left of what they should be to match exactly. I think this was a issue with tabs locally, not seeing it anymore so consider this resolved.

I'm also not sure that the condition I'm checking is what I should be using for this, but it works.

static lifetime is also hardcoded at the moment, I've seen the note_and_explain_region function and tried to use that but I struggled to find a ScopeTree to pass to it (flow_state.borrows.base_results.operator().scope_tree was closest I could find but was private).

I think it might also be worth splitting the report_borrowed_value_does_not_live_long_enough function into two smaller functions and moving the condition to the callers, it seems to be doing two different things. Interested to hear thoughts on this.

@davidtwco davidtwco changed the title from WIP: MIR borrowck: error message confuses locals and temporaries to MIR borrowck: error message confuses locals and temporaries Dec 10, 2017

@davidtwco

This comment has been minimized.

Show comment
Hide comment
@davidtwco

davidtwco Dec 10, 2017

Member

I've fixed issues referenced in my previous comment and also fixed #46472 as mentioned in this Gitter comment.

I think the function might be getting a bit a unwieldly since it's handling a bunch of cases. I've adjusted tests to allow the handful of places where spans are off by a character.

Member

davidtwco commented Dec 10, 2017

I've fixed issues referenced in my previous comment and also fixed #46472 as mentioned in this Gitter comment.

I think the function might be getting a bit a unwieldly since it's handling a bunch of cases. I've adjusted tests to allow the handful of places where spans are off by a character.

@arielb1

This comment has been minimized.

Show comment
Hide comment
@arielb1

arielb1 Dec 10, 2017

Contributor

Nice!

@bors r+ p=1

This should go before #46558, which changes the same code.

Contributor

arielb1 commented Dec 10, 2017

Nice!

@bors r+ p=1

This should go before #46558, which changes the same code.

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Dec 10, 2017

Contributor

📌 Commit e1723d2 has been approved by arielb1

Contributor

bors commented Dec 10, 2017

📌 Commit e1723d2 has been approved by arielb1

@arielb1

This comment has been minimized.

Show comment
Hide comment
@arielb1

arielb1 Dec 10, 2017

Contributor

@bors p=0

This hasn't passed travis yet

Contributor

arielb1 commented Dec 10, 2017

@bors p=0

This hasn't passed travis yet

@kennytm

This comment has been minimized.

Show comment
Hide comment
@kennytm

kennytm Dec 10, 2017

Member

@bors r-

Travis failed.

[00:57:14] failures:
[00:57:14]     [ui] ui/nll/capture-ref-in-struct.rs
[00:57:14]     [ui] ui/nll/closure-requirements/escape-argument.rs
[00:57:14]     [ui] ui/nll/closure-requirements/escape-upvar-nested.rs
[00:57:14]     [ui] ui/nll/closure-requirements/escape-upvar-ref.rs
Member

kennytm commented Dec 10, 2017

@bors r-

Travis failed.

[00:57:14] failures:
[00:57:14]     [ui] ui/nll/capture-ref-in-struct.rs
[00:57:14]     [ui] ui/nll/closure-requirements/escape-argument.rs
[00:57:14]     [ui] ui/nll/closure-requirements/escape-upvar-nested.rs
[00:57:14]     [ui] ui/nll/closure-requirements/escape-upvar-ref.rs
@davidtwco

This comment has been minimized.

Show comment
Hide comment
@davidtwco

davidtwco Dec 10, 2017

Member

I've fixed the previous issue where a local might need to live until an end_span. I've not looked at the other tests which are failing in the Travis build - do these just need updated to have the improved error messages or is this an issue with this PR @arielb1?

Member

davidtwco commented Dec 10, 2017

I've fixed the previous issue where a local might need to live until an end_span. I've not looked at the other tests which are failing in the Travis build - do these just need updated to have the improved error messages or is this an issue with this PR @arielb1?

@davidtwco

This comment has been minimized.

Show comment
Hide comment
@davidtwco

davidtwco Dec 11, 2017

Member

I've updated the existing tests to use the new error messages. This should now pass Travis.

Member

davidtwco commented Dec 11, 2017

I've updated the existing tests to use the new error messages. This should now pass Travis.

@kennytm

This comment has been minimized.

Show comment
Hide comment
@kennytm

kennytm Dec 11, 2017

Member
Travis is still failing.
[00:59:30] ---- [compile-fail] compile-fail/borrowck/borrowck-local-borrow-outlives-fn.rs stdout ----
[00:59:30] 	
[00:59:30] error in revision `mir`: /checkout/src/test/compile-fail/borrowck/borrowck-local-borrow-outlives-fn.rs:15: unexpected error: '15:5: 15:7: `x` does not live long enough [E0597]'
[00:59:30] 
[00:59:30] error in revision `mir`: /checkout/src/test/compile-fail/borrowck/borrowck-local-borrow-outlives-fn.rs:16: expected error not found: borrowed value does not live long enough
[00:59:30] 
[00:59:30] error in revision `mir`: 1 unexpected errors found, 1 expected errors not found
...
[00:59:30] ---- [compile-fail] compile-fail/borrowck/borrowck-local-borrow-with-panic-outlives-fn.rs stdout ----
[00:59:30] 	
[00:59:30] error in revision `mir`: /checkout/src/test/compile-fail/borrowck/borrowck-local-borrow-with-panic-outlives-fn.rs:16: unexpected error: '16:15: 16:23: `z.1` does not live long enough [E0597]'
[00:59:30] 
[00:59:30] error in revision `mir`: /checkout/src/test/compile-fail/borrowck/borrowck-local-borrow-with-panic-outlives-fn.rs:18: expected error not found: [E0597]
[00:59:30] 
[00:59:30] error in revision `mir`: 1 unexpected errors found, 1 expected errors not found
...
[00:59:30] ---- [compile-fail] compile-fail/region-borrow-params-issue-29793-big.rs stdout ----
[00:59:30] 	
[00:59:30] error in revision `mir`: /checkout/src/test/compile-fail/region-borrow-params-issue-29793-big.rs:84: unexpected error: '84:5: 84:6: `x` does not live long enough [E0597]'
[00:59:30] 
[00:59:30] error in revision `mir`: /checkout/src/test/compile-fail/region-borrow-params-issue-29793-big.rs:84: unexpected error: '84:5: 84:6: `y` does not live long enough [E0597]'
[00:59:30] 
[00:59:30] error in revision `mir`: /checkout/src/test/compile-fail/region-borrow-params-issue-29793-big.rs:84: expected error not found: borrowed value does not live long enough
[00:59:30] 
[00:59:30] error in revision `mir`: /checkout/src/test/compile-fail/region-borrow-params-issue-29793-big.rs:84: expected error not found: borrowed value does not live long enough
[00:59:30] 
[00:59:30] error in revision `mir`: 2 unexpected errors found, 2 expected errors not found

If nothing goes wrong, #46558 will be merged soon, so you'll likely need to rebase.

Member

kennytm commented Dec 11, 2017

Travis is still failing.
[00:59:30] ---- [compile-fail] compile-fail/borrowck/borrowck-local-borrow-outlives-fn.rs stdout ----
[00:59:30] 	
[00:59:30] error in revision `mir`: /checkout/src/test/compile-fail/borrowck/borrowck-local-borrow-outlives-fn.rs:15: unexpected error: '15:5: 15:7: `x` does not live long enough [E0597]'
[00:59:30] 
[00:59:30] error in revision `mir`: /checkout/src/test/compile-fail/borrowck/borrowck-local-borrow-outlives-fn.rs:16: expected error not found: borrowed value does not live long enough
[00:59:30] 
[00:59:30] error in revision `mir`: 1 unexpected errors found, 1 expected errors not found
...
[00:59:30] ---- [compile-fail] compile-fail/borrowck/borrowck-local-borrow-with-panic-outlives-fn.rs stdout ----
[00:59:30] 	
[00:59:30] error in revision `mir`: /checkout/src/test/compile-fail/borrowck/borrowck-local-borrow-with-panic-outlives-fn.rs:16: unexpected error: '16:15: 16:23: `z.1` does not live long enough [E0597]'
[00:59:30] 
[00:59:30] error in revision `mir`: /checkout/src/test/compile-fail/borrowck/borrowck-local-borrow-with-panic-outlives-fn.rs:18: expected error not found: [E0597]
[00:59:30] 
[00:59:30] error in revision `mir`: 1 unexpected errors found, 1 expected errors not found
...
[00:59:30] ---- [compile-fail] compile-fail/region-borrow-params-issue-29793-big.rs stdout ----
[00:59:30] 	
[00:59:30] error in revision `mir`: /checkout/src/test/compile-fail/region-borrow-params-issue-29793-big.rs:84: unexpected error: '84:5: 84:6: `x` does not live long enough [E0597]'
[00:59:30] 
[00:59:30] error in revision `mir`: /checkout/src/test/compile-fail/region-borrow-params-issue-29793-big.rs:84: unexpected error: '84:5: 84:6: `y` does not live long enough [E0597]'
[00:59:30] 
[00:59:30] error in revision `mir`: /checkout/src/test/compile-fail/region-borrow-params-issue-29793-big.rs:84: expected error not found: borrowed value does not live long enough
[00:59:30] 
[00:59:30] error in revision `mir`: /checkout/src/test/compile-fail/region-borrow-params-issue-29793-big.rs:84: expected error not found: borrowed value does not live long enough
[00:59:30] 
[00:59:30] error in revision `mir`: 2 unexpected errors found, 2 expected errors not found

If nothing goes wrong, #46558 will be merged soon, so you'll likely need to rebase.

@arielb1

This comment has been minimized.

Show comment
Hide comment
@arielb1

arielb1 Dec 11, 2017

Contributor

@kennytm

We're working on it.

Contributor

arielb1 commented Dec 11, 2017

@kennytm

We're working on it.

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Dec 11, 2017

Contributor

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

Contributor

bors commented Dec 11, 2017

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

davidtwco added some commits Dec 8, 2017

@davidtwco

This comment has been minimized.

Show comment
Hide comment
@davidtwco

davidtwco Dec 11, 2017

Member

All previous issues should be resolved and it's now rebased on top of the current master (or as of an hour ago).

Member

davidtwco commented Dec 11, 2017

All previous issues should be resolved and it's now rebased on top of the current master (or as of an hour ago).

davidtwco added some commits Dec 10, 2017

@davidtwco

This comment has been minimized.

Show comment
Hide comment
@davidtwco

davidtwco Dec 11, 2017

Member

Alright, fifth time lucky. I've fixed the indentation and other requested changes from Gitter and updated the failing tests from the recent Travis run.

Member

davidtwco commented Dec 11, 2017

Alright, fifth time lucky. I've fixed the indentation and other requested changes from Gitter and updated the failing tests from the recent Travis run.

@arielb1

This comment has been minimized.

Show comment
Hide comment
@arielb1

arielb1 Dec 12, 2017

Contributor

That's cool!

@bors r+

Contributor

arielb1 commented Dec 12, 2017

That's cool!

@bors r+

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Dec 12, 2017

Contributor

📌 Commit 3dbc11b has been approved by arielb1

Contributor

bors commented Dec 12, 2017

📌 Commit 3dbc11b has been approved by arielb1

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Dec 12, 2017

Contributor

⌛️ Testing commit 3dbc11b with merge 7245006...

Contributor

bors commented Dec 12, 2017

⌛️ Testing commit 3dbc11b with merge 7245006...

bors added a commit that referenced this pull request Dec 12, 2017

Auto merge of #46598 - davidtwco:issue-46471, r=arielb1
MIR borrowck: error message confuses locals and temporaries

Fixes #46471 and fixes #46472 (see [this Gitter comment](https://gitter.im/rust-impl-period/WG-compiler-nll?at=5a2d5cb53ae2aa6b3facf0c2)).

r? @arielb1
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Dec 12, 2017

Contributor

☀️ Test successful - status-appveyor, status-travis
Approved by: arielb1
Pushing 7245006 to master...

Contributor

bors commented Dec 12, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: arielb1
Pushing 7245006 to master...

@bors bors merged commit 3dbc11b into rust-lang:master Dec 12, 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-46471 branch Dec 20, 2017

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