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.

Sign up for GitHub

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

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f11570c
chore: REMOVE WHEN DONE: init flake
ysndr Aug 25, 2024
3ecaa53
test: Arg groups from enum
ysndr Aug 25, 2024
3249e4d
feat: Derive clap::Args for enums
ysndr Aug 25, 2024
75eb4ee
feat: Derive clap::FromArgMatches for enums
ysndr Aug 25, 2024
bd54e23
feat: Impl `update_from_arg_matches_mut` for enums
ysndr Aug 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
199 changes: 197 additions & 2 deletions clap_derive/src/derives/args.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ use syn::{
punctuated::Punctuated, spanned::Spanned, token::Comma, Data, DataStruct, DeriveInput, Field,
Fields, FieldsNamed, Generics,
};
use syn::{DataEnum, Variant};

use crate::item::{Item, Kind, Name};
use crate::utils::{inner_type, sub_type, Sp, Ty};
Expand Down Expand Up @@ -51,10 +52,188 @@ pub(crate) fn derive_args(input: &DeriveInput) -> Result<TokenStream, syn::Error
.collect::<Result<Vec<_>, syn::Error>>()?;
gen_for_struct(&item, ident, &input.generics, &fields)
}
Data::Enum(DataEnum { ref variants, .. }) => {
let name = Name::Derived(ident.clone());
let item = Item::from_args_struct(input, name)?;

let variant_items = variants
.iter()
.map(|variant| {
let item =
Item::from_args_enum_variant(variant, item.casing(), item.env_casing())?;
Ok((item, variant))
})
.collect::<Result<Vec<_>, syn::Error>>()?;

gen_for_enum(&item, ident, &input.generics, &variant_items)
}
_ => abort_call_site!("`#[derive(Args)]` only supports non-tuple structs"),
}
}

pub(crate) fn gen_for_enum(
_item: &Item,
item_name: &Ident,
generics: &Generics,
variants: &[(Item, &Variant)],
) -> Result<TokenStream, syn::Error> {
let (impl_generics, ty_generics, where_clause) = generics.split_for_impl();

let app_var = Ident::new("__clap_app", Span::call_site());
let mut augmentations = TokenStream::default();
let mut augmentations_update = TokenStream::default();

let mut constructors = TokenStream::default();
let mut updaters = TokenStream::default();

for (item, variant) in variants.iter() {
let Fields::Named(ref fields) = variant.fields else {
abort! { variant.span(),
"`#[derive(Args)]` only supports named enum variants if used on an enum",
}
};
Comment on lines +90 to +94
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

let group_id = item.group_id();

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)?;
Comment on lines +97 to +110
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@ysndr ysndr Aug 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

let augmentation = quote! {
let #app_var = #augmentation;
};
let augmentation_update = gen_augment(&fields, &app_var, item, &conflicts, true)?;
let augmentation_update = quote! {
let #app_var = #augmentation_update;
};

augmentations.extend(augmentation);
augmentations_update.extend(augmentation_update);

let variant_name = &variant.ident;
let genned_constructor = gen_constructor(&fields)?;
let constructor = quote! {
if __clap_arg_matches.contains_id(#group_id) {
let v = #item_name::#variant_name #genned_constructor;
return ::std::result::Result::Ok(v)
}
};

constructors.extend(constructor);

let genned_updater = gen_updater(&fields, false)?;

let field_names = fields
.iter()
.map(|(field, _)| field.ident.as_ref().unwrap());
let updater = quote! {

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,
    },
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

};
#genned_updater;
}
};

updaters.extend(updater);
}

let raw_deprecated = raw_deprecated();

Ok(quote! {
#[allow(
dead_code,
unreachable_code,
unused_variables,
unused_braces,
unused_qualifications,
)]
#[allow(
clippy::style,
clippy::complexity,
clippy::pedantic,
clippy::restriction,
clippy::perf,
clippy::deprecated,
clippy::nursery,
clippy::cargo,
clippy::suspicious_else_formatting,
clippy::almost_swapped,
clippy::redundant_locals,
)]
#[automatically_derived]
impl #impl_generics clap::FromArgMatches for #item_name #ty_generics #where_clause {
fn from_arg_matches(__clap_arg_matches: &clap::ArgMatches) -> ::std::result::Result<Self, clap::Error> {
Self::from_arg_matches_mut(&mut __clap_arg_matches.clone())
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

fn update_from_arg_matches(&mut self, __clap_arg_matches: &clap::ArgMatches) -> ::std::result::Result<(), clap::Error> {
self.update_from_arg_matches_mut(&mut __clap_arg_matches.clone())
}

fn update_from_arg_matches_mut(&mut self, __clap_arg_matches: &mut clap::ArgMatches) -> ::std::result::Result<(), clap::Error> {
#raw_deprecated
#updaters
::std::result::Result::Ok(())
}
}


