feat(derive): derive `clap::Args` for enum types #5700

ysndr · 2024-08-25T16:42:03Z

Implement derive(clap::Args) support for enum types, where each variant is a mutually exclusive ArgGroup.
Relevant discussion and motivation for this PR is in #2621.

Impl notes:

At the moment this is a rough initial implementation.

It only supports Named / struct like enum variants such as this:

#[derive(clap::Args, Clone, Debug, PartialEq, Eq)]
enum Source {
    A {
        #[arg(short)]
        a: bool,
        #[arg(long)]
        aaa: bool,
    },
    B {
        #[arg(short)]
        b: bool,
    },
}

it has issues with determining a default when neither branch matches explicitly
it's not fully covered with tests
parts of it are frankensteined together from the subcommand impl and deriving from structs

Regardless this is a dear feature to me so i'm looking for some guidance to polish this into completion.

ysndr · 2024-08-25T17:02:34Z

clap_derive/src/derives/args.rs

+ let Fields::Named(ref fields) = variant.fields else {
+ abort! { variant.span(),
+ "`#[derive(Args)]` only supports named enum variants if used on an enum",
+ }
+ };


Restricting this to named enums only, allows to reuse the struct codegen with little modification.

Unnamed variants, with a single item might somehow dispatch the parsing bits to the contained Type and somehow set the group conflicts using ArgGroup::mut_group on the augment side and ??? on the from arg matches (that might simply forward?).

Unit variants need to be parsed as flags?

What about enum Something { Variant(String) } would we expect this to parse as 1) a positional, 2) a flag, 3) not at all bc we cant forward to Strings implementations?

I'm fine with scoping the first PR toNamed variants. We should have compile error tests for all of the other kinds of variants.

I could see soon after support single-element tuple variants with #[command(flatten)] Variant(MoreArgs) to avoid the need for Variant { #[command(flatten)] args: MoreArgs }. I'm assuming supporting specifically that wouldn't be too bad.

Longer term...

Unit variant should probably be discussed in the issue. My first guess is to use it as an "else" case.

For other single-element tuples, I see it working just like a field. They are positional by default and need #[arg(short, long)] on the variant to change it to something else. We'd use value_parser! to understand what to do with the type inside of the variant.

For N-element tuples, we have an issue for supporting those on fields and I'd point to that for variants as well.

that all makes sense in my book.
I'm still getting familiar with Item and the parsing around it where Named variants were just the closest to intuitively scrape together.
Happy to look into that soon after.

ysndr · 2024-08-25T17:04:29Z

clap_derive/src/derives/args.rs

+ // when generating mutably exclusive arguments,
+ // ids of arguments that should conflict
+ conflicts: &[Name],


Not super clean to sqeeze this in here esp, since this is also used by the subcommand deriver, but i tried to avoid rewriting the entire method for this.

This goes away if we rely on groups, rather than conflicts

ysndr · 2024-08-25T17:07:39Z

clap_derive/src/item.rs

@@ -70,6 +70,66 @@ impl Item {
 Ok(res)
 }

