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

#[derive(Default)] on enums with a #[default] attribute #3107

Merged
merged 1 commit into from
Jul 27, 2021

Conversation

jhpratt
Copy link
Member

@jhpratt jhpratt commented Apr 12, 2021

Pre-RFC on IRLO

TLDR allow this:

#[derive(Default)]
enum Option<T> {
    #[default]
    None,
    Some(T),
}

Rendered

@jhpratt

This comment has been minimized.

@rustbot
Copy link
Collaborator

rustbot commented Apr 12, 2021

Error: This repository is not enabled to use triagebot.
Add a triagebot.toml in the root of the master branch to enable it.

Please let @rust-lang/release know if you're having trouble with this bot.

@joshtriplett joshtriplett added T-lang Relevant to the language team, which will review and decide on the RFC. A-derive Deriving related proposals & ideas A-enum Enum related proposals & ideas labels Apr 12, 2021
@clarfonthey
Copy link
Contributor

clarfonthey commented Apr 15, 2021

I strongly disagree with the non_exhaustive behaviour since it doesn't match the behaviour for structs. Why should a derived default not require bounds on the generics?

@jhpratt
Copy link
Member Author

jhpratt commented Apr 15, 2021

If the type itself is not used directly, but rather through some other type (like the example of *const T), the bounds need to be different. If only T: Default is required, a variant with a *const T field would be objectively wrong. In the other direction, Option<T> would then require T: Default, which is not necessary. There was significant discussion in the thread on IRLO, which you may want to check out.

As to #[non_exhaustive], is that an argument against this or for fixing #[derive(Default)] on structs? The ability to add a field to a #[non_exhaustive] struct without making bounds stricter is fundamentally required.

Keep in mind that this RFC is intentionally minimal such that #[non_exhaustive] and similar could be supported in the future in any situation.

@Lokathor

This comment has been minimized.

@kpreid
Copy link
Contributor

kpreid commented Apr 15, 2021

If I understand the design as specified, #[default] used by itself with no accompanying #[derive(Default)] has no effect. This seems like it would be surprising to many new users. A lint, or making lone #[default] inherently a compile error, would solve the problem of “why isn't my chosen default value recognized by the compiler?”, but it might still be seen as a frequently-encountered oddity of the language: “why do I have to declare this in two places when one should be sufficient?”

I don't have a good suggestion to solve this. (A wild suggestion: make it possible to put #[derive(...)] on an enum variant (which would give the derive macro the entire item, with the variant bearing the attribute identified in some fashion). This has its own language design issues, of course, but could have some other uses, such as offering #[derive(From)] for arbitrary enum variants similar to how thiserror currently does.)

@jhpratt
Copy link
Member Author

jhpratt commented Apr 15, 2021

#[default] without an accompanying derive would be a compile error. I'll review the text when I get a chance and clarify if necessary.