#[allow(
dead_code,
unreachable_code,
unused_variables,
unused_braces,
unused_qualifications,
)]
#[allow(
clippy::style,
clippy::complexity,
clippy::pedantic,
clippy::restriction,
clippy::perf,
clippy::deprecated,
clippy::nursery,
clippy::cargo,
clippy::suspicious_else_formatting,
clippy::almost_swapped,
clippy::redundant_locals,
)]
#[automatically_derived]
impl #impl_generics clap::Args for #item_name #ty_generics #where_clause {
fn group_id() -> Option<clap::Id> {
// todo: how does this interact with nested groups here
None
}
fn augment_args<'b>(#app_var: clap::Command) -> clap::Command {
#augmentations
#app_var
}
fn augment_args_for_update<'b>(#app_var: clap::Command) -> clap::Command {
#augmentations_update
#app_var
}
}

})
}

pub(crate) fn gen_for_struct(
item: &Item,
item_name: &Ident,
Expand All @@ -75,8 +254,8 @@ pub(crate) fn gen_for_struct(
let raw_deprecated = raw_deprecated();

let app_var = Ident::new("__clap_app", Span::call_site());
let augmentation = gen_augment(fields, &app_var, item, false)?;
let augmentation_update = gen_augment(fields, &app_var, item, true)?;
let augmentation = gen_augment(fields, &app_var, item, &[], false)?;
let augmentation_update = gen_augment(fields, &app_var, item, &[], true)?;

let group_id = if item.skip_group() {
quote!(None)
Expand Down Expand Up @@ -170,6 +349,9 @@ pub(crate) fn gen_augment(
fields: &[(&Field, Item)],
app_var: &Ident,
parent_item: &Item,
// when generating mutably exclusive arguments,
// ids of arguments that should conflict
conflicts: &[Name],
Comment on lines +352 to +354
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

override_required: bool,
) -> Result<TokenStream, syn::Error> {
let mut subcommand_specified = false;
Expand Down Expand Up @@ -420,12 +602,25 @@ pub(crate) fn gen_augment(

let group_methods = parent_item.group_methods();

let conflicts_method = if conflicts.is_empty() {
quote!()
} else {
let conflicts_len = conflicts.len();
quote! {
.conflicts_with_all({
let conflicts: [clap::Id; #conflicts_len] = [#( clap::Id::from(#conflicts) ),* ];
conflicts
})
}
};

quote!(
.group(
clap::ArgGroup::new(#group_id)
.multiple(true)
#group_methods
.args(#literal_group_members)
#conflicts_method
)
)
};
Expand Down
2 changes: 1 addition & 1 deletion clap_derive/src/derives/subcommand.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,7 +282,7 @@ fn gen_augment(
Named(ref fields) => {
// Defer to `gen_augment` for adding cmd methods
let fields = collect_args_fields(item, fields)?;
args::gen_augment(&fields, &subcommand_var, item, override_required)?
args::gen_augment(&fields, &subcommand_var, item, &[], override_required)?
}
Unit => {
let arg_block = quote!( #subcommand_var );
Expand Down
56 changes: 56 additions & 0 deletions clap_derive/src/item.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,62 @@ impl Item {
Ok(res)
}

pub(crate) fn from_args_enum_variant(
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

variant: &Variant,
argument_casing: Sp<CasingStyle>,
env_casing: Sp<CasingStyle>,
) -> Result<Self, syn::Error> {
let name = variant.ident.clone();
let ident = variant.ident.clone();
let span = variant.span();

let ty = match variant.fields {
syn::Fields::Unnamed(syn::FieldsUnnamed { ref unnamed, .. }) if unnamed.len() == 1 => {
Ty::from_syn_ty(&unnamed[0].ty)
}
syn::Fields::Named(_) | syn::Fields::Unnamed(..) | syn::Fields::Unit => {
Sp::new(Ty::Other, span)
}
};
let kind = Sp::new(Kind::Command(ty), span);
let mut res = Self::new(
Name::Derived(name),
ident,
None,
argument_casing,
env_casing,
kind,
);
let parsed_attrs = ClapAttr::parse_all(&variant.attrs)?;
res.infer_kind(&parsed_attrs)?;
res.push_attrs(&parsed_attrs)?;
if matches!(&*res.kind, Kind::Command(_) | Kind::Subcommand(_)) {
res.push_doc_comment(&variant.attrs, "about", Some("long_about"));
}

// TODO: ???
match &*res.kind {
Kind::Flatten(_) => {
if res.has_explicit_methods() {
abort!(
res.kind.span(),
"methods are not allowed for flattened entry"
);
}
}

Kind::Subcommand(_)
| Kind::ExternalSubcommand
| Kind::FromGlobal(_)
| Kind::Skip(_, _)
| Kind::Command(_)
| Kind::Value
| Kind::Arg(_) => (),
}

Ok(res)
}

pub(crate) fn from_subcommand_enum(
input: &DeriveInput,
name: Name,
Expand Down
63 changes: 63 additions & 0 deletions flake.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading