strum icon indicating copy to clipboard operation
strum copied to clipboard

Add Enum Map Derive Macro

Open PokeJofeJr4th opened this issue 1 year ago • 9 comments

This started as Issue #272

#[derive(EnumMap)]
enum MyEnum {
  VariantName,*
}

This macro creates a new type MyEnumMap<T> with a field of type T for each variant of MyEnum. It also adds the following:

  • Automatic derive for Debug, Clone, Default, PartialEq, Eq, Hash
  • Implement Index<T> and IndexMut<T>
  • fn new(variant_name: T,*) -> MyEnumMap<T> constructor to specify each struct field
  • fn from_closure(func: Fn(MyEnum)->T) -> MyEnumMap<T> constructor to initialize each struct field using a function
  • fn transform(&self: MyEnumMap<T>, func: Fn(MyEnum,&T) -> U) -> MyEnumMap<U> method to create a new map by applying a function to each field
  • if T implements Clone, generate fn filled(value: T) -> MyEnumMap<T> constructor to fill each struct field with the given vaalue
  • fn all(self: MyEnumMap<Option<T>>) -> Option<MyEnumMap<T>> checks if all fields are Some
  • fn all_ok(self: MyEnumMap<Result<T, E>>) -> Option<MyEnumMap<T>> checks if all fields are Ok
  • variants marked with #[strum(disabled)] are not included as struct fields and panic when passed as an index.

Questions:

  • Should EnumMap be renamed to clarify that it doesn't have many features of conventional maps?
  • Are the all and all_ok functions helpful? Are they within the scope of Strum?
  • I'm currently disallowing extra data on variants. Should I just ignore it and use the discriminant? Should I treat the underlying data as a key to a more conventional map?
  • Are there any other traits/functions I'm forgetting about that should be included?

PokeJofeJr4th avatar Jun 26 '23 16:06 PokeJofeJr4th

the build is only failing because the version of rust they're using is out of date

PokeJofeJr4th avatar Jun 26 '23 18:06 PokeJofeJr4th

In response to some of your other questions which I've been thinking about:

  1. I'm not quite certain what a better name would be. I guess if there was some way to specify that it's a map that has Some value for every possible key, that could be more clear, but I don't know what that would be.

  2. I feel like the all_ok function is definitely useful, but would be better if it returned a Result<T, E> where it returns Err with the first error encountered while looking through the variants (plus, this would also make it really simple to implement - just EnumMap { #(variant: variant?,)* } in the body). I was previously going to say that all wouldn't be super useful, but I was thinking about my use cases and realized it could actually be really useful. E.g. I would like to store data in an EnumMap to disk, and the library I'm using for that can store a HashMap right out of the box. So I plan to convert the EnumMap to a HashMap when storing like so:

    let mut map = HashMap::new();
    for var in Enum::iter() {
        map[var] = enum_map[var]
    }
    

    But to convert back after reading the HashMap from the disk, The only safe and clean way I can think of (besides manually getting the value for each variant) would be:

    let Some(enum_map) = EnumMap::from_closure(|v| hash_map.get(v)).all() else {
        println!("oops!");
    };
    

    So actually, I feel like all could definitely be useful if you need to convert from other similar storage mediums.

  3. I put my thoughts about non-unit variants in another comment

  4. I think impling Clone where T: Clone could be nice. I guess if you wanted to work around it, you could just do EnumMap::from_closure(|v| other_map[v].clone()), but just doing other_map.clone() would be cleaner and clearer a bit.

itsjunetime avatar Jun 30 '23 00:06 itsjunetime

  1. ChatGPT's best suggestion was MyEnumTable, which I kinda like
  2. I did change the all_ok to what you suggested. However, for your exact case, would it be better to implement a way to directly store the EnumMap?
  3. yeah that's still a hard one
  4. That's already a thing with the #[derive(...)] thingy; I've added a test to clarify that

PokeJofeJr4th avatar Jun 30 '23 13:06 PokeJofeJr4th

  1. Yeah, I think calling it a table could be good; if people would prefer that, then I have no objections
  2. It would definitely be better for me to directly store the EnumMap, but with the library that I'm using, that requires deriveing a bespoke macro, which I don't think I can do right now for the generated EnumMap. However, with that in mind, perhaps we could add an attribute like #[enum_map(derive = "MyTrait, MyOtherTrait")] so that you can add other traits to the generated struct's derive attribute? I'm not attached to that syntax, just the general idea of adding extra traits to derive.

And thanks for pointing out that it already derives Clone; I must've missed that.

itsjunetime avatar Jun 30 '23 16:06 itsjunetime

found a significant issue https://github.com/PokeJofeJr4th/strum/issues/8

PokeJofeJr4th avatar Jul 01 '23 12:07 PokeJofeJr4th

Could you clarify what the significant issue was? It's good to consider that someone could use a reserved word; can we fix that by prepending an underscore to their identifier?

Peternator7 avatar Jul 16 '23 22:07 Peternator7

Sorry, I see now that you have a perfectly fine fix (prepending _). I previously thought we'd have to figure out if it was a reserved keyword and use r# to escape it; but since it's not publicly facing the underscore solution is perfectly valid

PokeJofeJr4th avatar Jul 17 '23 01:07 PokeJofeJr4th

I feel like it would be really useful to add custom derive commands for each struct this is used on, e.g.

#[derive(EnumTable)]
#[strum(table_derives = "Serialize, Deserialize")]
enum Color {
...
}

this way, ColorTable would have #[derive(Serialize, Deserialize)] added on its declaration since you can't do that otherwise (at least, as far as I know). Would anyone be opposed to this?

itsjunetime avatar Sep 27 '23 21:09 itsjunetime

I like that but will the namespaces work on it? Would the user having macros like Serialize and Deserialize in the same scope as #[derive(EnumTable)] be enough?

PokeJofeJr4th avatar Sep 28 '23 01:09 PokeJofeJr4th

Published strum version 0.26.0 that includes this feature. I've labeled it experimental because I'm not totally sure the api surface won't change, but think it's an interesting start. Thanks for the work on this and sorry for the delay merging it.

Peternator7 avatar Jan 28 '24 01:01 Peternator7