Having derives on enum variants themselves just feels wrong to me, given that enum variants are not types in and of themselves (I'm aware of the RFC changing that).

@jhpratt
Copy link
Member Author

jhpratt commented Apr 15, 2021

I think this part is sufficiently clear that it's impermissible:

Placing #[derive(Default)] on an enum named $e is permissible iff that enum has some variant $v with #[default] on it.

The "iff" indicates that it's all or nothing.

@Lokathor
Copy link
Contributor

please don't use "iff", that's a very jargon-y variant of the word that honestly looks like a typo to most people.

@jhpratt
Copy link
Member Author

jhpratt commented Apr 15, 2021

@Lokathor It's not uncommon to see that term in technical writing, which this is. I'd agree if it were not in the reference-level explanation.

@programmerjake
Copy link
Member

please don't use "iff", that's a very jargon-y variant of the word that honestly looks like a typo to most people.

Please expand it out to "if and only if" even in the reference. I would have thought that was a typo even though I have seen "iff" before, but only in a mathematical logic setting, not usually in a programming language reference.

@jhpratt jhpratt force-pushed the derive-enum-default branch from bf0196b to bae1c39 Compare April 15, 2021 22:11
@jhpratt
Copy link
Member Author

jhpratt commented Apr 15, 2021

Changed to write it out.

@jhpratt jhpratt force-pushed the derive-enum-default branch from bae1c39 to d5757c9 Compare April 18, 2021 22:40
@clarfonthey
Copy link
Contributor

Browser ate my comment and I don't feel like writing it up again so basically

I think it would be better if there wasn't any special handling based upon what variant you mark as the default, or whether it's marked as non-exhaustive, and always just puts bounds on the generics like the default struct logic does.

@jhpratt
Copy link
Member Author

jhpratt commented Apr 19, 2021

@clarfonthey The bounds were widely discussed on the linked IRLO post. The overwhelming majority of people did not want that behavior. There is also a legitimate concern regarding forward-compatibility for #[non_exhaustive] — just saying "please allow this" without any discussion as to how the concerns should be avoided is unproductive.

@clarfonthey
Copy link
Contributor

clarfonthey commented Apr 19, 2021

@jhpratt as I said, I had initially typed up said discussion, but my browser ate the text, and I've already been having a pretty bad day and that's why I decided to simply clarify what my original objection was (since that seemed rather unclear) without retyping the whole argument. I have a strong feeling that you're arguing in good faith and just want a properly focused discussion, but having an ounce of empathy would be nice.

FWIW the entire RFC process is supposed to bring people into the discussion who otherwise might not have been in those earlier discussions. Part of that discussion can just be people stating their feelings on the proposal itself without fully elaborating, as people with more time and energy will likely be able to help fully work out those points. In other words: simply telling people off for not fully elaborating their points is rude and unwelcome, especially when they give explicit reasons for not fully elaborating their points.

I know that working on an RFC is hard work, and I actually was the person who wrote the original #[non_exhaustive] RFC, so I can fully empathise with that. I could have done better with my original post. But telling me off for not including that detail, unless I'm explicitly acting in bad faith, is rude and pushes away people from the discussion who might not have as much of an investment as you have.

@branpk
Copy link

branpk commented Apr 19, 2021

Sorry if I'm missing something, but I'm a bit confused about the bound behavior.

What is the generated bound on the following?

#[derive(Default)]
enum A<T> {
    #[default]
    A(Option<T>);
}

Is it T: Default or Option<T>: Default? Either way, I think the RFC should state it more clearly.

To avoid needlessly strict bounds, all types present in the tagged variant's fields shall be bound by Default in the generated code.

Based on this, it sounds like the latter. If so: this is different than how structs currently behave. There is a longstanding issue here with over 100 comments, and I feel this RFC should mention the issue and explain why the concerns raised there don't apply here.

@jhpratt
Copy link
Member Author

jhpratt commented Apr 19, 2021

It would be Option<T>: Default as currently worded. I am not familiar with the concerns of that — I will look through that thread when I get a chance.


Edit: Here's my thoughts after quickly looking through that thread.

This comment doesn't apply because everything in enums are public; this is what most of the comments seem to center around.

The only other issue I see is the possibility of having a recursive enum (boxed of course), which should be trivially solvable by just not including that exact type in the bound, no?

@jhpratt
Copy link
Member Author

jhpratt commented Apr 19, 2021

@clarfonthey I was not telling you off in any way. I was simply stating that your comment as worded didn't contribute much, as there was already a decent discussion on IRLO (which was intended to be used as a reference as to some reasoning). In my opinion, just stating "I don't like this" doesn't really give me anything to work with, as I'm sure you can understand.

If this is just you having a bad day, I'd like to kindly suggest sleeping on it. Sometimes that makes a world's difference 🙂

@clarfonthey
Copy link
Contributor

clarfonthey commented Apr 20, 2021

Now that I've had a bit of rest, again elaborating my position:

I don't think that the fact that enum fields are public should change how generic bounds are set. Today you can create a struct with entirely public fields and this still will not set bounds based upon fields, but the generics directly. (see example)

Additionally, while it's tempting to only constrain bounds based upon the specific default chosen, this is technically uncharted territory. Normally, structs will be forced to still use generics even if they're marked as non_exhaustive, but in this particular case, you can sidestep using them for a single variant by using them on others.

While I agree that this behaviour is natural and desirable in some form, it breaks the precedent we have for structs and may make the behaviour seem less predictable based upon context.

This is what I was trying to say by breaking symmetry with structs, which I tried to elaborate before my browser crashed and ate my post. I think that it's definitely nice, but I'm not sure that it's as simple as the RFC is making it out to be. Plus, the other traits don't act this way either, e.g. PartialEq won't look at the fields of the variants and use that to constrain bounds.(see example)

@jhpratt
Copy link
Member Author

jhpratt commented Apr 20, 2021

I don't think that the fact that enum fields are public should change how generic bounds are set.

If the primary concern being raised is that private fields exist (on structs), why shouldn't that difference be noteworthy?

Today you can create a struct with entirely public fields and this still will not set bounds based upon fields, but the generics directly.

it breaks the precedent we have for structs and may make the behaviour seem less predictable based upon context.

Plus, the other traits don't act this way either, e.g. PartialEq won't look at the fields of the variants and use that to constrain bounds.

This is widely regarded as incorrect, so why should we willfully repeat this mistake? I think it would be best to not do so and to try and fix the existing issues with bounds from other derives separately.

Additionally, while it's tempting to only constrain bounds based upon the specific default chosen, this is technically uncharted territory. Normally, structs will be forced to still use generics even if they're marked as non_exhaustive, but in this particular case, you can sidestep using them for a single variant by using them on others.

I'm not entirely sure what you're referring to here. This RFC doesn't change how generics are handled on the enum itself, which is different from how they work on structs to begin with (that's why you can have Option::None not use T). What is the "sidestepping" you're referring to?


