Skip to content

Update documentation for arbitrary_enum_discriminant feature - #639

Closed
jswrenn wants to merge 2 commits into
rust-lang:masterfrom
jswrenn:arbitrary_enum_discriminant
Closed

Update documentation for arbitrary_enum_discriminant feature#639
jswrenn wants to merge 2 commits into
rust-lang:masterfrom
jswrenn:arbitrary_enum_discriminant

Conversation

@jswrenn

Copy link
Copy Markdown
Member

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

@HavvyHavvy left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
MemberAuthor

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
Copy Markdown
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)]

Comment threadsrc/items/enumerations.md Outdated

#### Casting

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

Copy link
Copy Markdown
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
Copy Markdown
MemberAuthor

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
Copy Markdown
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
Copy Markdown
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 *<aname="field-less-enum">field-less enum</a>*.

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

```rust
# #![feature(arbitrary_enum_discriminant)]

Copy link
Copy Markdown
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)]

Comment threadsrc/items/enumerations.md Outdated
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
Copy Markdown
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
Copy Markdown
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.

@HavvyHavvy 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 <Ryan.havvy@gmail.com>
@jswrenn

jswrenn commented Jul 15, 2019

Copy link
Copy Markdown
MemberAuthor

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:

enumCLike{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:

enumFieldless{Unit,Tuple(),Struct{},}

This distinction unfortunately (ugh) matters:

  1. OnlyC-like enums may specify explicit discriminant without specifying a repr.
  2. Onlyfield-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
Copy Markdown
Contributor

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

@ehuss

Copy link
Copy Markdown
Contributor

@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.

@ehussehuss 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
Copy Markdown
Member

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

@JohnTitor

JohnTitor commented Jun 23, 2021

Copy link
Copy Markdown
Member

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
Copy Markdown
MemberAuthor

@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`
Closesrust-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

ehuss commented Nov 9, 2022

Copy link
Copy Markdown
Contributor

Closing as this has been continued by #1055.

@ehussehuss closed this Nov 9, 2022
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Language CleanupImprovements to existing language which is correct but not clear, or missing examples, or the like.New ContentMissing features or aspects of language not currently documented.RFC Stabilization DocsDocumentation required for stabilizing a featureS-waiting-on-stabilizationWaiting 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

@jswrenn@Havvy@ehuss@fee1-dead@JohnTitor