-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Allow arbitrary enums to have explicit discriminants #2363
Merged
Merged
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,268 @@ | ||
- Feature Name: arbitrary_enum_discriminant | ||
- Start Date: 2018-03-11 | ||
- RFC PR: (leave this empty) | ||
- Rust Issue: (leave this empty) | ||
|
||
# Summary | ||
[summary]: #summary | ||
|
||
This RFC gives users a way to control the discriminants of variants of all | ||
enumerations, not just the ones that are shaped like C-like enums (i.e. where | ||
all the variants have no fields). | ||
|
||
The change is minimal: allow any variant to be adorned with an explicit | ||
discriminant value, whether or not that variant has any field. | ||
|
||
# Motivation | ||
[motivation]: #motivation | ||
|
||
Stylo, the style system of Servo, represents CSS properties with a large | ||
enumeration `PropertyDeclaration` where each variant has only one field which | ||
represents the value of a given CSS property. Here is a subset of it: | ||
|
||
```rust | ||
#[repr(u16)] | ||
enum PropertyDeclaration { | ||
Color(Color), | ||
Height(Length), | ||
InlineSize(Length), | ||
TransformOrigin(TransformOrigin), | ||
} | ||
``` | ||
|
||
For various book-keeping reasons, Servo also generates a `LonghandId` | ||
enumeration with the same variants as `PropertyDeclaration` but without the | ||
fields, thus making `LonghandId` a C-like enumeration: | ||
|
||
```rust | ||
#[derive(Clone, Copy)] | ||
#[repr(u16)] | ||
enum LonghandId { | ||
Color, | ||
Height, | ||
InlineSize, | ||
TransformOrigin, | ||
} | ||
``` | ||
|
||
Given that rustc guarantees that `#[repr(u16)]` enumerations start with their | ||
discriminant stored as a `u16`, going from `&PropertyDeclaration` to | ||
`LonghandId` is then just a matter of unsafely coercing `&self` as a | ||
`&LonghandId`: | ||
|
||
```rust | ||
impl PropertyDeclaration { | ||
fn id(&self) -> LonghandId { | ||
unsafe { *(self as *const Self as *const LonghandId) } | ||
} | ||
} | ||
``` | ||
|
||
This works great, but doesn't scale if we want to replicate this behaviour for | ||
an enumeration that is a subset of `PropertyDeclaration`, for example an | ||
enumeration `AnimationValue` that is limited to animatable properties: | ||
|
||
```rust | ||
#[repr(u16)] | ||
enum AnimationValue { | ||
Color(Color), | ||
Height(Length), | ||
TransformOrigin(TransformOrigin), | ||
} | ||
|
||
impl AnimationValue { | ||
fn id(&self) -> LonghandId { | ||
// We can't just unsafely read `&self` as a `&LonghandId` because | ||
// the discriminant of `AnimationValue::TransformOrigin` isn't equal | ||
// to `LonghandId::TransformOrigin` anymore. | ||
match *self { | ||
AnimationValue::Color(_) => LonghandId::Color, | ||
AnimationValue::Height(_) => LonghandId::Height, | ||
AnimationValue::TransformOrigin(_) => LonghandId::TransformOrigin, | ||
} | ||
} | ||
} | ||
``` | ||
|
||
This is not sustainable, as the jump table generated by rustc to compile this | ||
huge match expression is larger than 4KB in the final Gecko binary, when this | ||
operation could be a trivial `u16` copy. This is worked around in Servo by | ||
generating spurious `Void` variants for the non-animatable properties in | ||
`AnimationValue`: | ||
|
||
```rust | ||
enum Void {} | ||
|
||
#[repr(u16)] | ||
enum AnimationValue { | ||
Color(Color), | ||
Height(Length), | ||
InlineSize(Void), | ||
TransformOrigin(TransformOrigin), | ||
} | ||
|
||
impl AnimationValue { | ||
fn id(&self) -> LonghandId | ||
// We can use the unsafe trick again. | ||
unsafe { *(self as *const Self as *const LonghandId) } | ||
} | ||
} | ||
``` | ||
|
||
This is unfortunately quite painful to use, given now all methods matching | ||
against `AnimationValue` need to have dummy arms for all of these variants: | ||
|
||
```rust | ||
impl AnimationValue { | ||
fn do_something(&self) { | ||
match *self { | ||
AnimationValue::Color(ref color) => { | ||
do_something_with_color(color) | ||
} | ||
AnimationValue::Height(ref height) => { | ||
do_something_with_height(height) | ||
} | ||
// This shouldn't be needed. | ||
AnimationValue::InlineSize(ref void) => { | ||
match *void {} | ||
} | ||
AnimationValue::TransformOrigin(ref origin) => { | ||
do_something_with_transform_origin(origin) | ||
} | ||
} | ||
} | ||
} | ||
``` | ||
|
||
We suggest generalising the explicit discriminant notation to all enums, | ||
regardless of whether their variants have fields or not: | ||
|
||
```rust | ||
#[repr(u16)] | ||
enum AnimationValue { | ||
Color(Color) = LonghandId::Color as u16, | ||
Height(Length) = LonghandId::Height as u16, | ||
TransformOrigin(TransformOrigin) = LonghandId::TransformOrigin as u16, | ||
} | ||
|
||
impl AnimationValue { | ||
fn id(&self) -> LonghandId | ||
// We can use the unsafe trick again. | ||
unsafe { *(self as *const Self as *const LonghandId) } | ||
} | ||
|
||
fn do_something(&self) { | ||
// No spurious variant anymore. | ||
match *self { | ||
AnimationValue::Color(ref color) => { | ||
do_something_with_color(color) | ||
} | ||
AnimationValue::Height(ref height) => { | ||
do_something_with_height(height) | ||
} | ||
AnimationValue::TransformOrigin(ref origin) => { | ||
do_something_with_transform_origin(origin) | ||
} | ||
} | ||
} | ||
} | ||
``` | ||
|
||
# Guide-level explanation | ||
[guide-level-explanation]: #guide-level-explanation | ||
|
||
An enumeration with only field-less variants can currently have explicit | ||
discriminant values: | ||
|
||
```rust | ||
enum ForceFromage { | ||
Emmental = 0, | ||
Camembert = 1, | ||
Roquefort = 2, | ||
} | ||
``` | ||
|
||
With this RFC, users are allowed to put explicit discriminant values on any | ||
variant of any enumeration, not just the ones where all variants are field-less: | ||
|
||
```rust | ||
enum ParisianSandwichIngredient { | ||
Bread(BreadKind) = 0, | ||
Ham(HamKind) = 1, | ||
Butter(ButterKind) = 2, | ||
} | ||
``` | ||
|
||
# Reference-level explanation | ||
[reference-level-explanation]: #reference-level-explanation | ||
|
||
## Grammar | ||
|
||
The production for enumeration items becomes: | ||
|
||
``` | ||
EnumItem : | ||
OuterAttribute* | ||
IDENTIFIER ( EnumItemTuple | EnumItemStruct)? EnumItemDiscriminant? | ||
``` | ||
|
||
## Semantics | ||
|
||
The limitation that only field-less enumerations can have explicit discriminant | ||
values is lifted, and no other change is made to their semantics: | ||
|
||
* enumerations with fields still can't be casted to numeric types | ||
with the `as` operator; | ||
* if the first variant doesn't have an explicit discriminant, | ||
it is set to zero; | ||
* any unspecified discriminant is set to one higher than the one from | ||
the previous variant; | ||
* under the default representation, the specified discriminants are | ||
interpreted as `isize`; | ||
* two variants cannot share the same discriminant. | ||
|
||
# Drawbacks | ||
[drawbacks]: #drawbacks | ||
|
||
This introduces one more knob to the representation of enumerations. | ||
|
||
# Rationale and alternatives | ||
[alternatives]: #alternatives | ||
|
||
Reusing the current syntax and semantics for explicit discriminants of | ||
field-less enumerations means that the changes to the grammar and semantics of | ||
the language are minimal. The alternative is to put the specified discriminants | ||
in variant attributes, but this would be at odds with the syntax for field-less | ||
enumerations. | ||
|
||
```rust | ||
enum ParisianSandwichIngredient { | ||
#[discriminant = 0] | ||
Bread(BreadKind), | ||
#[discriminant = 1] | ||
Ham(HamKind), | ||
#[discriminant = 2] | ||
Butter(ButterKind), | ||
} | ||
``` | ||
|
||
# Prior art | ||
[prior-art]: #prior-art | ||
|
||
No prior art. | ||
|
||
# Unresolved questions | ||
[unresolved]: #unresolved-questions | ||
|
||
## Should discriminants of enumerations with fields be specified as variant attributes? | ||
|
||
Should they? | ||
|
||
## Should this apply only to enumerations with an explicit representation? | ||
|
||
Should it? | ||
|
||
# Thanks | ||
|
||
Thanks to Mazdak Farrokhzad (@Centril) and Simon Sapin (@SimonSapin) for | ||
the reviews, and my local bakery for their delicious baguettes. 🥖 |
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you have encountered the XY Problem. You are finding a solution to the problem that different enums don't give guarantees about their variant indices, when what you originally wanted was converting enums whose variants have fields into enums whose variants have no fields.
So here's my alternative:
Allow annotating enums with
#[discriminant(OtherEnum)]
, whereOtherEnum
must have a variant for each variant in the annotated enum (with matching name). The annotated enum will then take all its discriminant values fromOtherEnum
.as
casts could then convert from annotated enums to their discriminant enum.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems more convoluted, IMO. Definitely harder to implement, at least.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, but it has a path forward for having this operation in safe code.
Alternatively a procedural macro could generate said conversions with this RFC. So the compiler magic might not be necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be great and nice, if only real world allowed me to do that.
I simplified my use case for the sake of the RFC:
PropertyDeclaration
actually has more variants thanLonghandId
, thanks to custom CSS properties (and some other stuff that I don't need to mention here):In general, I think this alternative is too specific to the exact use case described in the RFC, and cannot fulfil more intricate ones like the actual stuff I require in Servo, or use cases I could imagine for this feature with FFI, @gankro may have an opinion on that regard here.
Edit: I'll edit this RFC to include ite nonetheless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mention in the RFC that rust is generating a 4KB jump table for the required match expression - this seems excessive, even if the discriminants don't match exactly. How many variants does this enum have? Maybe part of the solution should be improving the code that rust generates in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Diggsey There are >320 variants, so far. I welcome any rustc improvement but they won't be able to remove all jump tables if the discriminants of the common variants to
AnimationValue
andPropertyDeclaration
don't coincide.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Diggsey Also if you are curious, here is the PR where
PropertyDeclaration::id
was slimmed down from 4KB to mere 96 bytes, and there is the PR where I simplifiedAnimationValue::id
.