Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(derive)!: Don't Panic, Error #2943

Merged
merged 5 commits into from Oct 26, 2021
Merged

fix(derive)!: Don't Panic, Error #2943

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f1bf9fc
refactor(error): Delay formatting until the end
epage Oct 23, 2021
83b074a
refactor(error): Extract formatting of raw messages
epage Oct 23, 2021
3c4340c
feat(error): Allow separate raise, format sites
epage Oct 23, 2021
f8bca3a
fix(error): Never show unrequested color
epage Oct 25, 2021
53e10b4
fix(derive)!: Error, don't panic!
epage Oct 23, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
72 changes: 43 additions & 29 deletions clap_derive/src/derives/args.rs
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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