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

TryFromPrimitive incorrectly uses default attribute #75

Closed
ollie-etl opened this issue Jun 22, 2022 · 2 comments · Fixed by #110
Closed

TryFromPrimitive incorrectly uses default attribute #75

ollie-etl opened this issue Jun 22, 2022 · 2 comments · Fixed by #110

Comments

@ollie-etl
Copy link

If we have an enum where a #[default] is applied for another derive, TryFromPrimitive becomes useless, and will silently return the default. This is both undocumented, and makes no sense - surely the point of TryFromPrimitive is to test the input is valid. If the default is required, FromPrimitive has this behaviour documented.

#[derive(
    Debug,
    TryFromPrimitive,
)]
pub enum MyEnum {
    #[default]
    E1 = 1,
    E2 = 2,
}

assert_eq!(MyEnum::try_from(42).is_ok(),  "This passes!!!!")

Closely related to #31, however the fix there cannot be applied, because the attribute is actually required for another macro.

@cgbur
Copy link

cgbur commented Mar 7, 2023

The issue is that there is documented in the readme #[num_enum(default)] but this is also applying to #[default] from core. This is a footgun that should be fixed. Is the crate author open to working on a fix for this?

@illicitonion
Copy link
Owner

Sorry for the super slow response here - yes, I agree that TryFromPrimitive should probably ignore default attributes completely, and only FromPrimitive should pay attention to it. This will, however, be a breaking change, so we should probably bump to 0.6.0 with this change. I'll put together a PR soon.

illicitonion added a commit that referenced this issue Mar 18, 2023
If exhaustive behaviour is desired, FromPrimitive should be derived
instead.

Because these traits are exclusive, there's no room for ambiguity here -
either TryFromPrimitive is derived and defaults are ignored, or
FromPrimitive is derived and defaults are paid attention to.

This is, however, a breaking change, as it changes the implementation of
TryFromPrimitive significantly.

Fixes #75
illicitonion added a commit that referenced this issue Mar 18, 2023
If exhaustive behaviour is desired, FromPrimitive should be derived
instead.

Because these traits are exclusive, there's no room for ambiguity here -
either TryFromPrimitive is derived and defaults are ignored, or
FromPrimitive is derived and defaults are paid attention to.

This is, however, a breaking change, as it changes the implementation of
TryFromPrimitive significantly.

Fixes #75
Fixes #31
illicitonion added a commit that referenced this issue Mar 18, 2023
If exhaustive behaviour is desired, FromPrimitive should be derived
instead.

Because these traits are exclusive, there's no room for ambiguity here -
either TryFromPrimitive is derived and defaults are ignored, or
FromPrimitive is derived and defaults are paid attention to.

This is, however, a breaking change, as it changes the implementation of
TryFromPrimitive significantly.

Fixes #75
Fixes #31
illicitonion added a commit that referenced this issue Mar 18, 2023
If exhaustive behaviour is desired, FromPrimitive should be derived
instead.

Because these traits are exclusive, there's no room for ambiguity here -
either TryFromPrimitive is derived and defaults are ignored, or
FromPrimitive is derived and defaults are paid attention to.

This is, however, a breaking change, as it changes the implementation of
TryFromPrimitive significantly.

Fixes #75
Fixes #31
illicitonion added a commit that referenced this issue Mar 18, 2023
If exhaustive behaviour is desired, FromPrimitive should be derived
instead.

Because these traits are exclusive, there's no room for ambiguity here -
either TryFromPrimitive is derived and defaults are ignored, or
FromPrimitive is derived and defaults are paid attention to.

This is, however, a breaking change, as it changes the implementation of
TryFromPrimitive significantly.

Fixes #75
Fixes #31
wprzytula added a commit to wprzytula/scylla-rust-driver that referenced this issue Jul 26, 2023
As the awaited issue in `num_enum` crate
(illicitonion/num_enum#75) has been resolved
in its 0.6 release, it is now possible to derive Default for Consistency
automatically.
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