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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update documentation for arbitrary_enum_discriminant feature #639

Closed
wants to merge 2 commits into from

Conversation

jswrenn
Copy link
Member

@jswrenn jswrenn commented Jul 14, 2019

Updates documentation to reflect the still unstable arbitrary_enum_discriminantfeature (tracking issue: rust-lang/rust#60553).

Copy link
Contributor

@Havvy Havvy left a comment

Choose a reason for hiding this comment

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

Lots of good work. ❤️

Lots of little changes requested; a couple bigger ones.

<ol>
<li>

if the enumeration is "C-like" (i.e., it has no tuple or struct variants); e.g.:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if the enumeration is "C-like" (i.e., it has no tuple or struct variants); e.g.:
if the enumeration is field-less. For example:

Copy link
Member Author

Choose a reason for hiding this comment

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

Per #639 (comment), these terms may not be truly interchangeable.

if the enumeration is "C-like" (i.e., it has no tuple or struct variants); e.g.:

```rust
# #![feature(arbitrary_enum_discriminant)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Feature flags aren't needed; the prose will only be merged after the feature is stable.

Suggested change
# #![feature(arbitrary_enum_discriminant)]

src/items/enumerations.md Outdated Show resolved Hide resolved

#### Casting

If there is no data attached to *any* of the variants of an enumeration, then
Copy link
Contributor

Choose a reason for hiding this comment

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

There's no need to describe a field-less enum here. Just use the term.

Suggested change
If there is no data attached to *any* of the variants of an enumeration, then
If the enum is a field-less enum, then

Copy link
Member Author

Choose a reason for hiding this comment

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

Per #639 (comment), these terms may not be completely interchangeable. If "field-less" is meant to be synonymous to "C-like", then this change would be incorrect.

If there is no data attached to *any* of the variants of an enumeration,
then the discriminant can be directly chosen and accessed.
Each enum instance has a _discriminant_: an integer logically associated to it
that is used to determine which variant it holds. An opaque reference to this
Copy link
Contributor

Choose a reason for hiding this comment

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

Using mem::discriminant should be in the accessing the discriminant section.

integer associated to it that is used to determine which variant it holds. An
opaque reference to this discriminant can be obtained with the
[`mem::discriminant`] function.
called an enum variant.
Copy link
Contributor

Choose a reason for hiding this comment

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

The re-organization looks fine, but we lose our definition of a field-less enum. This suggestion re-adds it. I don't actually know if the anchor actually works. It should just create an anchor that can be linked too without changing any styles or making it clickable.

Suggested change
called an enum variant.
called an enum variant.
An enum where no constructors contain fields are called a *<a name="field-less-enum">field-less enum</a>*.

if a [primitive representation] is used; e.g.:

```rust
# #![feature(arbitrary_enum_discriminant)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Feature flag not needed.

Suggested change
# #![feature(arbitrary_enum_discriminant)]

src/items/enumerations.md Outdated Show resolved Hide resolved
an `isize` value. However, the compiler is allowed to use a smaller type (or
another means of distinguishing variants) in its actual memory layout.

If the [primitive representation] or the [`C` representation] is used, the
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand what this paragraph is trying to say.

leading bytes of a variant (e.g., two bytes if `#[repr(u16)]` is used), will
correspond exactly to the discriminant.

### Assigning Discriminant Values
Copy link
Contributor

Choose a reason for hiding this comment

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

For this entire section, it only makes sense for field-less enums or enums with a primitive representation (or the C representation?). The way it is written, it seems like explicit discriminants are only for those and implicit applies to all. Instead, it should state that having a non-opaque (feel free to pick a different descriptor) discriminant applies in those situations and in all others, it is opaque and must only be accessed via mem::discriminant.

@Havvy Havvy added Language Cleanup Improvements to existing language which is correct but not clear, or missing examples, or the like. New Content Missing features or aspects of language not currently documented. RFC Stabilization Docs Documentation required for stabilizing a feature labels Jul 15, 2019
Co-Authored-By: Ryan Scheel <[email protected]>
@jswrenn
Copy link
Member Author

jswrenn commented Jul 15, 2019

Thank you so much for your review, @Havvy! If it's alright, I'm going to defer removing the feature flags until this gets a little closer to being merged; otherwise, I can't run mdbook tests.

Terminology

I have a lot of uncertainty about terminology. I only just saw that there's prior agreement (rust-lang/rust#46348, #244) to move away from the term "C-like enumeration" (which I believe is a very poor term) and towards "field-less enumeration". I am thrilled to strike the phrase "C-like" from this PR.

However, using "field-less enumeration" as a synonym for "C-like" isn't quite appropriate.

A C-like enumeration is one in which every variant is unit-like; for example:

enum CLike {
  VariantA,
  VariantB,
  VariantC,
}

All C-like enumerations are field-less, because they cannot include tuple-like or struct-like variants. However, not all field-less enums are are C-like. For instance:

enum Fieldless {
    Unit,
    Tuple(),
    Struct{},
}

This distinction unfortunately (ugh) matters:

  1. Only C-like enums may specify explicit discriminant without specifying a repr.
  2. Only field-less enums (regardless of whether they're also C-like) may be as-casted to their discriminant values.

It would be nice to have terms to describe both of these sets. My opinion:

  • "Field-less" is the right term to describe the set of enums which can be as-casted
  • However it cannot then also be used as a synonym for "C-like", so we should identify an alternative term to "C-like" to describe enums in which all variant are unit-like.

@Havvy
Copy link
Contributor

Havvy commented Jul 16, 2019

Well, that is quite unfortunate of an edge case. I'm failing to think of any good names.

@ehuss
Copy link
Contributor

ehuss commented Jul 16, 2019

@jswrenn Just letting you know that the reference has migrated to mdbook 0.3 (from 0.1). This means that the style of links are slightly different. They should use the .md extension, and are relative to the page they are on (previously they were relative to the root of the book). It looks like this PR should be fairly easy to rebase to resolve the conflicts. Please don't hesitate to ask if you have any questions or any trouble rebasing.

@ehuss ehuss added the S-waiting-on-stabilization Waiting for a stabilization PR to be merged in the main Rust repository label Apr 7, 2020
@fee1-dead
Copy link
Member

@jswrenn Are you still working on this? If not, I would like to work on it.

@JohnTitor
Copy link
Member

JohnTitor commented Jun 23, 2021

According to the label, this is waiting for stabilization Oh, I found you posted the report (rust-lang/rust#60553 (comment)), nvm!

@jswrenn
Copy link
Member Author

jswrenn commented Jul 6, 2021

@jswrenn Are you still working on this? If not, I would like to work on it.

I'm not! Go for it!

(Sorry for the delayed reply; I thought I had already replied.)

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 18, 2021
Stabilize `arbitrary_enum_discriminant`

Closes rust-lang#60553.

----

## Stabilization Report

_copied from rust-lang#60553 (comment)

### Summary

Enables a user to specify *explicit* discriminants on arbitrary enums.

Previously, this was hard to achieve:

```rust
#[repr(u8)]
enum Foo {
    A(u8) = 0,
    B(i8) = 1,
    C(bool) = 42,
}
```

Someone would need to add 41 hidden variants in between as a workaround with implicit discriminants.

In conjunction with [RFC 2195](https://github.com/rust-lang/rfcs/blob/master/text/2195-really-tagged-unions.md), this feature would provide more flexibility for FFI and unsafe code involving enums.

### Test cases

Most tests are in [`src/test/ui/enum-discriminant`](https://github.com/rust-lang/rust/tree/master/src/test/ui/enum-discriminant), there are two [historical](https://github.com/rust-lang/rust/blob/master/src/test/ui/parser/tag-variant-disr-non-nullary.rs) [tests](https://github.com/rust-lang/rust/blob/master/src/test/ui/parser/issue-17383.rs) that are now covered by the feature (removed by this pr due to them being obsolete).

### Edge cases

The feature is well defined and does not have many edge cases.
One [edge case](rust-lang#70509) was related to another unstable feature named `repr128` and is resolved.

### Previous PRs

The [implementation PR](rust-lang#60732) added documentation to the Unstable Book, rust-lang/reference#1055 was opened as a continuation of rust-lang/reference#639.

### Resolution of unresolved questions

The questions are resolved in rust-lang#60553 (comment).

----

(someone please add `needs-fcp`)
@ehuss
Copy link
Contributor

ehuss commented Nov 9, 2022

Closing as this has been continued by #1055.

@ehuss ehuss closed this Nov 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Language Cleanup Improvements to existing language which is correct but not clear, or missing examples, or the like. New Content Missing features or aspects of language not currently documented. RFC Stabilization Docs Documentation required for stabilizing a feature S-waiting-on-stabilization Waiting for a stabilization PR to be merged in the main Rust repository
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants