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: augmented assignment causes duplicate errors #47607

Merged
merged 7 commits into from Feb 7, 2018

Conversation

Projects
None yet
6 participants
@davidtwco
Copy link
Member

davidtwco commented Jan 20, 2018

Fixes #45697. This PR resolves the error duplication. I attempted to replace the existing sets since there were quite a few but only managed to replace two of them.

r? @nikomatsakis

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jan 22, 2018

@bors r+

nice!

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 22, 2018

📌 Commit 09d6682 has been approved by nikomatsakis

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Jan 25, 2018

Rollup merge of rust-lang#47607 - davidtwco:issue-45697, r=nikomatsakis
MIR-borrowck: augmented assignment causes duplicate errors

Fixes rust-lang#45697. This PR resolves the error duplication. I attempted to replace the existing sets since there were quite a few but only managed to replace two of them.

r? @nikomatsakis

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

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

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

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

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 25, 2018

@bors: r-

I think this may have caused this failure

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

@davidtwco

This comment has been minimized.

Copy link
Member

davidtwco commented Jan 25, 2018

It seems like the other of the two errors is being emitted now where it was being supressed before. I assume another PR in the rollup has changed something that causes the error order to change?

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 25, 2018

Hm yeah that may also be true! I'm not sure as to which though :(

@davidtwco davidtwco force-pushed the davidtwco:issue-45697 branch from 09d6682 to fd67863 Jan 25, 2018

@davidtwco

This comment has been minimized.

Copy link
Member

davidtwco commented Jan 25, 2018

Just pushed rebased changes, locally I'm not able to reproduce the issue yet and looking through the files that the rollup PRs have touched, nothing is jumping out at me as being the likely culprit so I'll keep digging.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 25, 2018

@bors: r=nikomatsakis

Ok let's keep this in the queue and see what happens, I may also be diagnosing in error!

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 25, 2018

📌 Commit fd67863 has been approved by nikomatsakis

@alexcrichton alexcrichton reopened this Jan 25, 2018

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 25, 2018

er sorry,

@bors: r=nikomatsakis

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 25, 2018

💡 This pull request was already approved, no need to approve it again.

  • There's another pull request that is currently being tested, blocking this pull request: #46450
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 25, 2018

📌 Commit fd67863 has been approved by nikomatsakis

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

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

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

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

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

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

Auto merge of #47607 - davidtwco:issue-45697, r=nikomatsakis
MIR-borrowck: augmented assignment causes duplicate errors

Fixes #45697. This PR resolves the error duplication. I attempted to replace the existing sets since there were quite a few but only managed to replace two of them.

r? @nikomatsakis
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 27, 2018

💔 Test failed - status-travis

29 | let z = copy_borrowed_ptr(&mut y);
| ------ borrow of `*y.pointer` occurs here
30 | *y.pointer += 1;
| ^^^^^^^^^^^^^^^ assignment to borrowed `*y.pointer` occurs here

This comment has been minimized.

@kennytm

kennytm Jan 27, 2018

Member

The borrow checker says it's a "use of y", not "assignment of y.pointer".

[00:55:48] failures:
[00:55:48] 
[00:55:48] ---- [ui] ui/issue-45697.rs stdout ----
[00:55:48] 	diff of stderr:
[00:55:48] 
[00:55:48] 6	30 |         *y.pointer += 1;
[00:55:48] 7	   |         ^^^^^^^^^^^^^^^ assignment to borrowed `*y.pointer` occurs here
[00:55:48] 8	
[00:55:48] +	error[E0503]: cannot use `*y.pointer` because it was mutably borrowed (Mir)
[00:55:48] -	error[E0506]: cannot assign to `*y.pointer` because it is borrowed (Mir)
[00:55:48] 10	  --> $DIR/issue-45697.rs:30:9
[00:55:48] 11	   |
[00:55:48] 12	29 |         let z = copy_borrowed_ptr(&mut y);
[00:55:48] 
[00:55:48] +	   |                                   ------ borrow of `y` occurs here
[00:55:48] -	   |                                   ------ borrow of `*y.pointer` occurs here
[00:55:48] 14	30 |         *y.pointer += 1;
[00:55:48] +	   |         ^^^^^^^^^^^^^^^ use of borrowed `y`
[00:55:48] -	   |         ^^^^^^^^^^^^^^^ assignment to borrowed `*y.pointer` occurs here
[00:55:48] 16	
[00:55:48] 17	error: aborting due to 2 previous errors
[00:55:48] 18	

This comment has been minimized.

@davidtwco

davidtwco Jan 27, 2018

Member

Hmm, seems like the issue we saw in the rollup again. I'll rebase it and take a look.

This comment has been minimized.

@davidtwco

davidtwco Jan 27, 2018

Member

Alright, so after a healthy amount of confusion and recompiling, I've made a little progress - it seems my changes simultaneously work and don't work:

/mnt/c/Users/David/Projects/personal/rust issue-45697* ↓↑
❯ ./build/x86_64-unknown-linux-gnu/stage1/bin/rustc src/test/ui/issue-45697.rs -Z borrowck=compare -Z emit-end-regions                    error[E0506]: cannot assign to `*y.pointer` because it is borrowed (Ast)
  --> src/test/ui/issue-45697.rs:30:9
   |
29 |         let z = copy_borrowed_ptr(&mut y);
   |                                        - borrow of `*y.pointer` occurs here
30 |         *y.pointer += 1;
   |         ^^^^^^^^^^^^^^^ assignment to borrowed `*y.pointer` occurs here

error[E0503]: cannot use `*y.pointer` because it was mutably borrowed (Mir)
  --> src/test/ui/issue-45697.rs:30:9
   |
29 |         let z = copy_borrowed_ptr(&mut y);
   |                                   ------ borrow of `y` occurs here
30 |         *y.pointer += 1;
   |         ^^^^^^^^^^^^^^^ use of borrowed `y`

error: aborting due to 2 previous errors


/mnt/c/Users/David/Projects/personal/rust issue-45697* ↓↑
❯ ./build/x86_64-unknown-linux-gnu/stage1/bin/rustc src/test/ui/issue-45697.rs -Z borrowck=compare -Z emit-end-regions -O
error[E0506]: cannot assign to `*y.pointer` because it is borrowed (Ast)
  --> src/test/ui/issue-45697.rs:30:9
   |
29 |         let z = copy_borrowed_ptr(&mut y);
   |                                        - borrow of `*y.pointer` occurs here
30 |         *y.pointer += 1;
   |         ^^^^^^^^^^^^^^^ assignment to borrowed `*y.pointer` occurs here

error[E0506]: cannot assign to `*y.pointer` because it is borrowed (Mir)
  --> src/test/ui/issue-45697.rs:30:9
   |
29 |         let z = copy_borrowed_ptr(&mut y);
   |                                   ------ borrow of `*y.pointer` occurs here
30 |         *y.pointer += 1;
   |         ^^^^^^^^^^^^^^^ assignment to borrowed `*y.pointer` occurs here

error: aborting due to 2 previous errors

It seems that when the -O flag is passed (as it is with compiletest), the write pass happens first in the MIR borrowck, but when it isn't passed, the read pass happens first. It's the write pass error we want, so that's a problem. Now, I've got to assume that when the tests are run for a merge, -O isn't passed since that's the only thing I've found that really makes a difference here (edit: in fact, we can see that it isn't in the failed travis output).

When I was working on this PR initially, I ran all my tests through x.py, and therefore thought my tests passed (and they did, technically) and Travis on the PR confirms that. Not sure what the solution is here, but I'm going to keep digging.

I guess one solution would be to add -O to the compile-flags of the test? Doesn't seem ideal though.

This comment has been minimized.

@davidtwco

davidtwco Jan 27, 2018

Member

Tried to dig around and find out what's causing this, but struggling to make any decent progress, would appreciate any pointers as to where I should be looking - @nikomatsakis.

In the paragraphs below, I'm defining passing run as when -O is passed as an argument and the intended error is output. Failing runs are everything else.

So, on the passing runs, the first error is from a Shallow/Write(Mutate) call to access_place from mutate_place which was called by visit_statement_entry. On the failing runs, we see a Deep/Read(Copy) call from the same places.

For the second error, on passing runs, we see a Deep/Read(Copy) run from access_place <- consume_operator <- visit_terminator_entry. On failing runs, we see a Shallow/Write(Mutate) call from access_place <- mutate_place <- visit_statement_entry.

Logs from the passing run and logs from the failing run. These logs include some logging statements I've added to help me trace the call stack back.

As best I can tell, the issue isn't in the borrow_check module but in the statements/terminator that is provided to it from the dataflow or transform module differing when -O is passed and otherwise.

davidtwco added some commits Jan 15, 2018

@shepmaster

This comment has been minimized.

Copy link
Member

shepmaster commented Feb 3, 2018

Ping from triage, @davidtwco! Will you have time to address the strange errors?

@davidtwco

This comment has been minimized.

Copy link
Member

davidtwco commented Feb 3, 2018

@shepmaster Yes, just been looking at other in-progress PRs. Will turn focus back on this one.

@davidtwco davidtwco force-pushed the davidtwco:issue-45697 branch from fd67863 to 970fb1a Feb 5, 2018

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Feb 5, 2018

OK, @davidtwco and I chatted a bit about this over gitter. First off, the problem arises because -- without -O -- we generate code like

tmp = CheckedAdd(*y.pointer, 1);
...
*y.pointer = tmp

but with -O we elide the bounds check and you get

*y.pointer = Add(*y.pointer, 1)

This, combined with the fact that we visit the LHS of an assignment first means that -- with -O only -- we see the write first:

self.mutate_place(
ContextKind::AssignLhs.new(location),
(lhs, span),
Shallow(None),
JustWrite,
flow_state,
);

We could do a bunch of work to fix this, but I personally am unconvinced that showing an error about the write is better than showing an error about the read (after all, both are illegal). I'm wondering if we should just visit the RHS of an assignment so that the order is consistent.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Feb 6, 2018

@davidtwco remind me, are we already testing (on travis) both with and without -O? I feel like it'd be nice to have a test to make sure we don't overlook the overflow checks in future.

@davidtwco

This comment has been minimized.

Copy link
Member

davidtwco commented Feb 6, 2018

@nikomatsakis when a PR is tested it uses -O, when bors runs tests before a merge, it does not.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Feb 6, 2018

@davidtwco that doesn't feel very intentional. It's not really about -O anyway, it's about overflow checks. How about we make two versions of the test (or two revisions), one using -C overflow-checks=on and one using -C overflow-checks=off? (You can pass flags with // compile-flags)

@davidtwco

This comment has been minimized.

Copy link
Member

davidtwco commented Feb 6, 2018

@nikomatsakis I've pushed a commit with a new ui test with overflow checks on, and modified the existing tests to have overflow checks on.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Feb 6, 2018

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 6, 2018

📌 Commit bb6e54d has been approved by nikomatsakis

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 7, 2018

⌛️ Testing commit bb6e54d with merge 4f93357...

bors added a commit that referenced this pull request Feb 7, 2018

Auto merge of #47607 - davidtwco:issue-45697, r=nikomatsakis
MIR-borrowck: augmented assignment causes duplicate errors

Fixes #45697. This PR resolves the error duplication. I attempted to replace the existing sets since there were quite a few but only managed to replace two of them.

r? @nikomatsakis
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 7, 2018

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

@bors bors merged commit bb6e54d into rust-lang:master Feb 7, 2018

2 checks passed

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

@davidtwco davidtwco deleted the davidtwco:issue-45697 branch Feb 25, 2018

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