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: Unions not reinitialized after assignment into field #55657

Merged
merged 3 commits into from Nov 11, 2018

Conversation

Projects
None yet
4 participants
@davidtwco
Member

davidtwco commented Nov 3, 2018

Fixes #55651, #55652.

This PR makes two changes:

First, it updates the dataflow builder to add an init for the place
containing a union if there is an assignment into the field of
that union.

Second, it stops a "use of uninitialized" error occuring when there is an
assignment into the field of an uninitialized union that was previously
initialized. Making this assignment would re-initialize the union, as
tested in src/test/ui/borrowck/borrowck-union-move-assign.nll.stderr.
The check for previous initialization ensures that we do not start
supporting partial initialization yet (cc #21232, #54499, #54986).

This PR also fixes #55652 which was marked as requiring investigation
as the changes in this PR add an error that was previously missing
(and mentioned in the review comments) and confirms that the error
that was present is correct and a result of earlier partial
initialization changes in NLL.

r? @pnkfelix (due to earlier work with partial initialization)
cc @nikomatsakis

Unions reinitialized after assignment into field.
This commit makes two changes:

First, it updates the dataflow builder to add an init for the place
containing a union if there is an assignment into the field of
that union.

Second, it stops a "use of uninitialized" error occuring when there is an
assignment into the field of an uninitialized union that was previously
initialized. Making this assignment would re-initialize the union, as
tested in `src/test/ui/borrowck/borrowck-union-move-assign.nll.stderr`.
The check for previous initialization ensures that we do not start
supporting partial initialization yet (cc #21232, #54499, #54986).
@pnkfelix

This comment was marked as resolved.

Member

pnkfelix commented Nov 5, 2018

I think it would be a good idea to make a run-pass test adapted from the two parts of the test that do not error, checking that the state of the union is set up in the expected manner.

Show resolved Hide resolved src/test/ui/nll/issue-55651.rs Outdated
Add run-pass test for reinitialized unions.
This commit adds a run-pass test for the subset of
`src/test/ui/borrowck/borrowck-union-move-assign.rs` that is intended to
pass as the union is reinitialized.

@davidtwco davidtwco force-pushed the davidtwco:issue-55651 branch from 34a3e48 to cd4869f Nov 5, 2018

Improve predecessor detection.
It is necessary to detect whether we are making the first
assignment into a union. This is checked by looking at the moves and
checking if there are any from locations earlier in the control flow
graph.

This commit improves the detection of this by switching from a naive
method that compared only the statement and basic block indices with
a more robust method that looks at the predecessors of a location.

@davidtwco davidtwco force-pushed the davidtwco:issue-55651 branch from cd4869f to 1672c27 Nov 5, 2018

@pnkfelix

This comment has been minimized.

Member

pnkfelix commented Nov 6, 2018

@bors r+

@bors

This comment has been minimized.

Contributor

bors commented Nov 6, 2018

📌 Commit 1672c27 has been approved by pnkfelix

@pnkfelix

This comment has been minimized.

Member

pnkfelix commented Nov 6, 2018

I accidentally beta-nominated this.

But for the record, It does not need to be backported, for two reasons:

  1. NLL migration mode means that NLL-complete things (apart from ICEs) should not cause problems for the 2018 edition
  2. It is a bug with how we were dealing with unions. But untagged_unions are not stabilized, so bugs with our treatment of them should not affect stable code.
@bors

This comment has been minimized.

Contributor

bors commented Nov 11, 2018

⌛️ Testing commit 1672c27 with merge 35ae96d...

bors added a commit that referenced this pull request Nov 11, 2018

Auto merge of #55657 - davidtwco:issue-55651, r=pnkfelix
NLL Diagnostic Review 3: Unions not reinitialized after assignment into field

Fixes #55651, #55652.

This PR makes two changes:

First, it updates the dataflow builder to add an init for the place
containing a union if there is an assignment into the field of
that union.

Second, it stops a "use of uninitialized" error occuring when there is an
assignment into the field of an uninitialized union that was previously
initialized. Making this assignment would re-initialize the union, as
tested in `src/test/ui/borrowck/borrowck-union-move-assign.nll.stderr`.
The check for previous initialization ensures that we do not start
supporting partial initialization yet (cc #21232, #54499, #54986).

This PR also fixes #55652 which was marked as requiring investigation
as the changes in this PR add an error that was previously missing
(and mentioned in the review comments) and confirms that the error
that was present is correct and a result of earlier partial
initialization changes in NLL.

r? @pnkfelix (due to earlier work with partial initialization)
cc @nikomatsakis
@bors

This comment has been minimized.

Contributor

bors commented Nov 11, 2018

💔 Test failed - status-appveyor

@davidtwco

This comment has been minimized.

Member

davidtwco commented Nov 11, 2018

@bors retry

@bors

This comment has been minimized.

Contributor

bors commented Nov 11, 2018

⌛️ Testing commit 1672c27 with merge 5a2ca1a...

bors added a commit that referenced this pull request Nov 11, 2018

Auto merge of #55657 - davidtwco:issue-55651, r=pnkfelix
NLL Diagnostic Review 3: Unions not reinitialized after assignment into field

Fixes #55651, #55652.

This PR makes two changes:

First, it updates the dataflow builder to add an init for the place
containing a union if there is an assignment into the field of
that union.

Second, it stops a "use of uninitialized" error occuring when there is an
assignment into the field of an uninitialized union that was previously
initialized. Making this assignment would re-initialize the union, as
tested in `src/test/ui/borrowck/borrowck-union-move-assign.nll.stderr`.
The check for previous initialization ensures that we do not start
supporting partial initialization yet (cc #21232, #54499, #54986).

This PR also fixes #55652 which was marked as requiring investigation
as the changes in this PR add an error that was previously missing
(and mentioned in the review comments) and confirms that the error
that was present is correct and a result of earlier partial
initialization changes in NLL.

r? @pnkfelix (due to earlier work with partial initialization)
cc @nikomatsakis
@bors

This comment has been minimized.

Contributor

bors commented Nov 11, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: pnkfelix
Pushing 5a2ca1a to master...

@bors bors merged commit 1672c27 into rust-lang:master Nov 11, 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-55651 branch Nov 11, 2018

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