Skip to content

Commit

Permalink
imp: Make clap_derive call FromStr::from_str only once per argument.
Browse files Browse the repository at this point in the history 
Currently the way `clap_derive` uses a `from_str` function is to call
it once as a validator, discarding the parsed value, and then to call
it again to recompute the parsed value. This is unfortunate in
cases where `from_str` is expensive or has side effects.

This PR changes `clap_derive` to not register `from_str` as a validator
so that it doesn't do the first of these two calls. Then, instead of
doing `unwrap()` on the other call, it handles the error. This eliminates
the redundancy, and also avoids the small performance hit mentioned in
[the documentation about validators].

[the documentation about validators]: https://docs.rs/clap-v3/3.0.0-beta.1/clap_v3/struct.Arg.html#method.validator

This PR doesn't yet use colorized messages for errors generated during
parsing because the `ColorWhen` setting isn't currently available.
That's fixable with some refactoring, but I'm interested in getting
feedback on the overall approach here first.
  • Loading branch information
@sunfishcode
sunfishcode committed Jul 13, 2021
1 parent af6f7af commit db8c060
Show file tree
Hide file tree
Showing 9 changed files with 611 additions and 174 deletions.
198 changes: 103 additions & 95 deletions clap_derive/src/derives/from_arg_matches.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,12 +43,16 @@ pub fn gen_for_struct(
)]
#[deny(clippy::correctness)]
impl clap::FromArgMatches for #struct_name {
fn from_arg_matches(arg_matches: &clap::ArgMatches) -> Self {
#struct_name #constructor
fn try_from_arg_matches(arg_matches: &clap::ArgMatches) -> clap::Result<Self> {
Ok(#struct_name #constructor)
}

fn update_from_arg_matches(&mut self, arg_matches: &clap::ArgMatches) {
fn try_update_from_arg_matches(
&mut self,
arg_matches: &clap::ArgMatches
) -> clap::Result<()> {
#updater
Ok(())
}
}
}
Expand All @@ -69,11 +73,14 @@ pub fn gen_for_enum(name: &Ident) -> TokenStream {
)]
#[deny(clippy::correctness)]
impl clap::FromArgMatches for #name {
fn from_arg_matches(arg_matches: &clap::ArgMatches) -> Self {
<#name as clap::Subcommand>::from_subcommand(arg_matches.subcommand()).unwrap()
fn try_from_arg_matches(arg_matches: &clap::ArgMatches) -> clap::Result<Self> {
<#name as clap::Subcommand>::from_subcommand(arg_matches.subcommand())
}
fn update_from_arg_matches(&mut self, arg_matches: &clap::ArgMatches) {
<#name as clap::Subcommand>::update_from_subcommand(self, arg_matches.subcommand());
fn try_update_from_arg_matches(
&mut self,
arg_matches: &clap::ArgMatches
) -> clap::Result<()> {
<#name as clap::Subcommand>::update_from_subcommand(self, arg_matches.subcommand())
}
}
}
Expand All @@ -83,7 +90,7 @@ fn gen_arg_enum_parse(ty: &Type, attrs: &Attrs) -> TokenStream {
let ci = attrs.case_insensitive();

quote_spanned! { ty.span()=>
|s| <#ty as clap::ArgEnum>::from_str(s, #ci).unwrap()
|s| <#ty as clap::ArgEnum>::from_str(s, #ci)
}
}