I'm not sure if you've had a chance to review the IRLO post or not (obviously fine if not!). There was a fair amount of discussion regarding bounds, and you will notice that I myself initially wanted the bounds to match the existing behavior of structs. I was convinced otherwise both due to the known erroneous bounds and an overwhelming opinion against my initial thoughts.

@branpk
Copy link

branpk commented Apr 20, 2021

Edit: Here's my thoughts after quickly looking through that thread.

This comment doesn't apply because everything in enums are public; this is what most of the comments seem to center around.

The only other issue I see is the possibility of having a recursive enum (boxed of course), which should be trivially solvable by just not including that exact type in the bound, no?

Just to point out (not sure it's significant) - another case mentioned in the thread is this where it's a public type but its Default implementation depends on a private trait.

Also, I think the cyclic case is a bit trickier than skipping recursive types, since it can also happen at the trait level. See here for example.

I personally actually feel like the approach described in this RFC is better than the current derive behavior. I'm just raising this concern because it seems weird that Default would work differently for enums than any other standard derive macro (including Default for struct, and any other trait for enums). Ultimately I would like all traits to work as described, but I'm wondering if it makes sense to do it for this specific case when the issue in question has been open for so long without clear consensus.

Edit: Just clarifying to avoid confusion - I believe that the concern I'm raising here is orthogonal to the question of whether bounds should be generated from all variants vs just the #[default] variant. If bounds are determined using the #[default] variant, then my question is concerning how those bounds are generated - whether it's based on the types used by the variant, or the type variables used by the variant.

@clarfonthey
Copy link
Contributor

This is widely regarded as incorrect, so why should we willfully repeat this mistake? I think it would be best to not do so and to try and fix the existing issues with bounds from other derives separately.

I feel like "widely regarded as incorrect" is a claim that requires a bit more evidence to be backed up. I feel like there's a lot of nuance to the decision and while I do think that the behaviour may be confusing or undesired at times, I don't think that it's explicitly bad.

If we do want to change the behaviour of this, I think it would be 100% reasonable to discuss and do on a new edition (since derive behaviour is definitely something that can change between editions), but that's not what this RFC is about. IMHO, unless the goal is to completely overhaul the way derives are done rather than just adding an extra case for defaulting enums, we should just go with the existing behaviour to limit confusion.

@programmerjake
Copy link
Member

+1 for fixing derives!!! been having issues with derive(PartialEq) forcing lifetimes to be equal.

@agurney
Copy link

agurney commented Apr 28, 2021

May also be worth noting that https://crates.io/crates/num_enum has a #[num_enum(default)] annotation which means something a little different. That one is about converting from a primitive representation to an enum, and the variant with this annotation will be the one chosen for an input value that doesn't match any of the other variants - this is similar to what serde spells #[serde(other)].

So the #[default] in this proposal is about picking a variant that's a sensible neutral choice, and #[num_enum(default)] is about a "none of the above" case. For example, in an enum Opcode we might want the #[default] to be Nop but the #[num_enum(default)] to be InvalidInstruction. In other cases these could coincide.

@scottmcm scottmcm removed the T-lang Relevant to the language team, which will review and decide on the RFC. label May 8, 2021
@rfcbot
Copy link
Collaborator

rfcbot commented Jul 9, 2021

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

@jhpratt

This comment has been minimized.

@Lokathor

This comment has been minimized.

@jhpratt

This comment has been minimized.

@Diggsey

This comment has been minimized.

@jhpratt

This comment has been minimized.

@Diggsey

This comment has been minimized.

@jhpratt

This comment has been minimized.

@jhpratt jhpratt force-pushed the derive-enum-default branch from 3996301 to ee1e93c Compare July 15, 2021 17:24
@jhpratt
Copy link
Member Author

jhpratt commented Jul 15, 2021

Updated to match the fact that the #[default] attribute doesn't have to be built-in (and won't be in reality).

pnkfelix added a commit that referenced this pull request Jul 27, 2021
RFC 3107: "`#[derive(Default)]` on enums with a `#[default]` attribute" (#3107).
@pnkfelix pnkfelix merged commit ee1e93c into rust-lang:master Jul 27, 2021
@jhpratt jhpratt deleted the derive-enum-default branch July 27, 2021 17:09
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Apr 14, 2022
…um, r=davidtwco

Stabilize `derive_default_enum`

This stabilizes `#![feature(derive_default_enum)]`, as proposed in [RFC 3107](rust-lang/rfcs#3107) and tracked in rust-lang#87517. In short, it permits you to `#[derive(Default)]` on `enum`s, indicating what the default should be by placing a `#[default]` attribute on the desired variant (which must be a unit variant in the interest of forward compatibility).

`@rustbot` label +S-waiting-on-review +T-lang
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Apr 14, 2022
…um, r=davidtwco

Stabilize `derive_default_enum`

This stabilizes `#![feature(derive_default_enum)]`, as proposed in [RFC 3107](rust-lang/rfcs#3107) and tracked in rust-lang#87517. In short, it permits you to `#[derive(Default)]` on `enum`s, indicating what the default should be by placing a `#[default]` attribute on the desired variant (which must be a unit variant in the interest of forward compatibility).

``@rustbot`` label +S-waiting-on-review +T-lang
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Apr 14, 2022
…um, r=davidtwco

Stabilize `derive_default_enum`

This stabilizes `#![feature(derive_default_enum)]`, as proposed in [RFC 3107](rust-lang/rfcs#3107) and tracked in rust-lang#87517. In short, it permits you to `#[derive(Default)]` on `enum`s, indicating what the default should be by placing a `#[default]` attribute on the desired variant (which must be a unit variant in the interest of forward compatibility).

```@rustbot``` label +S-waiting-on-review +T-lang
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Apr 14, 2022
…um, r=davidtwco

Stabilize `derive_default_enum`

This stabilizes `#![feature(derive_default_enum)]`, as proposed in [RFC 3107](rust-lang/rfcs#3107) and tracked in rust-lang#87517. In short, it permits you to `#[derive(Default)]` on `enum`s, indicating what the default should be by placing a `#[default]` attribute on the desired variant (which must be a unit variant in the interest of forward compatibility).

````@rustbot```` label +S-waiting-on-review +T-lang
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Apr 15, 2022
…um, r=davidtwco

Stabilize `derive_default_enum`

This stabilizes `#![feature(derive_default_enum)]`, as proposed in [RFC 3107](rust-lang/rfcs#3107) and tracked in rust-lang#87517. In short, it permits you to `#[derive(Default)]` on `enum`s, indicating what the default should be by placing a `#[default]` attribute on the desired variant (which must be a unit variant in the interest of forward compatibility).

`````@rustbot````` label +S-waiting-on-review +T-lang
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Apr 15, 2022
…um, r=davidtwco

Stabilize `derive_default_enum`

This stabilizes `#![feature(derive_default_enum)]`, as proposed in [RFC 3107](rust-lang/rfcs#3107) and tracked in rust-lang#87517. In short, it permits you to `#[derive(Default)]` on `enum`s, indicating what the default should be by placing a `#[default]` attribute on the desired variant (which must be a unit variant in the interest of forward compatibility).

``````@rustbot`````` label +S-waiting-on-review +T-lang
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Apr 15, 2022
…um, r=davidtwco

Stabilize `derive_default_enum`

This stabilizes `#![feature(derive_default_enum)]`, as proposed in [RFC 3107](rust-lang/rfcs#3107) and tracked in rust-lang#87517. In short, it permits you to `#[derive(Default)]` on `enum`s, indicating what the default should be by placing a `#[default]` attribute on the desired variant (which must be a unit variant in the interest of forward compatibility).

```````@rustbot``````` label +S-waiting-on-review +T-lang
workingjubilee pushed a commit to tcdi/postgrestd that referenced this pull request Sep 15, 2022
…idtwco

Stabilize `derive_default_enum`

This stabilizes `#![feature(derive_default_enum)]`, as proposed in [RFC 3107](rust-lang/rfcs#3107) and tracked in #87517. In short, it permits you to `#[derive(Default)]` on `enum`s, indicating what the default should be by placing a `#[default]` attribute on the desired variant (which must be a unit variant in the interest of forward compatibility).

```````@rustbot``````` label +S-waiting-on-review +T-lang
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-derive Deriving related proposals & ideas A-enum Enum related proposals & ideas disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this RFC. T-lang Relevant to the language team, which will review and decide on the RFC. T-libs-api Relevant to the library API team, which will review and decide on the RFC. to-announce
Projects
None yet
Development

Successfully merging this pull request may close these issues.