Skip to content

Commit

Permalink
Merge #2943
Browse files Browse the repository at this point in the history 
2943: fix(derive)!: Don't Panic, Error r=epage a=epage



Co-authored-by: Ed Page <eopage@gmail.com>
  • Loading branch information
@bors @epage
bors[bot] and epage authored Oct 26, 2021
2 parents c0ac536 + 53e10b4 commit 07fcaa9
Show file tree
Hide file tree
Showing 8 changed files with 189 additions and 98 deletions.
72 changes: 43 additions & 29 deletions clap_derive/src/derives/args.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<Self> {
fn from_arg_matches(__clap_arg_matches: &clap::ArgMatches) -> Result<Self, clap::Error> {
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(())
}
}
}
Expand Down Expand Up @@ -379,20 +380,30 @@ pub fn gen_constructor(fields: &Punctuated<Field, Comma>, 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 {
Expand Down Expand Up @@ -448,17 +459,17 @@ 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 {
Ty::Option => quote_spanned! { kind.span()=>
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()=>
Expand All @@ -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)?;
}
},

Expand Down Expand Up @@ -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),
Expand All @@ -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
Expand All @@ -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
}
Expand All @@ -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::<Result<#convert_type, clap::Error>, _>(#parse).collect::<Result<Vec<_>, clap::Error>>())
.transpose()?
.unwrap_or_else(Vec::new))
} else {
None
}
Expand All @@ -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::<Result<#convert_type, clap::Error>, _>(#parse).collect::<Result<Vec<_>, clap::Error>>())
.transpose()?
.unwrap_or_else(Vec::new)
}
}
Expand All @@ -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)?
}
}
};
Expand Down
29 changes: 16 additions & 13 deletions clap_derive/src/derives/subcommand.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
});
Expand All @@ -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));
}
}
}
Expand All @@ -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)
Expand All @@ -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<Self> {
fn from_arg_matches(__clap_arg_matches: &clap::ArgMatches) -> Result<Self, clap::Error> {
if let Some((#subcommand_name_var, #sub_arg_matches_var)) = __clap_arg_matches.subcommand() {
{
let __clap_arg_matches = #sub_arg_matches_var;
Expand All @@ -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."))
}
}
}
Expand Down Expand Up @@ -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)
Expand All @@ -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(());
}
}
}
Expand All @@ -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 = <Self as clap::FromArgMatches>::from_arg_matches(__clap_arg_matches).unwrap();
*s = <Self as clap::FromArgMatches>::from_arg_matches(__clap_arg_matches)?;
}
}
}
::std::result::Result::Ok(())
}
}
}
4 changes: 2 additions & 2 deletions clap_derive/src/dummies.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<Self> {
fn from_arg_matches(_m: &clap::ArgMatches) -> Result<Self, clap::Error> {
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!()
}
}
Expand Down
8 changes: 3 additions & 5 deletions src/build/app/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1813,9 +1813,7 @@ impl<'help> App<'help> {
/// let err = app.error(ErrorKind::InvalidValue, "Some failure case");
/// ```
pub fn error(&mut self, kind: ErrorKind, message: impl std::fmt::Display) -> Error {
self._build();
let usage = self.render_usage();
Error::user_error(self, usage, kind, message)
Error::raw(kind, message).format(self)
}

/// Prints the full help message to [`io::stdout()`] using a [`BufWriter`] using the same
Expand Down Expand Up @@ -2032,7 +2030,7 @@ impl<'help> App<'help> {
.unwrap_or_else(|e| {
// Otherwise, write to stderr and exit
if e.use_stderr() {
e.message.print().expect("Error writing Error to stderr");
e.print().expect("Error writing Error to stderr");

if self.settings.is_set(AppSettings::WaitOnError) {
wlnerr!("\nPress [ENTER] / [RETURN] to continue...");
Expand Down Expand Up @@ -2114,7 +2112,7 @@ impl<'help> App<'help> {
self.try_get_matches_from_mut(itr).unwrap_or_else(|e| {
// Otherwise, write to stderr and exit
if e.use_stderr() {
e.message.print().expect("Error writing Error to stderr");
e.print().expect("Error writing Error to stderr");

if self.settings.is_set(AppSettings::WaitOnError) {
wlnerr!("\nPress [ENTER] / [RETURN] to continue...");
Expand Down
Loading

0 comments on commit 07fcaa9

Please sign in to comment.