From 9abcae2ac31b73b3939901787bf4bbbaffb6d43a Mon Sep 17 00:00:00 2001 From: Ed Page Date: Sat, 3 Sep 2022 20:56:33 -0500 Subject: [PATCH 01/17] refactor(derive): Separate parsing from pushing of attrs --- clap_derive/src/item.rs | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/clap_derive/src/item.rs b/clap_derive/src/item.rs index 21e02412236..0c003c999e0 100644 --- a/clap_derive/src/item.rs +++ b/clap_derive/src/item.rs @@ -88,7 +88,8 @@ impl Item { kind: Sp, ) -> Self { let mut res = Self::new(name, None, argument_casing, env_casing, kind); - res.push_attrs(attrs); + let parsed_attrs = ClapAttr::parse_all(attrs); + res.push_attrs(&parsed_attrs); res.push_doc_comment(attrs, "about"); res @@ -103,7 +104,8 @@ impl Item { let span = variant.span(); let kind = Sp::new(Kind::Command(Sp::new(Ty::Other, span)), span); let mut res = Self::new(Name::Derived(name), None, struct_casing, env_casing, kind); - res.push_attrs(&variant.attrs); + let parsed_attrs = ClapAttr::parse_all(&variant.attrs); + res.push_attrs(&parsed_attrs); res.push_doc_comment(&variant.attrs, "about"); match &*res.kind { @@ -182,7 +184,8 @@ impl Item { env_casing, kind, ); - res.push_attrs(&variant.attrs); + let parsed_attrs = ClapAttr::parse_all(&variant.attrs); + res.push_attrs(&parsed_attrs); res.push_doc_comment(&variant.attrs, "help"); res @@ -203,7 +206,8 @@ impl Item { env_casing, kind, ); - res.push_attrs(&field.attrs); + let parsed_attrs = ClapAttr::parse_all(&field.attrs); + res.push_attrs(&parsed_attrs); res.push_doc_comment(&field.attrs, "help"); match &*res.kind { @@ -376,10 +380,8 @@ impl Item { } } - fn push_attrs(&mut self, attrs: &[Attribute]) { - let parsed = ClapAttr::parse_all(attrs); - - for attr in &parsed { + fn push_attrs(&mut self, attrs: &[ClapAttr]) { + for attr in attrs { if let Some(AttrValue::Call(_)) = &attr.value { continue; } @@ -435,7 +437,7 @@ impl Item { } } - for attr in &parsed { + for attr in attrs { let actual_attr_kind = *attr.kind.get(); let expected_attr_kind = self.kind.attr_kind(); match (actual_attr_kind, expected_attr_kind) { @@ -576,7 +578,7 @@ impl Item { quote!(<#ty as ::std::default::Default>::default()) }; - let val = if parsed + let val = if attrs .iter() .any(|a| a.magic == Some(MagicAttrName::ValueEnum)) { @@ -628,7 +630,7 @@ impl Item { // Use `Borrow<#inner_type>` so we accept `&Vec<#inner_type>` and // `Vec<#inner_type>`. - let val = if parsed + let val = if attrs .iter() .any(|a| a.magic == Some(MagicAttrName::ValueEnum)) { @@ -691,7 +693,7 @@ impl Item { quote!(<#ty as ::std::default::Default>::default()) }; - let val = if parsed + let val = if attrs .iter() .any(|a| a.magic == Some(MagicAttrName::ValueEnum)) { @@ -743,7 +745,7 @@ impl Item { // Use `Borrow<#inner_type>` so we accept `&Vec<#inner_type>` and // `Vec<#inner_type>`. - let val = if parsed + let val = if attrs .iter() .any(|a| a.magic == Some(MagicAttrName::ValueEnum)) { From f5718216702e26ffcc51e0d2c9efbd3a76bf8ab4 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Sat, 3 Sep 2022 20:58:26 -0500 Subject: [PATCH 02/17] refactor(derive): Decouple attr and kind processing --- clap_derive/src/item.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/clap_derive/src/item.rs b/clap_derive/src/item.rs index 0c003c999e0..812b43e8070 100644 --- a/clap_derive/src/item.rs +++ b/clap_derive/src/item.rs @@ -89,6 +89,7 @@ impl Item { ) -> Self { let mut res = Self::new(name, None, argument_casing, env_casing, kind); let parsed_attrs = ClapAttr::parse_all(attrs); + res.infer_kind(&parsed_attrs); res.push_attrs(&parsed_attrs); res.push_doc_comment(attrs, "about"); @@ -105,6 +106,7 @@ impl Item { let kind = Sp::new(Kind::Command(Sp::new(Ty::Other, span)), span); let mut res = Self::new(Name::Derived(name), None, struct_casing, env_casing, kind); let parsed_attrs = ClapAttr::parse_all(&variant.attrs); + res.infer_kind(&parsed_attrs); res.push_attrs(&parsed_attrs); res.push_doc_comment(&variant.attrs, "about"); @@ -185,6 +187,7 @@ impl Item { kind, ); let parsed_attrs = ClapAttr::parse_all(&variant.attrs); + res.infer_kind(&parsed_attrs); res.push_attrs(&parsed_attrs); res.push_doc_comment(&variant.attrs, "help"); @@ -207,6 +210,7 @@ impl Item { kind, ); let parsed_attrs = ClapAttr::parse_all(&field.attrs); + res.infer_kind(&parsed_attrs); res.push_attrs(&parsed_attrs); res.push_doc_comment(&field.attrs, "help"); @@ -380,7 +384,7 @@ impl Item { } } - fn push_attrs(&mut self, attrs: &[ClapAttr]) { + fn infer_kind(&mut self, attrs: &[ClapAttr]) { for attr in attrs { if let Some(AttrValue::Call(_)) = &attr.value { continue; @@ -436,7 +440,9 @@ impl Item { self.set_kind(kind); } } + } + fn push_attrs(&mut self, attrs: &[ClapAttr]) { for attr in attrs { let actual_attr_kind = *attr.kind.get(); let expected_attr_kind = self.kind.attr_kind(); From 6648cf60095fcdca4221c165bf50eb91623b736f Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 5 Sep 2022 19:55:13 -0500 Subject: [PATCH 03/17] refactor(derive): Remove unused Type for values They have no type --- clap_derive/src/derives/args.rs | 6 +++--- clap_derive/src/derives/value_enum.rs | 2 +- clap_derive/src/item.rs | 16 ++++++++-------- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/clap_derive/src/derives/args.rs b/clap_derive/src/derives/args.rs index 5313c1dbdf0..ab2babb0699 100644 --- a/clap_derive/src/derives/args.rs +++ b/clap_derive/src/derives/args.rs @@ -202,7 +202,7 @@ pub fn gen_augment( let kind = item.kind(); match &*kind { Kind::Command(_) - | Kind::Value(_) + | Kind::Value | Kind::Subcommand(_) | Kind::Skip(_, _) | Kind::FromGlobal(_) @@ -346,7 +346,7 @@ pub fn gen_constructor(fields: &[(&Field, Item)]) -> TokenStream { let arg_matches = format_ident!("__clap_arg_matches"); match &*kind { Kind::Command(_) - | Kind::Value(_) + | Kind::Value | Kind::ExternalSubcommand => { abort! { kind.span(), "`{}` cannot be used with `arg`", @@ -417,7 +417,7 @@ pub fn gen_updater(fields: &[(&Field, Item)], use_self: bool) -> TokenStream { match &*kind { Kind::Command(_) - | Kind::Value(_) + | Kind::Value | Kind::ExternalSubcommand => { abort! { kind.span(), "`{}` cannot be used with `arg`", diff --git a/clap_derive/src/derives/value_enum.rs b/clap_derive/src/derives/value_enum.rs index c3184c03ff9..e724129bb68 100644 --- a/clap_derive/src/derives/value_enum.rs +++ b/clap_derive/src/derives/value_enum.rs @@ -42,7 +42,7 @@ pub fn derive_value_enum(input: &DeriveInput) -> TokenStream { } pub fn gen_for_enum(item: &Item, item_name: &Ident, variants: &[(&Variant, Item)]) -> TokenStream { - if !matches!(&*item.kind(), Kind::Value(_)) { + if !matches!(&*item.kind(), Kind::Value) { abort! { item.kind().span(), "`{}` cannot be used with `value`", item.kind().name(), diff --git a/clap_derive/src/item.rs b/clap_derive/src/item.rs index 812b43e8070..f0d029d62b8 100644 --- a/clap_derive/src/item.rs +++ b/clap_derive/src/item.rs @@ -76,7 +76,7 @@ impl Item { let attrs = &input.attrs; let argument_casing = Sp::call_site(DEFAULT_CASING); let env_casing = Sp::call_site(DEFAULT_ENV_CASING); - let kind = Sp::new(Kind::Value(Sp::new(Ty::Other, span)), span); + let kind = Sp::new(Kind::Value, span); Self::from_struct(attrs, name, argument_casing, env_casing, kind) } @@ -165,7 +165,7 @@ impl Item { | Kind::FromGlobal(_) | Kind::Skip(_, _) | Kind::Command(_) - | Kind::Value(_) + | Kind::Value | Kind::Arg(_) => (), } @@ -178,7 +178,7 @@ impl Item { env_casing: Sp, ) -> Self { let span = variant.span(); - let kind = Sp::new(Kind::Value(Sp::new(Ty::Other, span)), span); + let kind = Sp::new(Kind::Value, span); let mut res = Self::new( Name::Derived(variant.ident.clone()), None, @@ -304,7 +304,7 @@ impl Item { ); } - Kind::Command(_) | Kind::Value(_) | Kind::ExternalSubcommand => {} + Kind::Command(_) | Kind::Value | Kind::ExternalSubcommand => {} } res @@ -888,7 +888,7 @@ impl Item { | (Kind::Command(_), Kind::Flatten) | (Kind::Command(_), Kind::Skip(_, _)) | (Kind::Command(_), Kind::ExternalSubcommand) - | (Kind::Value(_), Kind::Skip(_, _)) => { + | (Kind::Value, Kind::Skip(_, _)) => { self.kind = kind; } @@ -1128,7 +1128,7 @@ fn default_action(field_type: &Type, span: Span) -> Method { pub enum Kind { Arg(Sp), Command(Sp), - Value(Sp), + Value, FromGlobal(Sp), Subcommand(Sp), Flatten, @@ -1141,7 +1141,7 @@ impl Kind { match self { Self::Arg(_) => "arg", Self::Command(_) => "command", - Self::Value(_) => "value", + Self::Value => "value", Self::FromGlobal(_) => "from_global", Self::Subcommand(_) => "subcommand", Self::Flatten => "flatten", @@ -1154,7 +1154,7 @@ impl Kind { match self { Self::Arg(_) => AttrKind::Arg, Self::Command(_) => AttrKind::Command, - Self::Value(_) => AttrKind::Value, + Self::Value => AttrKind::Value, Self::FromGlobal(_) => AttrKind::Arg, Self::Subcommand(_) => AttrKind::Command, Self::Flatten => AttrKind::Command, From f9e1ba2c1e11dd7c46f12c39479657c4dc92bda1 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 6 Sep 2022 06:55:43 -0500 Subject: [PATCH 04/17] fix(derive): Improve the default span --- clap_derive/src/item.rs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/clap_derive/src/item.rs b/clap_derive/src/item.rs index f0d029d62b8..2f0bdcda510 100644 --- a/clap_derive/src/item.rs +++ b/clap_derive/src/item.rs @@ -54,28 +54,28 @@ pub struct Item { impl Item { pub fn from_args_struct(input: &DeriveInput, name: Name) -> Self { - let span = Span::call_site(); + let span = input.ident.span(); let attrs = &input.attrs; - let argument_casing = Sp::call_site(DEFAULT_CASING); - let env_casing = Sp::call_site(DEFAULT_ENV_CASING); + let argument_casing = Sp::new(DEFAULT_CASING, span); + let env_casing = Sp::new(DEFAULT_ENV_CASING, span); let kind = Sp::new(Kind::Command(Sp::new(Ty::Other, span)), span); Self::from_struct(attrs, name, argument_casing, env_casing, kind) } pub fn from_subcommand_enum(input: &DeriveInput, name: Name) -> Self { - let span = Span::call_site(); + let span = input.ident.span(); let attrs = &input.attrs; - let argument_casing = Sp::call_site(DEFAULT_CASING); - let env_casing = Sp::call_site(DEFAULT_ENV_CASING); + let argument_casing = Sp::new(DEFAULT_CASING, span); + let env_casing = Sp::new(DEFAULT_ENV_CASING, span); let kind = Sp::new(Kind::Command(Sp::new(Ty::Other, span)), span); Self::from_struct(attrs, name, argument_casing, env_casing, kind) } pub fn from_value_enum(input: &DeriveInput, name: Name) -> Self { - let span = Span::call_site(); + let span = input.ident.span(); let attrs = &input.attrs; - let argument_casing = Sp::call_site(DEFAULT_CASING); - let env_casing = Sp::call_site(DEFAULT_ENV_CASING); + let argument_casing = Sp::new(DEFAULT_CASING, span); + let env_casing = Sp::new(DEFAULT_ENV_CASING, span); let kind = Sp::new(Kind::Value, span); Self::from_struct(attrs, name, argument_casing, env_casing, kind) } From b29f3ff22f90de7d68b65c857f956f0081e89b4d Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 6 Sep 2022 07:10:38 -0500 Subject: [PATCH 05/17] refactor(derive): Delay error handling --- clap_derive/src/derives/args.rs | 11 +++++- clap_derive/src/item.rs | 48 ++--------------------- clap_derive/src/utils/ty.rs | 10 +++++ tests/derive_ui/subcommand_opt_opt.stderr | 6 +-- tests/derive_ui/subcommand_opt_vec.stderr | 6 +-- 5 files changed, 29 insertions(+), 52 deletions(-) diff --git a/clap_derive/src/derives/args.rs b/clap_derive/src/derives/args.rs index ab2babb0699..0af76b63da6 100644 --- a/clap_derive/src/derives/args.rs +++ b/clap_derive/src/derives/args.rs @@ -370,13 +370,22 @@ pub fn gen_constructor(fields: &[(&Field, Item)]) -> TokenStream { } } }, - _ => { + Ty::Vec | + Ty::Other => { quote_spanned! { kind.span()=> #field_name: { <#subcmd_type as clap::FromArgMatches>::from_arg_matches_mut(#arg_matches)? } } }, + Ty::OptionOption | + Ty::OptionVec => { + abort!( + ty.span(), + "{} types are not supported for subcommand", + ty.as_str() + ); + } } } diff --git a/clap_derive/src/item.rs b/clap_derive/src/item.rs index 2f0bdcda510..a7386964511 100644 --- a/clap_derive/src/item.rs +++ b/clap_derive/src/item.rs @@ -126,38 +126,12 @@ impl Item { Kind::Subcommand(_) => { use syn::Fields::*; use syn::FieldsUnnamed; - let field_ty = match variant.fields { - Named(_) => { - abort!(variant.span(), "structs are not allowed for subcommand"); - } - Unit => abort!(variant.span(), "unit-type is not allowed for subcommand"), + let ty = match variant.fields { Unnamed(FieldsUnnamed { ref unnamed, .. }) if unnamed.len() == 1 => { - &unnamed[0].ty - } - Unnamed(..) => { - abort!( - variant, - "non single-typed tuple is not allowed for subcommand" - ) + Ty::from_syn_ty(&unnamed[0].ty) } + Named(_) | Unnamed(..) | Unit => Sp::new(Ty::Other, span), }; - let ty = Ty::from_syn_ty(field_ty); - match *ty { - Ty::OptionOption => { - abort!( - field_ty, - "Option> type is not allowed for subcommand" - ); - } - Ty::OptionVec => { - abort!( - field_ty, - "Option> type is not allowed for subcommand" - ); - } - _ => (), - } - res.kind = Sp::new(Kind::Subcommand(ty), res.kind.span()); } @@ -236,22 +210,6 @@ impl Item { } let ty = Ty::from_syn_ty(&field.ty); - match *ty { - Ty::OptionOption => { - abort!( - field.ty, - "Option> type is not allowed for subcommand" - ); - } - Ty::OptionVec => { - abort!( - field.ty, - "Option> type is not allowed for subcommand" - ); - } - _ => (), - } - res.kind = Sp::new(Kind::Subcommand(ty), res.kind.span()); } Kind::Skip(_, _) => { diff --git a/clap_derive/src/utils/ty.rs b/clap_derive/src/utils/ty.rs index 517f564e72a..a540e5f623f 100644 --- a/clap_derive/src/utils/ty.rs +++ b/clap_derive/src/utils/ty.rs @@ -35,6 +35,16 @@ impl Ty { t(Other) } } + + pub fn as_str(&self) -> &'static str { + match self { + Self::Vec => "Vec", + Self::Option => "Option", + Self::OptionOption => "Option>", + Self::OptionVec => "Option>", + Self::Other => "...other...", + } + } } pub fn inner_type(field_ty: &syn::Type) -> &syn::Type { diff --git a/tests/derive_ui/subcommand_opt_opt.stderr b/tests/derive_ui/subcommand_opt_opt.stderr index 1b4eaf8eb24..b1df1744d57 100644 --- a/tests/derive_ui/subcommand_opt_opt.stderr +++ b/tests/derive_ui/subcommand_opt_opt.stderr @@ -1,5 +1,5 @@ -error: Option> type is not allowed for subcommand - --> $DIR/subcommand_opt_opt.rs:17:10 +error: Option> types are not supported for subcommand + --> tests/derive_ui/subcommand_opt_opt.rs:17:10 | 17 | cmd: Option>, - | ^^^^^^^^^^^^^^^^^^^^^^^ + | ^^^^^^ diff --git a/tests/derive_ui/subcommand_opt_vec.stderr b/tests/derive_ui/subcommand_opt_vec.stderr index 6cd191576b6..f1457674ef5 100644 --- a/tests/derive_ui/subcommand_opt_vec.stderr +++ b/tests/derive_ui/subcommand_opt_vec.stderr @@ -1,5 +1,5 @@ -error: Option> type is not allowed for subcommand - --> $DIR/subcommand_opt_vec.rs:17:10 +error: Option> types are not supported for subcommand + --> tests/derive_ui/subcommand_opt_vec.rs:17:10 | 17 | cmd: Option>, - | ^^^^^^^^^^^^^^^^^^^^ + | ^^^^^^ From ade931be57cdd0f66f55ef3b43030d4955ad13f9 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 6 Sep 2022 07:13:53 -0500 Subject: [PATCH 06/17] fix(derive): Be less prescriptive With how flexible clap's API is, it can be hard to determine what is reasonable to use with its API, so let's just stop. --- clap_derive/src/item.rs | 26 -------------------- tests/derive_ui/opt_opt_nonpositional.rs | 20 --------------- tests/derive_ui/opt_opt_nonpositional.stderr | 5 ---- tests/derive_ui/opt_vec_nonpositional.rs | 20 --------------- tests/derive_ui/opt_vec_nonpositional.stderr | 5 ---- tests/derive_ui/option_default_value.rs | 21 ---------------- tests/derive_ui/option_default_value.stderr | 5 ---- 7 files changed, 102 deletions(-) delete mode 100644 tests/derive_ui/opt_opt_nonpositional.rs delete mode 100644 tests/derive_ui/opt_opt_nonpositional.stderr delete mode 100644 tests/derive_ui/opt_vec_nonpositional.rs delete mode 100644 tests/derive_ui/opt_vec_nonpositional.stderr delete mode 100644 tests/derive_ui/option_default_value.rs delete mode 100644 tests/derive_ui/option_default_value.stderr diff --git a/clap_derive/src/item.rs b/clap_derive/src/item.rs index a7386964511..ff8664183c1 100644 --- a/clap_derive/src/item.rs +++ b/clap_derive/src/item.rs @@ -226,32 +226,6 @@ impl Item { } Kind::Arg(_) => { let ty = Ty::from_syn_ty(&field.ty); - - match *ty { - Ty::Option => { - if let Some(m) = res.find_default_method() { - abort!(m.name, "default_value is meaningless for Option") - } - } - Ty::OptionOption => { - if res.is_positional() { - abort!( - field.ty, - "Option> type is meaningless for positional argument" - ) - } - } - Ty::OptionVec => { - if res.is_positional() { - abort!( - field.ty, - "Option> type is meaningless for positional argument" - ) - } - } - - _ => (), - } res.kind = Sp::new( Kind::Arg(ty), field diff --git a/tests/derive_ui/opt_opt_nonpositional.rs b/tests/derive_ui/opt_opt_nonpositional.rs deleted file mode 100644 index af018ab9760..00000000000 --- a/tests/derive_ui/opt_opt_nonpositional.rs +++ /dev/null @@ -1,20 +0,0 @@ -// Copyright 2018 Guillaume Pinot (@TeXitoi) -// -// Licensed under the Apache License, Version 2.0 or the MIT license -// , at your -// option. This file may not be copied, modified, or distributed -// except according to those terms. - -use clap::Parser; - -#[derive(Parser, Debug)] -#[command(name = "basic")] -struct Opt { - n: Option>, -} - -fn main() { - let opt = Opt::parse(); - println!("{:?}", opt); -} diff --git a/tests/derive_ui/opt_opt_nonpositional.stderr b/tests/derive_ui/opt_opt_nonpositional.stderr deleted file mode 100644 index cb9f1728975..00000000000 --- a/tests/derive_ui/opt_opt_nonpositional.stderr +++ /dev/null @@ -1,5 +0,0 @@ -error: Option> type is meaningless for positional argument - --> $DIR/opt_opt_nonpositional.rs:14:8 - | -14 | n: Option>, - | ^^^^^^^^^^^^^^^^^^^ diff --git a/tests/derive_ui/opt_vec_nonpositional.rs b/tests/derive_ui/opt_vec_nonpositional.rs deleted file mode 100644 index 169c576178d..00000000000 --- a/tests/derive_ui/opt_vec_nonpositional.rs +++ /dev/null @@ -1,20 +0,0 @@ -// Copyright 2018 Guillaume Pinot (@TeXitoi) -// -// Licensed under the Apache License, Version 2.0 or the MIT license -// , at your -// option. This file may not be copied, modified, or distributed -// except according to those terms. - -use clap::Parser; - -#[derive(Parser, Debug)] -#[command(name = "basic")] -struct Opt { - n: Option>, -} - -fn main() { - let opt = Opt::parse(); - println!("{:?}", opt); -} diff --git a/tests/derive_ui/opt_vec_nonpositional.stderr b/tests/derive_ui/opt_vec_nonpositional.stderr deleted file mode 100644 index c6b343f947d..00000000000 --- a/tests/derive_ui/opt_vec_nonpositional.stderr +++ /dev/null @@ -1,5 +0,0 @@ -error: Option> type is meaningless for positional argument - --> $DIR/opt_vec_nonpositional.rs:14:8 - | -14 | n: Option>, - | ^^^^^^^^^^^^^^^^ diff --git a/tests/derive_ui/option_default_value.rs b/tests/derive_ui/option_default_value.rs deleted file mode 100644 index 72a6f07e79d..00000000000 --- a/tests/derive_ui/option_default_value.rs +++ /dev/null @@ -1,21 +0,0 @@ -// Copyright 2018 Guillaume Pinot (@TeXitoi) -// -// Licensed under the Apache License, Version 2.0 or the MIT license -// , at your -// option. This file may not be copied, modified, or distributed -// except according to those terms. - -use clap::Parser; - -#[derive(Parser, Debug)] -#[command(name = "basic")] -struct Opt { - #[arg(short, default_value = 1)] - n: Option, -} - -fn main() { - let opt = Opt::parse(); - println!("{:?}", opt); -} diff --git a/tests/derive_ui/option_default_value.stderr b/tests/derive_ui/option_default_value.stderr deleted file mode 100644 index 38ee3296f71..00000000000 --- a/tests/derive_ui/option_default_value.stderr +++ /dev/null @@ -1,5 +0,0 @@ -error: default_value is meaningless for Option - --> tests/derive_ui/option_default_value.rs:14:18 - | -14 | #[arg(short, default_value = 1)] - | ^^^^^^^^^^^^^ From 71b9209a34f8762c0a2e06ac8ff8e996afcf1ccc Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 6 Sep 2022 07:24:31 -0500 Subject: [PATCH 07/17] refactor(derive): Simplify Kind initialization --- clap_derive/src/item.rs | 74 ++++++++++++++++---------------- clap_derive/src/utils/spanned.rs | 7 --- 2 files changed, 37 insertions(+), 44 deletions(-) diff --git a/clap_derive/src/item.rs b/clap_derive/src/item.rs index ff8664183c1..6265ff0cb94 100644 --- a/clap_derive/src/item.rs +++ b/clap_derive/src/item.rs @@ -103,7 +103,15 @@ impl Item { ) -> Self { let name = variant.ident.clone(); let span = variant.span(); - let kind = Sp::new(Kind::Command(Sp::new(Ty::Other, span)), 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), None, struct_casing, env_casing, kind); let parsed_attrs = ClapAttr::parse_all(&variant.attrs); res.infer_kind(&parsed_attrs); @@ -123,19 +131,8 @@ impl Item { res.doc_comment = vec![]; } - Kind::Subcommand(_) => { - use syn::Fields::*; - use syn::FieldsUnnamed; - let ty = match variant.fields { - Unnamed(FieldsUnnamed { ref unnamed, .. }) if unnamed.len() == 1 => { - Ty::from_syn_ty(&unnamed[0].ty) - } - Named(_) | Unnamed(..) | Unit => Sp::new(Ty::Other, span), - }; - res.kind = Sp::new(Kind::Subcommand(ty), res.kind.span()); - } - - Kind::ExternalSubcommand + Kind::Subcommand(_) + | Kind::ExternalSubcommand | Kind::FromGlobal(_) | Kind::Skip(_, _) | Kind::Command(_) @@ -175,7 +172,8 @@ impl Item { ) -> Self { let name = field.ident.clone().unwrap(); let span = field.span(); - let kind = Sp::new(Kind::Arg(Sp::new(Ty::Other, span)), span); + let ty = Ty::from_syn_ty(&field.ty); + let kind = Sp::new(Kind::Arg(ty), span); let mut res = Self::new( Name::Derived(name), Some(field.ty.clone()), @@ -208,9 +206,6 @@ impl Item { "methods in attributes are not allowed for subcommand" ); } - - let ty = Ty::from_syn_ty(&field.ty); - res.kind = Sp::new(Kind::Subcommand(ty), res.kind.span()); } Kind::Skip(_, _) => { if res.has_explicit_methods() { @@ -220,23 +215,11 @@ impl Item { ); } } - Kind::FromGlobal(orig_ty) => { - let ty = Ty::from_syn_ty(&field.ty); - res.kind = Sp::new(Kind::FromGlobal(ty), orig_ty.span()); - } - Kind::Arg(_) => { - let ty = Ty::from_syn_ty(&field.ty); - res.kind = Sp::new( - Kind::Arg(ty), - field - .ident - .as_ref() - .map(|i| i.span()) - .unwrap_or_else(|| field.ty.span()), - ); - } - - Kind::Command(_) | Kind::Value | Kind::ExternalSubcommand => {} + Kind::FromGlobal(_) + | Kind::Arg(_) + | Kind::Command(_) + | Kind::Value + | Kind::ExternalSubcommand => {} } res @@ -328,7 +311,11 @@ impl Item { let expr = attr.value_or_abort(); abort!(expr, "attribute `{}` does not accept a value", attr.name); } - let ty = Sp::call_site(Ty::Other); + let ty = self + .kind() + .ty() + .cloned() + .unwrap_or_else(|| Sp::new(Ty::Other, self.kind.span())); let kind = Sp::new(Kind::FromGlobal(ty), attr.name.clone().span()); Some(kind) } @@ -337,7 +324,11 @@ impl Item { let expr = attr.value_or_abort(); abort!(expr, "attribute `{}` does not accept a value", attr.name); } - let ty = Sp::call_site(Ty::Other); + let ty = self + .kind() + .ty() + .cloned() + .unwrap_or_else(|| Sp::new(Ty::Other, self.kind.span())); let kind = Sp::new(Kind::Subcommand(ty), attr.name.clone().span()); Some(kind) } @@ -1094,6 +1085,15 @@ impl Kind { Self::ExternalSubcommand => AttrKind::Command, } } + + pub fn ty(&self) -> Option<&Sp> { + match self { + Self::Arg(ty) | Self::Command(ty) | Self::FromGlobal(ty) | Self::Subcommand(ty) => { + Some(ty) + } + Self::Value | Self::Flatten | Self::Skip(_, _) | Self::ExternalSubcommand => None, + } + } } #[derive(Clone)] diff --git a/clap_derive/src/utils/spanned.rs b/clap_derive/src/utils/spanned.rs index e064b53f037..339a654e6d7 100644 --- a/clap_derive/src/utils/spanned.rs +++ b/clap_derive/src/utils/spanned.rs @@ -16,13 +16,6 @@ impl Sp { Sp { val, span } } - pub fn call_site(val: T) -> Self { - Sp { - val, - span: Span::call_site(), - } - } - pub fn get(&self) -> &T { &self.val } From eece51fe0422b7c68b3830c5b519c472d384c732 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 6 Sep 2022 10:35:19 -0500 Subject: [PATCH 08/17] refactor(derive): Give more control to the derive --- clap_derive/src/item.rs | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/clap_derive/src/item.rs b/clap_derive/src/item.rs index 6265ff0cb94..5142997e921 100644 --- a/clap_derive/src/item.rs +++ b/clap_derive/src/item.rs @@ -59,7 +59,14 @@ impl Item { let argument_casing = Sp::new(DEFAULT_CASING, span); let env_casing = Sp::new(DEFAULT_ENV_CASING, span); let kind = Sp::new(Kind::Command(Sp::new(Ty::Other, span)), span); - Self::from_struct(attrs, name, argument_casing, env_casing, kind) + + let mut res = Self::new(name, None, argument_casing, env_casing, kind); + let parsed_attrs = ClapAttr::parse_all(attrs); + res.infer_kind(&parsed_attrs); + res.push_attrs(&parsed_attrs); + res.push_doc_comment(attrs, "about"); + + res } pub fn from_subcommand_enum(input: &DeriveInput, name: Name) -> Self { @@ -68,7 +75,14 @@ impl Item { let argument_casing = Sp::new(DEFAULT_CASING, span); let env_casing = Sp::new(DEFAULT_ENV_CASING, span); let kind = Sp::new(Kind::Command(Sp::new(Ty::Other, span)), span); - Self::from_struct(attrs, name, argument_casing, env_casing, kind) + + let mut res = Self::new(name, None, argument_casing, env_casing, kind); + let parsed_attrs = ClapAttr::parse_all(attrs); + res.infer_kind(&parsed_attrs); + res.push_attrs(&parsed_attrs); + res.push_doc_comment(attrs, "about"); + + res } pub fn from_value_enum(input: &DeriveInput, name: Name) -> Self { @@ -77,16 +91,7 @@ impl Item { let argument_casing = Sp::new(DEFAULT_CASING, span); let env_casing = Sp::new(DEFAULT_ENV_CASING, span); let kind = Sp::new(Kind::Value, span); - Self::from_struct(attrs, name, argument_casing, env_casing, kind) - } - fn from_struct( - attrs: &[Attribute], - name: Name, - argument_casing: Sp, - env_casing: Sp, - kind: Sp, - ) -> Self { let mut res = Self::new(name, None, argument_casing, env_casing, kind); let parsed_attrs = ClapAttr::parse_all(attrs); res.infer_kind(&parsed_attrs); From 1d973bd1c158281a1f33a549467100caf23d9571 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 6 Sep 2022 10:36:22 -0500 Subject: [PATCH 09/17] perf(derive): Don't parse ValueEnum's doc comment to ignore it --- clap_derive/src/item.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/clap_derive/src/item.rs b/clap_derive/src/item.rs index 5142997e921..d7b930c2702 100644 --- a/clap_derive/src/item.rs +++ b/clap_derive/src/item.rs @@ -96,7 +96,8 @@ impl Item { let parsed_attrs = ClapAttr::parse_all(attrs); res.infer_kind(&parsed_attrs); res.push_attrs(&parsed_attrs); - res.push_doc_comment(attrs, "about"); + // Ignoring `push_doc_comment` as there is no top-level clap builder to add documentation + // to res } From 9eb72ea5b6bc8931fba6ef78c4e02b27861812f9 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 6 Sep 2022 10:40:12 -0500 Subject: [PATCH 10/17] fix(derive): Disallow attributes on top-level ValuEnum --- clap_derive/src/item.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/clap_derive/src/item.rs b/clap_derive/src/item.rs index d7b930c2702..e4695d8bd3d 100644 --- a/clap_derive/src/item.rs +++ b/clap_derive/src/item.rs @@ -99,6 +99,14 @@ impl Item { // Ignoring `push_doc_comment` as there is no top-level clap builder to add documentation // to + if res.has_explicit_methods() { + abort!( + res.methods[0].name.span(), + "{} doesn't exist for `ValueEnum` enums", + res.methods[0].name + ); + } + res } From 8a5a9ba931dd45a4976a5600eb24a91b202472f3 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 6 Sep 2022 10:43:16 -0500 Subject: [PATCH 11/17] perf(derive): Only parse doc comments when needed for field/variant --- clap_derive/src/item.rs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/clap_derive/src/item.rs b/clap_derive/src/item.rs index e4695d8bd3d..c19310eaf56 100644 --- a/clap_derive/src/item.rs +++ b/clap_derive/src/item.rs @@ -130,7 +130,9 @@ impl Item { let parsed_attrs = ClapAttr::parse_all(&variant.attrs); res.infer_kind(&parsed_attrs); res.push_attrs(&parsed_attrs); - res.push_doc_comment(&variant.attrs, "about"); + if matches!(&*res.kind, Kind::Command(_)) { + res.push_doc_comment(&variant.attrs, "about"); + } match &*res.kind { Kind::Flatten => { @@ -140,9 +142,6 @@ impl Item { "methods are not allowed for flattened entry" ); } - - // ignore doc comments - res.doc_comment = vec![]; } Kind::Subcommand(_) @@ -174,7 +173,9 @@ impl Item { let parsed_attrs = ClapAttr::parse_all(&variant.attrs); res.infer_kind(&parsed_attrs); res.push_attrs(&parsed_attrs); - res.push_doc_comment(&variant.attrs, "help"); + if matches!(&*res.kind, Kind::Value) { + res.push_doc_comment(&variant.attrs, "help"); + } res } @@ -198,7 +199,9 @@ impl Item { let parsed_attrs = ClapAttr::parse_all(&field.attrs); res.infer_kind(&parsed_attrs); res.push_attrs(&parsed_attrs); - res.push_doc_comment(&field.attrs, "help"); + if matches!(&*res.kind, Kind::Arg(_)) { + res.push_doc_comment(&field.attrs, "help"); + } match &*res.kind { Kind::Flatten => { @@ -208,9 +211,6 @@ impl Item { "methods are not allowed for flattened entry" ); } - - // ignore doc comments - res.doc_comment = vec![]; } Kind::Subcommand(_) => { From abcee38466a7f5d90e7841312dde57955d8c0407 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 6 Sep 2022 10:48:54 -0500 Subject: [PATCH 12/17] fix(derive): Improve `skip` method error Really this is about consolidating the skip checks but it also provideda chance to improve the error. --- clap_derive/src/item.rs | 22 +++++++++++-------- .../derive_ui/skip_with_other_options.stderr | 6 ++--- 2 files changed, 16 insertions(+), 12 deletions(-) diff --git a/clap_derive/src/item.rs b/clap_derive/src/item.rs index c19310eaf56..fffc62f91be 100644 --- a/clap_derive/src/item.rs +++ b/clap_derive/src/item.rs @@ -221,15 +221,8 @@ impl Item { ); } } - Kind::Skip(_, _) => { - if res.has_explicit_methods() { - abort!( - res.kind.span(), - "methods are not allowed for skipped fields" - ); - } - } - Kind::FromGlobal(_) + Kind::Skip(_, _) + | Kind::FromGlobal(_) | Kind::Arg(_) | Kind::Command(_) | Kind::Value @@ -792,6 +785,17 @@ impl Item { } } } + + if let Kind::Skip(_, attr) = &*self.kind { + if self.has_explicit_methods() { + abort!( + self.methods[0].name.span(), + "`{}` cannot be used with `#[{}(skip)]", + self.methods[0].name, + attr.as_str(), + ); + } + } } fn push_doc_comment(&mut self, attrs: &[Attribute], name: &str) { diff --git a/tests/derive_ui/skip_with_other_options.stderr b/tests/derive_ui/skip_with_other_options.stderr index c7db5f4d31c..dc15f44c773 100644 --- a/tests/derive_ui/skip_with_other_options.stderr +++ b/tests/derive_ui/skip_with_other_options.stderr @@ -1,5 +1,5 @@ -error: methods are not allowed for skipped fields - --> tests/derive_ui/skip_with_other_options.rs:8:11 +error: `long` cannot be used with `#[arg(skip)] + --> tests/derive_ui/skip_with_other_options.rs:8:17 | 8 | #[arg(skip, long)] - | ^^^^ + | ^^^^ From a71b2eb8b8a75f872344b1e4eb61fdbd9d8ece3d Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 6 Sep 2022 11:33:13 -0500 Subject: [PATCH 13/17] refactor(derive): Shift responsibility for long help --- clap_derive/src/derives/args.rs | 2 +- clap_derive/src/derives/value_enum.rs | 2 +- clap_derive/src/item.rs | 35 +++++++++++---------------- clap_derive/src/utils/doc_comments.rs | 24 ++++++++++-------- 4 files changed, 30 insertions(+), 33 deletions(-) diff --git a/clap_derive/src/derives/args.rs b/clap_derive/src/derives/args.rs index 0af76b63da6..9f777b55f3a 100644 --- a/clap_derive/src/derives/args.rs +++ b/clap_derive/src/derives/args.rs @@ -299,7 +299,7 @@ pub fn gen_augment( }; let id = item.id(); - let explicit_methods = item.field_methods(true); + let explicit_methods = item.field_methods(); let deprecations = if !override_required { item.deprecations() } else { diff --git a/clap_derive/src/derives/value_enum.rs b/clap_derive/src/derives/value_enum.rs index e724129bb68..a1411d02ab0 100644 --- a/clap_derive/src/derives/value_enum.rs +++ b/clap_derive/src/derives/value_enum.rs @@ -84,7 +84,7 @@ fn lits(variants: &[(&Variant, Item)]) -> Vec<(TokenStream, Ident)> { if !matches!(variant.fields, Fields::Unit) { abort!(variant.span(), "`#[derive(ValueEnum)]` only supports unit variants. Non-unit variants must be skipped"); } - let fields = item.field_methods(false); + let fields = item.field_methods(); let deprecations = item.deprecations(); let name = item.cased_name(); Some(( diff --git a/clap_derive/src/item.rs b/clap_derive/src/item.rs index fffc62f91be..b5127ebd943 100644 --- a/clap_derive/src/item.rs +++ b/clap_derive/src/item.rs @@ -64,7 +64,7 @@ impl Item { let parsed_attrs = ClapAttr::parse_all(attrs); res.infer_kind(&parsed_attrs); res.push_attrs(&parsed_attrs); - res.push_doc_comment(attrs, "about"); + res.push_doc_comment(attrs, "about", true); res } @@ -80,7 +80,7 @@ impl Item { let parsed_attrs = ClapAttr::parse_all(attrs); res.infer_kind(&parsed_attrs); res.push_attrs(&parsed_attrs); - res.push_doc_comment(attrs, "about"); + res.push_doc_comment(attrs, "about", true); res } @@ -131,7 +131,7 @@ impl Item { res.infer_kind(&parsed_attrs); res.push_attrs(&parsed_attrs); if matches!(&*res.kind, Kind::Command(_)) { - res.push_doc_comment(&variant.attrs, "about"); + res.push_doc_comment(&variant.attrs, "about", true); } match &*res.kind { @@ -174,7 +174,7 @@ impl Item { res.infer_kind(&parsed_attrs); res.push_attrs(&parsed_attrs); if matches!(&*res.kind, Kind::Value) { - res.push_doc_comment(&variant.attrs, "help"); + res.push_doc_comment(&variant.attrs, "help", false); } res @@ -200,7 +200,7 @@ impl Item { res.infer_kind(&parsed_attrs); res.push_attrs(&parsed_attrs); if matches!(&*res.kind, Kind::Arg(_)) { - res.push_doc_comment(&field.attrs, "help"); + res.push_doc_comment(&field.attrs, "help", true); } match &*res.kind { @@ -798,7 +798,7 @@ impl Item { } } - fn push_doc_comment(&mut self, attrs: &[Attribute], name: &str) { + fn push_doc_comment(&mut self, attrs: &[Attribute], name: &str, supports_long_help: bool) { use syn::Lit::*; use syn::Meta::*; @@ -816,7 +816,11 @@ impl Item { }) .collect(); - self.doc_comment = process_doc_comment(comment_parts, name, !self.verbatim_doc_comment); + let (short, long) = process_doc_comment(comment_parts, name, !self.verbatim_doc_comment); + self.doc_comment.extend(short); + if supports_long_help { + self.doc_comment.extend(long); + } } fn set_kind(&mut self, kind: Sp) { @@ -865,21 +869,10 @@ impl Item { } /// generate methods on top of a field - pub fn field_methods(&self, supports_long_help: bool) -> proc_macro2::TokenStream { + pub fn field_methods(&self) -> proc_macro2::TokenStream { let methods = &self.methods; - match supports_long_help { - true => { - let doc_comment = &self.doc_comment; - quote!( #(#doc_comment)* #(#methods)* ) - } - false => { - let doc_comment = self - .doc_comment - .iter() - .filter(|mth| mth.name != "long_help"); - quote!( #(#doc_comment)* #(#methods)* ) - } - } + let doc_comment = &self.doc_comment; + quote!( #(#doc_comment)* #(#methods)* ) } pub fn deprecations(&self) -> proc_macro2::TokenStream { diff --git a/clap_derive/src/utils/doc_comments.rs b/clap_derive/src/utils/doc_comments.rs index a815874cb56..0da1f3b0267 100644 --- a/clap_derive/src/utils/doc_comments.rs +++ b/clap_derive/src/utils/doc_comments.rs @@ -8,7 +8,11 @@ use crate::item::Method; use quote::{format_ident, quote}; use std::iter; -pub fn process_doc_comment(lines: Vec, name: &str, preprocess: bool) -> Vec { +pub fn process_doc_comment( + lines: Vec, + name: &str, + preprocess: bool, +) -> (Option, Option) { // multiline comments (`/** ... */`) may have LFs (`\n`) in them, // we need to split so we could handle the lines correctly // @@ -31,7 +35,7 @@ pub fn process_doc_comment(lines: Vec, name: &str, preprocess: bool) -> } if lines.is_empty() { - return vec![]; + return (None, None); } let short_name = format_ident!("{}", name); @@ -49,10 +53,10 @@ pub fn process_doc_comment(lines: Vec, name: &str, preprocess: bool) -> (short, long) }; - vec![ - Method::new(short_name, quote!(#short)), - Method::new(long_name, quote!(#long)), - ] + ( + Some(Method::new(short_name, quote!(#short))), + Some(Method::new(long_name, quote!(#long))), + ) } else { let short = if preprocess { let s = merge_lines(&lines); @@ -61,10 +65,10 @@ pub fn process_doc_comment(lines: Vec, name: &str, preprocess: bool) -> lines.join("\n") }; - vec![ - Method::new(short_name, quote!(#short)), - Method::new(long_name, quote!(None)), - ] + ( + Some(Method::new(short_name, quote!(#short))), + Some(Method::new(long_name, quote!(None))), + ) } } From 222003abe15637ff231c08c916620c0170c75961 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 6 Sep 2022 11:41:03 -0500 Subject: [PATCH 14/17] fix(derive): Disallow general attributes with from_global --- clap_derive/src/item.rs | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/clap_derive/src/item.rs b/clap_derive/src/item.rs index b5127ebd943..e08e8ad441c 100644 --- a/clap_derive/src/item.rs +++ b/clap_derive/src/item.rs @@ -786,8 +786,8 @@ impl Item { } } - if let Kind::Skip(_, attr) = &*self.kind { - if self.has_explicit_methods() { + if self.has_explicit_methods() { + if let Kind::Skip(_, attr) = &*self.kind { abort!( self.methods[0].name.span(), "`{}` cannot be used with `#[{}(skip)]", @@ -795,6 +795,13 @@ impl Item { attr.as_str(), ); } + if let Kind::FromGlobal(_) = &*self.kind { + abort!( + self.methods[0].name.span(), + "`{}` cannot be used with `#[arg(from_global)]", + self.methods[0].name, + ); + } } } From 51dc3c63d6c74ab359d2a92dd0fc428f81cad660 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 6 Sep 2022 11:49:15 -0500 Subject: [PATCH 15/17] refactor(derive): Treat subcommands like other Kinds --- clap_derive/src/derives/args.rs | 75 ++++++++++++++------------------- 1 file changed, 32 insertions(+), 43 deletions(-) diff --git a/clap_derive/src/derives/args.rs b/clap_derive/src/derives/args.rs index 9f777b55f3a..05256650b47 100644 --- a/clap_derive/src/derives/args.rs +++ b/clap_derive/src/derives/args.rs @@ -157,56 +157,46 @@ pub fn gen_augment( parent_item: &Item, override_required: bool, ) -> TokenStream { - let mut subcmds = fields.iter().filter_map(|(field, item)| { - let kind = item.kind(); - if let Kind::Subcommand(ty) = &*kind { - let subcmd_type = match (**ty, sub_type(&field.ty)) { - (Ty::Option, Some(sub_type)) => sub_type, - _ => &field.ty, - }; - let required = if **ty == Ty::Option { - quote!() - } else { - quote_spanned! { kind.span()=> - let #app_var = #app_var - .subcommand_required(true) - .arg_required_else_help(true); - } - }; - - let span = field.span(); - let ts = if override_required { - quote! { - let #app_var = <#subcmd_type as clap::Subcommand>::augment_subcommands_for_update( #app_var ); - } - } else{ - quote! { - let #app_var = <#subcmd_type as clap::Subcommand>::augment_subcommands( #app_var ); - #required - } - }; - Some((span, ts)) - } else { - None - } - }); - let subcmd = subcmds.next().map(|(_, ts)| ts); - if let Some((span, _)) = subcmds.next() { - abort!( - span, - "multiple subcommand sets are not allowed, that's the second" - ); - } - + let mut subcommand_specified = false; let args = fields.iter().filter_map(|(field, item)| { let kind = item.kind(); match &*kind { Kind::Command(_) | Kind::Value - | Kind::Subcommand(_) | Kind::Skip(_, _) | Kind::FromGlobal(_) | Kind::ExternalSubcommand => None, + Kind::Subcommand(ty) => { + if subcommand_specified { + abort!(field.span(), "`#[command(subcommand)]` can only be used once per container"); + } + subcommand_specified = true; + + let subcmd_type = match (**ty, sub_type(&field.ty)) { + (Ty::Option, Some(sub_type)) => sub_type, + _ => &field.ty, + }; + let required = if **ty == Ty::Option { + quote!() + } else { + quote_spanned! { kind.span()=> + let #app_var = #app_var + .subcommand_required(true) + .arg_required_else_help(true); + } + }; + + if override_required { + Some(quote! { + let #app_var = <#subcmd_type as clap::Subcommand>::augment_subcommands_for_update( #app_var ); + }) + } else{ + Some(quote! { + let #app_var = <#subcmd_type as clap::Subcommand>::augment_subcommands( #app_var ); + #required + }) + } + } Kind::Flatten => { let ty = &field.ty; let old_heading_var = format_ident!("__clap_old_heading"); @@ -334,7 +324,6 @@ pub fn gen_augment( #deprecations let #app_var = #app_var #initial_app_methods; #( #args )* - #subcmd #app_var #final_app_methods }} } From 9c858397056e3552df50fb501e01f5879c908526 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 6 Sep 2022 11:51:29 -0500 Subject: [PATCH 16/17] refactor(derive): Simplify processing of Subcommand --- clap_derive/src/derives/args.rs | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/clap_derive/src/derives/args.rs b/clap_derive/src/derives/args.rs index 05256650b47..738ccb26b92 100644 --- a/clap_derive/src/derives/args.rs +++ b/clap_derive/src/derives/args.rs @@ -176,26 +176,20 @@ pub fn gen_augment( (Ty::Option, Some(sub_type)) => sub_type, _ => &field.ty, }; - let required = if **ty == Ty::Option { + let implicit_methods = if **ty == Ty::Option || override_required { quote!() } else { quote_spanned! { kind.span()=> - let #app_var = #app_var - .subcommand_required(true) - .arg_required_else_help(true); + .subcommand_required(true) + .arg_required_else_help(true) } }; - if override_required { - Some(quote! { - let #app_var = <#subcmd_type as clap::Subcommand>::augment_subcommands_for_update( #app_var ); - }) - } else{ - Some(quote! { - let #app_var = <#subcmd_type as clap::Subcommand>::augment_subcommands( #app_var ); - #required - }) - } + Some(quote! { + let #app_var = <#subcmd_type as clap::Subcommand>::augment_subcommands( #app_var ); + let #app_var = #app_var + #implicit_methods; + }) } Kind::Flatten => { let ty = &field.ty; From ede8fd6dbd4a4ac2bf9d2336ad23297be826699b Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 6 Sep 2022 12:28:01 -0500 Subject: [PATCH 17/17] style(derive): Clean up to make explicit methods easier --- clap_derive/src/derives/args.rs | 8 +++++-- clap_derive/src/derives/subcommand.rs | 33 +++++++++++++++------------ 2 files changed, 25 insertions(+), 16 deletions(-) diff --git a/clap_derive/src/derives/args.rs b/clap_derive/src/derives/args.rs index 738ccb26b92..f14ac09d95e 100644 --- a/clap_derive/src/derives/args.rs +++ b/clap_derive/src/derives/args.rs @@ -199,14 +199,18 @@ pub fn gen_augment( if override_required { Some(quote_spanned! { kind.span()=> let #old_heading_var = #app_var.get_next_help_heading().map(|s| clap::builder::Str::from(s.to_owned())); - let #app_var = #app_var #next_help_heading #next_display_order; + let #app_var = #app_var + #next_help_heading + #next_display_order; let #app_var = <#ty as clap::Args>::augment_args_for_update(#app_var); let #app_var = #app_var.next_help_heading(clap::builder::Resettable::from(#old_heading_var)); }) } else { Some(quote_spanned! { kind.span()=> let #old_heading_var = #app_var.get_next_help_heading().map(|s| clap::builder::Str::from(s.to_owned())); - let #app_var = #app_var #next_help_heading #next_display_order; + let #app_var = #app_var + #next_help_heading + #next_display_order; let #app_var = <#ty as clap::Args>::augment_args(#app_var); let #app_var = #app_var.next_help_heading(clap::builder::Resettable::from(#old_heading_var)); }) diff --git a/clap_derive/src/derives/subcommand.rs b/clap_derive/src/derives/subcommand.rs index 07ec6de187e..d55b669fc8f 100644 --- a/clap_derive/src/derives/subcommand.rs +++ b/clap_derive/src/derives/subcommand.rs @@ -137,7 +137,10 @@ fn gen_augment( let kind = item.kind(); match &*kind { - Kind::Skip(_, _) => None, + Kind::Skip(_, _) | + Kind::Arg(_) | + Kind::FromGlobal(_) | + Kind::Value => None, Kind::ExternalSubcommand => { let ty = match variant.fields { @@ -155,19 +158,17 @@ fn gen_augment( } else { quote!() }; - let subcommand = match subty_if_name(ty, "Vec") { - Some(subty) => { - quote_spanned! { kind.span()=> - #deprecations - let #app_var = #app_var.external_subcommand_value_parser(clap::value_parser!(#subty)); - } - } - - None => abort!( + let subty = subty_if_name(ty, "Vec").unwrap_or_else(|| { + abort!( ty.span(), "The type must be `Vec<_>` \ to be used with `external_subcommand`." - ), + ) + }); + let subcommand = quote_spanned! { kind.span()=> + #deprecations + let #app_var = #app_var + .external_subcommand_value_parser(clap::value_parser!(#subty)); }; Some(subcommand) } @@ -187,7 +188,9 @@ fn gen_augment( quote! { #deprecations let #old_heading_var = #app_var.get_next_help_heading().map(|s| clap::builder::Str::from(s.to_owned())); - let #app_var = #app_var #next_help_heading #next_display_order; + let #app_var = #app_var + #next_help_heading + #next_display_order; let #app_var = <#ty as clap::Subcommand>::augment_subcommands_for_update(#app_var); let #app_var = #app_var.next_help_heading(clap::builder::Resettable::from(#old_heading_var)); } @@ -195,7 +198,9 @@ fn gen_augment( quote! { #deprecations let #old_heading_var = #app_var.get_next_help_heading().map(|s| clap::builder::Str::from(s.to_owned())); - let #app_var = #app_var #next_help_heading #next_display_order; + let #app_var = #app_var + #next_help_heading + #next_display_order; let #app_var = <#ty as clap::Subcommand>::augment_subcommands(#app_var); let #app_var = #app_var.next_help_heading(clap::builder::Resettable::from(#old_heading_var)); } @@ -259,7 +264,7 @@ fn gen_augment( Some(subcommand) } - _ => { + Kind::Command(_) => { let subcommand_var = Ident::new("__clap_subcommand", Span::call_site()); let sub_augment = match variant.fields { Named(ref fields) => {