Skip to content

Commit

Permalink
UnsafeFromPrimitive ignores default/alternatives (#112)
Browse files Browse the repository at this point in the history
* 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...

* Deprecate from_unchecked, and rename it unchecked_transmute_from

---------

Co-authored-by: Daniel Henry-Mantilla <daniel.henry.mantilla@gmail.com>
  • Loading branch information
illicitonion and danielhenrymantilla authored Mar 25, 2023
1 parent 9b4642e commit e02abdc
Show file tree
Hide file tree
Showing 11 changed files with 145 additions and 104 deletions.
30 changes: 25 additions & 5 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -224,14 +224,34 @@ 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.
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`.

- 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::unchecked_transmute_from(2) };
```

```rust
use num_enum::UnsafeFromPrimitive;

Expand All @@ -244,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!
}
```

Expand Down
15 changes: 14 additions & 1 deletion num_enum/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<Enum: TryFromPrimitive> {
Expand Down
Original file line number Diff line number Diff line change
@@ -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);
}
}
Original file line number Diff line number Diff line change
@@ -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);
| ^^^^^^^^^^^^^^
11 changes: 0 additions & 11 deletions num_enum/tests/try_build/compile_fail/unpexpected_alternatives.rs

This file was deleted.

This file was deleted.

11 changes: 0 additions & 11 deletions num_enum/tests/try_build/compile_fail/unpexpected_default.rs

This file was deleted.

This file was deleted.

60 changes: 60 additions & 0 deletions num_enum/tests/unsafe_from_primitive.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,68 @@ fn has_unsafe_from_primitive_number() {
One,
}

unsafe {
assert_eq!(Enum::unchecked_transmute_from(0_u8), Enum::Zero);
assert_eq!(Enum::unchecked_transmute_from(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::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);
}
}

#[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::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);
}
}
Loading

0 comments on commit e02abdc

Please sign in to comment.