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.

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

NLL Diagnostic Review 3: Missing errors for borrows of union fields #55675

Closed
davidtwco opened this issue Nov 4, 2018 · 3 comments · Fixed by #55696
Closed

NLL Diagnostic Review 3: Missing errors for borrows of union fields #55675

davidtwco opened this issue Nov 4, 2018 · 3 comments · Fixed by #55696
Assignees
Labels
A-NLL Area: Non Lexical Lifetimes (NLL) NLL-diagnostics Working torwads the "diagnostic parity" goal NLL-sound Working towards the "invalid code does not compile" goal

Comments

@davidtwco
Copy link
Member

@pnkfelix's review comments:

ui/union/union-borrow-move-parent-sibling.nll.stderr has 3 errors when AST-borrowck reports 6 errors. Double-check.

@davidtwco davidtwco created this issue from a note in NLL Diagnostic Review (NLL incorrectly handling unions (not backport worthy; P-high)) Nov 4, 2018
@davidtwco davidtwco self-assigned this Nov 4, 2018
@davidtwco davidtwco added A-NLL Area: Non Lexical Lifetimes (NLL) NLL-diagnostics Working torwads the "diagnostic parity" goal NLL-sound Working towards the "invalid code does not compile" goal labels Nov 4, 2018
@davidtwco
Copy link
Member Author

I've looked into this some - NLL misses three errors, all of the form cannot borrow `u.y` as immutable because `u.x` is also borrowed as mutable.

In each of the three cases, there is a mutable borrow of some field of the union and then a shared borrow of some other field immediately following. The issue seems to be that the mutable borrow is killed straight away as it isn't used later - therefore not causing a conflict with the shared borrow.

I'm inclined to think that this is fine - and that NLL is correct in having less errors here. However, we might want to update the test to use the first mutable borrow for each case in order to make the error happen and demonstrate the diagnostic - if this is the case, I've got a branch here that we can open a PR for that will update the test.

cc @nikomatsakis @pnkfelix

@pnkfelix
Copy link
Member

pnkfelix commented Nov 5, 2018

I'm inclined to agree that this is an instance where the test was not robust and that we need to add uses of those first mutable borrows.

@davidtwco
Copy link
Member Author

I've submitted a PR (#55696) that makes the test more robust.

kennytm added a commit to kennytm/rust that referenced this issue Nov 6, 2018
NLL Diagnostic Review 3: Missing errors for borrows of union fields

Fixes rust-lang#55675.

This PR modifies a test to make it more robust (it also fixes indentation on a doc comment, but that's not the point of the PR). See the linked issue for details.

r? @pnkfelix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-NLL Area: Non Lexical Lifetimes (NLL) NLL-diagnostics Working torwads the "diagnostic parity" goal NLL-sound Working towards the "invalid code does not compile" goal
Projects
No open projects
NLL Diagnostic Review
NLL incorrectly handling unions (not ...
Development

Successfully merging a pull request may close this issue.

2 participants