From 3b418fe1a13667409a90634174725566fa925daa Mon Sep 17 00:00:00 2001 From: CreepySkeleton Date: Thu, 30 Apr 2020 15:11:39 +0300 Subject: [PATCH 1/4] Cleaning --- clap_derive/src/{derives => }/attrs.rs | 5 ++++- clap_derive/src/derives/arg_enum.rs | 7 +++++-- clap_derive/src/derives/clap.rs | 6 +++++- clap_derive/src/derives/from_argmatches.rs | 5 ++++- clap_derive/src/derives/into_app.rs | 6 +++--- clap_derive/src/derives/mod.rs | 11 ----------- clap_derive/src/derives/subcommand.rs | 7 +++++-- clap_derive/src/{derives => }/dummies.rs | 0 clap_derive/src/lib.rs | 4 ++++ clap_derive/src/{derives => }/parse.rs | 0 clap_derive/src/{derives => utils}/doc_comments.rs | 3 ++- clap_derive/src/utils/mod.rs | 9 +++++++++ clap_derive/src/{derives => utils}/spanned.rs | 2 +- clap_derive/src/{derives => utils}/ty.rs | 0 14 files changed, 42 insertions(+), 23 deletions(-) rename clap_derive/src/{derives => }/attrs.rs (99%) rename clap_derive/src/{derives => }/dummies.rs (100%) rename clap_derive/src/{derives => }/parse.rs (100%) rename clap_derive/src/{derives => utils}/doc_comments.rs (99%) create mode 100644 clap_derive/src/utils/mod.rs rename clap_derive/src/{derives => utils}/spanned.rs (97%) rename clap_derive/src/{derives => utils}/ty.rs (100%) diff --git a/clap_derive/src/derives/attrs.rs b/clap_derive/src/attrs.rs similarity index 99% rename from clap_derive/src/derives/attrs.rs rename to clap_derive/src/attrs.rs index 2220da16151..b710f6d7ee5 100644 --- a/clap_derive/src/derives/attrs.rs +++ b/clap_derive/src/attrs.rs @@ -12,7 +12,10 @@ // commit#ea76fa1b1b273e65e3b0b1046643715b49bec51f which is licensed under the // MIT/Apache 2.0 license. -use super::{doc_comments::process_doc_comment, parse::*, spanned::Sp, ty::Ty}; +use crate::{ + parse::*, + utils::{process_doc_comment, Sp, Ty}, +}; use std::env; diff --git a/clap_derive/src/derives/arg_enum.rs b/clap_derive/src/derives/arg_enum.rs index 88f021a8f9a..2b3c1f705e3 100644 --- a/clap_derive/src/derives/arg_enum.rs +++ b/clap_derive/src/derives/arg_enum.rs @@ -7,8 +7,11 @@ // , at your // option. This file may not be copied, modified, or distributed // except according to those terms. -use crate::derives::attrs::{Attrs, Name, DEFAULT_CASING, DEFAULT_ENV_CASING}; -use crate::derives::spanned::Sp; + +use crate::{ + attrs::{Attrs, Name, DEFAULT_CASING, DEFAULT_ENV_CASING}, + utils::Sp, +}; use proc_macro2::{Span, TokenStream}; use quote::quote; diff --git a/clap_derive/src/derives/clap.rs b/clap_derive/src/derives/clap.rs index b16163e8eff..9c454e11b41 100644 --- a/clap_derive/src/derives/clap.rs +++ b/clap_derive/src/derives/clap.rs @@ -12,7 +12,11 @@ // commit#ea76fa1b1b273e65e3b0b1046643715b49bec51f which is licensed under the // MIT/Apache 2.0 license. -use super::{arg_enum, dummies, from_argmatches, into_app, subcommand}; +use crate::{ + derives::{arg_enum, from_argmatches, into_app, subcommand}, + dummies, +}; + use proc_macro2::TokenStream; use proc_macro_error::abort_call_site; use quote::quote; diff --git a/clap_derive/src/derives/from_argmatches.rs b/clap_derive/src/derives/from_argmatches.rs index df7b2713b2b..cd3c58183db 100644 --- a/clap_derive/src/derives/from_argmatches.rs +++ b/clap_derive/src/derives/from_argmatches.rs @@ -15,7 +15,10 @@ use proc_macro2::TokenStream; use quote::{quote, quote_spanned}; use syn::{punctuated::Punctuated, spanned::Spanned, Field, Ident, Token, Type}; -use super::{sub_type, subty_if_name, Attrs, Kind, ParserKind, Ty}; +use crate::{ + attrs::{Attrs, Kind, ParserKind}, + utils::{sub_type, subty_if_name, Ty}, +}; pub fn gen_for_struct( struct_name: &Ident, diff --git a/clap_derive/src/derives/into_app.rs b/clap_derive/src/derives/into_app.rs index b42d55b331c..1bd53a9dfaf 100644 --- a/clap_derive/src/derives/into_app.rs +++ b/clap_derive/src/derives/into_app.rs @@ -19,9 +19,9 @@ use proc_macro_error::abort; use quote::{quote, quote_spanned}; use syn::{punctuated::Punctuated, spanned::Spanned, Attribute, Field, Ident, Token, Type}; -use super::{ - spanned::Sp, sub_type, subty_if_name, ty::Ty, Attrs, GenOutput, Kind, Name, ParserKind, - DEFAULT_CASING, DEFAULT_ENV_CASING, +use crate::{ + attrs::{Attrs, GenOutput, Kind, Name, ParserKind, DEFAULT_CASING, DEFAULT_ENV_CASING}, + utils::{sub_type, subty_if_name, Sp, Ty}, }; pub fn gen_for_struct( diff --git a/clap_derive/src/derives/mod.rs b/clap_derive/src/derives/mod.rs index e3afdc33666..8fe7aed2d3f 100644 --- a/clap_derive/src/derives/mod.rs +++ b/clap_derive/src/derives/mod.rs @@ -12,20 +12,9 @@ // commit#ea76fa1b1b273e65e3b0b1046643715b49bec51f which is licensed under the // MIT/Apache 2.0 license. mod arg_enum; -pub mod attrs; mod clap; -mod doc_comments; -mod dummies; mod from_argmatches; mod into_app; -pub mod parse; -pub mod spanned; mod subcommand; -pub mod ty; -pub use self::attrs::{ - Attrs, CasingStyle, GenOutput, Kind, Name, Parser, ParserKind, DEFAULT_CASING, - DEFAULT_ENV_CASING, -}; pub use self::clap::derive_clap; -pub use self::ty::{sub_type, subty_if_name, Ty}; diff --git a/clap_derive/src/derives/subcommand.rs b/clap_derive/src/derives/subcommand.rs index 739bfc73672..1d076cae468 100644 --- a/clap_derive/src/derives/subcommand.rs +++ b/clap_derive/src/derives/subcommand.rs @@ -1,5 +1,8 @@ -use crate::derives::attrs::{Attrs, Kind, Name, DEFAULT_CASING, DEFAULT_ENV_CASING}; -use crate::derives::{from_argmatches, into_app, spanned::Sp}; +use crate::{ + attrs::{Attrs, Kind, Name, DEFAULT_CASING, DEFAULT_ENV_CASING}, + derives::{from_argmatches, into_app}, + utils::Sp, +}; use proc_macro2::{Ident, Span, TokenStream}; use proc_macro_error::{abort, abort_call_site}; diff --git a/clap_derive/src/derives/dummies.rs b/clap_derive/src/dummies.rs similarity index 100% rename from clap_derive/src/derives/dummies.rs rename to clap_derive/src/dummies.rs diff --git a/clap_derive/src/lib.rs b/clap_derive/src/lib.rs index 4bb373ae33c..682441229dc 100644 --- a/clap_derive/src/lib.rs +++ b/clap_derive/src/lib.rs @@ -20,7 +20,11 @@ extern crate proc_macro; use proc_macro_error::proc_macro_error; +mod attrs; mod derives; +mod dummies; +mod parse; +mod utils; // /// It is required to have this seperate and specificly defined. // #[proc_macro_derive(ArgEnum, attributes(case_sensitive))] diff --git a/clap_derive/src/derives/parse.rs b/clap_derive/src/parse.rs similarity index 100% rename from clap_derive/src/derives/parse.rs rename to clap_derive/src/parse.rs diff --git a/clap_derive/src/derives/doc_comments.rs b/clap_derive/src/utils/doc_comments.rs similarity index 99% rename from clap_derive/src/derives/doc_comments.rs rename to clap_derive/src/utils/doc_comments.rs index 7f8822f85f5..49c794c9c2d 100644 --- a/clap_derive/src/derives/doc_comments.rs +++ b/clap_derive/src/utils/doc_comments.rs @@ -3,7 +3,8 @@ //! #[derive(Clap)] works in terms of "paragraphs". Paragraph is a sequence of //! non-empty adjacent lines, delimited by sequences of blank (whitespace only) lines. -use super::attrs::Method; +use crate::attrs::Method; + use quote::{format_ident, quote}; use std::iter; diff --git a/clap_derive/src/utils/mod.rs b/clap_derive/src/utils/mod.rs new file mode 100644 index 00000000000..c810f4e450c --- /dev/null +++ b/clap_derive/src/utils/mod.rs @@ -0,0 +1,9 @@ +mod doc_comments; +mod spanned; +mod ty; + +pub use self::{ + doc_comments::process_doc_comment, + spanned::Sp, + ty::{sub_type, subty_if_name, Ty}, +}; diff --git a/clap_derive/src/derives/spanned.rs b/clap_derive/src/utils/spanned.rs similarity index 97% rename from clap_derive/src/derives/spanned.rs rename to clap_derive/src/utils/spanned.rs index adde02dddae..c8ba8062e45 100644 --- a/clap_derive/src/derives/spanned.rs +++ b/clap_derive/src/utils/spanned.rs @@ -81,7 +81,7 @@ impl> AsRef for Sp { impl ToTokens for Sp { fn to_tokens(&self, stream: &mut TokenStream) { // this is the simplest way out of correct ones to change span on - // arbitrary token tree I can come up with + // arbitrary token tree I could come up with let tt = self.val.to_token_stream().into_iter().map(|mut tt| { tt.set_span(self.span.clone()); tt diff --git a/clap_derive/src/derives/ty.rs b/clap_derive/src/utils/ty.rs similarity index 100% rename from clap_derive/src/derives/ty.rs rename to clap_derive/src/utils/ty.rs From ee463ba834fe2296fc0fa59fee463ba236c84c4a Mon Sep 17 00:00:00 2001 From: CreepySkeleton Date: Thu, 30 Apr 2020 20:20:21 +0300 Subject: [PATCH 2/4] Make `extrernal_subcommand` ALMOST work --- clap_derive/src/attrs.rs | 17 +- clap_derive/src/derives/from_argmatches.rs | 8 +- clap_derive/src/derives/into_app.rs | 2 +- clap_derive/src/derives/subcommand.rs | 197 +++++++++++++----- clap_derive/src/parse.rs | 3 +- clap_derive/src/utils/mod.rs | 2 +- clap_derive/src/utils/ty.rs | 2 +- clap_derive/tests/subcommands.rs | 92 ++++++++ .../ui/external_subcommand_wrong_type.rs | 19 ++ .../tests/ui/multiple_external_subcommand.rs | 21 ++ src/util/argstr.rs | 8 +- 11 files changed, 310 insertions(+), 61 deletions(-) create mode 100644 clap_derive/tests/ui/external_subcommand_wrong_type.rs create mode 100644 clap_derive/tests/ui/multiple_external_subcommand.rs diff --git a/clap_derive/src/attrs.rs b/clap_derive/src/attrs.rs index b710f6d7ee5..860fcc2fb54 100644 --- a/clap_derive/src/attrs.rs +++ b/clap_derive/src/attrs.rs @@ -41,6 +41,7 @@ pub enum Kind { Subcommand(Sp), Flatten, Skip(Option), + ExternalSubcommand, } #[derive(Clone)] @@ -299,6 +300,11 @@ impl Attrs { self.set_kind(kind); } + ExternalSubcommand(ident) => { + let kind = Sp::new(Kind::ExternalSubcommand, ident.span()); + self.set_kind(kind); + } + Flatten(ident) => { let kind = Sp::new(Kind::Flatten, ident.span()); self.set_kind(kind); @@ -425,7 +431,7 @@ impl Attrs { match &*res.kind { Kind::Subcommand(_) => abort!(res.kind.span(), "subcommand is only allowed on fields"), Kind::Skip(_) => abort!(res.kind.span(), "skip is only allowed on fields"), - Kind::Arg(_) | Kind::Flatten => res, + Kind::Arg(_) | Kind::Flatten | Kind::ExternalSubcommand => res, } } @@ -460,6 +466,13 @@ impl Attrs { ); } } + + Kind::ExternalSubcommand => { + abort! { res.kind.span(), + "`external_subcommand` can be used only on enum variants" + } + } + Kind::Subcommand(_) => { if res.has_custom_parser { abort!( @@ -571,7 +584,7 @@ impl Attrs { } else { abort!( kind.span(), - "subcommand, flatten and skip cannot be used together" + "`subcommand`, `flatten`, `external_subcommand` and `skip` cannot be used together" ); } } diff --git a/clap_derive/src/derives/from_argmatches.rs b/clap_derive/src/derives/from_argmatches.rs index cd3c58183db..06dc125d268 100644 --- a/clap_derive/src/derives/from_argmatches.rs +++ b/clap_derive/src/derives/from_argmatches.rs @@ -12,6 +12,7 @@ // commit#ea76fa1b1b273e65e3b0b1046643715b49bec51f which is licensed under the // MIT/Apache 2.0 license. use proc_macro2::TokenStream; +use proc_macro_error::abort; use quote::{quote, quote_spanned}; use syn::{punctuated::Punctuated, spanned::Spanned, Field, Ident, Token, Type}; @@ -92,7 +93,12 @@ pub fn gen_constructor( ); let field_name = field.ident.as_ref().unwrap(); let kind = attrs.kind(); - match &*attrs.kind() { + match &*kind { + Kind::ExternalSubcommand => { + abort! { kind.span(), + "`external_subcommand` can be used only on enum variants" + } + } Kind::Subcommand(ty) => { let subcmd_type = match (**ty, sub_type(&field.ty)) { (Ty::Option, Some(sub_type)) => sub_type, diff --git a/clap_derive/src/derives/into_app.rs b/clap_derive/src/derives/into_app.rs index 1bd53a9dfaf..b5aecdb7478 100644 --- a/clap_derive/src/derives/into_app.rs +++ b/clap_derive/src/derives/into_app.rs @@ -179,7 +179,7 @@ pub fn gen_app_augmentation( ); let kind = attrs.kind(); match &*kind { - Kind::Subcommand(_) | Kind::Skip(_) => None, + Kind::Subcommand(_) | Kind::Skip(_) | Kind::ExternalSubcommand => None, Kind::Flatten => { let ty = &field.ty; Some(quote_spanned! { kind.span()=> diff --git a/clap_derive/src/derives/subcommand.rs b/clap_derive/src/derives/subcommand.rs index 1d076cae468..909e3e53ad5 100644 --- a/clap_derive/src/derives/subcommand.rs +++ b/clap_derive/src/derives/subcommand.rs @@ -1,7 +1,7 @@ use crate::{ attrs::{Attrs, Kind, Name, DEFAULT_CASING, DEFAULT_ENV_CASING}, derives::{from_argmatches, into_app}, - utils::Sp, + utils::{is_simple_ty, subty_if_name, Sp}, }; use proc_macro2::{Ident, Span, TokenStream}; @@ -49,62 +49,72 @@ fn gen_augment_subcommands( ) -> TokenStream { use syn::Fields::*; - let subcommands = variants.iter().map(|variant| { - let attrs = Attrs::from_struct( - variant.span(), - &variant.attrs, - Name::Derived(variant.ident.clone()), - parent_attribute.casing(), - parent_attribute.env_casing(), - ); - let kind = attrs.kind(); - match &*kind { - Kind::Flatten => match variant.fields { - Unnamed(FieldsUnnamed { ref unnamed, .. }) if unnamed.len() == 1 => { - let ty = &unnamed[0]; - quote! { - let app = <#ty as ::clap::Subcommand>::augment_subcommands(app); + let subcommands: Vec<_> = variants + .iter() + .map(|variant| { + let attrs = Attrs::from_struct( + variant.span(), + &variant.attrs, + Name::Derived(variant.ident.clone()), + parent_attribute.casing(), + parent_attribute.env_casing(), + ); + let kind = attrs.kind(); + + match &*kind { + Kind::ExternalSubcommand => { + quote_spanned! { kind.span()=> + let app = app.setting(::clap::AppSettings::AllowExternalSubcommands); } } - _ => abort!( - variant, - "`flatten` is usable only with single-typed tuple variants" - ), - }, - - _ => { - let app_var = Ident::new("subcommand", Span::call_site()); - let arg_block = match variant.fields { - Named(ref fields) => { - into_app::gen_app_augmentation(&fields.named, &app_var, &attrs) - } - Unit => quote!( #app_var ), + + Kind::Flatten => match variant.fields { Unnamed(FieldsUnnamed { ref unnamed, .. }) if unnamed.len() == 1 => { let ty = &unnamed[0]; - quote_spanned! { ty.span()=> - { - <#ty as ::clap::IntoApp>::augment_clap(#app_var) - } + quote! { + let app = <#ty as ::clap::Subcommand>::augment_subcommands(app); } } - Unnamed(..) => { - abort!(variant, "non single-typed tuple enums are not supported") - } - }; + _ => abort!( + variant, + "`flatten` is usable only with single-typed tuple variants" + ), + }, - let name = attrs.cased_name(); - let from_attrs = attrs.top_level_methods(); - let version = attrs.version(); - quote! { - let app = app.subcommand({ - let #app_var = ::clap::App::new(#name); - let #app_var = #arg_block; - #app_var#from_attrs#version - }); + _ => { + let app_var = Ident::new("subcommand", Span::call_site()); + let arg_block = match variant.fields { + Named(ref fields) => { + into_app::gen_app_augmentation(&fields.named, &app_var, &attrs) + } + Unit => quote!( #app_var ), + Unnamed(FieldsUnnamed { ref unnamed, .. }) if unnamed.len() == 1 => { + let ty = &unnamed[0]; + quote_spanned! { ty.span()=> + { + <#ty as ::clap::IntoApp>::augment_clap(#app_var) + } + } + } + Unnamed(..) => { + abort!(variant, "non single-typed tuple enums are not supported") + } + }; + + let name = attrs.cased_name(); + let from_attrs = attrs.top_level_methods(); + let version = attrs.version(); + quote! { + let app = app.subcommand({ + let #app_var = ::clap::App::new(#name); + let #app_var = #arg_block; + #app_var#from_attrs#version + }); + } } } - } - }); + }) + .collect(); let app_methods = parent_attribute.top_level_methods(); let version = parent_attribute.version(); @@ -123,9 +133,12 @@ fn gen_from_subcommand( parent_attribute: &Attrs, ) -> TokenStream { use syn::Fields::*; + + let mut ext_subcmd = None; + let (flatten_variants, variants): (Vec<_>, Vec<_>) = variants .iter() - .map(|variant| { + .filter_map(|variant| { let attrs = Attrs::from_struct( variant.span(), &variant.attrs, @@ -133,7 +146,56 @@ fn gen_from_subcommand( parent_attribute.casing(), parent_attribute.env_casing(), ); - (variant, attrs) + + if let Kind::ExternalSubcommand = &*attrs.kind() { + if ext_subcmd.is_some() { + abort!( + attrs.kind().span(), + "Only one variant can be marked with `external_subcommand`, \ + this is the second" + ); + } + + let ty = match variant.fields { + Unnamed(ref fields) if fields.unnamed.len() == 1 => &fields.unnamed[0].ty, + + _ => abort!( + variant.span(), + "The enum variant marked with `external_attribute` must be \ + a single-typed tuple, and the type must be either `Vec` \ + or `Vec`." + ), + }; + + let (span, str_ty, values_of) = match subty_if_name(ty, "Vec") { + Some(subty) => { + if is_simple_ty(subty, "String") { + ( + subty.span(), + quote!(::std::string::String), + quote!(values_of), + ) + } else { + ( + subty.span(), + quote!(::std::ffi::OsString), + quote!(values_of_os), + ) + } + } + + None => abort!( + ty.span(), + "The type must be either `Vec` or `Vec` \ + to be used with `external_subcommand`." + ), + }; + + ext_subcmd = Some((span, &variant.ident, str_ty, values_of)); + None + } else { + Some((variant, attrs)) + } }) .partition(|(_, attrs)| { let kind = attrs.kind(); @@ -180,16 +242,45 @@ fn gen_from_subcommand( } }); + let wildcard = match ext_subcmd { + Some((span, var_name, str_ty, values_of)) => quote_spanned! { span=> + ("", ::std::option::Option::None) => ::std::option::Option::None, + + (external, Some(matches)) => { + ::std::option::Option::Some(#name::#var_name( + ::std::iter::once(#str_ty::from(external)) + .chain( + matches.#values_of("").into_iter().flatten().map(#str_ty::from) + ) + .collect::<::std::vec::Vec<_>>() + )) + } + + (external, None) => { + ::std::option::Option::Some(#name::#var_name({ + let mut v = ::std::vec::Vec::with_capacity(1); + v.push(#str_ty::from(external)); + v + })) + } + }, + + None => quote!(_ => None), + }; + quote! { fn from_subcommand<'b>( name: &'b str, sub: Option<&'b ::clap::ArgMatches>) -> Option { match (name, sub) { - #( #match_arms ),*, + #( #match_arms, )* other => { - #( #child_subcommands )*; - None + #( #child_subcommands )else* + + match other { + #wildcard + } } } } diff --git a/clap_derive/src/parse.rs b/clap_derive/src/parse.rs index bbdc4777e6d..04fc2453dba 100644 --- a/clap_derive/src/parse.rs +++ b/clap_derive/src/parse.rs @@ -19,6 +19,7 @@ pub enum ClapAttr { ArgEnum(Ident), Subcommand(Ident), VerbatimDocComment(Ident), + ExternalSubcommand(Ident), // ident [= "string literal"] About(Ident, Option), @@ -170,7 +171,7 @@ impl Parse for ClapAttr { "flatten" => Ok(Flatten(name)), "arg_enum" => Ok(ArgEnum(name)), "subcommand" => Ok(Subcommand(name)), - + "external_subcommand" => Ok(ExternalSubcommand(name)), "verbatim_doc_comment" => Ok(VerbatimDocComment(name)), "default_value" => Ok(DefaultValue(name, None)), diff --git a/clap_derive/src/utils/mod.rs b/clap_derive/src/utils/mod.rs index c810f4e450c..896913321fb 100644 --- a/clap_derive/src/utils/mod.rs +++ b/clap_derive/src/utils/mod.rs @@ -5,5 +5,5 @@ mod ty; pub use self::{ doc_comments::process_doc_comment, spanned::Sp, - ty::{sub_type, subty_if_name, Ty}, + ty::{is_simple_ty, sub_type, subty_if_name, Ty}, }; diff --git a/clap_derive/src/utils/ty.rs b/clap_derive/src/utils/ty.rs index b046ac6d61e..c2cec83fb56 100644 --- a/clap_derive/src/utils/ty.rs +++ b/clap_derive/src/utils/ty.rs @@ -84,7 +84,7 @@ pub fn subty_if_name<'a>(ty: &'a syn::Type, name: &str) -> Option<&'a syn::Type> subty_if(ty, |seg| seg.ident == name) } -fn is_simple_ty(ty: &syn::Type, name: &str) -> bool { +pub fn is_simple_ty(ty: &syn::Type, name: &str) -> bool { only_last_segment(ty) .map(|segment| { if let PathArguments::None = segment.arguments { diff --git a/clap_derive/tests/subcommands.rs b/clap_derive/tests/subcommands.rs index f012b3a5cf4..43ab56a9a29 100644 --- a/clap_derive/tests/subcommands.rs +++ b/clap_derive/tests/subcommands.rs @@ -161,6 +161,98 @@ fn test_tuple_commands() { assert!(!output.contains("Not shown")); } +#[test] +#[ignore] +fn external_subcommand() { + #[derive(Debug, PartialEq, Clap)] + struct Opt { + #[clap(subcommand)] + sub: Subcommands, + } + + #[derive(Debug, PartialEq, Clap)] + enum Subcommands { + // Add, + // Remove, + #[clap(external_subcommand)] + Other(Vec), + } + + // assert_eq!( + // Opt::parse_from(&["test", "add"]), + // Opt { + // sub: Subcommands::Add + // } + // ); + + // assert_eq!( + // Opt::parse_from(&["test", "remove"]), + // Opt { + // sub: Subcommands::Remove + // } + // ); + + assert!(Opt::try_parse_from(&["test"]).is_err()); + + assert_eq!( + Opt::try_parse_from(&["test", "git", "status"]).unwrap(), + Opt { + sub: Subcommands::Other(vec!["git".into(), "status".into()]) + } + ); +} + +#[test] +#[ignore] +fn external_subcommand_os_string() { + use std::ffi::OsString; + + #[derive(Debug, PartialEq, Clap)] + struct Opt { + #[clap(subcommand)] + sub: Subcommands, + } + + #[derive(Debug, PartialEq, Clap)] + enum Subcommands { + #[clap(external_subcommand)] + Other(Vec), + } + + assert_eq!( + Opt::try_parse_from(&["test", "git", "status"]).unwrap(), + Opt { + sub: Subcommands::Other(vec!["git".into(), "status".into()]) + } + ); + + assert!(Opt::try_parse_from(&["test"]).is_err()); +} + +#[test] +fn external_subcommand_optional() { + #[derive(Debug, PartialEq, Clap)] + struct Opt { + #[clap(subcommand)] + sub: Option, + } + + #[derive(Debug, PartialEq, Clap)] + enum Subcommands { + #[clap(external_subcommand)] + Other(Vec), + } + + assert_eq!( + Opt::try_parse_from(&["test", "git", "status"]).unwrap(), + Opt { + sub: Some(Subcommands::Other(vec!["git".into(), "status".into()])) + } + ); + + assert_eq!(Opt::try_parse_from(&["test"]).unwrap(), Opt { sub: None }); +} + // #[test] // #[ignore] // FIXME (@CreepySkeleton) // fn enum_in_enum_subsubcommand() { diff --git a/clap_derive/tests/ui/external_subcommand_wrong_type.rs b/clap_derive/tests/ui/external_subcommand_wrong_type.rs new file mode 100644 index 00000000000..700c145a494 --- /dev/null +++ b/clap_derive/tests/ui/external_subcommand_wrong_type.rs @@ -0,0 +1,19 @@ +use clap::Clap; +use std::ffi::CString; + +#[derive(Clap, Debug)] +struct Opt { + #[clap(subcommand)] + cmd: Command, +} + +#[derive(Clap, Debug)] +enum Command { + #[clap(external_subcommand)] + Other(Vec), +} + +fn main() { + let opt = Opt::parse(); + println!("{:?}", opt); +} diff --git a/clap_derive/tests/ui/multiple_external_subcommand.rs b/clap_derive/tests/ui/multiple_external_subcommand.rs new file mode 100644 index 00000000000..3cd342a310b --- /dev/null +++ b/clap_derive/tests/ui/multiple_external_subcommand.rs @@ -0,0 +1,21 @@ +use clap::Clap; + +#[derive(Clap, Debug)] +struct Opt { + #[clap(subcommand)] + cmd: Command, +} + +#[derive(Clap, Debug)] +enum Command { + #[clap(external_subcommand)] + Run(Vec), + + #[clap(external_subcommand)] + Other(Vec), +} + +fn main() { + let opt = Opt::parse(); + println!("{:?}", opt); +} diff --git a/src/util/argstr.rs b/src/util/argstr.rs index 098712dc0a0..7ad80b64cc3 100644 --- a/src/util/argstr.rs +++ b/src/util/argstr.rs @@ -1,12 +1,12 @@ use std::{ borrow::Cow, ffi::{OsStr, OsString}, + fmt::{self, Debug}, str, }; use os_str_bytes::{OsStrBytes, OsStringBytes}; -#[derive(Debug)] pub(crate) struct ArgStr<'a>(Cow<'a, [u8]>); impl<'a> ArgStr<'a> { @@ -127,6 +127,12 @@ impl<'a> ArgStr<'a> { } } +impl<'a> Debug for ArgStr<'a> { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + write!(f, "{:?}", self.to_string_lossy()) + } +} + impl<'a> PartialEq for ArgStr<'a> { fn eq(&self, other: &str) -> bool { self.0 == other.as_bytes() From 6bdb6d9b2eb484a34d302f29943e8a455d63465f Mon Sep 17 00:00:00 2001 From: CreepySkeleton Date: Fri, 1 May 2020 15:33:36 +0300 Subject: [PATCH 3/4] Marry AllowExternalSubcommands with SubcommandRequiredElse* --- clap_derive/tests/subcommands.rs | 30 ++++++------ .../ui/external_subcommand_wrong_type.stderr | 8 +++ .../ui/multiple_external_subcommand.stderr | 5 ++ clap_derive/tests/ui/skip_flatten.stderr | 2 +- clap_derive/tests/ui/skip_subcommand.stderr | 2 +- .../tests/ui/subcommand_and_flatten.stderr | 2 +- src/parse/parser.rs | 49 ++++++++++--------- 7 files changed, 57 insertions(+), 41 deletions(-) create mode 100644 clap_derive/tests/ui/external_subcommand_wrong_type.stderr create mode 100644 clap_derive/tests/ui/multiple_external_subcommand.stderr diff --git a/clap_derive/tests/subcommands.rs b/clap_derive/tests/subcommands.rs index 43ab56a9a29..9fbb6323bc1 100644 --- a/clap_derive/tests/subcommands.rs +++ b/clap_derive/tests/subcommands.rs @@ -162,7 +162,6 @@ fn test_tuple_commands() { } #[test] -#[ignore] fn external_subcommand() { #[derive(Debug, PartialEq, Clap)] struct Opt { @@ -172,25 +171,25 @@ fn external_subcommand() { #[derive(Debug, PartialEq, Clap)] enum Subcommands { - // Add, - // Remove, + Add, + Remove, #[clap(external_subcommand)] Other(Vec), } - // assert_eq!( - // Opt::parse_from(&["test", "add"]), - // Opt { - // sub: Subcommands::Add - // } - // ); + assert_eq!( + Opt::parse_from(&["test", "add"]), + Opt { + sub: Subcommands::Add + } + ); - // assert_eq!( - // Opt::parse_from(&["test", "remove"]), - // Opt { - // sub: Subcommands::Remove - // } - // ); + assert_eq!( + Opt::parse_from(&["test", "remove"]), + Opt { + sub: Subcommands::Remove + } + ); assert!(Opt::try_parse_from(&["test"]).is_err()); @@ -203,7 +202,6 @@ fn external_subcommand() { } #[test] -#[ignore] fn external_subcommand_os_string() { use std::ffi::OsString; diff --git a/clap_derive/tests/ui/external_subcommand_wrong_type.stderr b/clap_derive/tests/ui/external_subcommand_wrong_type.stderr new file mode 100644 index 00000000000..70e90e82a25 --- /dev/null +++ b/clap_derive/tests/ui/external_subcommand_wrong_type.stderr @@ -0,0 +1,8 @@ +error[E0308]: mismatched types + --> $DIR/external_subcommand_wrong_type.rs:13:15 + | +13 | Other(Vec), + | ^^^^^^^ expected struct `std::ffi::CString`, found struct `std::ffi::OsString` + | + = note: expected struct `std::vec::Vec` + found struct `std::vec::Vec` diff --git a/clap_derive/tests/ui/multiple_external_subcommand.stderr b/clap_derive/tests/ui/multiple_external_subcommand.stderr new file mode 100644 index 00000000000..270fd36d144 --- /dev/null +++ b/clap_derive/tests/ui/multiple_external_subcommand.stderr @@ -0,0 +1,5 @@ +error: Only one variant can be marked with `external_subcommand`, this is the second + --> $DIR/multiple_external_subcommand.rs:14:12 + | +14 | #[clap(external_subcommand)] + | ^^^^^^^^^^^^^^^^^^^ diff --git a/clap_derive/tests/ui/skip_flatten.stderr b/clap_derive/tests/ui/skip_flatten.stderr index e5230c0db7d..328dc9c3512 100644 --- a/clap_derive/tests/ui/skip_flatten.stderr +++ b/clap_derive/tests/ui/skip_flatten.stderr @@ -1,4 +1,4 @@ -error: subcommand, flatten and skip cannot be used together +error: `subcommand`, `flatten`, `external_subcommand` and `skip` cannot be used together --> $DIR/skip_flatten.rs:17:18 | 17 | #[clap(skip, flatten)] diff --git a/clap_derive/tests/ui/skip_subcommand.stderr b/clap_derive/tests/ui/skip_subcommand.stderr index fff8796a1d9..aeea102976c 100644 --- a/clap_derive/tests/ui/skip_subcommand.stderr +++ b/clap_derive/tests/ui/skip_subcommand.stderr @@ -1,4 +1,4 @@ -error: subcommand, flatten and skip cannot be used together +error: `subcommand`, `flatten`, `external_subcommand` and `skip` cannot be used together --> $DIR/skip_subcommand.rs:17:24 | 17 | #[clap(subcommand, skip)] diff --git a/clap_derive/tests/ui/subcommand_and_flatten.stderr b/clap_derive/tests/ui/subcommand_and_flatten.stderr index b509b7379c9..c9fcc4312cd 100644 --- a/clap_derive/tests/ui/subcommand_and_flatten.stderr +++ b/clap_derive/tests/ui/subcommand_and_flatten.stderr @@ -1,4 +1,4 @@ -error: subcommand, flatten and skip cannot be used together +error: `subcommand`, `flatten`, `external_subcommand` and `skip` cannot be used together --> $DIR/subcommand_and_flatten.rs:16:24 | 16 | #[clap(subcommand, flatten)] diff --git a/src/parse/parser.rs b/src/parse/parser.rs index 7fff483d81a..d764458cbbb 100644 --- a/src/parse/parser.rs +++ b/src/parse/parser.rs @@ -381,6 +381,7 @@ where let has_args = self.has_args(); let mut subcmd_name: Option = None; + let mut external_subcommand = false; let mut needs_val_of: ParseResult = ParseResult::NotFound; let mut pos_counter = 1; let mut replace: Option<&[&str]> = None; @@ -677,6 +678,8 @@ where matches: sc_m.into_inner(), }); + external_subcommand = true; + break; } else if !((self.is_set(AS::AllowLeadingHyphen) || self.is_set(AS::AllowNegativeNumbers)) @@ -718,28 +721,30 @@ where } } - if let Some(ref pos_sc_name) = subcmd_name { - let sc_name = find_subcmd!(self.app, *pos_sc_name) - .expect(INTERNAL_ERROR_MSG) - .name - .clone(); - self.parse_subcommand(&sc_name, matcher, it)?; - } else if self.is_set(AS::SubcommandRequired) { - let bn = self.app.bin_name.as_ref().unwrap_or(&self.app.name); - return Err(ClapError::missing_subcommand( - bn, - &Usage::new(self).create_usage_with_title(&[]), - self.app.color(), - )?); - } else if self.is_set(AS::SubcommandRequiredElseHelp) { - debug!("Parser::get_matches_with: SubcommandRequiredElseHelp=true"); - let message = self.write_help_err()?; - return Err(ClapError { - cause: String::new(), - message, - kind: ErrorKind::MissingArgumentOrSubcommand, - info: None, - }); + if !external_subcommand { + if let Some(ref pos_sc_name) = subcmd_name { + let sc_name = find_subcmd!(self.app, *pos_sc_name) + .expect(INTERNAL_ERROR_MSG) + .name + .clone(); + self.parse_subcommand(&sc_name, matcher, it)?; + } else if self.is_set(AS::SubcommandRequired) { + let bn = self.app.bin_name.as_ref().unwrap_or(&self.app.name); + return Err(ClapError::missing_subcommand( + bn, + &Usage::new(self).create_usage_with_title(&[]), + self.app.color(), + )?); + } else if self.is_set(AS::SubcommandRequiredElseHelp) { + debug!("Parser::get_matches_with: SubcommandRequiredElseHelp=true"); + let message = self.write_help_err()?; + return Err(ClapError { + cause: String::new(), + message, + kind: ErrorKind::MissingArgumentOrSubcommand, + info: None, + }); + } } self.remove_overrides(matcher); From a21372e9ed43e2b607e2094baf58f18cea25af64 Mon Sep 17 00:00:00 2001 From: CreepySkeleton Date: Fri, 1 May 2020 19:50:52 +0300 Subject: [PATCH 4/4] Fix ui tests --- clap_derive/src/derives/subcommand.rs | 2 +- .../tests/ui/external_subcommand_misuse.rs | 11 ++++++++ .../ui/external_subcommand_misuse.stderr | 5 ++++ .../ui/external_subcommand_wrong_type.rs | 21 ++++++++++------ .../ui/external_subcommand_wrong_type.stderr | 25 ++++++++++++++----- 5 files changed, 50 insertions(+), 14 deletions(-) create mode 100644 clap_derive/tests/ui/external_subcommand_misuse.rs create mode 100644 clap_derive/tests/ui/external_subcommand_misuse.stderr diff --git a/clap_derive/src/derives/subcommand.rs b/clap_derive/src/derives/subcommand.rs index 909e3e53ad5..ccee7c6ce4c 100644 --- a/clap_derive/src/derives/subcommand.rs +++ b/clap_derive/src/derives/subcommand.rs @@ -160,7 +160,7 @@ fn gen_from_subcommand( Unnamed(ref fields) if fields.unnamed.len() == 1 => &fields.unnamed[0].ty, _ => abort!( - variant.span(), + variant, "The enum variant marked with `external_attribute` must be \ a single-typed tuple, and the type must be either `Vec` \ or `Vec`." diff --git a/clap_derive/tests/ui/external_subcommand_misuse.rs b/clap_derive/tests/ui/external_subcommand_misuse.rs new file mode 100644 index 00000000000..19d242584b8 --- /dev/null +++ b/clap_derive/tests/ui/external_subcommand_misuse.rs @@ -0,0 +1,11 @@ +use clap::Clap; + +#[derive(Clap, Debug)] +struct Opt { + #[clap(external_subcommand)] + field: String, +} + +fn main() { + let _ = Opt::parse(); +} diff --git a/clap_derive/tests/ui/external_subcommand_misuse.stderr b/clap_derive/tests/ui/external_subcommand_misuse.stderr new file mode 100644 index 00000000000..14540920b6c --- /dev/null +++ b/clap_derive/tests/ui/external_subcommand_misuse.stderr @@ -0,0 +1,5 @@ +error: `external_subcommand` can be used only on enum variants + --> $DIR/external_subcommand_misuse.rs:5:12 + | +5 | #[clap(external_subcommand)] + | ^^^^^^^^^^^^^^^^^^^ diff --git a/clap_derive/tests/ui/external_subcommand_wrong_type.rs b/clap_derive/tests/ui/external_subcommand_wrong_type.rs index 700c145a494..e8233ef6ee4 100644 --- a/clap_derive/tests/ui/external_subcommand_wrong_type.rs +++ b/clap_derive/tests/ui/external_subcommand_wrong_type.rs @@ -2,18 +2,25 @@ use clap::Clap; use std::ffi::CString; #[derive(Clap, Debug)] -struct Opt { - #[clap(subcommand)] - cmd: Command, +enum Opt { + #[clap(external_subcommand)] + Other(Vec), } #[derive(Clap, Debug)] -enum Command { +enum Opt2 { #[clap(external_subcommand)] - Other(Vec), + Other(String), +} + +#[derive(Clap, Debug)] +enum Opt3 { + #[clap(external_subcommand)] + Other { a: String }, } fn main() { - let opt = Opt::parse(); - println!("{:?}", opt); + let _ = Opt::parse(); + let _ = Opt2::parse(); + let _ = Opt3::parse(); } diff --git a/clap_derive/tests/ui/external_subcommand_wrong_type.stderr b/clap_derive/tests/ui/external_subcommand_wrong_type.stderr index 70e90e82a25..d4ff9026902 100644 --- a/clap_derive/tests/ui/external_subcommand_wrong_type.stderr +++ b/clap_derive/tests/ui/external_subcommand_wrong_type.stderr @@ -1,8 +1,21 @@ -error[E0308]: mismatched types - --> $DIR/external_subcommand_wrong_type.rs:13:15 +error: The type must be either `Vec` or `Vec` to be used with `external_subcommand`. + --> $DIR/external_subcommand_wrong_type.rs:13:11 | -13 | Other(Vec), - | ^^^^^^^ expected struct `std::ffi::CString`, found struct `std::ffi::OsString` +13 | Other(String), + | ^^^^^^ + +error: The enum variant marked with `external_attribute` must be a single-typed tuple, and the type must be either `Vec` or `Vec`. + --> $DIR/external_subcommand_wrong_type.rs:18:5 | - = note: expected struct `std::vec::Vec` - found struct `std::vec::Vec` +18 | / #[clap(external_subcommand)] +19 | | Other { a: String }, + | |_______________________^ + +error[E0308]: mismatched types + --> $DIR/external_subcommand_wrong_type.rs:7:15 + | +7 | Other(Vec), + | ^^^^^^^ expected struct `std::ffi::CString`, found struct `std::ffi::OsString` + | + = note: expected struct `std::vec::Vec` + found struct `std::vec::Vec`