Github `#[derive(Default)]` on enums with a `#[default]` attribute by jhpratt ·...

Copy link

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

jhpratt commented 16 days ago

@rustbot modify labels to +A-derive +A-enum +T-lang

(not sure on whether this should be lang or libs)

rustbot commented 16 days ago

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.

clarfonthey commented 13 days ago •

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 commented 13 days ago

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.

This comment was marked as off-topic.

kpreid commented 12 days ago

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 commented 12 days ago

#[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 commented 12 days ago

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 commented 12 days ago

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 commented 12 days ago

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

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 commented 12 days ago

Changed to write it out.

clarfonthey commented 8 days ago

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 commented 8 days ago

@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 commented 8 days ago •

@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 commented 8 days ago

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 commented 8 days ago •

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 commented 8 days ago

@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 commented 8 days ago •

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 commented 8 days ago

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 commented 7 days ago •

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 commented 6 days ago

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.

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