EnumIndex implementation

[drewkett]

@Peternator7 This is an initial pass at an implementation of an index function that is const. The following lists some of the outstanding items. In addition, I'm really not sure I captured all of the desired behavior in that macro implementation.

• Is this the best name for the macro name or the method name (currently its index and const_index)
  • Should there by try_ variants of the functions so that the non-try versions panic on unknown value similar to Index trait returns an Option?
• Add documentation
• The argument to the get/index function should be the same type as the repr (ie if the enum is repr(u8))
• Add ability to rename get/index function?
• Discriminant
  • Add error messages related to discriminant code
  • Make sure the implementation is correct beyond the basic tests that are currently written.

[drewkett]

I went ahead and updated some things.

• Added documentation
• Renamed macro to EnumIndex
• The macro now generates up to two functions. index in all cases (except when an error is raised due to the existence of lifetimes). And const_index when all the variants are unit variants (Default::default is not const).

[drewkett]

I got a version working to handle custom repr working. The code needs cleaning up in general but just wanted to get something committed that was working, since it ended up being a bit more complicated that I thought it would be

[drewkett]

So in trying to get the CI passing, I finally installed rust 1.32 locally so I could run the tests, only to realize that if and match in const have a MSRV of 1.46. It looks like it is possible to use build.rs to enable features based on rust version which could then be used to conditionally enable the const function. Alternatively, we either need to bump MSRV, or I can just drop the const code for now and leave it in a PR for a future date.

[Peternator7]

Hey Drew; this looks like a great start. Thanks for putting in all the effort. In #186, someone is adding the rustversion crate to help with this type of issue. I think following the pattern they are using is probably the best solution. Then add a note to the documentation that says const functions are only derived if a new-ish rustc version is used.

[drewkett]

Think I fixed everything mentioned. I added rustversion as well. The way I made the const_index doc tests conditional on the rust version seems weird to me but I didn't know of a simpler way. Not sure if you have any other ideas for that.

[Peternator7]

This is looking really good. I've got 2 questions for you:

1. What do you think of the name FromDiscriminant for the Derive instead of EnumIndex? I'm trying to move away from prefixing everything with Enum. Then we would call the method from_discriminant

2. For newer versions of rust, does it make sense to still expose the non-const versions of the method at all? I'm wondering if instead of exposing 2 copies of the method, we just expose one method and only add a const modifier if it's a newish version of rust. Are there any disadvantages to doing something like this?

quote! {
    impl #impl_generics #name #ty_generics #where_clause {
        #vis #const_if_possible fn from_discriminant(idx: #index_type) -> Option<#name #ty_generics> {
            match idx {
                #(#arms),*
            }
        }
    }
}

[drewkett]

The different name makes sense and seems clearer. I went ahead and made the change.

That's a good idea, and definitely simpler. I didn't like the const_ name all that much. I've incorporated that change as well.

One thing I'm wondering is how I've exposed the constants. I renamed them so they make a bit more sense, but I'm still not sure I like exposing them. Especially since there is an EnumDiscriminants macro which exposes the discriminants as a new type. I think I'll just go ahead and remove them from the documentation for now.

[drewkett]

I removed the constants from the interface. It seems cleaner to just have this macro only generate the from_discriminant function

[Peternator7]

This is looking really good! Looks like there's a merge conflict that needs to be resolved.

I don't want to bikeshed on the name too much, but it is easier to change before anyone depends on it. What do you think of FromRepr? I like FromDiscriminant a lot, but there is also the EnumDiscriminants macro that generates a Discriminant type + To/From methods, and that might get confusing. Any other name ideas? Sorry for the 11th hour change of heart.

[drewkett]

I noticed the overlap with EnumDiscriminants. And no worries with wanting to change the name now, it's not a big deal. I think FromRepr is okay to avoid any confusion with the discriminants feature and I don't have a better name. I think its best to avoid discriminant even if it's most descriptive because this macro won't be compatible with EnumDiscriminants.

My other thought was just to extend EnumDiscriminants. Add the from_discriminant as a method on MyEnum. Incorporate the repr type and discriminant values into the MyEnumDiscriminants. The nice thing is the two features would be compatible. The downside is that incorporating the repr and discriminant info into discriminants is a breaking change though I'd imagine it's generally a minor one.

[Peternator7]

Combining them is certainly interesting. One obstacle is that EnumIndex requires variant data have a Default impl where EnumDiscriminants doesn't since it's strictly Enum -> Discriminant. That alone might be enough to justify separating them.

More generally, I wonder if the the 2 macros serve different users:

• People using EnumDiscriminants probably have complicated enum types that they want to be able to reduce to just the discriminant. Since Tracking issue for RFC 2363, "Allow arbitrary enums to have explicit discriminants" rust-lang/rust#60553 is open, they probably aren't using a custom repr or explicit discriminants on these enums. These enums strike me as unconcerned with the representation of their discriminant. It's an implementation detail mostly.

• My guess is that users of EnumIndex probably have simple enum types with few or no data fields; they also might have a custom repr and explicit integral discriminant values. In this case, the representation seems like it might be one of the defining features of the particular enum.

I could certainly be wrong and feel free to disagree, but I think they are probably best as 2 different macros because they are interesting in different scenarios.

So having said all that, I'm still in favor of FromRepr, but open to other ideas if anything comes to mind.

[drewkett]

Looking at more things, I agree with you. I came across this PR which is adding a FromRepr trait to rust only for repr enums without any data. Since this feature is fundamentally similar it should probably be the same name with similar behavior. The three deviations this library would have compared to the above feature would be: 1) add the ability to use it with any that implements Default, 2) having from_repr be able to be const 3) its a safe function that returns None on invalid repr. All of those might be useful to people. I'll go ahead and update the name of the macro/function.

The only other aspect then is that this macro would have the same name as the rust feature. I don't see that as a problem as you would explicitly import this macro on use which should shadow the rust macro

[Peternator7]

Everything looks good! Thanks for all the work on this. I'll try and get a release out in the next week or so with this and #186

[drewkett]

It was a pleasure working with you on this. Thanks from a happy user of the library.

[Peternator7]

Glad it works well for you! :D

Just release 0.23 on crates. https://crates.io/crates/strum