+ pub(crate) fn from_args_enum_variant(


this method is basically a copy of from_subcommand_enum_variant.
I'm not yet entirely clear if that is sensible since my picture of what Item is is still somewhat blurry.

Item is a mess. It is a storage for all of our highlevel attribute information for a single type, field, variant, etc. We use the same type for all of those and for whichever trait or attribute type is used.

Making this function fits exactly within the current model

Good to know, is there any better documentation on Item that I can read up on?

Not really. A lot of this is ad hoc. It'd be good to clean up at some point.

ysndr · 2024-08-25T17:08:59Z

flake.nix

don't mind this it just helps me get a rust compiler using nix...
will be removed before this is ready to go anywhere

ysndr · 2024-08-25T17:35:52Z

clap_derive/src/derives/args.rs

+
+ if __clap_arg_matches.contains_id(#group_id) {
+ let #item_name::#variant_name { #( #field_names ),* } = self else {
+ unreachable!();


i guess this might fail if the current variant is different, should it be possible to "change" variants using update_from_arg_matches_mut?

This should probably be an error, or instead of trying to update we'd attempt to parse all fields of the "other" variant and replace self with that.

for a slightly more concrete discussion:
this currently generates:

fn update_from_arg_matches_mut( &mut self, __clap_arg_matches: &mut clap::ArgMatches, ) -> ::std::result::Result<(), clap::Error> { #![allow(deprecated)] if __clap_arg_matches.contains_id("A") { let Source::A { a, aaa } = self else { unreachable!(); }; if __clap_arg_matches.contains_id("a") { *a = __clap_arg_matches.remove_one::<bool>("a").ok_or_else(|| { clap::Error::raw( clap::error::ErrorKind::MissingRequiredArgument, concat!("The following required argument was not provided: ", "a"), ) })? } if __clap_arg_matches.contains_id("aaa") { *aaa = __clap_arg_matches .remove_one::<bool>("aaa") .ok_or_else(|| { clap::Error::raw( clap::error::ErrorKind::MissingRequiredArgument, concat!("The following required argument was not provided: ", "aaa"), ) })? }; } if __clap_arg_matches.contains_id("B") { let Source::B { b } = self else { unreachable!(); }; if __clap_arg_matches.contains_id("b") { *b = __clap_arg_matches.remove_one::<bool>("b").ok_or_else(|| { clap::Error::raw( clap::error::ErrorKind::MissingRequiredArgument, concat!("The following required argument was not provided: ", "b"), ) })? }; } ::std::result::Result::Ok(()) }

for my test enum:

#[derive(clap::Args, Clone, Debug, PartialEq, Eq)] enum Source { A { #[arg(short)] a: bool, #[arg(long)] aaa: bool, }, B { #[arg(short)] b: bool, }, }

I would look to subcommands for a starting point in designing how to model this.

I also am really tempted to remove all of the update code...

ysndr · 2024-08-25T17:39:48Z

clap_derive/src/derives/args.rs

+ fn from_arg_matches_mut(__clap_arg_matches: &mut clap::ArgMatches) -> ::std::result::Result<Self, clap::Error> {
+ #raw_deprecated
+ #constructors
+ unreachable!()


we actually do get here if neither variant was explcitly constructed.
In a way that makes sense, if all variants have defaultable arguments, either is a valid candidate. However I cant decide if that's a conflict or resolvable by defining a default varaint for such situations

If the user has an Option<Enum>, we'll check if the group is present and do None if it isn't, so this is only a problem with Enum. I lean towards hand constructing a clap error like we do if you have #[arg(required = False)] String. If we make unit variants a fallback (or add a fallback attribute). then users can avoid the error here.

epage · 2024-08-26T17:51:28Z

Especially if I've not looked at it yet, feel free to squash all of your fixups. In general, its a big help to review things when there is a clean commit history and changes are made as small as possible (while still being atomic).

epage · 2024-08-26T17:53:00Z

tests/derive/groups.rs

+ assert_eq!(
+ clap::error::ErrorKind::ArgumentConflict,
+ Opt::try_parse_from(["test", "-b", "-a"])
+ .unwrap_err()
+ .kind(),
+ );


When adding a test commit separate from a feature, it
needs to still pass tests / compile. I'm assuming this commit does neither.

How would i add a test that does compile for a thing that doesn't yet compile, or where the subject of the PR is allowing these constructs to compile in the first place?

"it depends"

Let's step back to the goals of why we encourage tests in a commit before the feature

Help test the tests (when fixing a bug, did the tests actually need to change?)

Through a commit's diff, making it clear to reviewers and the wider community what the intended behavior is

All of this is trivial when its a bug fix to help output. You show the existing behavior in the tests, then change the test when you change the behavior.

When the behavior doesn't exist yet, your options are

Just don't do this and have the test added in the commit.

Capture the feature failing spectacularly

Find the closest parallel feature and test that

The ideal is the last but sometimes there isn't always a close enough parallel and it can take the most work.

As an example of the last, take https://github.com/rust-lang/cargo/pull/14435/commits. This adds a new feature to Cargo. Originally, the test commit used the new features which didn't exist and errored out quickly. The commit that implemented the feature then made them not error. This was still more helpful than having them in the same commit because I only had to review the test output and not all of the test. However, I found a parallel feature and suggested it to the contributor. The tests are now much easier to see what the intended behavior is.

Thats easy when its all textual output and you aren't dealing with compile errors. One option is to write the tests with structs, rater than enums. The commit that adds the feature could also switch the structs to enums, causing some cases to still work while other cases will error.

That might or might not be worth it. If that seems like it will be too much or not show enough, feel free to say so, squash the commits, and move on.

ysndr · 2024-08-26T17:55:20Z

Hej @epage thanks for having a look
I'll go ahead and squash the fixups, constantly rewriting history in a PR isnt always welcomed so i held off for now, thanks for understanding.

epage · 2024-08-26T18:12:42Z

clap_derive/src/derives/args.rs

+ let conflicts = variants
+ .iter()
+ .filter_map(|(_, v)| {
+ if v.ident == variant.ident {
+ None
+ } else {
+ Some(Name::Derived(v.ident.clone()))
+ }
+ })
+ .collect::<Vec<_>>();
+
+ let fields = collect_args_fields(item, fields)?;
+
+ let augmentation = gen_augment(&fields, &app_var, item, &conflicts, false)?;


Why are we collecting and manually specifying conflicts rather than having group.multiple(false)?

There are bugs in nested group support but we should instead focus on those.

Ah specify multiple(false) for the "outer" group generated for the enum itself?
Guess that could work.
I explicitly stated the conflicts more out of intuition and reading that nested groups are behaving weirdly.

So to elaborate on "behaving weirdly", adding a group as an argument of a group doesnt seem to work at all currently

if i build something like this:

Command::new() .arg(clap::Arg::new("a") ...) .arg(clap::Arg::new("b") ...) .group(clap::ArgGroup("A").args(["a"]) .group(clap::ArgGroup("B").args(["b"]) .group(clap::ArgGroup("Outer").args(["A", "B"])

i get a runtime error: Command clap: Argument group 'Outer' contains non-existent argument 'A'.
Before I'm digging into the ArgGroup parser which seems to specifically address args (rather than args or groups), i'm looking to confirm that this is indeed how we would want to specify nested groups to begin with.

Continue reading here if i haven't gone astray yet.

From thereon i'm looking at clap_builder::parser::validator::gather_arg_direct_conflicts which would recursively resolve all ancestor chains, and then from the root(s) down check each group whether it is a multiple == false group, and if it is add all args from other branches not on the current path as conflicts; or in pseudocode:

conflicts(arg_id: &Id, path: Vec<&Id>, conf: &mut Vec<Id>) parent_id = path[-1] ? arg_id for group in groups_for_arg(parnt_id) { for conflict in group.conflicts { if conflict.is_group { for member in members_recursive_ignore_path(path, group) { conf.push(member.id) } } else { conf.push(conflict.id) } } if !group.multiple { for member in members_recursive_ignore_path(path, group) { conf.push(member.id) } } path = path.clone() path.push(group.id) conflicts(arg_id, path, conf) }

that is if the goal is to support arbitrary nesting
I'm not too happy with the amount of potential duplicates iff groups are densely nested.
Also keep calling groups_for_arg (or the impl of it) may hold potential for optimization down the road.

I believe a similar impl is also necessary for requireing with nested groups.

Oh wow, I thought we supported that. There was some case where we had a bug related to ArgGroups and I thought we had broader support for ArgGroups than we apparently do (which also means I blocked #4697 on the wrong thing...)

I would like for this to move forward with groups, rather than conflicts, so we make sure we stabilize the right semantics. So that means we need nested groups first...

One approach

Deprecate ArgGroup::arg and ArgGroup::args and add ArgGroup::member or ArgGroup::child

Update the assertions related to ensuring group members are present

Add associated tests to make sure nested arg groups work, fixing bugs along the way

If someone wants to take this on, this should be its own PR.

I'd be up to look into that.
Do you think my understanding of conflict detection aligns with your intended changes to arggroup?
If not, how/where could we plan this in more detail than 3 bullet points?

Created #5711. I don't have more specific guidance at this time. In general, a child group should behave like a child arg. I don't more more specific guidance than that at this time.

epage · 2024-08-26T18:13:06Z

clap_derive/src/derives/args.rs

+ // when generating mutably exclusive arguments,
+ // ids of arguments that should conflict
+ conflicts: &[Name],


This goes away if we rely on groups, rather than conflicts

epage · 2024-08-26T18:14:28Z

clap_derive/src/item.rs

@@ -70,6 +70,66 @@ impl Item {
 Ok(res)
 }

+ pub(crate) fn from_args_enum_variant(


Item is a mess. It is a storage for all of our highlevel attribute information for a single type, field, variant, etc. We use the same type for all of those and for whichever trait or attribute type is used.

Making this function fits exactly within the current model

ysndr changed the title ~~feat(derive): derive Args for enum types~~ feat(derive): derive clap::Args for enum types Aug 25, 2024

ysndr mentioned this pull request Aug 25, 2024

Support for using ArgGroup as Enum with derive #2621

Open

2 tasks

ysndr added 2 commits August 25, 2024 18:52

chore: REMOVE WHEN DONE: init flake

f11570c

test: Arg groups from enum

3ecaa53

ysndr force-pushed the derive-arg-groups branch from 19629c4 to bd0e12f Compare August 25, 2024 16:52

ysndr commented Aug 25, 2024

View reviewed changes

epage reviewed Aug 26, 2024

View reviewed changes

ysndr added 3 commits August 26, 2024 20:05

feat: Derive clap::Args for enums

3249e4d

feat: Derive clap::FromArgMatches for enums

75eb4ee

feat: Impl update_from_arg_matches_mut for enums

bd54e23

ysndr force-pushed the derive-arg-groups branch from 8c3b3b0 to bd54e23 Compare August 26, 2024 18:05

epage reviewed Aug 26, 2024

View reviewed changes

YaLTeR mentioned this pull request Sep 2, 2024

Add by-id workspace referencing to the niri msg action CLI YaLTeR/niri#647

Open

epage mentioned this pull request Sep 4, 2024

Allow groups of groups #5711

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(derive): derive `clap::Args` for enum types #5700

feat(derive): derive `clap::Args` for enum types #5700

ysndr commented Aug 25, 2024

ysndr Aug 25, 2024

epage Aug 26, 2024

ysndr Aug 26, 2024

ysndr Aug 25, 2024

epage Aug 26, 2024

ysndr Aug 25, 2024

epage Aug 26, 2024

ysndr Aug 26, 2024

epage Aug 26, 2024

ysndr Aug 25, 2024

ysndr Aug 25, 2024

ysndr Aug 25, 2024

ysndr Aug 25, 2024

epage Aug 26, 2024

ysndr Aug 25, 2024

epage Aug 26, 2024

epage commented Aug 26, 2024

epage Aug 26, 2024

ysndr Aug 26, 2024

epage Aug 26, 2024

ysndr commented Aug 26, 2024

epage Aug 26, 2024

ysndr Aug 26, 2024

ysndr Aug 28, 2024

epage Aug 29, 2024

ysndr Aug 29, 2024 •

edited

Loading

epage Aug 30, 2024

epage Aug 26, 2024

epage Aug 26, 2024

feat(derive): derive clap::Args for enum types #5700

Are you sure you want to change the base?

feat(derive): derive clap::Args for enum types #5700

Conversation

ysndr commented Aug 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epage commented Aug 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ysndr commented Aug 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ysndr Aug 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat(derive): derive `clap::Args` for enum types #5700

feat(derive): derive `clap::Args` for enum types #5700

ysndr Aug 29, 2024 •

edited

Loading