Skip to content
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

Add StaticVariantsArray trait and derive #297

Merged
merged 4 commits into from
Jan 21, 2024

Conversation

Syndelis
Copy link
Contributor

About this PR

This PR introduces a new trait StaticVariantsArray which defines an associated constant ALL_VARIANTS: &'static [Self]. Accompained to it is a derive that will create this array from an enum composed of only unit-type variants.

Motivation

While working in a UI project with ImGui, I stumbled upon this method, Ui::combo, which expects a slice and a mapping function to build the combobox's options. My options, as you might've guessed, are defined in an enumerator, which looks a little like this:

enum Operation {
  Add,
  Sub,
  Mul,
  Div,
  ...
}

The user, however, wouldn't see those names and, instead, I implement Into<char> for the respective characters you can imagine.

Presented with that problem, I could go for one of two solutions:

  1. Use EnumVariantNames's <enum>::VARIANTS, which is a static slice of strings. The problem with that approach is that it would generate an array with the names of the variants, which could be desired in many cases, but is not on mine as you can see above;
  2. Use EnumIter and collect the result into a Vec. While the application would work fine with this approach, it seems overkill to heap-allocate a vector whose elements we know at compile time.

Considering those two points, I've decided to introduce this trait (and derive).

Concerns

My only concern with this implementation is the name of both the trait/derive and the associated constant. I believe VARIANTS is a more fitting name, but it is already taken. I thought about renaming that to VARIANT_NAMES, but it feels like an unecessary breaking change, although I'd like to hear more opinions on this.

Closing thoughts

This is my first public macro contribution and, while I think it's quite simple and small, I'd love to get feedback on the implementation!

@Syndelis
Copy link
Contributor Author

Apparently it fails CI because the new trait I added to strum is obviously not available on crates.io yet. Should I remove the doctest for the time being and introduce it in a new PR? How do you guys usually deal with this problem?

@Syndelis
Copy link
Contributor Author

Hi there, @Peternator7 ! Can I get some feedback on this one? I could split the tests into a separate PR, which would likely make the CI pass

image

@Peternator7
Copy link
Owner

Hey @Syndelis , this looks really good! Don't worry too much about the CI if that's the only issue; it's been an annoying footgun with strum since early versions.

@Peternator7
Copy link
Owner

What do you think about VARIANTS vs ALL_VARIANTS. It "conflicts" with the VariantNames derive, but that might be okay since type resolution should be able to figure out which one you want?

@Syndelis
Copy link
Contributor Author

Syndelis commented Dec 3, 2023

What do you think about VARIANTS vs ALL_VARIANTS. It "conflicts" with the VariantNames derive, but that might be okay since type resolution should be able to figure out which one you want?

I'm not a fan of this name either haha. I think best case scenario is renaming VariantNames::VARIANTS to VariantNames::VARIANT_NAMES and attributing VARIANTS to this new trait, but I understand this may not be worthy of a breaking change.

Other than that, I really don't know what name would best suit it. I've considered STATIC_VARIANTS and VARIANTS_SLICE, but it feels weird to add the type or storage into the constant's name. I'm open to suggestions!

@polarathene
Copy link

polarathene commented Dec 5, 2023

This PR would remove the need for this snippet to have a const array/slice of all variants?:

// Strum presently lacks a derive for this:
const VARIANTS: &'static [MyEnum] = MyEnum::ALL_VARIANTS.as_slice();

// Effectively a const compatible equivalent of: MyEnum::iter().collect()
impl MyEnum {
    const ALL_VARIANTS: [Self; Self::COUNT] = {
        // Cannot rely on `Option<Self>` to default to `None`, as conversion
        // to output type is not possible? Thus use the first variant as a default:
        let default = Self::variant_from(0);
        let mut variants = [default; Self::COUNT];

        // Cannot use for loop (or iterators with collect) in a const fn,
        // Hence initializing an array and replacing default elements via while loop:
        let mut i = 0;
        while i < Self::COUNT {
            variants[i] = Self::variant_from(i);
            i+=1;
        }

        variants
    };

    // Must unwrap value due to no support for const traits,
    // otherwise could have collected with adaptors over [Option<Self>]:
    const fn variant_from(discriminant: usize) -> Self {
        let Some(variant) = Self::from_repr(discriminant) else { panic!("Invalid discriminant provided") };
        variant
    }
}

I was going to raise a feature request for that since it took a while to figure out a solution with all the const fn limitations 😅

My use-case is for input into divan with #[divan::bench(consts = VARIANTS)]. Since it takes in const, I'd need to create a function for all benched methods using that input to convert the current VARIANTS: &'static [&'static str] to the actual variant before the actual benched functionality 😅


Other than that, I really don't know what name would best suit it.

VARIANTS_LIST? Although I agree that the current VARIANTS would seem more appropriate.

I think this issue is requesting the same feature here? It suggested ITEMS.

My snippet above could also be a const fn instead resulting in MyEnum::variants() / MyEnum::list_variants() / MyEnum::get_variants() / MyEnum::itemize(), etc. With that last one, it kinda relates to ITEMS suggestion 🤔

@Syndelis
Copy link
Contributor Author

Syndelis commented Jan 8, 2024

Hi there, @Peternator7! Just wondering if you think anything else must be done to this PR or #315 in order to merge them?

@nvzqz
Copy link

nvzqz commented Jan 21, 2024

@polarathene Divan can now use runtime arguments via the args option, so a custom MyEnum type would be usable now.

@Peternator7 Peternator7 merged commit 2868766 into Peternator7:master Jan 21, 2024
@Peternator7
Copy link
Owner

Just published version 0.26.0 on crates.io with this feature. I renamed the macro VariantArray and the associated constant VARIANTS. The type-inference system should be able to differentiate with VARIANTS you're referring to if they're both in scope. Thanks for your help on this feature!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants