Can ArgEnum be both more general and performant?

[epage]

#2762 initially had common API calls for ArgValue allocating a Vec. We instead dropped that at the cost of adding the Clone super trait.

Can we do better?

[ModProg]

not ArgValue but ArgEnum.

[ModProg]

One possible soluion would be returning an array using const generics and using the 2021 edition into_iter to be able to return the actual value of the array without a need to clone.

At least in theorie.

[epage]

@pksunkara posted the following snippet as one idea

trait ArgEnum<const COUNT: usize>: Sized {
    fn variants() -> [Self; COUNT];
}

enum E {
    One,
    Two,
}

impl ArgEnum<2> for E {
    fn variants() -> [Self; 2] {
        [Self::One, Self::Two]
    }
}

fn main() {}

https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=8ccbfd753fb15dab675ba0491c7f48a3

[epage]

@ModProg are you referring to using trait ArgEnum<const COUNT:usize>?

I see this is a trade off of making the trait harder to use vs requiring Clone.

Personally, I feel the Clone requirement is fairly minimal. Probably 99% of ArgEnums will be able to be Copy. On some occasions, someone will have a hidden variant. Among those using a hidden variant, what is the likelihood the type won't be Clone? My guess is fairly small. This is a niche of a niche that doesn't seem worth supporting. Releasing as-is, we can see what feedback we end up getting and re-examine this for clap4.

I recommend we close this, creating a new issue based on specific user feedback if it comes in.

[ModProg]

@ModProg are you referring to using trait ArgEnum<const COUNT:usize>?

yes.

I see this is a trade off of making the trait harder to use vs requiring Clone.

The only thing about this that makes me consider it is, that most people will only derive but never implement it, so this only affects a small number of people. And while the Clone is only a minor inconvenience it affects a lot more, who probably don't even use most of the features provided by my new implementation.

But I also see another implementation that just moves the to_string implementation back to the derive macro, that way we don't need either the Clone nor const generics.

[pksunkara]

I agree with @ModProg regarding the estimates of how many use what kind of implementation.

But I also see another implementation that just moves the to_string implementation back to the derive macro, that way we don't need either the Clone nor const generics.

Do we have a clear example on how to achieve this?

[ModProg]

This was the way it was done before, it was changed here:
480035a#diff-1ea1dfdc52ed49bd532e4bd309c902a621e3bf02709b1fea86466c47fc7df7d9L118-L122

[epage]

To clarify, I'm not concerned about the implementer. With that said, I have a PR out for one hand-written ArgEnum and expect to create another,. I also see more being written by crates that provide re-usable clap logic. Updating a length as code changes feels like busy work and is a breaking change for those crates. One the other hand, the cost of adding a #[derive(Clone)] is small, especially since people will most likely use it anyways.

However, creating a trait that can only be used in very specific ways because of the way the generic parameter is defined, severely limits our ability to evolve clap.

[epage]

@ModProg, I thought the inherent from_str was a side benefit to us having to already make this change so that arg_values wouldn't return a Vec.

[ModProg]

@ModProg, I thought the inherent from_str was a side benefit to us having to already make this change so that arg_values wouldn't return a Vec.

It is, but it more enables the inherent from_str than requiring it.

The problem with the "generated" form is that we have to create the ArgValue for each call. tho I don't know if there is some optimazation happening because it technicly is a constant value even tho due to alias being a Vec we cannot declare it as const.

Another though is, we could also create the actual ArgValue during deriving, call get_name_and_aliases on it and put that value in the derived implementation. So it really is a constant array/slice for every variant.

[ModProg]

The advantage to using the matches on ArgValue is, that it makes sure we use the same implementation in both the derived and build.

[kbknapp]

I've only skimmed this issue, but in general I don't believe adding a const generic type argument to ArgEnum is worth the performance gain by not allocating a small Vec. My reasoning is clap still allocates, so it's not like we're removing all allocations. Additionally, Enum::values() would most likely only be called when either generating the help message, or other "generating" operation such as completion script, etc. I.e. it should not be allocating in the happy path of standard parsing. And even if it does we're still only talking a small Vec since allocation.

The cost however is a generic argument that's not entirely clear at first glance what it's for: Enum<3> doesn't mean a whole ton when the enum's variants are Foo, Bar, and Baz and it turns out that generic argument just so we can return an array size hint.

I would like to see improvement around ArgEnum for sure, but I think this is one of those issues that we can place on the back burner and garner some good ideas as v3 fleshes.

[epage]

For comparison purposes

Original

pub trait ArgEnum: Sized {
    const VARIANTS: &'static [&'static str];
    fn from_str(input: &str, case_insensitive: bool) -> Result<Self, String>;
    fn as_arg(&self) -> Option<&'static str>;
}

• Wanted to add ArgValue support for native alias, hidden support, etc

Current Solution

pub trait ArgEnum: Sized + Clone {
    fn value_variants<'a>() -> &'a [Self];
    fn from_str(input: &str, case_insensitive: bool) -> Result<Self, String>;
    fn to_arg_value<'a>(&self) -> Option<ArgValue<'a>>;
}

• Forces Clone
  • Most arg enums would work as Copy
  • A few use hidden, and of those, they mostly likely support Clone, making the impact very small

Vec Solution

pub trait ArgEnum: Sized {
    fn variants() -> Vec<ArgValue<'static>>;
    fn from_str(input: &str, case_insensitive: bool) -> Result<Self, String>;
    fn as_arg(&self) -> Option<ArgValue<'static>>;
}

• Arg::new("foo").possible_values(arg_enum.variants()) does a throw-away allocation when building
• An inherent implementation for from_str would cause an allocation for parsing as well, since it'd call variants

const Solution

trait ArgEnum<const COUNT: usize>: Sized {
    fn variants() -> [Self; COUNT];
    fn from_str(input: &str, case_insensitive: bool) -> Result<Self, String>;
    fn as_arg(&self) -> Option<ArgValue<'static>>;
}

[pksunkara]

I am happy with where it is for 3.0, but we can keep this open for future. Removing the milestone.

[epage]

I feel like our current option is "good enough" until we get real world use cases with problems. Closing for now