Treat repr(Rust) univariant fieldless enums as ZSTs #49513
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @estebank (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
|
Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
@alexcrichton @nikomatsakis Do you know whether we can just revert the previous decision here? |
|
@eddyb I don't personally know of any instances of this in the wild, so it seems fine to me to land. I might prefer though to land it after the beta branches |
|
The failure is that |
arielb1
reviewed
Mar 31, 2018
src/test/compile-fail/issue-41394.rs
Outdated
| @@ -14,7 +14,7 @@ enum Foo { | |||
| } | |||
|
|
|||
| enum Bar { | |||
| A = Foo::A as isize | |||
| A = Foo::A as isize //~ ERROR enums without inhabited variants do not have discriminants | |||
There was a problem hiding this comment.
This makes the test no longer be a test of what it used to be testing. Could you add another variant before?
arielb1
reviewed
Mar 31, 2018
src/test/ui/issue-23302-1.rs
Outdated
| @@ -11,7 +11,7 @@ | |||
| // Check that an enum with recursion in the discriminant throws | |||
| // the appropriate error (rather than, say, blowing the stack). | |||
| enum X { | |||
| A = X::A as isize, //~ ERROR E0391 | |||
| A = X::A as isize, //~ ERROR enums without inhabited variants do not have discriminants | |||
There was a problem hiding this comment.
This makes the test no longer be a test of what it used to be testing. Could you add another variant B = 0 before A?
arielb1
reviewed
Mar 31, 2018
src/test/ui/issue-23302-1.rs
Outdated
| @@ -11,7 +11,7 @@ | |||
| // Check that an enum with recursion in the discriminant throws | |||
| // the appropriate error (rather than, say, blowing the stack). | |||
| enum X { | |||
| A = X::A as isize, //~ ERROR E0391 | |||
| A = X::A as isize, //~ ERROR enums without inhabited variants do not have discriminants | |||
| } | |||
There was a problem hiding this comment.
Afterwards, you should add a specific test for this.
|
Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
So apparently enum X { A = 0 as isize }is now a ZST. I did not expect that, even though it makes sense. And it also explains the other failures we had before. |
oli-obk
force-pushed
the
univariant-fieldless-enum-as-zst
branch
from
a652155 to
4efed8f
Compare
|
I can implement this, but I'm getting slightly confused about the exact semantics we want here. Since enum X { A = 42 as isize }is a zst, that means let x = X::A;is a zst value (so no value, just type system magic). This raises the question of how let y = x as isize;would get the Casting a println!("{}", y);
|
|
Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
@oli-obk The code for casting has access to the source and target types, right? If the source type is a single-variant fieldless enum, the result of the cast is just always the integer value of that single variant (i.e., it'll codegen a constant). Is there any problem with that? |
|
Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
There might be missing code to get the right discriminant (42) from the variant_index (0). |
Hmm, there must be code somewhere for how |
|
Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
RalfJung
reviewed
Apr 1, 2018
src/test/run-pass/zst_enum.rs
Outdated
| let x = X::A; | ||
| let y = x as isize; | ||
| println!("{:?}", x); | ||
| println!("{}", y); |
There was a problem hiding this comment.
These printlns are not actually tested for, are they? Could a ui test check them?
There was a problem hiding this comment.
Those should simply be an assert
|
@oli-obk I feel like my additional commit broke your newly-introduced test |
|
After your change the op isn't true anymore, right? Can you add some size_of assert tests? |
|
If by "the op" you mean the fact that |
|
Somewhere in run-pass. Could you find an existing test? |
|
src/test/run-pass/type-sizes.rs |
|
So... I'm confused as to what your latest commit changed. Why isn't the failing test not producing zsts anymore? |
|
It still is, the following program prints two zeroes: use std::mem;
fn main() {
enum E { V = 42 as isize }
let e = E::V;
println!("{}", mem::size_of::<E>());
println!("{}", e as isize);
}The first one because E is still a ZST; the second one because there is a bug somewhere that I'm currently trying to fix. The change I reverted was an unrelated change that I shouldn't have committed in the first place, because it relates to the niche-filling optim which is orthogonal to what I'm trying to achieve here. Does that make sense to you @oli-obk? |
|
Yes it does. Thanks for the explanation. I can fix the miri part after the runtime part works |
|
💔 Test failed - status-travis |
bors
added
S-waiting-on-review
|
Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
1 similar comment
|
Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
kennytm
added
S-waiting-on-author
|
☔ The latest upstream changes (presumably #49933) made this pull request unmergeable. Please resolve the merge conflicts. |
nox
force-pushed
the
univariant-fieldless-enum-as-zst
branch
from
98fb286 to
b11c244
Compare
|
☔ The latest upstream changes (presumably #50072) made this pull request unmergeable. Please resolve the merge conflicts. |
…#15747) This makes all those enums be represented the same way: ```rust enum A1 { B1 } enum A2 { B2 = 0 } enum A3 { B3, C3(!) } ```
I should rather properly fix debuginfo but I have no clue how to do that.
nox
force-pushed
the
univariant-fieldless-enum-as-zst
branch
from
b11c244 to
1c09977
Compare
|
@bors r+ |
|
📌 Commit 1c09977 has been approved by |
bors
added
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Apr 26, 2018
Treat repr(Rust) univariant fieldless enums as ZSTs
This makes all those enums be represented the same way:
```rust
enum A1 { B1 }
enum A2 { B2 = 0 }
enum A3 { B3, C3(!) }
```
Related to #15747.
Cc @rust-lang/wg-codegen @rust-lang/lang
|
☀️ Test successful - status-appveyor, status-travis |
kennytm
added a commit
to kennytm/rust
that referenced
this pull request
May 3, 2018
Allow unaligned reads in constants fixes rust-lang#50356 introduced in rust-lang#49513
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This makes all those enums be represented the same way:
Related to #15747.
Cc @rust-lang/wg-codegen @rust-lang/lang