From 0e2d006daaf156fb60d1cc677a52d09186e48b86 Mon Sep 17 00:00:00 2001 From: Daniel Wagner-Hall Date: Mon, 13 Mar 2023 23:28:16 +0000 Subject: [PATCH] TryFromPrimitive ignores default attributes 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 --- README.md | 20 +++++---- num_enum/src/lib.rs | 10 +++++ num_enum/tests/from_primitive.rs | 12 +++-- .../compile_fail/conflicting_derive.stderr | 2 +- num_enum/tests/try_from_primitive.rs | 10 ++++- num_enum_derive/src/lib.rs | 44 ++++--------------- 6 files changed, 43 insertions(+), 55 deletions(-) diff --git a/README.md b/README.md index 902cee7..9b2f645 100644 --- a/README.md +++ b/README.md @@ -127,15 +127,15 @@ Default variant Sometimes it is desirable to have an `Other` variant in an enum that acts as a kind of a wildcard matching all the value not yet covered by other variants. -The `#[num_enum(default)]` attribute allows you to mark variant as the default. +The `#[num_enum(default)]` attribute (or the stdlib `#[default]` attribute) allows you to mark variant as the default. (The behavior of `IntoPrimitive` is unaffected by this attribute, it will always return the canonical value.) ```rust -use num_enum::TryFromPrimitive; +use num_enum::FromPrimitive; use std::convert::TryFrom; -#[derive(Debug, Eq, PartialEq, TryFromPrimitive)] +#[derive(Debug, Eq, PartialEq, FromPrimitive)] #[repr(u8)] enum Number { Zero = 0, @@ -144,17 +144,19 @@ enum Number { } fn main() { - let zero = Number::try_from(0u8); - assert_eq!(zero, Ok(Number::Zero)); + let zero = Number::from(0u8); + assert_eq!(zero, Number::Zero); - let one = Number::try_from(1u8); - assert_eq!(one, Ok(Number::NonZero)); + let one = Number::from(1u8); + assert_eq!(one, Number::NonZero); - let two = Number::try_from(2u8); - assert_eq!(two, Ok(Number::NonZero)); + let two = Number::from(2u8); + assert_eq!(two, Number::NonZero); } ``` +Only `FromPrimitive` pays attention to `default` attributes, `TryFromPrimitive` ignores them. + Safely turning a primitive into an exhaustive enum with from_primitive ------------------------------------------------------------- diff --git a/num_enum/src/lib.rs b/num_enum/src/lib.rs index e349c52..cb71f29 100644 --- a/num_enum/src/lib.rs +++ b/num_enum/src/lib.rs @@ -65,3 +65,13 @@ impl fmt::Display for TryFromPrimitiveError { #[cfg(feature = "std")] impl ::std::error::Error for TryFromPrimitiveError {} + +// This trait exists to try to give a more clear error message when someone attempts to derive both FromPrimitive and TryFromPrimitive. +// This isn't allowed because both end up creating a `TryFrom` implementation. +// TryFromPrimitive explicitly implements TryFrom with Error=TryFromPrimitiveError, which conflicts with: +// FromPrimitive explicitly implements From which has a blanket implementation of TryFrom with Error=Infallible. +// +// This is a private implementation detail of the num_enum crate which should not be depended on externally. +// It is subject to change in any release regardless of semver. +#[doc(hidden)] +pub trait CannotDeriveBothFromPrimitiveAndTryFromPrimitive {} diff --git a/num_enum/tests/from_primitive.rs b/num_enum/tests/from_primitive.rs index 1be33fc..5f75eb5 100644 --- a/num_enum/tests/from_primitive.rs +++ b/num_enum/tests/from_primitive.rs @@ -1,6 +1,4 @@ -use ::std::convert::TryFrom; - -use ::num_enum::{FromPrimitive, TryFromPrimitive}; +use ::num_enum::FromPrimitive; // Guard against https://github.com/illicitonion/num_enum/issues/27 mod alloc {} @@ -89,11 +87,11 @@ fn from_primitive_number() { let from = Enum::from(0_u8); assert_eq!(from, Enum::Whatever); - let try_from_primitive = Enum::try_from_primitive(0_u8); - assert_eq!(try_from_primitive, Ok(Enum::Whatever)); + let from_primitive = Enum::from_primitive(1_u8); + assert_eq!(from_primitive, Enum::Whatever); - let try_from = Enum::try_from(0_u8); - assert_eq!(try_from, Ok(Enum::Whatever)); + let from = Enum::from(1_u8); + assert_eq!(from, Enum::Whatever); } #[test] diff --git a/num_enum/tests/try_build/compile_fail/conflicting_derive.stderr b/num_enum/tests/try_build/compile_fail/conflicting_derive.stderr index 679eb06..6ef3a22 100644 --- a/num_enum/tests/try_build/compile_fail/conflicting_derive.stderr +++ b/num_enum/tests/try_build/compile_fail/conflicting_derive.stderr @@ -1,4 +1,4 @@ -error[E0119]: conflicting implementations of trait `TryFromPrimitive` for type `Numbers` +error[E0119]: conflicting implementations of trait `CannotDeriveBothFromPrimitiveAndTryFromPrimitive` for type `Numbers` --> tests/try_build/compile_fail/conflicting_derive.rs:1:35 | 1 | #[derive(num_enum::FromPrimitive, num_enum::TryFromPrimitive)] diff --git a/num_enum/tests/try_from_primitive.rs b/num_enum/tests/try_from_primitive.rs index e6d19f6..b6454be 100644 --- a/num_enum/tests/try_from_primitive.rs +++ b/num_enum/tests/try_from_primitive.rs @@ -440,7 +440,10 @@ fn default_value() { assert_eq!(two, Ok(Enum::Other)); let max_value: Result = u8::max_value().try_into(); - assert_eq!(max_value, Ok(Enum::Other)); + assert_eq!( + max_value.unwrap_err().to_string(), + "No discriminant in enum `Enum` matches the value `255`" + ); } #[test] @@ -472,7 +475,10 @@ fn alternative_values_and_default_value() { assert_eq!(four, Ok(Enum::Four)); let five: Result = 5u8.try_into(); - assert_eq!(five, Ok(Enum::Zero)); + assert_eq!( + five.unwrap_err().to_string(), + "No discriminant in enum `Enum` matches the value `5`" + ); } #[test] diff --git a/num_enum_derive/src/lib.rs b/num_enum_derive/src/lib.rs index 600730f..b7ffc2e 100644 --- a/num_enum_derive/src/lib.rs +++ b/num_enum_derive/src/lib.rs @@ -785,24 +785,8 @@ pub fn derive_from_primitive(input: TokenStream) -> TokenStream { } } - // The Rust stdlib will implement `#name: From<#repr>` for us for free! - - impl ::#krate::TryFromPrimitive for #name { - type Primitive = #repr; - - const NAME: &'static str = stringify!(#name); - - #[inline] - fn try_from_primitive ( - number: Self::Primitive, - ) -> ::core::result::Result< - Self, - ::#krate::TryFromPrimitiveError, - > - { - Ok(::#krate::FromPrimitive::from_primitive(number)) - } - } + #[doc(hidden)] + impl ::#krate::CannotDeriveBothFromPrimitiveAndTryFromPrimitive for #name {} }) } @@ -846,23 +830,6 @@ pub fn derive_try_from_primitive(input: TokenStream) -> TokenStream { debug_assert_eq!(variant_idents.len(), variant_expressions.len()); - let default_arm = match enum_info.default() { - Some(ident) => { - quote! { - _ => ::core::result::Result::Ok( - #name::#ident - ) - } - } - None => { - quote! { - _ => ::core::result::Result::Err( - ::#krate::TryFromPrimitiveError { number } - ) - } - } - }; - TokenStream::from(quote! { impl ::#krate::TryFromPrimitive for #name { type Primitive = #repr; @@ -890,7 +857,9 @@ pub fn derive_try_from_primitive(input: TokenStream) -> TokenStream { => ::core::result::Result::Ok(Self::#variant_idents), )* #[allow(unreachable_patterns)] - #default_arm, + _ => ::core::result::Result::Err( + ::#krate::TryFromPrimitiveError { number } + ), } } } @@ -906,6 +875,9 @@ pub fn derive_try_from_primitive(input: TokenStream) -> TokenStream { ::#krate::TryFromPrimitive::try_from_primitive(number) } } + + #[doc(hidden)] + impl ::#krate::CannotDeriveBothFromPrimitiveAndTryFromPrimitive for #name {} }) }