Add Enum Map Derive Macro #279
Conversation
need to make error detection (dependency on EnumIter, etc.) more robust
switched from array to struct
|
the build is only failing because the version of rust they're using is out of date |
strum_macros/src/macros/enum_map.rs
Outdated
| let mut disabled_matches = Vec::new(); | ||
|
|
||
| for variant in variants { | ||
| // skip disabled variants |
There was a problem hiding this comment.
Having a variant that could be disabled and cause a panic if indexed with makes me nervous, but it seems to be in line with what this crate normally does. I feel like it would make more sense, though, to ignore the disabled attribute for this specific macro (among others, like AsRefStr) since generating code for the ignored attributes can't cause any harm.
If they already weren't going to index into this map with that variant (e.g. they were using EnumIter with a disabled variant to iterate through all the variants and modify this EnumMap with each variant), then there's no harm in not panicking if they try to index with what would be a 'disabled' variant. It could cause confusing behavior if they accidentally do index into this with the disabled variant (since it would basically do nothing instead of panicking), but I feel like that's much better behavior than panicking. Perhaps this comment should be made into an issue instead to discuss there.
There was a problem hiding this comment.
I think that unexpected behavior would be more detrimental to a project than a panic, since the panic is obvious what the problem is
strum_macros/src/macros/enum_map.rs
Outdated
| continue; | ||
| } | ||
|
|
||
| // Error on fields with data |
There was a problem hiding this comment.
I agree with this decision - calling this a map implies that every possible variant has its own value and if you have a variant that has data (e.g. Name(&'static str)) it could potentially be confusing if indexing with that variant but different data (e.g. map[Name("John")] vs map[Name("Jane")]) returns/modifies the same value.
However, this doesn't exactly seem to go in line with what this crate normally does - e.g. with EnumIter, it just generates default::Default() for each field if a variant has data (instead of refusing to expand the macro). If we wanted to keep behavior similar to the other derive macros here, we might want to still expand the macro but just ignore all data on each variant and only match on variant type. I can't think of a specific use-case where allowing data would be necessary, so I'm still in favor of keeping this as-is, but I'm not quite certain what other contributors would be in favor of.
Co-authored-by: June <[email protected]>
moved snakify into `helpers` mod
|
In response to some of your other questions which I've been thinking about:
|
|
And thanks for pointing out that it already derives |
|
found a significant issue |
|
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? |
|
Sorry, I see now that you have a perfectly fine fix (prepending |
|
|
||
| // Error on empty enums | ||
| if pascal_idents.is_empty() { | ||
| return Err(syn::Error::new( |
There was a problem hiding this comment.
Maybe I missed something. Is this strictly necessary?
There was a problem hiding this comment.
It's not strictly necessary, but I think it's a good idea to include. Otherwise an empty enum produces this error, which tells the developer a lot less about what they're doing wrong.
error[E0392]: parameter `T` is never used
--> strum_tests\tests\enum_table.rs:22:10
|
22 | #[derive(EnumTable)]
| ^^^^^^^^^ unused parameter
|
= help: consider removing `T`, referring to it in a field, or using a marker such as `PhantomData`
= help: if you intended `T` to be a const parameter, use `const T: usize` instead
= note: this error originates in the derive macro `EnumTable` (in Nightly builds, run with -Z macro-backtrace for more info)
For more information about this error, try `rustc --explain E0392`.
error: could not compile `strum_tests` (test "enum_table") due to previous error
The only other valid option I see is using phantomdata, and I don't think it makes sense to allow someone to derive a data structure that can't hold any data.
| } | ||
| } | ||
|
|
||
| impl<T> #table_name<T> { |
There was a problem hiding this comment.
Can we add a get and get_mut methods to compliment Index and match the semantics of arrays, Vecs and HashMaps?
There was a problem hiding this comment.
I don't believe we need these. The main reason to have both Index and get for existing types is that a key doesn't always map to a value. For EnumTable, those methods wouldn't be any different from just using Index and IndexMut. Again, I don't care much on this, just want to make sure we're on the same page.
| } | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
What's the plan for Iterator?
There was a problem hiding this comment.
I think it makes sense to just use MyEnum::iter().map(|variant| my_enum_map[variant]). I don't know how to implement it otherwise without basically rewriting all of EnumIter.
| /// assert_eq!(complex_map, ColorTable::new(0, 3, 0, 3)); | ||
| /// | ||
| /// ``` | ||
| #[proc_macro_derive(EnumTable, attributes(strum))] |
There was a problem hiding this comment.
I don't feel too strongly, but rust tends to use the "Map" terminology rather than "Table." Thoughts about calling this EnumMap instead?
*I thoughts that's what the feature would be called based on the PR title.
There was a problem hiding this comment.
Edit for clarity: It was originally called EnumMap.
We had a conversation about this a while back and the conclusion was that Maps tend to be dynamically sized based on how full they are, whereas this type always has a value for each key. We thought these differences were enough that calling the structure a Map would cause people to make incorrect assumptions about how it works. That said, I don't feel too strongly about it either.
… they'll work with rust 1.56.1
|
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, |
|
I like that but will the namespaces work on it? Would the user having macros like Serialize and Deserialize in the same scope as |
|
Published strum version |
This started as Issue #272
This macro creates a new type
MyEnumMap<T>with a field of typeTfor each variant ofMyEnum. It also adds the following:Index<T>andIndexMut<T>fn new(variant_name: T,*) -> MyEnumMap<T>constructor to specify each struct fieldfn from_closure(func: Fn(MyEnum)->T) -> MyEnumMap<T>constructor to initialize each struct field using a functionfn transform(&self: MyEnumMap<T>, func: Fn(MyEnum,&T) -> U) -> MyEnumMap<U>method to create a new map by applying a function to each fieldTimplementsClone, generatefn filled(value: T) -> MyEnumMap<T>constructor to fill each struct field with the given vaaluefn all(self: MyEnumMap<Option<T>>) -> Option<MyEnumMap<T>>checks if all fields areSomefn all_ok(self: MyEnumMap<Result<T, E>>) -> Option<MyEnumMap<T>>checks if all fields areOk#[strum(disabled)]are not included as struct fields and panic when passed as an index.Questions:
EnumMapbe renamed to clarify that it doesn't have many features of conventional maps?allandall_okfunctions helpful? Are they within the scope of Strum?