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

How aboyt impl Default when use #[num_enum(default)]? #56

Closed
rise0chen opened this issue Aug 13, 2021 · 5 comments · Fixed by #57
Closed

How aboyt impl Default when use #[num_enum(default)]? #56

rise0chen opened this issue Aug 13, 2021 · 5 comments · Fixed by #57

Comments

@rise0chen
Copy link

How aboyt auto impl Default when use #[num_enum(default)] of FromPrimitive?

@illicitonion
Copy link
Owner

I can see that this would be useful, but I'm slightly concerned that automatically deriving a standard trait may lead to conflicting implementations. Deriving Default for enums just landed on nightly - there's a tracking issue here: rust-lang/rust#87517 which makes this code work on nightly:

#![feature(derive_default_enum)]

#[derive(Default)]
enum Number {
    Zero,
    #[default]
    One,
}

It would be a shame if this code:

#![feature(derive_default_enum)]

#[derive(Default, num_enum::FromPrimitive)]
enum Number {
    Zero,
    #[default]
    #[num_enum(default)]
    One,
}

generated two conflicting implementations of Default. And I don't think num_enum can detect whether there's already another implementation of Default and choose not to implement one.

What do you think about waiting for Default-deriving to stabilise?

Alternatively, we could add an explicit #[derive(num_enum::Default)] so that someone has to opt in to deriving a Default impl.

I definitely think that we should make the #[default] attribute an alias for the #[num_enum(default)] attribute when this feature stabilises, though - using things from the standard library where possible is a good thing.

@danielhenrymantilla
Copy link
Collaborator

danielhenrymantilla commented Aug 13, 2021

@illicitonion I believe that we could emit the impl Default spanned at #[num_enum(default)], so that the user would see a nice conflicting impls error (pseudo example); I don't think it would be a surprising or a confusing error 🙂

@illicitonion
Copy link
Owner

I agree that we could give a clear error message - my concern is more around what the user can do to resolve the error.

Generally I think people should prefer using std to crates, if identical functionality is supported by both. But if we're automatically deriving Default, the only response the user can take to this error is to stop using the std Default derive.

I could get more on-board with a num_enum::Default derive as a stop-gap until the std behaviour stabilises:

#[derive(num_enum::Default, num_enum::FromPrimitive)]
enum Number {
    Zero,
    #[num_enum(default)]
    One,
}

so that if someone wrote:

#[derive(Default)]
#[derive(num_enum::Default, num_enum::FromPrimitive)]
enum Number {
    Zero,
    #[num_enum(default)]
    One,
}

they could choose to remove the num_enum::Default derive. It's the "replicating std as an implicit side-effect" I find problematic :)

@danielhenrymantilla
Copy link
Collaborator

Agreed, I somehow thought #[num_enum(default)] was just for this impl, and forgot it was also for something else in num_enum. 💯 we shouldn't conflate two functionalities if one of those is susceptible to collide with std's derive.

@illicitonion
Copy link
Owner

I just released 0.5.4 which includes a #[derive(num_enum::Default)] macro - it will use whichever variant is tagged either #[num_enum(default)] or #[default] - hopefully this is useful to you!

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 a pull request may close this issue.

3 participants