Skip to content

Extended error message for mut borrow conflicts in loops #43479

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

Merged
merged 2 commits into from
Jul 27, 2017

Conversation

ivanbakel
Copy link
Contributor

RFC issue: rust-lang/rfcs#2080

The error message for multiple mutable borrows on the same value over loop iterations now makes it clear that the conflict comes from the borrow outlasting the loop. The wording of the error is based on the special case of the moved-value error for a value moved in a loop. Following the example of that error, the code remains the same for the special case.

This is mainly because I felt the current message is confusing in the loop case : #43437. It's not clear that the two conflicting borrows are in different iterations of the loop, and instead it just looks like the compiler has an issue with a single line.

Error message now makes clear that mutable borrow conflicts on a single
value in a loop body is caused by the borrow outlasting a single pass of
the loop.
Loop conflicts are detected by seeing when two borrow locations are the
same - which indicates the same code being run more than once.
@estebank
Copy link
Contributor

@ivanbakel This is great! Could you add a ui test that shows the two possible outputs affected by this PR?

@ivanbakel
Copy link
Contributor Author

Sure. What are the two cases? All the current message tests still pass. Or should I write tests for the different kind of loops?

@estebank
Copy link
Contributor

There should be at least two test cases, one that expects "first mutable borrow occurs here"/"second mutable borrow occurs here" to be displayed, and another that expects "mutable borrow starts here in previous iteration of loop". If in the test you can add you also add one case for each possible loop type (loop/while/for), that'd be icing in the cake.

One set of tests is to ensure the current message is correct.
The other set is to check the messages are generated correctly for every
type of loop.
@ivanbakel
Copy link
Contributor Author

Alright, the tests are added. Do I have to do something else as well as just writing the files? They run on my machine using `./x.py test'.

@estebank
Copy link
Contributor

estebank commented Jul 27, 2017

Now we just wait for travis to finish so we can make sure there're no regressions in other platforms. When testing locally you can also do ./x.py test src/test/ui --stage 1 -j 4 to test only the ui tests.

@estebank
Copy link
Contributor

@bors r+

@bors
Copy link
Collaborator

bors commented Jul 27, 2017

📌 Commit 6888520 has been approved by estebank

@estebank
Copy link
Contributor

@bors rollup

@bors
Copy link
Collaborator

bors commented Jul 27, 2017

⌛ Testing commit 6888520 with merge da4d490...

bors added a commit that referenced this pull request Jul 27, 2017
Extended error message for mut borrow conflicts in loops

RFC issue: rust-lang/rfcs#2080

The error message for multiple mutable borrows on the same value over loop iterations now makes it clear that the conflict comes from the borrow outlasting the loop. The wording of the error is based on the special case of the moved-value error for a value moved in a loop. Following the example of that error, the code remains the same for the special case.

This is mainly because I felt the current message is confusing in the loop case : #43437. It's not clear that the two conflicting borrows are in different iterations of the loop, and instead it just looks like the compiler has an issue with a single line.
@bors
Copy link
Collaborator

bors commented Jul 27, 2017

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

@bors bors merged commit 6888520 into rust-lang:master Jul 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants