From a1514101449529fde46525110ac443d53c7b0d15 Mon Sep 17 00:00:00 2001 From: Daniel Wagner-Hall Date: Sun, 19 Mar 2023 10:01:15 +0000 Subject: [PATCH 1/3] UnsafeFromPrimitive ignores default/alternatives Previously it would fail to derive, but there are valid use-cases for wanting to perform the unsafe conversion for known "natural" values. This potentially opens up a bit of confusion around what the behaviours are when these attributes are present but ignored, but these are also documented, and if someone is reaching for this unsafe function rather than using `{,Try}FromPrimitive` or just an `as` cast, presumably they're paying attention... --- README.md | 3 + .../compile_fail/unpexpected_alternatives.rs | 11 ---- .../unpexpected_alternatives.stderr | 5 -- .../compile_fail/unpexpected_default.rs | 11 ---- .../compile_fail/unpexpected_default.stderr | 5 -- num_enum/tests/unsafe_from_primitive.rs | 44 ++++++++++++++ num_enum_derive/src/lib.rs | 60 +------------------ 7 files changed, 50 insertions(+), 89 deletions(-) delete mode 100644 num_enum/tests/try_build/compile_fail/unpexpected_alternatives.rs delete mode 100644 num_enum/tests/try_build/compile_fail/unpexpected_alternatives.stderr delete mode 100644 num_enum/tests/try_build/compile_fail/unpexpected_default.rs delete mode 100644 num_enum/tests/try_build/compile_fail/unpexpected_default.stderr diff --git a/README.md b/README.md index 9b2f645..7af57a1 100644 --- a/README.md +++ b/README.md @@ -232,6 +232,9 @@ for any of its variants), and want to avoid a small amount of overhead, you can Unless you have data showing that the match statement generated in the `try_from` above is a bottleneck for you, you should avoid doing this, as the unsafe code has potential to cause serious memory issues in your program. +Note that this derive ignores any `default`, `catch_all`, and `alternatives` attributes on the enum. +If you need support for conversions from these values, you should use `TryFromPrimitive` or `FromPrimitive`. + ```rust use num_enum::UnsafeFromPrimitive; diff --git a/num_enum/tests/try_build/compile_fail/unpexpected_alternatives.rs b/num_enum/tests/try_build/compile_fail/unpexpected_alternatives.rs deleted file mode 100644 index 4dbdabb..0000000 --- a/num_enum/tests/try_build/compile_fail/unpexpected_alternatives.rs +++ /dev/null @@ -1,11 +0,0 @@ -#[derive(num_enum::UnsafeFromPrimitive)] -#[repr(u8)] -enum Numbers { - Zero, - #[num_enum(alternatives = [2])] - One, -} - -fn main() { - -} diff --git a/num_enum/tests/try_build/compile_fail/unpexpected_alternatives.stderr b/num_enum/tests/try_build/compile_fail/unpexpected_alternatives.stderr deleted file mode 100644 index 7aac45c..0000000 --- a/num_enum/tests/try_build/compile_fail/unpexpected_alternatives.stderr +++ /dev/null @@ -1,5 +0,0 @@ -error: #[derive(UnsafeFromPrimitive)] does not support `#[num_enum(alternatives = [..])]` - --> $DIR/unpexpected_alternatives.rs:5:16 - | -5 | #[num_enum(alternatives = [2])] - | ^^^^^^^^^^^^ diff --git a/num_enum/tests/try_build/compile_fail/unpexpected_default.rs b/num_enum/tests/try_build/compile_fail/unpexpected_default.rs deleted file mode 100644 index ffb1a0e..0000000 --- a/num_enum/tests/try_build/compile_fail/unpexpected_default.rs +++ /dev/null @@ -1,11 +0,0 @@ -#[derive(num_enum::UnsafeFromPrimitive)] -#[repr(u8)] -enum Numbers { - Zero, - #[num_enum(default)] - NoneZero, -} - -fn main() { - -} diff --git a/num_enum/tests/try_build/compile_fail/unpexpected_default.stderr b/num_enum/tests/try_build/compile_fail/unpexpected_default.stderr deleted file mode 100644 index c4127cb..0000000 --- a/num_enum/tests/try_build/compile_fail/unpexpected_default.stderr +++ /dev/null @@ -1,5 +0,0 @@ -error: #[derive(UnsafeFromPrimitive)] does not support `#[num_enum(default)]` - --> $DIR/unpexpected_default.rs:5:16 - | -5 | #[num_enum(default)] - | ^^^^^^^ diff --git a/num_enum/tests/unsafe_from_primitive.rs b/num_enum/tests/unsafe_from_primitive.rs index 79fb582..ab7cedc 100644 --- a/num_enum/tests/unsafe_from_primitive.rs +++ b/num_enum/tests/unsafe_from_primitive.rs @@ -20,3 +20,47 @@ fn has_unsafe_from_primitive_number() { assert_eq!(Enum::from_unchecked(1_u8), Enum::One); } } + +#[test] +fn has_unsafe_from_primitive_number_with_alternatives_and_default_which_are_ignored() { + #[derive(Debug, Eq, PartialEq, UnsafeFromPrimitive)] + #[repr(u8)] + enum Enum { + Zero, + One, + #[num_enum(alternatives = [3, 4])] + Some, + #[num_enum(default)] + Many = 5, + } + + unsafe { + assert_eq!(Enum::from_unchecked(0_u8), Enum::Zero); + assert_eq!(Enum::from_unchecked(1_u8), Enum::One); + assert_eq!(Enum::from_unchecked(2_u8), Enum::Some); + assert_eq!(Enum::from_unchecked(5_u8), Enum::Many); + // Any other conversions would be undefined behavior. + } +} + +#[test] +fn has_unsafe_from_primitive_number_with_alternatives_and_std_default_which_are_ignored() { + #[derive(Debug, Default, Eq, PartialEq, UnsafeFromPrimitive)] + #[repr(u8)] + enum Enum { + Zero, + One, + #[num_enum(alternatives = [3, 4])] + Some, + #[default] + Many = 5, + } + + unsafe { + assert_eq!(Enum::from_unchecked(0_u8), Enum::Zero); + assert_eq!(Enum::from_unchecked(1_u8), Enum::One); + assert_eq!(Enum::from_unchecked(2_u8), Enum::Some); + assert_eq!(Enum::from_unchecked(5_u8), Enum::Many); + // Any other conversions would be undefined behavior. + } +} diff --git a/num_enum_derive/src/lib.rs b/num_enum_derive/src/lib.rs index 5773bd7..feee813 100644 --- a/num_enum_derive/src/lib.rs +++ b/num_enum_derive/src/lib.rs @@ -227,16 +227,8 @@ impl Spanned for VariantAlternativesAttribute { } } -#[derive(::core::default::Default)] -struct AttributeSpans { - default: Vec, - catch_all: Vec, - alternatives: Vec, -} - struct VariantInfo { ident: Ident, - attr_spans: AttributeSpans, is_default: bool, is_catch_all: bool, canonical_value: Expr, @@ -247,10 +239,6 @@ impl VariantInfo { fn all_values(&self) -> impl Iterator { ::core::iter::once(&self.canonical_value).chain(self.alternative_values.iter()) } - - fn is_complex(&self) -> bool { - !self.alternative_values.is_empty() - } } struct EnumInfo { @@ -284,14 +272,6 @@ impl EnumInfo { die!(self.repr.clone() => "Failed to parse repr into bit size"); } - fn has_default_variant(&self) -> bool { - self.default().is_some() - } - - fn has_complex_variant(&self) -> bool { - self.variants.iter().any(|info| info.is_complex()) - } - fn default(&self) -> Option<&Ident> { self.variants .iter() @@ -306,18 +286,6 @@ impl EnumInfo { .map(|info| &info.ident) } - fn first_default_attr_span(&self) -> Option<&Span> { - self.variants - .iter() - .find_map(|info| info.attr_spans.default.first()) - } - - fn first_alternatives_attr_span(&self) -> Option<&Span> { - self.variants - .iter() - .find_map(|info| info.attr_spans.alternatives.first()) - } - fn variant_idents(&self) -> Vec { self.variants .iter() @@ -406,7 +374,6 @@ impl Parse for EnumInfo { None => next_discriminant.clone(), }; - let mut attr_spans: AttributeSpans = Default::default(); let mut raw_alternative_values: Vec = vec![]; // Keep the attribute around for better error reporting. let mut alt_attr_ref: Vec<&Attribute> = vec![]; @@ -428,7 +395,6 @@ impl Parse for EnumInfo { "Attribute `default` is mutually exclusive with `catch_all`" ); } - attr_spans.default.push(attribute.span()); is_default = true; has_default_variant = true; } @@ -448,7 +414,6 @@ impl Parse for EnumInfo { "Attribute `default` is mutually exclusive with `catch_all`" ); } - attr_spans.default.push(default.span()); is_default = true; has_default_variant = true; } @@ -473,7 +438,6 @@ impl Parse for EnumInfo { ty: syn::Type::Path(syn::TypePath { path, .. }), .. }] if path.is_ident(&repr) => { - attr_spans.catch_all.push(catch_all.span()); is_catch_all = true; has_catch_all_variant = true; } @@ -485,7 +449,6 @@ impl Parse for EnumInfo { } } NumEnumVariantAttributeItem::Alternatives(alternatives) => { - attr_spans.alternatives.push(alternatives.span()); raw_alternative_values.extend(alternatives.expressions); alt_attr_ref.push(attribute); } @@ -615,7 +578,6 @@ impl Parse for EnumInfo { variants.push(VariantInfo { ident, - attr_spans, is_default, is_catch_all, canonical_value: discriminant, @@ -916,6 +878,9 @@ fn get_crate_name() -> String { /// bottleneck for you, you should avoid doing this, as the unsafe code has potential to cause serious memory issues in /// your program. /// +/// Note that this derive ignores any `default`, `catch_all`, and `alternatives` attributes on the enum. +/// If you need support for conversions from these values, you should use `TryFromPrimitive` or `FromPrimitive`. +/// /// ```rust /// use num_enum::UnsafeFromPrimitive; /// @@ -946,25 +911,6 @@ pub fn derive_unsafe_from_primitive(stream: TokenStream) -> TokenStream { let enum_info = parse_macro_input!(stream as EnumInfo); let krate = Ident::new(&get_crate_name(), Span::call_site()); - if enum_info.has_default_variant() { - let span = enum_info - .first_default_attr_span() - .cloned() - .expect("Expected span"); - let message = "#[derive(UnsafeFromPrimitive)] does not support `#[num_enum(default)]`"; - return syn::Error::new(span, message).to_compile_error().into(); - } - - if enum_info.has_complex_variant() { - let span = enum_info - .first_alternatives_attr_span() - .cloned() - .expect("Expected span"); - let message = - "#[derive(UnsafeFromPrimitive)] does not support `#[num_enum(alternatives = [..])]`"; - return syn::Error::new(span, message).to_compile_error().into(); - } - let EnumInfo { ref name, ref repr, .. } = enum_info; From cf3e627a19e83e4ce258bf15209398bc6cf9d3f3 Mon Sep 17 00:00:00 2001 From: Daniel Wagner-Hall Date: Sun, 19 Mar 2023 23:19:13 +0000 Subject: [PATCH 2/3] Add example of UB Co-authored-by: Daniel Henry-Mantilla --- README.md | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/README.md b/README.md index 7af57a1..3c70090 100644 --- a/README.md +++ b/README.md @@ -235,6 +235,23 @@ you should avoid doing this, as the unsafe code has potential to cause serious m Note that this derive ignores any `default`, `catch_all`, and `alternatives` attributes on the enum. If you need support for conversions from these values, you should use `TryFromPrimitive` or `FromPrimitive`. + - This means, for instance, that the following is UB: + + ```rust,no_run + use num_enum::UnsafeFromPrimitive; + + #[derive(UnsafeFromPrimitive)] + #[repr(u8)] + enum Number { + Zero = 0, + + // Same for `#[num_enum(catch_all)]`, and `#[num_enum(alternatives = [2, ...])]` + #[num_enum(default)] + One = 1, + } + let _undefined_behavior = unsafe { Number::from_unchecked(2) }; + ``` + ```rust use num_enum::UnsafeFromPrimitive; From 5792a56176672150705e0647680d8686339c2ed3 Mon Sep 17 00:00:00 2001 From: Daniel Wagner-Hall Date: Sun, 19 Mar 2023 23:49:04 +0000 Subject: [PATCH 3/3] Deprecate from_unchecked, and rename it unchecked_transmute_from --- README.md | 12 +++++----- num_enum/src/lib.rs | 15 +++++++++++- .../from_unchecked_deprecated_warning.rs | 17 +++++++++++++ .../from_unchecked_deprecated_warning.stderr | 17 +++++++++++++ num_enum/tests/unsafe_from_primitive.rs | 24 +++++++++++++++---- num_enum_derive/src/lib.rs | 14 +++++------ renamed_num_enum/src/lib.rs | 4 ++-- 7 files changed, 83 insertions(+), 20 deletions(-) create mode 100644 num_enum/tests/try_build/compile_fail/from_unchecked_deprecated_warning.rs create mode 100644 num_enum/tests/try_build/compile_fail/from_unchecked_deprecated_warning.stderr diff --git a/README.md b/README.md index 3c70090..5442ec3 100644 --- a/README.md +++ b/README.md @@ -224,8 +224,8 @@ fn main() { As this is naturally exhaustive, this is only supported for `FromPrimitive`, not also `TryFromPrimitive`. -Unsafely turning a primitive into an enum with from_unchecked -------------------------------------------------------------- +Unsafely turning a primitive into an enum with unchecked_transmute_from +----------------------------------------------------------------------- If you're really certain a conversion will succeed (and have not made use of `#[num_enum(default)]` or `#[num_enum(alternatives = [..])]` for any of its variants), and want to avoid a small amount of overhead, you can use unsafe code to do this conversion. @@ -249,7 +249,7 @@ If you need support for conversions from these values, you should use `TryFromPr #[num_enum(default)] One = 1, } - let _undefined_behavior = unsafe { Number::from_unchecked(2) }; + let _undefined_behavior = unsafe { Number::unchecked_transmute_from(2) }; ``` ```rust @@ -264,17 +264,17 @@ enum Number { fn main() { assert_eq!( - unsafe { Number::from_unchecked(0_u8) }, + unsafe { Number::unchecked_transmute_from(0_u8) }, Number::Zero, ); assert_eq!( - unsafe { Number::from_unchecked(1_u8) }, + unsafe { Number::unchecked_transmute_from(1_u8) }, Number::One, ); } unsafe fn undefined_behavior() { - let _ = Number::from_unchecked(2); // 2 is not a valid discriminant! + let _ = Number::unchecked_transmute_from(2); // 2 is not a valid discriminant! } ``` diff --git a/num_enum/src/lib.rs b/num_enum/src/lib.rs index bccb08e..9d4161e 100644 --- a/num_enum/src/lib.rs +++ b/num_enum/src/lib.rs @@ -35,7 +35,20 @@ pub trait UnsafeFromPrimitive: Sized { /// # Safety /// /// - `number` must represent a valid discriminant of `Self`. - unsafe fn from_unchecked(number: Self::Primitive) -> Self; + #[deprecated( + since = "0.6.0", + note = "Prefer to use `unchecked_transmute_from`, `from_unchecked` will be removed in a future release." + )] + unsafe fn from_unchecked(number: Self::Primitive) -> Self { + Self::unchecked_transmute_from(number) + } + + /// Transmutes into an enum from its primitive. + /// + /// # Safety + /// + /// - `number` must represent a valid discriminant of `Self`. + unsafe fn unchecked_transmute_from(number: Self::Primitive) -> Self; } pub struct TryFromPrimitiveError { diff --git a/num_enum/tests/try_build/compile_fail/from_unchecked_deprecated_warning.rs b/num_enum/tests/try_build/compile_fail/from_unchecked_deprecated_warning.rs new file mode 100644 index 0000000..6e65827 --- /dev/null +++ b/num_enum/tests/try_build/compile_fail/from_unchecked_deprecated_warning.rs @@ -0,0 +1,17 @@ +#![deny(deprecated)] + +use num_enum::UnsafeFromPrimitive; + +#[derive(Debug, Eq, PartialEq, UnsafeFromPrimitive)] +#[repr(u8)] +enum Enum { + Zero, + One, +} + +fn main() { + unsafe { + assert_eq!(Enum::from_unchecked(0_u8), Enum::Zero); + assert_eq!(Enum::from_unchecked(1_u8), Enum::One); + } +} diff --git a/num_enum/tests/try_build/compile_fail/from_unchecked_deprecated_warning.stderr b/num_enum/tests/try_build/compile_fail/from_unchecked_deprecated_warning.stderr new file mode 100644 index 0000000..0c450ed --- /dev/null +++ b/num_enum/tests/try_build/compile_fail/from_unchecked_deprecated_warning.stderr @@ -0,0 +1,17 @@ +error: use of deprecated associated function `num_enum::UnsafeFromPrimitive::from_unchecked`: Prefer to use `unchecked_transmute_from`, `from_unchecked` will be removed in a future release. + --> tests/try_build/compile_fail/from_unchecked_deprecated_warning.rs:14:26 + | +14 | assert_eq!(Enum::from_unchecked(0_u8), Enum::Zero); + | ^^^^^^^^^^^^^^ + | +note: the lint level is defined here + --> tests/try_build/compile_fail/from_unchecked_deprecated_warning.rs:1:9 + | +1 | #![deny(deprecated)] + | ^^^^^^^^^^ + +error: use of deprecated associated function `num_enum::UnsafeFromPrimitive::from_unchecked`: Prefer to use `unchecked_transmute_from`, `from_unchecked` will be removed in a future release. + --> tests/try_build/compile_fail/from_unchecked_deprecated_warning.rs:15:26 + | +15 | assert_eq!(Enum::from_unchecked(1_u8), Enum::One); + | ^^^^^^^^^^^^^^ diff --git a/num_enum/tests/unsafe_from_primitive.rs b/num_enum/tests/unsafe_from_primitive.rs index ab7cedc..e9c72f0 100644 --- a/num_enum/tests/unsafe_from_primitive.rs +++ b/num_enum/tests/unsafe_from_primitive.rs @@ -16,8 +16,8 @@ fn has_unsafe_from_primitive_number() { } unsafe { - assert_eq!(Enum::from_unchecked(0_u8), Enum::Zero); - assert_eq!(Enum::from_unchecked(1_u8), Enum::One); + assert_eq!(Enum::unchecked_transmute_from(0_u8), Enum::Zero); + assert_eq!(Enum::unchecked_transmute_from(1_u8), Enum::One); } } @@ -34,12 +34,20 @@ fn has_unsafe_from_primitive_number_with_alternatives_and_default_which_are_igno Many = 5, } + unsafe { + assert_eq!(Enum::unchecked_transmute_from(0_u8), Enum::Zero); + assert_eq!(Enum::unchecked_transmute_from(1_u8), Enum::One); + assert_eq!(Enum::unchecked_transmute_from(2_u8), Enum::Some); + assert_eq!(Enum::unchecked_transmute_from(5_u8), Enum::Many); + // Any other conversions would be undefined behavior. + } + + #[allow(deprecated)] unsafe { assert_eq!(Enum::from_unchecked(0_u8), Enum::Zero); assert_eq!(Enum::from_unchecked(1_u8), Enum::One); assert_eq!(Enum::from_unchecked(2_u8), Enum::Some); assert_eq!(Enum::from_unchecked(5_u8), Enum::Many); - // Any other conversions would be undefined behavior. } } @@ -56,11 +64,19 @@ fn has_unsafe_from_primitive_number_with_alternatives_and_std_default_which_are_ Many = 5, } + unsafe { + assert_eq!(Enum::unchecked_transmute_from(0_u8), Enum::Zero); + assert_eq!(Enum::unchecked_transmute_from(1_u8), Enum::One); + assert_eq!(Enum::unchecked_transmute_from(2_u8), Enum::Some); + assert_eq!(Enum::unchecked_transmute_from(5_u8), Enum::Many); + // Any other conversions would be undefined behavior. + } + + #[allow(deprecated)] unsafe { assert_eq!(Enum::from_unchecked(0_u8), Enum::Zero); assert_eq!(Enum::from_unchecked(1_u8), Enum::One); assert_eq!(Enum::from_unchecked(2_u8), Enum::Some); assert_eq!(Enum::from_unchecked(5_u8), Enum::Many); - // Any other conversions would be undefined behavior. } } diff --git a/num_enum_derive/src/lib.rs b/num_enum_derive/src/lib.rs index feee813..c4e22fa 100644 --- a/num_enum_derive/src/lib.rs +++ b/num_enum_derive/src/lib.rs @@ -867,11 +867,11 @@ fn get_crate_name() -> String { String::from("num_enum") } -/// Generates a `unsafe fn from_unchecked (number: Primitive) -> Self` +/// Generates a `unsafe fn unchecked_transmute_from(number: Primitive) -> Self` /// associated function. /// -/// Allows unsafely turning a primitive into an enum with from_unchecked. -/// ------------------------------------------------------------- +/// Allows unsafely turning a primitive into an enum with unchecked_transmute_from +/// ------------------------------------------------------------------------------ /// /// If you're really certain a conversion will succeed, and want to avoid a small amount of overhead, you can use unsafe /// code to do this conversion. Unless you have data showing that the match statement generated in the `try_from` above is a @@ -894,16 +894,16 @@ fn get_crate_name() -> String { /// fn main() { /// assert_eq!( /// Number::Zero, -/// unsafe { Number::from_unchecked(0_u8) }, +/// unsafe { Number::unchecked_transmute_from(0_u8) }, /// ); /// assert_eq!( /// Number::One, -/// unsafe { Number::from_unchecked(1_u8) }, +/// unsafe { Number::unchecked_transmute_from(1_u8) }, /// ); /// } /// /// unsafe fn undefined_behavior() { -/// let _ = Number::from_unchecked(2); // 2 is not a valid discriminant! +/// let _ = Number::unchecked_transmute_from(2); // 2 is not a valid discriminant! /// } /// ``` #[proc_macro_derive(UnsafeFromPrimitive, attributes(num_enum))] @@ -919,7 +919,7 @@ pub fn derive_unsafe_from_primitive(stream: TokenStream) -> TokenStream { impl ::#krate::UnsafeFromPrimitive for #name { type Primitive = #repr; - unsafe fn from_unchecked(number: Self::Primitive) -> Self { + unsafe fn unchecked_transmute_from(number: Self::Primitive) -> Self { ::core::mem::transmute(number) } } diff --git a/renamed_num_enum/src/lib.rs b/renamed_num_enum/src/lib.rs index 43bb4e0..d751342 100644 --- a/renamed_num_enum/src/lib.rs +++ b/renamed_num_enum/src/lib.rs @@ -48,8 +48,8 @@ mod submodule { "No discriminant in enum `Number` matches the value `2`", ); - assert_eq!(unsafe { Number::from_unchecked(0) }, Number::Zero); + assert_eq!(unsafe { Number::unchecked_transmute_from(0) }, Number::Zero); - assert_eq!(unsafe { Number::from_unchecked(1) }, Number::One); + assert_eq!(unsafe { Number::unchecked_transmute_from(1) }, Number::One); } }