-
Notifications
You must be signed in to change notification settings - Fork 40
feature: const variants of into, from, try_from, and default #147
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
base: main
Are you sure you want to change the base?
Conversation
|
The |
illicitonion
left a comment
There was a problem hiding this 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 :)
| fn try_from_primitive(number: Self::Primitive) -> Result<Self, Self::Error>; | ||
| } | ||
|
|
||
| pub trait ConstTryFromPrimitive: Sized { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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!
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 |
b4f24b6 to
88ce96f
Compare
|
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 |
|
Hi! I'm also interested in the features in this PR. What's the current status of this implementation? If there is still interest in it I can try implement any missing/incomplete features (if there are any). |
|
I think with the addition of rust-lang/rust#143773, it looks like it might not be necessary. |
Motivation
num_enumis very effective for manipulating data representations at runtime. However, there are some cases where we might want this functionality inconstcontexts, such as when generatingstaticvalues or composingnum_enumtypes into otherCopystructs which we also want to manipulate inconstcontexts.Alternatives
const_trait_implandeffectsare able to avoid the need for parts of this, by makingFrom/Into/Defaultimplsconst-compatible, so for my other work that uses nightly, I actually don't require these at all!constand non-constmethods fornum_enumderives until Rust has developed a more formal mechanism for manipulatingconsteffects.Implementation
Four new derive macros were added, along with auxiliary traits/structs in
lib.rsas needed. Since traits can't defineconst fns, all of these instead generate a method on the enum directly:ConstIntoPrimitive:const_into(self) -> #reprConstFromPrimitive:const_from(#repr) -> SelfConstTryFromPrimitive:const_try_from(#repr) -> Result<Self, ConstTryFromPrimitiveError<Self>>ConstDefault:const_default() -> SelfAdditionally, the
#[num_enum(method_names(...)]attribute was added to the parser, to override the names of generatedconstmethods. This is done because unlike the non-constderives, we do not have a canonical trait to implement, so we risk stomping on users' method names:Result
The boilerplate that
num_enumgenerates no longer has be written by hand when using anum_enumtype inconstcontexts!