From ca2f6bd371c94c2120bae44010daf96e9dc88d78 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Sat, 23 Oct 2021 16:30:32 -0500 Subject: [PATCH] fix(derive)!: Error, don't panic! The derive is generating `Error::raw` (from scratch or by converting existing erors) and then the inherent `Parser` methods format them. Fixes #2255 --- clap_derive/src/derives/args.rs | 72 ++++++++++++++++----------- clap_derive/src/derives/subcommand.rs | 29 ++++++----- clap_derive/src/dummies.rs | 4 +- src/derive.rs | 53 +++++++++++++++----- 4 files changed, 101 insertions(+), 57 deletions(-) diff --git a/clap_derive/src/derives/args.rs b/clap_derive/src/derives/args.rs index 4bb24710b6c1..6f7a0e6e6990 100644 --- a/clap_derive/src/derives/args.rs +++ b/clap_derive/src/derives/args.rs @@ -118,13 +118,14 @@ pub fn gen_from_arg_matches_for_struct( )] #[deny(clippy::correctness)] impl clap::FromArgMatches for #struct_name { - fn from_arg_matches(__clap_arg_matches: &clap::ArgMatches) -> Option { + fn from_arg_matches(__clap_arg_matches: &clap::ArgMatches) -> Result { let v = #struct_name #constructor; - Some(v) + ::std::result::Result::Ok(v) } - fn update_from_arg_matches(&mut self, __clap_arg_matches: &clap::ArgMatches) { + fn update_from_arg_matches(&mut self, __clap_arg_matches: &clap::ArgMatches) -> Result<(), clap::Error> { #updater + ::std::result::Result::Ok(()) } } } @@ -379,20 +380,30 @@ pub fn gen_constructor(fields: &Punctuated, parent_attribute: &Att (Ty::Option, Some(sub_type)) => sub_type, _ => &field.ty, }; - let unwrapper = match **ty { - Ty::Option => quote!(), - _ => quote_spanned!( ty.span()=> .expect("app should verify subcommand is required") ), - }; - quote_spanned! { kind.span()=> - #field_name: { - <#subcmd_type as clap::FromArgMatches>::from_arg_matches(#arg_matches) - #unwrapper - } + match **ty { + Ty::Option => { + quote_spanned! { kind.span()=> + #field_name: { + if #arg_matches.subcommand_name().map(<#subcmd_type as clap::Subcommand>::has_subcommand).unwrap_or(false) { + Some(<#subcmd_type as clap::FromArgMatches>::from_arg_matches(#arg_matches)?) + } else { + None + } + } + } + }, + _ => { + quote_spanned! { kind.span()=> + #field_name: { + <#subcmd_type as clap::FromArgMatches>::from_arg_matches(#arg_matches)? + } + } + }, } } Kind::Flatten => quote_spanned! { kind.span()=> - #field_name: clap::FromArgMatches::from_arg_matches(#arg_matches).unwrap() + #field_name: clap::FromArgMatches::from_arg_matches(#arg_matches)? }, Kind::Skip(val) => match val { @@ -448,7 +459,7 @@ pub fn gen_updater( }; let updater = quote_spanned! { ty.span()=> - <#subcmd_type as clap::FromArgMatches>::update_from_arg_matches(#field_name, #arg_matches); + <#subcmd_type as clap::FromArgMatches>::update_from_arg_matches(#field_name, #arg_matches)?; }; let updater = match **ty { @@ -456,9 +467,9 @@ pub fn gen_updater( if let Some(#field_name) = #field_name.as_mut() { #updater } else { - *#field_name = <#subcmd_type as clap::FromArgMatches>::from_arg_matches( + *#field_name = Some(<#subcmd_type as clap::FromArgMatches>::from_arg_matches( #arg_matches - ) + )?); } }, _ => quote_spanned! { kind.span()=> @@ -476,7 +487,7 @@ pub fn gen_updater( Kind::Flatten => quote_spanned! { kind.span()=> { #access - clap::FromArgMatches::update_from_arg_matches(#field_name, #arg_matches); + clap::FromArgMatches::update_from_arg_matches(#field_name, #arg_matches)?; } }, @@ -504,26 +515,27 @@ fn gen_parsers( let func = &parser.func; let span = parser.kind.span(); let convert_type = inner_type(**ty, &field.ty); + let name = attrs.cased_name(); let (value_of, values_of, mut parse) = match *parser.kind { FromStr => ( quote_spanned!(span=> value_of), quote_spanned!(span=> values_of), - func.clone(), + quote_spanned!(func.span()=> |s| ::std::result::Result::Ok::<_, clap::Error>(#func(s))), ), TryFromStr => ( quote_spanned!(span=> value_of), quote_spanned!(span=> values_of), - quote_spanned!(func.span()=> |s| #func(s).unwrap()), + quote_spanned!(func.span()=> |s| #func(s).map_err(|err| clap::Error::raw(clap::ErrorKind::ValueValidation, format!("Invalid value for {}: {}", #name, err)))), ), FromOsStr => ( quote_spanned!(span=> value_of_os), quote_spanned!(span=> values_of_os), - func.clone(), + quote_spanned!(func.span()=> |s| ::std::result::Result::Ok::<_, clap::Error>(#func(s))), ), TryFromOsStr => ( quote_spanned!(span=> value_of_os), quote_spanned!(span=> values_of_os), - quote_spanned!(func.span()=> |s| #func(s).unwrap()), + quote_spanned!(func.span()=> |s| #func(s).map_err(|err| clap::Error::raw(clap::ErrorKind::ValueValidation, format!("Invalid value for {}: {}", #name, err)))), ), FromOccurrences => ( quote_spanned!(span=> occurrences_of), @@ -536,13 +548,12 @@ fn gen_parsers( let ci = attrs.case_insensitive(); parse = quote_spanned! { convert_type.span()=> - |s| <#convert_type as clap::ArgEnum>::from_str(s, #ci).expect("app should verify the choice was valid") + |s| <#convert_type as clap::ArgEnum>::from_str(s, #ci).map_err(|err| clap::Error::raw(clap::ErrorKind::ValueValidation, format!("Invalid value for {}: {}", #name, err))) } } let flag = *attrs.parser().kind == ParserKind::FromFlag; let occurrences = *attrs.parser().kind == ParserKind::FromOccurrences; - let name = attrs.cased_name(); // Give this identifier the same hygiene // as the `arg_matches` parameter definition. This // allows us to refer to `arg_matches` within a `quote_spanned` block @@ -565,12 +576,13 @@ fn gen_parsers( quote_spanned! { ty.span()=> #arg_matches.#value_of(#name) .map(#parse) + .transpose()? } } Ty::OptionOption => quote_spanned! { ty.span()=> if #arg_matches.is_present(#name) { - Some(#arg_matches.#value_of(#name).map(#parse)) + Some(#arg_matches.#value_of(#name).map(#parse).transpose()?) } else { None } @@ -579,8 +591,9 @@ fn gen_parsers( Ty::OptionVec => quote_spanned! { ty.span()=> if #arg_matches.is_present(#name) { Some(#arg_matches.#values_of(#name) - .map(|v| v.map::<#convert_type, _>(#parse).collect()) - .unwrap_or_else(Vec::new)) + .map(|v| v.map::, _>(#parse).collect::, clap::Error>>()) + .transpose()? + .unwrap_or_else(Vec::new)) } else { None } @@ -589,7 +602,8 @@ fn gen_parsers( Ty::Vec => { quote_spanned! { ty.span()=> #arg_matches.#values_of(#name) - .map(|v| v.map::<#convert_type, _>(#parse).collect()) + .map(|v| v.map::, _>(#parse).collect::, clap::Error>>()) + .transpose()? .unwrap_or_else(Vec::new) } } @@ -605,8 +619,8 @@ fn gen_parsers( Ty::Other => { quote_spanned! { ty.span()=> #arg_matches.#value_of(#name) - .map(#parse) - .expect("app should verify arg is required") + .ok_or_else(|| clap::Error::raw(clap::ErrorKind::MissingRequiredArgument, format!("The following required argument was not provided: {}", #name))) + .and_then(#parse)? } } }; diff --git a/clap_derive/src/derives/subcommand.rs b/clap_derive/src/derives/subcommand.rs index 7593673398a0..1f17d53553a9 100644 --- a/clap_derive/src/derives/subcommand.rs +++ b/clap_derive/src/derives/subcommand.rs @@ -448,14 +448,14 @@ fn gen_from_arg_matches( Unit => quote!(), Unnamed(ref fields) if fields.unnamed.len() == 1 => { let ty = &fields.unnamed[0]; - quote!( ( <#ty as clap::FromArgMatches>::from_arg_matches(__clap_arg_matches).unwrap() ) ) + quote!( ( <#ty as clap::FromArgMatches>::from_arg_matches(__clap_arg_matches)? ) ) } Unnamed(..) => abort_call_site!("{}: tuple enums are not supported", variant.ident), }; quote! { if #sub_name == #subcommand_name_var { - return Some(#name :: #variant_name #constructor_block) + return ::std::result::Result::Ok(#name :: #variant_name #constructor_block) } } }); @@ -466,8 +466,8 @@ fn gen_from_arg_matches( let ty = &fields.unnamed[0]; quote! { if <#ty as clap::Subcommand>::has_subcommand(__clap_name) { - let __clap_res = <#ty as clap::FromArgMatches>::from_arg_matches(__clap_arg_matches).unwrap(); - return Some(#name :: #variant_name (__clap_res)); + let __clap_res = <#ty as clap::FromArgMatches>::from_arg_matches(__clap_arg_matches)?; + return ::std::result::Result::Ok(#name :: #variant_name (__clap_res)); } } } @@ -480,7 +480,7 @@ fn gen_from_arg_matches( let wildcard = match ext_subcmd { Some((span, var_name, str_ty, values_of)) => quote_spanned! { span=> - ::std::option::Option::Some(#name::#var_name( + ::std::result::Result::Ok(#name::#var_name( ::std::iter::once(#str_ty::from(#subcommand_name_var)) .chain( #sub_arg_matches_var.#values_of("").into_iter().flatten().map(#str_ty::from) @@ -489,11 +489,13 @@ fn gen_from_arg_matches( )) }, - None => quote!(None), + None => quote! { + ::std::result::Result::Err(clap::Error::raw(clap::ErrorKind::UnrecognizedSubcommand, format!("The subcommand '{}' wasn't recognized", #subcommand_name_var))) + }, }; quote! { - fn from_arg_matches(__clap_arg_matches: &clap::ArgMatches) -> Option { + fn from_arg_matches(__clap_arg_matches: &clap::ArgMatches) -> Result { if let Some((#subcommand_name_var, #sub_arg_matches_var)) = __clap_arg_matches.subcommand() { { let __clap_arg_matches = #sub_arg_matches_var; @@ -504,7 +506,7 @@ fn gen_from_arg_matches( #wildcard } else { - None + ::std::result::Result::Err(clap::Error::raw(clap::ErrorKind::MissingSubcommand, "A subcommand is required but one was not provided.")) } } } @@ -568,7 +570,7 @@ fn gen_update_from_arg_matches( quote!(clap::FromArgMatches::update_from_arg_matches( __clap_arg, __clap_sub_arg_matches - )), + )?), ) } else { abort_call_site!("{}: tuple enums are not supported", variant.ident) @@ -592,8 +594,8 @@ fn gen_update_from_arg_matches( quote! { if <#ty as clap::Subcommand>::has_subcommand(__clap_name) { if let #name :: #variant_name (child) = s { - <#ty as clap::FromArgMatches>::update_from_arg_matches(child, __clap_arg_matches); - return; + <#ty as clap::FromArgMatches>::update_from_arg_matches(child, __clap_arg_matches)?; + return ::std::result::Result::Ok(()); } } } @@ -609,16 +611,17 @@ fn gen_update_from_arg_matches( fn update_from_arg_matches<'b>( &mut self, __clap_arg_matches: &clap::ArgMatches, - ) { + ) -> Result<(), clap::Error> { if let Some((__clap_name, __clap_sub_arg_matches)) = __clap_arg_matches.subcommand() { match self { #( #subcommands ),* s => { #( #child_subcommands )* - *s = ::from_arg_matches(__clap_arg_matches).unwrap(); + *s = ::from_arg_matches(__clap_arg_matches)?; } } } + ::std::result::Result::Ok(()) } } } diff --git a/clap_derive/src/dummies.rs b/clap_derive/src/dummies.rs index 40250eaa8ae2..2314a05cf62b 100644 --- a/clap_derive/src/dummies.rs +++ b/clap_derive/src/dummies.rs @@ -32,10 +32,10 @@ pub fn into_app(name: &Ident) { pub fn from_arg_matches(name: &Ident) { append_dummy(quote! { impl clap::FromArgMatches for #name { - fn from_arg_matches(_m: &clap::ArgMatches) -> Option { + fn from_arg_matches(_m: &clap::ArgMatches) -> Result { unimplemented!() } - fn update_from_arg_matches(&mut self, matches: &clap::ArgMatches) { + fn update_from_arg_matches(&mut self, matches: &clap::ArgMatches) -> Result<(), clap::Error>{ unimplemented!() } } diff --git a/src/derive.rs b/src/derive.rs index e73ff4f06e6e..c0477a3aa909 100644 --- a/src/derive.rs +++ b/src/derive.rs @@ -74,14 +74,22 @@ pub trait Parser: FromArgMatches + IntoApp + Sized { /// Parse from `std::env::args_os()`, exit on error fn parse() -> Self { let matches = ::into_app().get_matches(); - ::from_arg_matches(&matches).expect("IntoApp validated everything") + let res = + ::from_arg_matches(&matches).map_err(format_error::); + match res { + Ok(s) => s, + Err(e) => { + // Since this is more of a development-time error, we aren't doing as fancy of a quit + // as `get_matches` + e.exit() + } + } } /// Parse from `std::env::args_os()`, return Err on error. fn try_parse() -> Result { let matches = ::into_app().try_get_matches()?; - Ok(::from_arg_matches(&matches) - .expect("IntoApp validated everything")) + ::from_arg_matches(&matches).map_err(format_error::) } /// Parse from iterator, exit on error @@ -92,7 +100,16 @@ pub trait Parser: FromArgMatches + IntoApp + Sized { T: Into + Clone, { let matches = ::into_app().get_matches_from(itr); - ::from_arg_matches(&matches).expect("IntoApp validated everything") + let res = + ::from_arg_matches(&matches).map_err(format_error::); + match res { + Ok(s) => s, + Err(e) => { + // Since this is more of a development-time error, we aren't doing as fancy of a quit + // as `get_matches_from` + e.exit() + } + } } /// Parse from iterator, return Err on error. @@ -103,8 +120,7 @@ pub trait Parser: FromArgMatches + IntoApp + Sized { T: Into + Clone, { let matches = ::into_app().try_get_matches_from(itr)?; - Ok(::from_arg_matches(&matches) - .expect("IntoApp validated everything")) + ::from_arg_matches(&matches).map_err(format_error::) } /// Update from iterator, exit on error @@ -116,7 +132,13 @@ pub trait Parser: FromArgMatches + IntoApp + Sized { { // TODO find a way to get partial matches let matches = ::into_app_for_update().get_matches_from(itr); - ::update_from_arg_matches(self, &matches); + let res = ::update_from_arg_matches(self, &matches) + .map_err(format_error::); + if let Err(e) = res { + // Since this is more of a development-time error, we aren't doing as fancy of a quit + // as `get_matches_from` + e.exit() + } } /// Update from iterator, return Err on error. @@ -127,8 +149,8 @@ pub trait Parser: FromArgMatches + IntoApp + Sized { T: Into + Clone, { let matches = ::into_app_for_update().try_get_matches_from(itr)?; - ::update_from_arg_matches(self, &matches); - Ok(()) + ::update_from_arg_matches(self, &matches) + .map_err(format_error::) } } @@ -178,10 +200,10 @@ pub trait FromArgMatches: Sized { /// } /// } /// ``` - fn from_arg_matches(matches: &ArgMatches) -> Option; + fn from_arg_matches(matches: &ArgMatches) -> Result; /// Assign values from `ArgMatches` to `self`. - fn update_from_arg_matches(&mut self, matches: &ArgMatches); + fn update_from_arg_matches(&mut self, matches: &ArgMatches) -> Result<(), Error>; } /// Parse arguments into a user-defined container. @@ -347,10 +369,10 @@ impl IntoApp for Box { } impl FromArgMatches for Box { - fn from_arg_matches(matches: &ArgMatches) -> Option { + fn from_arg_matches(matches: &ArgMatches) -> Result { ::from_arg_matches(matches).map(Box::new) } - fn update_from_arg_matches(&mut self, matches: &ArgMatches) { + fn update_from_arg_matches(&mut self, matches: &ArgMatches) -> Result<(), Error> { ::update_from_arg_matches(self, matches) } } @@ -375,3 +397,8 @@ impl Subcommand for Box { ::has_subcommand(name) } } + +fn format_error(err: crate::Error) -> crate::Error { + let mut app = I::into_app(); + err.format(&mut app) +}