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

feature: const variants of into, from, try_from, and default #147

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

cosmicexplorer
Copy link

@cosmicexplorer cosmicexplorer commented May 3, 2024

Motivation

num_enum is very effective for manipulating data representations at runtime. However, there are some cases where we might want this functionality in const contexts, such as when generating static values or composing num_enum types into other Copy structs which we also want to manipulate in const contexts.

Alternatives

  • Note that while I have wanted this feature for both https://github.com/signalapp/libsignal and https://github.com/zip-rs/zip2, I cannot say that it is truly necessary, just convenient.
  • Certain nightly features such as const_trait_impl and effects are able to avoid the need for parts of this, by making From/Into/Default impls const-compatible, so for my other work that uses nightly, I actually don't require these at all!
    • However, I don't believe those features are expected to be stabilized soon, and I think it makes sense to have explicit/separate const and non-const methods for num_enum derives until Rust has developed a more formal mechanism for manipulating const effects.

Implementation

Four new derive macros were added, along with auxiliary traits/structs in lib.rs as needed. Since traits can't define const fns, all of these instead generate a method on the enum directly:

  • ConstIntoPrimitive: const_into(self) -> #repr
  • ConstFromPrimitive: const_from(#repr) -> Self
  • ConstTryFromPrimitive: const_try_from(#repr) -> Result<Self, ConstTryFromPrimitiveError<Self>>
  • ConstDefault: const_default() -> Self
#[derive(num_enum::ConstIntoPrimitive)]
#[repr(u8)]
pub enum E {
  Zero = 0,
  One = 1,
}

const e: u8 = E::Zero.const_into();
assert_eq!(e, 0);

Additionally, the #[num_enum(method_names(...)] attribute was added to the parser, to override the names of generated const methods. This is done because unlike the non-const derives, we do not have a canonical trait to implement, so we risk stomping on users' method names:

#[derive(num_enum::ConstIntoPrimitive)]
#[num_enum(method_names(const_into = f))]
#[repr(u8)]
pub enum E {
  Zero = 0,
  One = 1,
}

const e: u8 = E::Zero.f();
assert_eq!(e, 0);

Result

The boilerplate that num_enum generates no longer has be written by hand when using a num_enum type in const contexts!

@cosmicexplorer
Copy link
Author

The method_names() override functionality was added after the other work to just generate the const functions, and method name overrides can be moved into a separate PR if that would make it easier to review.

Copy link
Owner

@illicitonion illicitonion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! (Also, hi! 👋)

Philosophically, I think this change totally makes sense - as you noted in the description const_trait_impl may make this obsolete in the future, but we're not in that future :) I'm also hoping this entire crate becomes obsolete in the future by pulling this functionality into stdlib, but that's not where we are!

I've left a couple of suggestions, I hope they're both relatively minor, but feel free to push back on them if you think they're not good changes.

On the method_names piece - is this a real issue you've seen in practice where you're hoping to use this functionality, or a hypothetical concern? I'd be inclined to remove the method_names stuff (and possibly not even bring it back in a separate PR), as it feels like it makes things less intelligible (mostly as a user, rather than for this crate itself), and suggest that instead folks rename existing methods if there's a clash (e.g. if you have two methods named const_default, they're both probably unclear as to what they do!), and to maybe rename our generated const_into and friends to const_into_repr or const_into_discriminant? But I'd be interested to see any examples that motivated you towards renames if there are any? Either way I'd definitely split this part out into a separate PR :)

@@ -28,6 +29,13 @@ pub trait TryFromPrimitive: Sized {
fn try_from_primitive(number: Self::Primitive) -> Result<Self, Self::Error>;
}

pub trait ConstTryFromPrimitive: Sized {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would you feel about making ConstTryFromPrimitive a subtrait of TryFromPrimitive (and similarly for the others)? I think that would allow us to remove ConstTryFromPrimitiveError (we could just use TryFromPrimitiveError as the bound would still hold), and would mean the new const generic on EnumInfo could disappear?

I guess it's a bit annoying to have to derive both if you just want the const version, but I'm also not sure how common it would be to only want the const version, and also it mirrors how things like PartialEq/Eq work?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds great! It reminds me of what I was trying to do with ConstantTimePartialOrd in subtle a while ago (dalek-cryptography/subtle#98). I'll see if this works.

Copy link
Author

@cosmicexplorer cosmicexplorer May 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally do not have a use case for const-only derives -- I think I was a little confused about how const fn can also be a normal runtime fn and how to separate those. I think a subtrait and merging the errors (which I would definitely like to do) would be ideal -- will check that out now.

/// assert_eq!(zero, 0u8);
/// ```
#[proc_macro_derive(ConstIntoPrimitive, attributes(num_enum, catch_all))]
pub fn derive_const_into_primitive(input: TokenStream) -> TokenStream {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little concerned that this implementation and the derive_into_primitive may drift in the future - can you extract a common function that both this and derive_into_primitive can call, which is parameterised on a boolean/enum, to avoid that drift? (Same for the other derives)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part was hammered out by rote to get a prototype working and I will absolutely see if I can join them together as you say!

@cosmicexplorer
Copy link
Author

cosmicexplorer commented May 18, 2024

On the method_names piece - is this a real issue you've seen in practice where you're hoping to use this functionality, or a hypothetical concern?

This is a completely hypothetical concern and I have no qualms about reverting it entirely! I also think that there is value in canonical names, and I totally align with your thoughts here. I have rebased the commits with that functionality away, which includes the trybuild testing. It was very fun to play around with that though, and thanks for helping me narrow this down!

@tyrone-wu
Copy link

Hi 👋. Thanks for your work!

I was wondering what the current status of this PR. Our project has a bunch of generated FFI enums with direct mappings to idiomatic Rust enums, and we're currently pushing to reduce as much as castings, and so this feature would greatly help.

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 this pull request may close these issues.

3 participants