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.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

add a chapter on enum representation #57

Merged
merged 14 commits into from Jan 10, 2019

Conversation

nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Dec 19, 2018

This is a proposed summary chapter on enum representation. I made a best effort to represent what seemed to be the consensus from #10 but I would definitely appreciate review, as I confess I kind of wrote this in a hurry and could easily have missed some details or objections. 馃槢

Here are the important details:

  • Uninhabited enums (no variants) are guaranteed to share layout with !
  • C-like enums are
  • I copied the definition of the layout of #[repr] enums from RFC #2195
  • I coined the term "discriminant elision" to describe the "Option optimization"
  • I adopted what seemed like a fairly restrictive definition of "option-like enums"; these are the enums for which we guarantee "discriminant elision".
    • It does permit enums with more than one unit variant. only one unit variant now
    • The unit variant must have no fields, like None, and hence this definition excludes Result<T, ()>
    • I was thinking this would be better for now, so as to sidestep the problem of defining zero-sized structs and so forth, but I would be happy for us to revisit it.
  • I left a few (non-normative, caveated) notes about niche values. mostly removed

Here are a few questions:

  • Is my definition of "option-like enums" acceptable? I think it is crucial to include Option<&T> as having a guaranteed optimization, but am somewhat flexible beyond that
  • We never settled on a term for "niche values". I simply used "niche values" here but I was thinking after writing that perhaps "illegal" or "undefined values" would be better?
  • In writing this summary, I realize I did not specify anything about variants with uninhabited types and so forth. Perhaps I should leave this as "future work"?

Fixes #10

cc @eddyb in particular for technical review

reference/src/representation/enums.md Outdated Show resolved Hide resolved
reference/src/representation/enums.md Outdated Show resolved Hide resolved
reference/src/representation/enums.md Outdated Show resolved Hide resolved
reference/src/representation/enums.md Outdated Show resolved Hide resolved
reference/src/representation/enums.md Outdated Show resolved Hide resolved
reference/src/representation/enums.md Outdated Show resolved Hide resolved
reference/src/representation/enums.md Outdated Show resolved Hide resolved
reference/src/representation/enums.md Outdated Show resolved Hide resolved
reference/src/representation/enums.md Outdated Show resolved Hide resolved
@asajeffrey
Copy link

Comments added, mostly nits. The big one is that it would be nice to have the defn of "niche value" in this section rather than hidden away in another "representation invariant" section. This would make the "representation" section self-contained, and get rid of the need for "bitstring validity".

@nikomatsakis
Copy link
Contributor Author

OK, pushed a few updates to resolve comments so far.

reference/src/representation/enums.md Outdated Show resolved Hide resolved
reference/src/representation/enums.md Outdated Show resolved Hide resolved
Copy link
Contributor

@gnzlbg gnzlbg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a good start and I agree with everything here.

I wish we would guarantee more niche optimizations though. I have some code using Result in C FFI to map a c_int == 0 to an Ok(()) and a c_int != 0 to an Err(NonZero<c_int>) that relies on size_of::<Result> == size_of::<c_int>. But we can add more guaranteed discriminant elision optimizations to this document in the future. These do not need to be part of the first version of this RFC.

reference/src/representation/enums.md Outdated Show resolved Hide resolved
@nikomatsakis
Copy link
Contributor Author

Addressed outstanding feedback I believe. =)

@nikomatsakis
Copy link
Contributor Author

I'm marking this with FINAL COMMENT PERIOD, it's been idle for quite a while.

@gnzlbg
Copy link
Contributor

gnzlbg commented Jan 3, 2019

Still LGTM.

My only nits are that Rust code in the wild already relies on many enum optimizations that are not guaranteed, and that makes the unsafe code guidelines less useful and more complicated than what they could be.

For example, the answer to the question: "Can I use Option<&T> on C FFI where pointers are expected?" is not "Yes.", but rather, "That relies on an optimization that is not guaranteed to happen, but in practice, Rust always performs this optimization, so yes, you can rely on that.". The same applies to data-carrying enums with ZST variants, fieldless enums with one variant, etc.

We don't have to provide better answers to these questions right now, but we should strive to do so in 2019, e.g., via an RFC about guaranteed enum layout optimizations or something like that.

@RalfJung
Copy link
Member

RalfJung commented Jan 3, 2019

That relies on an optimization that is not guaranteed to happen

Doesn't this guarantee the optimization?

Option-like enums where the payload defines at least one niche value
are guaranteed to be represented using the same memory layout as their
payload.

@gnzlbg
Copy link
Contributor

gnzlbg commented Jan 4, 2019

Doesn't this guarantee the optimization?

If it were normative, it would, but a couple of lines above the document states that everything that follows isn't normative :/

We discussed this briefly in the meeting, and @nikomatsakis wanted to open an issue to track all non-normative things that we'd like to RFC at some point and why.

@RalfJung
Copy link
Member

RalfJung commented Jan 4, 2019

but a couple of lines above the document states that everything that follows isn't normative :/

I agree it should be made normative through an RFC, but you made it sound like a change in this document could make you happy ("my only nits are ..."). I don't understand how that can be the case. Nothing we write here can change the fact that this is not normative.

@gnzlbg
Copy link
Contributor

gnzlbg commented Jan 4, 2019

Ah no, sorry! I didn't meant to say that, only that we should fill some issues about this and once we do that hyperlink them here, but that can be done as a follow up PR, once this is merged and those issues are filled.

nikomatsakis and others added 8 commits January 10, 2019 11:51
- only guarantee with one unit variant
- fieldless enums
- remove potentially confusing discussion of what compiler does today
- remove discussion of niche values from enums, not important (yet)
- generally reorganize layout rules to be by category of enum
Co-Authored-By: nikomatsakis <niko@alum.mit.edu>
@nikomatsakis
Copy link
Contributor Author

I opened #79 but I didn't open an issue pertaining to the normative aspects of repr layout, since I don't think it's specific to this PR. We could do that separately if desired.

@nikomatsakis nikomatsakis merged commit b6730dd into rust-lang:master Jan 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants