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.

Sign up for GitHub

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

Jump to bottom

Compute the correct layout for variants of uninhabited enums #69768

Merged
merged 3 commits into from
Mar 20, 2020
Merged

Compute the correct layout for variants of uninhabited enums #69768

merged 3 commits into from
Mar 20, 2020

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Mar 6, 2020 •
edited
Loading

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
Contributor

bors commented Mar 6, 2020

📌 Commit 5c7ef5ce7b16f958a1252b4371943c3355e2da5e has been approved by eddyb

@bors
Copy link
Contributor

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
@eddyb eddyb mentioned this pull request Mar 6, 2020
Merged
@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 6, 2020

@bors r=eddyb

@bors
Copy link
Contributor

bors commented Mar 6, 2020

📌 Commit 875959635bf156e9564d245b20c21784c224ee2a has been approved by eddyb

@bors
Copy link
Contributor

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
Contributor

bors commented Mar 6, 2020

📌 Commit a2dfd7e57fcb0af47d9c4ffdfb60c35a9f50e8dd has been approved by eddyb

@bors
Copy link
Contributor

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

@pnkfelix pnkfelix mentioned this pull request Mar 6, 2020
Closed
@nikomatsakis
Copy link
Contributor

nikomatsakis commented Mar 6, 2020 •
edited
Loading

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

@oli-obk
Compute the correct layout for variants of uninhabited enums and read…
cdc7304 
…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
@Centril
Rollup merge of rust-lang#69768 - oli-obk:union_field_ice, r=eddyb,Ra…
3dc5218 
…lfJung

Compute the correct layout for variants of uninhabited enums

r? @eddyb
cc @RalfJung

fixes rust-lang#69191
cc rust-lang#69763
@Centril Centril mentioned this pull request Mar 11, 2020
Closed
Centril added a commit to Centril/rust that referenced this pull request Mar 11, 2020
@Centril
Rollup merge of rust-lang#69768 - oli-obk:union_field_ice, r=eddyb,Ra…
7210c6c 
…lfJung

Compute the correct layout for variants of uninhabited enums

r? @eddyb
cc @RalfJung

fixes rust-lang#69191
cc rust-lang#69763
@Centril Centril mentioned this pull request Mar 11, 2020
Closed
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Mar 14, 2020
@Dylan-DPC
Rollup merge of rust-lang#69768 - oli-obk:union_field_ice, r=eddyb,Ra…
8f06c62 
…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 Dylan-DPC-zz mentioned this pull request Mar 14, 2020
Closed
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Mar 14, 2020
@JohnTitor
Rollup merge of rust-lang#69768 - oli-obk:union_field_ice, r=eddyb,Ra…
5ef02a5 
…lfJung

Compute the correct layout for variants of uninhabited enums

r? @eddyb
cc @RalfJung

fixes rust-lang#69191
cc rust-lang#69763
@JohnTitor JohnTitor mentioned this pull request Mar 14, 2020
Closed
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Mar 17, 2020
@Dylan-DPC
Rollup merge of rust-lang#69768 - oli-obk:union_field_ice, r=eddyb,Ra…
dd0d904 
…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 Dylan-DPC-zz mentioned this pull request Mar 17, 2020
Closed
Manishearth added a commit to Manishearth/rust that referenced this pull request Mar 17, 2020
@Manishearth
Rollup merge of rust-lang#69768 - oli-obk:union_field_ice, r=eddyb,Ra…
45ed7e1 
…lfJung

Compute the correct layout for variants of uninhabited enums

r? @eddyb
cc @RalfJung

fixes rust-lang#69191
cc rust-lang#69763
@Manishearth Manishearth mentioned this pull request Mar 17, 2020
Closed
Centril added a commit to Centril/rust that referenced this pull request Mar 19, 2020
@Centril
Rollup merge of rust-lang#69768 - oli-obk:union_field_ice, r=eddyb,Ra…
46d8095 
…lfJung

Compute the correct layout for variants of uninhabited enums

r? @eddyb
cc @RalfJung

fixes rust-lang#69191
cc rust-lang#69763
@Centril Centril mentioned this pull request Mar 19, 2020
Closed
@JohnTitor JohnTitor mentioned this pull request Mar 20, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 20, 2020
@bors
Auto merge of rust-lang#70174 - JohnTitor:rollup-0lum0jh, r=JohnTitor
2835ca6 
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lqd lqd lqd left review comments

@RalfJung RalfJung RalfJung left review comments

@eddyb eddyb eddyb approved these changes

Assignees

@eddyb eddyb

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

beta regression: ICE on Tried to access field 0 of union Layout
7 participants
@oli-obk @RalfJung @eddyb @bors @nikomatsakis @rust-highfive @lqd