Skip to content

Compute the correct layout for variants of uninhabited enums #69768

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 3 commits into from
Mar 20, 2020

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Mar 6, 2020

r? @eddyb
cc @RalfJung

fixes #69191
cc #69763

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 6, 2020
@RalfJung
Copy link
Member

RalfJung commented Mar 6, 2020

fixes #69763

That issue also says to check other code for unnecessary is_uninhabited checks, so maybe we shouldn't close it yet?
@eddyb seems to disagree that we should get rid of these checks in codegen, but I want to at least audit Miri for this.

@eddyb
Copy link
Member

eddyb commented Mar 6, 2020

Oh yeah I missed the bit about #69763, it shouldn't be closed until the plan at the end of #69763 (comment), or something similarly thorough, is implemented.

@eddyb
Copy link
Member

eddyb commented Mar 6, 2020

@bors r+

@bors
Copy link
Collaborator

bors commented Mar 6, 2020

📌 Commit 5c7ef5ce7b16f958a1252b4371943c3355e2da5e has been approved by eddyb

@bors
Copy link
Collaborator

bors commented Mar 6, 2020

🌲 The tree is currently closed for pull requests below priority 1000, this pull request will be tested once the tree is reopened

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 6, 2020
@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 6, 2020

@bors r=eddyb

@bors
Copy link
Collaborator

bors commented Mar 6, 2020

📌 Commit 875959635bf156e9564d245b20c21784c224ee2a has been approved by eddyb

@bors
Copy link
Collaborator

bors commented Mar 6, 2020

🌲 The tree is currently closed for pull requests below priority 1000, this pull request will be tested once the tree is reopened

@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 6, 2020

@bors r=eddyb

@bors
Copy link
Collaborator

bors commented Mar 6, 2020

📌 Commit a2dfd7e57fcb0af47d9c4ffdfb60c35a9f50e8dd has been approved by eddyb

@bors
Copy link
Collaborator

bors commented Mar 6, 2020

🌲 The tree is currently closed for pull requests below priority 1000, this pull request will be tested once the tree is reopened

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Mar 6, 2020

Discussed in today's meeting and decided against backporting this, since #69753 fixes the same problem in a narrow way (though this change is still desired).

@RalfJung
Copy link
Member

RalfJung commented Mar 6, 2020

@bors r- because #69753 goes first, and this PR should be rebased on top of that and revert the Miri changes.

Also @oli-obk could you add a corresponding assertion here that checks if i is in-bounds for the fields of this union? I think that should make sure codegen also notices these kinds of MIR incoherences.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 6, 2020
@eddyb
Copy link
Member

eddyb commented Mar 6, 2020

Also @oli-obk could you add a corresponding assertion here that checks if i is in-bounds for the fields of this union? I think that should make sure codegen also notices these kinds of MIR incoherences.

cc #64987 (comment)

@RalfJung
Copy link
Member

RalfJung commented Mar 6, 2020

@eddyb that PR actually did add the assertion, but it seems someone removed it again later?

@RalfJung
Copy link
Member

RalfJung commented Mar 6, 2020

Ah it got reverted in #66250

…d a long lost assertion

This reverts part of commit 9712fa4.
Centril added a commit to Centril/rust that referenced this pull request Mar 11, 2020
…lfJung

Compute the correct layout for variants of uninhabited enums

r? @eddyb
cc @RalfJung

fixes rust-lang#69191
cc rust-lang#69763
Centril added a commit to Centril/rust that referenced this pull request Mar 11, 2020
…lfJung

Compute the correct layout for variants of uninhabited enums

r? @eddyb
cc @RalfJung

fixes rust-lang#69191
cc rust-lang#69763
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Mar 14, 2020
…lfJung

Compute the correct layout for variants of uninhabited enums

r? @eddyb
cc @RalfJung

fixes rust-lang#69191
cc rust-lang#69763
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Mar 14, 2020
…lfJung

Compute the correct layout for variants of uninhabited enums

r? @eddyb
cc @RalfJung

fixes rust-lang#69191
cc rust-lang#69763
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Mar 17, 2020
…lfJung

Compute the correct layout for variants of uninhabited enums

r? @eddyb
cc @RalfJung

fixes rust-lang#69191
cc rust-lang#69763
Manishearth added a commit to Manishearth/rust that referenced this pull request Mar 17, 2020
…lfJung

Compute the correct layout for variants of uninhabited enums

r? @eddyb
cc @RalfJung

fixes rust-lang#69191
cc rust-lang#69763
Centril added a commit to Centril/rust that referenced this pull request Mar 19, 2020
…lfJung

Compute the correct layout for variants of uninhabited enums

r? @eddyb
cc @RalfJung

fixes rust-lang#69191
cc rust-lang#69763
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 20, 2020
Rollup of 9 pull requests

Successful merges:

 - rust-lang#69618 (Clarify the relationship between `forget()` and `ManuallyDrop`.)
 - rust-lang#69768 (Compute the correct layout for variants of uninhabited enums)
 - rust-lang#69935 (codegen/mir: support polymorphic `InstanceDef`s)
 - rust-lang#70103 (Clean up E0437 explanation)
 - rust-lang#70131 (Add regression test for TAIT lifetime inference (issue rust-lang#55099))
 - rust-lang#70133 (remove unused imports)
 - rust-lang#70145 (doc: Add quote to .init_array)
 - rust-lang#70146 (Clean up e0438 explanation)
 - rust-lang#70150 (triagebot.toml: accept cleanup-crew)

Failed merges:

r? @ghost
@bors bors merged commit 3554f2d into rust-lang:master Mar 20, 2020
@oli-obk oli-obk deleted the union_field_ice branch March 22, 2020 21:33
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

beta regression: ICE on Tried to access field 0 of union Layout
7 participants