Expand All @@ -99,35 +106,18 @@ fn gen_parsers(
let parser = attrs.parser();
let func = &parser.func;
let span = parser.kind.span();
let (value_of, values_of, mut parse) = match *parser.kind {
FromStr => (
quote_spanned!(span=> value_of),
quote_spanned!(span=> values_of),
func.clone(),
),
TryFromStr => (
quote_spanned!(span=> value_of),
quote_spanned!(span=> values_of),
quote_spanned!(func.span()=> |s| #func(s).unwrap()),
),
FromOsStr => (
quote_spanned!(span=> value_of_os),
quote_spanned!(span=> values_of_os),
func.clone(),
),
TryFromOsStr => (
quote_spanned!(span=> value_of_os),
quote_spanned!(span=> values_of_os),
quote_spanned!(func.span()=> |s| #func(s).unwrap()),
),
FromOccurrences => (
quote_spanned!(span=> occurrences_of),
quote!(),
func.clone(),
),
FromFlag => (quote!(), quote!(), func.clone()),

// The operand type of the `parse` function.
let parse_operand_type = match *parser.kind {
FromStr | TryFromStr => quote_spanned!(ty.span()=> &str),
FromOsStr | TryFromOsStr => quote_spanned!(ty.span()=> &::std::ffi::OsStr),
FromOccurrences => quote_spanned!(ty.span()=> u64),
FromFlag => quote_spanned!(ty.span()=> bool),
};

// Wrap `parse` in a closure so that we can give the operand a concrete type.
let mut parse = quote_spanned!(span=> |s: #parse_operand_type| #func(s));

let flag = *attrs.parser().kind == ParserKind::FromFlag;
let occurrences = *attrs.parser().kind == ParserKind::FromOccurrences;
let name = attrs.cased_name();
Expand All @@ -136,83 +126,96 @@ fn gen_parsers(
// allows us to refer to `arg_matches` within a `quote_spanned` block
let arg_matches = quote! { arg_matches };

let field_value = match **ty {
Ty::Bool => {
if update.is_some() {
quote_spanned! { ty.span()=>
*#field_name || #arg_matches.is_present(#name)
}
} else {
quote_spanned! { ty.span()=>
#arg_matches.is_present(#name)
if attrs.is_enum() {
match **ty {
Ty::Option => {
if let Some(subty) = subty_if_name(&field.ty, "Option") {
parse = gen_arg_enum_parse(subty, attrs);
}
}
}

Ty::Option => {
if attrs.is_enum() {
if let Some(subty) = subty_if_name(&field.ty, "Option") {
Ty::Vec => {
if let Some(subty) = subty_if_name(&field.ty, "Vec") {
parse = gen_arg_enum_parse(subty, attrs);
}
}

quote_spanned! { ty.span()=>
#arg_matches.#value_of(#name)
.map(#parse)
Ty::Other => {
parse = gen_arg_enum_parse(&field.ty, attrs);
}
_ => {}
}
}

let (value_of, values_of) = match *parser.kind {
FromStr => (
quote_spanned!(span=> #arg_matches.value_of(#name).map(#parse)),
quote_spanned!(span=>
#arg_matches.values_of(#name).map(|values| values.map(#parse)).collect()
),
),
TryFromStr => (
quote_spanned!(span=> #arg_matches.parse_optional_t(#name, #parse)?),
quote_spanned!(span=> #arg_matches.parse_optional_vec_t(#name, #parse)?),
),
FromOsStr => (
quote_spanned!(span=> #arg_matches.value_of_os(#name).map(#parse)),
quote_spanned!(span=>
#arg_matches.values_of_os(#name).map(|values| values.map(#parse).collect())
),
),
TryFromOsStr => (
quote_spanned!(span=> #arg_matches.parse_optional_t_os(#name, #parse)?),
quote_spanned!(span=> #arg_matches.parse_optional_vec_t_os(#name, #parse)?),
),
FromOccurrences => (
quote_spanned!(span=> (#parse)(#arg_matches.occurrences_of(#name))),
quote!(),
),
FromFlag => (
quote_spanned!(span => (#parse)(#arg_matches.is_present(#name))),
quote!(),
),
};

let field_value = match **ty {
Ty::Bool => quote_spanned! { ty.span()=>
#arg_matches.is_present(#name)
},

Ty::Option => quote_spanned! { ty.span()=>
#value_of
},

Ty::OptionOption => quote_spanned! { ty.span()=>
if #arg_matches.is_present(#name) {
Some(#arg_matches.#value_of(#name).map(#parse))
Some(#value_of)
} else {
None
}
},

Ty::OptionVec => quote_spanned! { ty.span()=>
if #arg_matches.is_present(#name) {
Some(#arg_matches.#values_of(#name)
.map(|v| v.map(#parse).collect())
.unwrap_or_else(Vec::new))
Some(#values_of.unwrap_or_else(Vec::new))
} else {
None
}
},

Ty::Vec => {
if attrs.is_enum() {
if let Some(subty) = subty_if_name(&field.ty, "Vec") {
parse = gen_arg_enum_parse(subty, attrs);
}
}

quote_spanned! { ty.span()=>
#arg_matches.#values_of(#name)
.map(|v| v.map(#parse).collect())
.unwrap_or_else(Vec::new)
}
}
Ty::Vec => quote_spanned! { ty.span()=>
#values_of.unwrap_or_else(Vec::new)
},

Ty::Other if occurrences => quote_spanned! { ty.span()=>
#parse(#arg_matches.#value_of(#name))
#value_of
},

Ty::Other if flag => quote_spanned! { ty.span()=>
#parse(#arg_matches.is_present(#name))
#value_of
},

Ty::Other => {
if attrs.is_enum() {
parse = gen_arg_enum_parse(&field.ty, attrs);
}

quote_spanned! { ty.span()=>
#arg_matches.#value_of(#name)
.map(#parse)
.unwrap()
}
}
Ty::Other => quote_spanned! { ty.span()=>
#value_of.unwrap()
},
};

if let Some(access) = update {
Expand Down Expand Up @@ -248,20 +251,25 @@ 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()=> .unwrap() ),
let from_subcommand = quote_spanned! { kind.span() =>
<#subcmd_type as clap::Subcommand>::from_subcommand(#arg_matches.subcommand())
};
quote_spanned! { kind.span()=>
#field_name: {
<#subcmd_type as clap::Subcommand>::from_subcommand(#arg_matches.subcommand())
#unwrapper
}
match **ty {
Ty::Option => quote_spanned! { kind.span()=>
#field_name: match #from_subcommand {
Ok(cmd) => Some(cmd),
Err(clap::Error { kind: clap::ErrorKind::UnrecognizedSubcommand, .. }) => None,
Err(e) => return Err(e),
}
},
_ => quote_spanned! { kind.span()=>
#field_name: #from_subcommand?
},
}
}

Kind::Flatten => quote_spanned! { kind.span()=>
#field_name: clap::FromArgMatches::from_arg_matches(#arg_matches)
#field_name: clap::FromArgMatches::try_from_arg_matches(#arg_matches)?
},

Kind::Skip(val) => match val {
Expand Down Expand Up @@ -315,17 +323,17 @@ pub fn gen_updater(
};

let updater = quote_spanned! { ty.span()=>
<#subcmd_type as clap::Subcommand>::update_from_subcommand(#field_name, #arg_matches.subcommand());
<#subcmd_type as clap::Subcommand>::update_from_subcommand(#field_name, #arg_matches.subcommand())?;
};

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::Subcommand>::from_subcommand(
*#field_name = Some(<#subcmd_type as clap::Subcommand>::from_subcommand(
#arg_matches.subcommand()
)
)?)
}
},
_ => quote_spanned! { kind.span()=>
Expand Down
33 changes: 2 additions & 31 deletions clap_derive/src/derives/into_app.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -242,33 +242,9 @@ pub fn gen_app_augmentation(
})
}
Kind::Arg(ty) => {
let convert_type = match **ty {
Ty::Vec | Ty::Option => sub_type(&field.ty).unwrap_or(&field.ty),
Ty::OptionOption | Ty::OptionVec => {
sub_type(&field.ty).and_then(sub_type).unwrap_or(&field.ty)
}
_ => &field.ty,
};

let occurrences = *attrs.parser().kind == ParserKind::FromOccurrences;
let flag = *attrs.parser().kind == ParserKind::FromFlag;

let parser = attrs.parser();
let func = &parser.func;

let validator = match *parser.kind {
_ if attrs.is_enum() => quote!(),
ParserKind::TryFromStr => quote_spanned! { func.span()=>
.validator(|s| {
#func(s)
.map(|_: #convert_type| ())
})
},
ParserKind::TryFromOsStr => quote_spanned! { func.span()=>
.validator_os(|s| #func(s).map(|_: #convert_type| ()))
},
_ => quote!(),
};
let occurrences = parser.kind == ParserKind::FromOccurrences;
let flag = parser.kind == ParserKind::FromFlag;

let modifier = match **ty {
Ty::Bool => quote!(),
Expand All @@ -285,7 +261,6 @@ pub fn gen_app_augmentation(
quote_spanned! { ty.span()=>
.takes_value(true)
#possible_values
#validator
}
}

Expand All @@ -294,14 +269,12 @@ pub fn gen_app_augmentation(
.multiple_values(false)
.min_values(0)
.max_values(1)
#validator
},

Ty::OptionVec => quote_spanned! { ty.span()=>
.takes_value(true)
.multiple_values(true)
.min_values(0)
#validator
},

Ty::Vec => {
Expand All @@ -317,7 +290,6 @@ pub fn gen_app_augmentation(
.takes_value(true)
.multiple_values(true)
#possible_values
#validator
}
}

Expand All @@ -342,7 +314,6 @@ pub fn gen_app_augmentation(
.takes_value(true)
.required(#required)
#possible_values
#validator
}
}
};
Expand Down
Loading

0 comments on commit db8c060

Please sign in to comment.