rust-num / num-derive

Procedural macros to derive numeric traits in Rust
Apache License 2.0
166 stars 24 forks source link

Add special case to ToPrimitive for enums with primitive representation #64

Closed okamt closed 3 weeks ago

okamt commented 1 month ago

ToPrimitive can be safely derived for any enum with the #[repr(u/i)] attribute

See: https://doc.rust-lang.org/reference/items/enumerations.html#pointer-casting https://doc.rust-lang.org/std/mem/fn.discriminant.html#accessing-the-numeric-value-of-the-discriminant

cuviper commented 1 month ago

Do I understand correctly that this will convert non-unit enums based on their discriminant? If so, I think that's a strange use of ToPrimitive that we shouldn't encourage, since that will completely ignore the data in the enum. In other words, this is a numeric trait, but I think it's unlikely that the discriminant represents the entire numeric value of an enum type. Do you have an example otherwise?

okamt commented 1 month ago

Do I understand correctly that this will convert non-unit enums based on their discriminant?

Yes, my use case for this is an enum that represents different types of messages with a numeric ID like:

#[repr(u8)]
enum Packet {
    Ack = 0x00,
    Message { content: String } = 0x01,
}

Makes it easy to get the ID (discriminant) from a Packet value to encode it and send it over the network or whatever else. I thought it would make sense for ToPrimitive to work here since I was already using it to encode other unitary enums that I also need to send over the network based on their discriminant

cuviper commented 1 month ago

Ok -- it makes sense that you want to extract the discriminant for that, but I don't think ToPrimitive is right for that. In particular, you wouldn't want to use that type with an arbitrary fn foo<T: ToPrimitive>. Even for your narrow use case, you would probably be better off for type safety with something that gave you the very specific repr type, and nothing else.

I don't know if another crate exists for that yet, but it would be nice!

cuviper commented 1 month ago

This looks like a good fit: https://crates.io/crates/safe-discriminant (although I haven't used it and can't vouch for it...)

okamt commented 3 weeks ago

safe-discriminant works but it doesn't have num -> enum conversion, I could use another crate for that but I'd rather have just one dependency for both. https://crates.io/crates/num_enum looks like a better fit for this functionality so I might PR there instead