From 790a0f5e620deef49af12308851c3635712635c9 Mon Sep 17 00:00:00 2001 From: CreepySkeleton Date: Thu, 2 Jul 2020 06:16:13 +0300 Subject: [PATCH 1/6] Make sure that App & Arg & ArgGroup implement Send + Sync Also relaxes 'static restriction on validators. --- src/build/app/tests.rs | 7 +++++++ src/build/arg/mod.rs | 22 +++++++++++----------- src/build/arg/tests.rs | 7 +++++++ src/build/arg_group.rs | 7 +++++++ 4 files changed, 32 insertions(+), 11 deletions(-) diff --git a/src/build/app/tests.rs b/src/build/app/tests.rs index f5a8e2dffdb..bdf21114530 100644 --- a/src/build/app/tests.rs +++ b/src/build/app/tests.rs @@ -44,3 +44,10 @@ fn global_settings() { .unwrap() .is_set(AppSettings::TrailingVarArg)); } + +// This test will *fail to compile* if App is not Send + Sync +#[test] +fn app_send_sync() { + fn foo(_: T) {} + foo(App::new("test")) +} diff --git a/src/build/arg/mod.rs b/src/build/arg/mod.rs index 828487da1ae..8476b2b9371 100644 --- a/src/build/arg/mod.rs +++ b/src/build/arg/mod.rs @@ -13,8 +13,8 @@ use std::{ env, ffi::{OsStr, OsString}, fmt::{self, Display, Formatter}, - rc::Rc, str, + sync::Arc, }; // Third Party @@ -30,8 +30,8 @@ use crate::{ #[cfg(feature = "yaml")] use yaml_rust::Yaml; -type Validator = Rc Result<(), String>>; -type ValidatorOs = Rc Result<(), String>>; +type Validator<'a> = dyn Fn(&str) -> Result<(), String> + Send + Sync + 'a; +type ValidatorOs<'a> = dyn Fn(&OsStr) -> Result<(), String> + Send + Sync + 'a; /// The abstract representation of a command line argument. Used to set all the options and /// relationships that define a valid argument for the program. @@ -80,8 +80,8 @@ pub struct Arg<'help> { pub(crate) num_vals: Option, pub(crate) max_vals: Option, pub(crate) min_vals: Option, - pub(crate) validator: Option, - pub(crate) validator_os: Option, + pub(crate) validator: Option>>, + pub(crate) validator_os: Option>>, pub(crate) val_delim: Option, pub(crate) default_vals: Vec<&'help OsStr>, pub(crate) default_vals_ifs: VecMap<(Id, Option<&'help OsStr>, &'help OsStr)>, @@ -1844,7 +1844,7 @@ impl<'help> Arg<'help> { /// arg, and `` is the `String` you return as the error. /// /// **NOTE:** There is a small performance hit for using validators, as they are implemented - /// with [`Rc`] pointers. And the value to be checked will be allocated an extra time in order + /// with [`Arc`] pointers. And the value to be checked will be allocated an extra time in order /// to be passed to the closure. This performance hit is extremely minimal in the grand /// scheme of things. /// @@ -1869,13 +1869,13 @@ impl<'help> Arg<'help> { /// [`String`]: https://doc.rust-lang.org/std/string/struct.String.html /// [`Result`]: https://doc.rust-lang.org/std/result/enum.Result.html /// [`Err(String)`]: https://doc.rust-lang.org/std/result/enum.Result.html#variant.Err - /// [`Rc`]: https://doc.rust-lang.org/std/rc/struct.Rc.html + /// [`Arc`]: https://doc.rust-lang.org/std/sync/struct.Arc.html pub fn validator(mut self, f: F) -> Self where - F: Fn(&str) -> Result + 'static, + F: Fn(&str) -> Result + Sync + Send + 'help, E: ToString, { - self.validator = Some(Rc::new(move |s| { + self.validator = Some(Arc::new(move |s| { f(s).map(|_| ()).map_err(|e| e.to_string()) })); self @@ -1913,9 +1913,9 @@ impl<'help> Arg<'help> { /// [`Rc`]: https://doc.rust-lang.org/std/rc/struct.Rc.html pub fn validator_os(mut self, f: F) -> Self where - F: Fn(&OsStr) -> Result + 'static, + F: Fn(&OsStr) -> Result + Sync + Send + 'help, { - self.validator_os = Some(Rc::new(move |s| f(s).map(|_| ()))); + self.validator_os = Some(Arc::new(move |s| f(s).map(|_| ()))); self } diff --git a/src/build/arg/tests.rs b/src/build/arg/tests.rs index 9d529293d91..0d78630e3bd 100644 --- a/src/build/arg/tests.rs +++ b/src/build/arg/tests.rs @@ -23,3 +23,10 @@ fn short_flag_name_missing() { assert!(a.val_names.is_empty()); assert!(a.num_vals.is_none()); } + +// This test will *fail to compile* if Arg is not Send + Sync +#[test] +fn arg_send_sync() { + fn foo(_: T) {} + foo(Arg::new("test")) +} diff --git a/src/build/arg_group.rs b/src/build/arg_group.rs index bd8d73c8c23..bb0c478a0c4 100644 --- a/src/build/arg_group.rs +++ b/src/build/arg_group.rs @@ -550,6 +550,13 @@ requires: assert_eq!(g.requires, reqs); assert_eq!(g.conflicts, confs); } + + // This test will *fail to compile* if ArgGroup is not Send + Sync + #[test] + fn arg_group_send_sync() { + fn foo(_: T) {} + foo(ArgGroup::new("test")) + } } impl Clone for ArgGroup<'_> { From 18a58af3acd5799b5685d734cc13193602288ad4 Mon Sep 17 00:00:00 2001 From: CreepySkeleton Date: Fri, 3 Jul 2020 09:44:58 +0300 Subject: [PATCH 2/6] Allow validators to be FnMut --- src/build/app/mod.rs | 2 +- src/build/arg/mod.rs | 28 ++++++++++++++-------------- src/output/help.rs | 6 +++--- src/parse/matches/arg_matches.rs | 2 +- src/parse/validator.rs | 10 ++++++++-- 5 files changed, 27 insertions(+), 21 deletions(-) diff --git a/src/build/app/mod.rs b/src/build/app/mod.rs index 6e5c38febb7..4522c9c13ca 100644 --- a/src/build/app/mod.rs +++ b/src/build/app/mod.rs @@ -2091,7 +2091,7 @@ impl<'help> App<'help> { where F: Fn(&Arg) -> bool, { - two_elements_of(self.args.args.iter().filter(|a| condition(a))) + two_elements_of(self.args.args.iter().filter(|a: &&Arg| condition(a))) } // just in case diff --git a/src/build/arg/mod.rs b/src/build/arg/mod.rs index 8476b2b9371..37742445023 100644 --- a/src/build/arg/mod.rs +++ b/src/build/arg/mod.rs @@ -14,7 +14,7 @@ use std::{ ffi::{OsStr, OsString}, fmt::{self, Display, Formatter}, str, - sync::Arc, + sync::{Arc, Mutex}, }; // Third Party @@ -30,8 +30,8 @@ use crate::{ #[cfg(feature = "yaml")] use yaml_rust::Yaml; -type Validator<'a> = dyn Fn(&str) -> Result<(), String> + Send + Sync + 'a; -type ValidatorOs<'a> = dyn Fn(&OsStr) -> Result<(), String> + Send + Sync + 'a; +type Validator<'a> = dyn FnMut(&str) -> Result<(), String> + Send + 'a; +type ValidatorOs<'a> = dyn FnMut(&OsStr) -> Result<(), String> + Send + 'a; /// The abstract representation of a command line argument. Used to set all the options and /// relationships that define a valid argument for the program. @@ -80,8 +80,8 @@ pub struct Arg<'help> { pub(crate) num_vals: Option, pub(crate) max_vals: Option, pub(crate) min_vals: Option, - pub(crate) validator: Option>>, - pub(crate) validator_os: Option>>, + pub(crate) validator: Option>>>, + pub(crate) validator_os: Option>>>, pub(crate) val_delim: Option, pub(crate) default_vals: Vec<&'help OsStr>, pub(crate) default_vals_ifs: VecMap<(Id, Option<&'help OsStr>, &'help OsStr)>, @@ -1870,14 +1870,14 @@ impl<'help> Arg<'help> { /// [`Result`]: https://doc.rust-lang.org/std/result/enum.Result.html /// [`Err(String)`]: https://doc.rust-lang.org/std/result/enum.Result.html#variant.Err /// [`Arc`]: https://doc.rust-lang.org/std/sync/struct.Arc.html - pub fn validator(mut self, f: F) -> Self + pub fn validator(mut self, mut f: F) -> Self where - F: Fn(&str) -> Result + Sync + Send + 'help, + F: FnMut(&str) -> Result + Send + 'help, E: ToString, { - self.validator = Some(Arc::new(move |s| { + self.validator = Some(Arc::new(Mutex::new(move |s: &str| { f(s).map(|_| ()).map_err(|e| e.to_string()) - })); + }))); self } @@ -1911,11 +1911,11 @@ impl<'help> Arg<'help> { /// [`Result`]: https://doc.rust-lang.org/std/result/enum.Result.html /// [`Err(String)`]: https://doc.rust-lang.org/std/result/enum.Result.html#variant.Err /// [`Rc`]: https://doc.rust-lang.org/std/rc/struct.Rc.html - pub fn validator_os(mut self, f: F) -> Self + pub fn validator_os(mut self, mut f: F) -> Self where - F: Fn(&OsStr) -> Result + Sync + Send + 'help, + F: FnMut(&OsStr) -> Result + Send + 'help, { - self.validator_os = Some(Arc::new(move |s| f(s).map(|_| ()))); + self.validator_os = Some(Arc::new(Mutex::new(move |s: &OsStr| f(s).map(|_| ())))); self } @@ -4494,11 +4494,11 @@ impl<'help> fmt::Debug for Arg<'help> { .field("min_vals", &self.min_vals) .field( "validator", - &self.validator.as_ref().map_or("None", |_| "Some(Fn)"), + &self.validator.as_ref().map_or("None", |_| "Some(FnMut)"), ) .field( "validator_os", - &self.validator_os.as_ref().map_or("None", |_| "Some(Fn)"), + &self.validator_os.as_ref().map_or("None", |_| "Some(FnMut)"), ) .field("val_delim", &self.val_delim) .field("default_vals", &self.default_vals) diff --git a/src/output/help.rs b/src/output/help.rs index d378dfdefc2..a9a2c72687b 100644 --- a/src/output/help.rs +++ b/src/output/help.rs @@ -222,7 +222,7 @@ impl<'help, 'app, 'parser, 'writer> Help<'help, 'app, 'parser, 'writer> { } /// Writes help for an argument to the wrapped stream. - fn write_arg(&mut self, arg: &Arg, prevent_nlh: bool) -> io::Result<()> { + fn write_arg(&mut self, arg: &Arg<'help>, prevent_nlh: bool) -> io::Result<()> { debug!("Help::write_arg"); self.short(arg)?; self.long(arg)?; @@ -232,7 +232,7 @@ impl<'help, 'app, 'parser, 'writer> Help<'help, 'app, 'parser, 'writer> { } /// Writes argument's short command to the wrapped stream. - fn short(&mut self, arg: &Arg) -> io::Result<()> { + fn short(&mut self, arg: &Arg<'help>) -> io::Result<()> { debug!("Help::short"); self.none(TAB)?; @@ -247,7 +247,7 @@ impl<'help, 'app, 'parser, 'writer> Help<'help, 'app, 'parser, 'writer> { } /// Writes argument's long command to the wrapped stream. - fn long(&mut self, arg: &Arg) -> io::Result<()> { + fn long(&mut self, arg: &Arg<'help>) -> io::Result<()> { debug!("Help::long"); if !arg.has_switch() { return Ok(()); diff --git a/src/parse/matches/arg_matches.rs b/src/parse/matches/arg_matches.rs index 02849bb097c..25a7a42f39a 100644 --- a/src/parse/matches/arg_matches.rs +++ b/src/parse/matches/arg_matches.rs @@ -214,7 +214,7 @@ impl ArgMatches { /// ``` /// [`Values`]: ./struct.Values.html /// [`Iterator`]: https://doc.rust-lang.org/std/iter/trait.Iterator.html - pub fn values_of(&self, id: T) -> Option> { + pub fn values_of(&self, id: T) -> Option { self.args.get(&Id::from(id)).map(|arg| { fn to_str_slice(o: &OsString) -> &str { o.to_str().expect(INVALID_UTF8) diff --git a/src/parse/validator.rs b/src/parse/validator.rs index 33bc32840e6..5df7697a4fc 100644 --- a/src/parse/validator.rs +++ b/src/parse/validator.rs @@ -136,9 +136,14 @@ impl<'help, 'app, 'parser> Validator<'help, 'app, 'parser> { self.p.app.color(), )?); } + + // FIXME: `(&mut *vtor)(args...)` can be simplified to `vtor(args...)` + // once https://github.com/rust-lang/rust/pull/72280 is landed on stable + // (about 10 weeks from now) if let Some(ref vtor) = arg.validator { debug!("Validator::validate_arg_values: checking validator..."); - if let Err(e) = vtor(&*val.to_string_lossy()) { + let mut vtor = vtor.lock().unwrap(); + if let Err(e) = (&mut *vtor)(&*val.to_string_lossy()) { debug!("error"); return Err(Error::value_validation(Some(arg), &e, self.p.app.color())?); } else { @@ -147,7 +152,8 @@ impl<'help, 'app, 'parser> Validator<'help, 'app, 'parser> { } if let Some(ref vtor) = arg.validator_os { debug!("Validator::validate_arg_values: checking validator_os..."); - if let Err(e) = vtor(val) { + let mut vtor = vtor.lock().unwrap(); + if let Err(e) = (&mut *vtor)(val) { debug!("error"); return Err(Error::value_validation( Some(arg), From 619658e17aa1b4a3fe8063cf4b9ec0dbca1400b2 Mon Sep 17 00:00:00 2001 From: CreepySkeleton Date: Fri, 3 Jul 2020 11:00:11 +0300 Subject: [PATCH 3/6] Add test --- tests/validators.rs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tests/validators.rs b/tests/validators.rs index bcc8f56bafd..2b8bb64aa06 100644 --- a/tests/validators.rs +++ b/tests/validators.rs @@ -48,3 +48,17 @@ fn test_validator_msg_newline() { let msg = format!("{}", err); assert!(msg.ends_with('\n')); } + +#[test] +fn stateful_validator() { + let mut state = false; + App::new("test") + .arg(Arg::new("test").validator(|val| { + state = true; + val.parse::().map_err(|e| e.to_string()) + })) + .try_get_matches_from(&["app", "10"]) + .unwrap(); + + assert!(state); +} From 32414fbdffabb0dc2dc1b52157026df2195a3760 Mon Sep 17 00:00:00 2001 From: CreepySkeleton Date: Sun, 2 Aug 2020 20:53:07 +0300 Subject: [PATCH 4/6] Fix and rearrange debug asserts --- src/build/app/debug_asserts.rs | 282 +++++++++++++++++++++++++++++ src/build/app/mod.rs | 321 +++------------------------------ tests/flag_subcommands.rs | 12 +- 3 files changed, 314 insertions(+), 301 deletions(-) create mode 100644 src/build/app/debug_asserts.rs diff --git a/src/build/app/debug_asserts.rs b/src/build/app/debug_asserts.rs new file mode 100644 index 00000000000..ee68ea3fd02 --- /dev/null +++ b/src/build/app/debug_asserts.rs @@ -0,0 +1,282 @@ +use crate::{App, AppSettings, ArgSettings}; +use std::cmp::Ordering; + +#[derive(Eq)] +enum Flag<'a> { + App(String, &'a str), + Arg(String, &'a str), +} + +impl PartialEq for Flag<'_> { + fn eq(&self, other: &Flag) -> bool { + self.cmp(other) == Ordering::Equal + } +} + +impl PartialOrd for Flag<'_> { + fn partial_cmp(&self, other: &Flag) -> Option { + use Flag::*; + + match (self, other) { + (App(s1, _), App(s2, _)) + | (Arg(s1, _), Arg(s2, _)) + | (App(s1, _), Arg(s2, _)) + | (Arg(s1, _), App(s2, _)) => { + if s1 == s2 { + Some(Ordering::Equal) + } else { + s1.partial_cmp(s2) + } + } + } + } +} + +impl Ord for Flag<'_> { + fn cmp(&self, other: &Self) -> Ordering { + self.partial_cmp(other).unwrap() + } +} + +pub(crate) fn assert_app(app: &App) { + debug!("App::_debug_asserts"); + + let mut short_flags = vec![]; + let mut long_flags = vec![]; + + for sc in &app.subcommands { + if let Some(s) = sc.short_flag.as_ref() { + short_flags.push(Flag::App(format!("-{}", s), &sc.name)); + } + + for (short_alias, _) in &sc.short_flag_aliases { + short_flags.push(Flag::App(format!("-{}", short_alias), &sc.name)); + } + + if let Some(l) = sc.long_flag.as_ref() { + long_flags.push(Flag::App(format!("--{}", l), &sc.name)); + } + + for (long_alias, _) in &sc.long_flag_aliases { + long_flags.push(Flag::App(format!("--{}", long_alias), &sc.name)); + } + } + + for arg in &app.args.args { + arg._debug_asserts(); + + if let Some(s) = arg.short.as_ref() { + short_flags.push(Flag::Arg(format!("-{}", s), &*arg.name)); + } + + for (short_alias, _) in &arg.short_aliases { + short_flags.push(Flag::Arg(format!("-{}", short_alias), &arg.name)); + } + + if let Some(l) = arg.long.as_ref() { + long_flags.push(Flag::Arg(format!("--{}", l), &*arg.name)); + } + + for (long_alias, _) in &arg.aliases { + long_flags.push(Flag::Arg(format!("--{}", long_alias), &arg.name)); + } + + // Name conflicts + assert!( + app.two_args_of(|x| x.id == arg.id).is_none(), + "Argument names must be unique, but '{}' is in use by more than one argument or group", + arg.name, + ); + + // Long conflicts + if let Some(l) = arg.long { + if let Some((first, second)) = app.two_args_of(|x| x.long == Some(l)) { + panic!( + "Long option names must be unique for each argument, \ + but '--{}' is in use by both '{}' and '{}'", + l, first.name, second.name + ) + } + } + + // Short conflicts + if let Some(s) = arg.short { + if let Some((first, second)) = app.two_args_of(|x| x.short == Some(s)) { + panic!( + "Short option names must be unique for each argument, \ + but '-{}' is in use by both '{}' and '{}'", + s, first.name, second.name + ) + } + } + + // Index conflicts + if let Some(idx) = arg.index { + if let Some((first, second)) = + app.two_args_of(|x| x.is_positional() && x.index == Some(idx)) + { + panic!( + "Argument '{}' has the same index as '{}' \ + and they are both positional arguments\n\n\t \ + Use Arg::multiple_values(true) to allow one \ + positional argument to take multiple values", + first.name, second.name + ) + } + } + + // requires, r_if, r_unless + for req in &arg.requires { + assert!( + app.id_exists(&req.1), + "Argument or group '{:?}' specified in 'requires*' for '{}' does not exist", + req.1, + arg.name, + ); + } + + for req in &arg.r_ifs { + assert!( + app.id_exists(&req.0), + "Argument or group '{:?}' specified in 'required_if*' for '{}' does not exist", + req.0, + arg.name + ); + } + + for req in &arg.r_unless { + assert!( + app.id_exists(req), + "Argument or group '{:?}' specified in 'required_unless*' for '{}' does not exist", + req, + arg.name, + ); + } + + // blacklist + for req in &arg.blacklist { + assert!( + app.id_exists(req), + "Argument or group '{:?}' specified in 'conflicts_with*' for '{}' does not exist", + req, + arg.name, + ); + } + + if arg.is_set(ArgSettings::Last) { + assert!( + arg.long.is_none(), + "Flags or Options cannot have last(true) set. '{}' has both a long and last(true) set.", + arg.name + ); + assert!( + arg.short.is_none(), + "Flags or Options cannot have last(true) set. '{}' has both a short and last(true) set.", + arg.name + ); + } + + assert!( + !(arg.is_set(ArgSettings::Required) && arg.global), + "Global arguments cannot be required.\n\n\t'{}' is marked as both global and required", + arg.name + ); + + // validators + assert!( + arg.validator.is_none() || arg.validator_os.is_none(), + "Argument '{}' has both `validator` and `validator_os` set which is not allowed", + arg.name + ); + + if arg.value_hint == ValueHint::CommandWithArguments { + assert!( + arg.short.is_none() && arg.long.is_none(), + "Argument '{}' has hint CommandWithArguments and must be positional.", + arg.name + ); + + assert!( + self.is_set(AppSettings::TrailingVarArg), + "Positional argument '{}' has hint CommandWithArguments, so App must have TrailingVarArg set.", + arg.name + ); + } + } + + for group in &app.groups { + // Name conflicts + assert!( + app.groups.iter().filter(|x| x.id == group.id).count() < 2, + "Argument group name must be unique\n\n\t'{}' is already in use", + group.name, + ); + + // Groups should not have naming conflicts with Args + assert!( + !app.args.args.iter().any(|x| x.id == group.id), + "Argument group name '{}' must not conflict with argument name", + group.name, + ); + + // Args listed inside groups should exist + for arg in &group.args { + assert!( + app.args.args.iter().any(|x| x.id == *arg), + "Argument group '{}' contains non-existent argument '{:?}'", + group.name, + arg + ) + } + } + + // Conflicts between flags and subcommands + + long_flags.sort_unstable(); + short_flags.sort_unstable(); + + detect_duplicate_flags(&long_flags, "long"); + detect_duplicate_flags(&short_flags, "short"); + + app._panic_on_missing_help(app.g_settings.is_set(AppSettings::HelpRequired)); +} + +fn detect_duplicate_flags(flags: &[Flag], short_or_long: &str) { + use Flag::*; + + for (one, two) in find_duplicates(flags) { + match (one, two) { + (App(flag, one), App(_, another)) if one != another => panic!( + "the '{}' {} flag is specified for both '{}' and '{}' subcommands", + flag, short_or_long, one, another + ), + + (Arg(flag, one), Arg(_, another)) if one != another => panic!( + "{} option names must be unique, but '{}' is in use by both '{}' and '{}'", + short_or_long, flag, one, another + ), + + (Arg(flag, arg), App(_, sub)) | (App(flag, sub), Arg(_, arg)) => panic!( + "the '{}' {} flag for the '{}' argument conflicts with the short flag \ + for '{}' subcommand", + flag, short_or_long, arg, sub + ), + + _ => {} + } + } +} + +/// Find duplicates in a sorted array. +/// +/// The algorithm is simple: the array is sorted, duplicates +/// must be placed next to each other, we can check only adjacent elements. +fn find_duplicates(slice: &[T]) -> impl Iterator { + slice.windows(2).filter_map(|w| { + if w[0] == w[1] { + Some((&w[0], &w[1])) + } else { + None + } + }) +} diff --git a/src/build/app/mod.rs b/src/build/app/mod.rs index 4522c9c13ca..6ba037fec42 100644 --- a/src/build/app/mod.rs +++ b/src/build/app/mod.rs @@ -1,3 +1,5 @@ +#[cfg(debug_assertions)] +mod debug_asserts; mod settings; #[cfg(test)] mod tests; @@ -1888,41 +1890,6 @@ impl<'help> App<'help> { } } -// Allows checking for conflicts between `Args` and `Apps` with subcommand flags -#[cfg(debug_assertions)] -#[derive(Debug)] -enum Flag<'r, 'help> { - App(&'r App<'help>, String), - Arg(&'r Arg<'help>, String), -} - -#[cfg(debug_assertions)] -impl Flag<'_, '_> { - pub fn value(&self) -> &str { - match self { - Self::App(_, value) => value, - Self::Arg(_, value) => value, - } - } - - pub fn id(&self) -> &Id { - match self { - Self::App(app, _) => &app.id, - Self::Arg(arg, _) => &arg.id, - } - } -} - -#[cfg(debug_assertions)] -impl fmt::Display for Flag<'_, '_> { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - match self { - Self::App(app, _) => write!(f, "App named '{}'", app.name), - Self::Arg(arg, _) => write!(f, "Arg named '{}'", arg.name), - } - } -} - // Internally used only impl<'help> App<'help> { fn _do_parse(&mut self, it: &mut Input) -> ClapResult { @@ -1997,7 +1964,7 @@ impl<'help> App<'help> { self.settings.set(AppSettings::Built); #[cfg(debug_assertions)] - self._debug_asserts(); + self::debug_asserts::assert_app(self); } fn _panic_on_missing_help(&self, help_required_globally: bool) { @@ -2025,69 +1992,7 @@ impl<'help> App<'help> { } #[cfg(debug_assertions)] - fn two_long_flags_of(&self, condition: F) -> Option<(Flag, Flag)> - where - F: Fn(&Flag) -> bool, - { - let mut flags: Vec = Vec::new(); - for sc in &self.subcommands { - if let Some(long) = sc.long_flag { - flags.push(Flag::App(&sc, long.to_string())); - } - flags.extend( - sc.get_all_long_flag_aliases() - .map(|alias| Flag::App(&sc, alias.to_string())), - ); - self.args.args.iter().for_each(|arg| { - flags.extend( - arg.aliases - .iter() - .map(|(alias, _)| Flag::Arg(arg, alias.to_string())), - ) - }); - flags.extend( - self.args - .args - .iter() - .filter_map(|arg| arg.long.map(|l| Flag::Arg(arg, l.to_string()))), - ); - } - two_elements_of(flags.into_iter().filter(|f| condition(f))) - } - - #[cfg(debug_assertions)] - fn two_short_flags_of(&self, condition: F) -> Option<(Flag, Flag)> - where - F: Fn(&Flag) -> bool, - { - let mut flags: Vec = Vec::new(); - for sc in &self.subcommands { - if let Some(short) = sc.short_flag { - flags.push(Flag::App(&sc, short.to_string())); - } - flags.extend( - sc.get_all_short_flag_aliases() - .map(|alias| Flag::App(&sc, alias.to_string())), - ); - self.args.args.iter().for_each(|arg| { - flags.extend( - arg.short_aliases - .iter() - .map(|(alias, _)| Flag::Arg(arg, alias.to_string())), - ) - }); - flags.extend( - self.args - .args - .iter() - .filter_map(|arg| arg.short.map(|l| Flag::Arg(arg, l.to_string()))), - ); - } - two_elements_of(flags.into_iter().filter(|f| condition(f))) - } - - #[cfg(debug_assertions)] - fn two_args_of(&self, condition: F) -> Option<(&Arg, &Arg)> + fn two_args_of(&self, condition: F) -> Option<(&Arg<'help>, &Arg<'help>)> where F: Fn(&Arg) -> bool, { @@ -2103,197 +2008,6 @@ impl<'help> App<'help> { two_elements_of(self.groups.iter().filter(|a| condition(a))) } - // Perform some expensive assertions on the Parser itself - #[allow(clippy::cognitive_complexity)] - #[cfg(debug_assertions)] - fn _debug_asserts(&self) { - debug!("App::_debug_asserts"); - - for sc in &self.subcommands { - // Conflicts between flag subcommands and long args - if let Some(l) = sc.long_flag { - if let Some((first, second)) = self.two_long_flags_of(|f| f.value() == l) { - // Prevent conflicts with itself - if first.id() != second.id() { - panic!( - "Long option names must be unique for each argument, \ - but '--{}' is used by both an {} and an {}", - l, first, second - ); - } - } - } - - // Conflicts between flag subcommands and long args - if let Some(s) = sc.short_flag { - if let Some((first, second)) = - self.two_short_flags_of(|f| f.value() == s.to_string()) - { - // Prevent conflicts with itself - if first.id() != second.id() { - panic!( - "Short option names must be unique for each argument, \ - but '-{}' is used by both an {} and an {}", - s, first, second - ); - } - } - } - } - - for arg in &self.args.args { - arg._debug_asserts(); - - // Name conflicts - assert!( - self.two_args_of(|x| x.id == arg.id).is_none(), - "Argument names must be unique, but '{}' is in use by more than one argument or group", - arg.name, - ); - - // Long conflicts - if let Some(l) = arg.long { - if let Some((first, second)) = self.two_args_of(|x| x.long == Some(l)) { - panic!( - "Long option names must be unique for each argument, \ - but '--{}' is in use by both '{}' and '{}'", - l, first.name, second.name - ) - } - } - - // Short conflicts - if let Some(s) = arg.short { - if let Some((first, second)) = self.two_args_of(|x| x.short == Some(s)) { - panic!( - "Short option names must be unique for each argument, \ - but '-{}' is in use by both '{}' and '{}'", - s, first.name, second.name - ) - } - } - - // Index conflicts - if let Some(idx) = arg.index { - if let Some((first, second)) = - self.two_args_of(|x| x.is_positional() && x.index == Some(idx)) - { - panic!( - "Argument '{}' has the same index as '{}' \ - and they are both positional arguments\n\n\t \ - Use Arg::multiple_values(true) to allow one \ - positional argument to take multiple values", - first.name, second.name - ) - } - } - - // requires, r_if, r_unless - for req in &arg.requires { - assert!( - self.id_exists(&req.1), - "Argument or group '{:?}' specified in 'requires*' for '{}' does not exist", - req.1, - arg.name, - ); - } - - for req in &arg.r_ifs { - assert!( - self.id_exists(&req.0), - "Argument or group '{:?}' specified in 'required_if*' for '{}' does not exist", - req.0, - arg.name - ); - } - - for req in &arg.r_unless { - assert!( - self.id_exists(req), - "Argument or group '{:?}' specified in 'required_unless*' for '{}' does not exist", - req, arg.name, - ); - } - - // blacklist - for req in &arg.blacklist { - assert!( - self.id_exists(req), - "Argument or group '{:?}' specified in 'conflicts_with*' for '{}' does not exist", - req, arg.name, - ); - } - - if arg.is_set(ArgSettings::Last) { - assert!( - arg.long.is_none(), - "Flags or Options cannot have last(true) set. '{}' has both a long and last(true) set.", - arg.name - ); - assert!( - arg.short.is_none(), - "Flags or Options cannot have last(true) set. '{}' has both a short and last(true) set.", - arg.name - ); - } - - assert!( - !(arg.is_set(ArgSettings::Required) && arg.global), - "Global arguments cannot be required.\n\n\t'{}' is marked as both global and required", - arg.name - ); - - // validators - assert!( - arg.validator.is_none() || arg.validator_os.is_none(), - "Argument '{}' has both `validator` and `validator_os` set which is not allowed", - arg.name - ); - - if arg.value_hint == ValueHint::CommandWithArguments { - assert!( - arg.short.is_none() && arg.long.is_none(), - "Argument '{}' has hint CommandWithArguments and must be positional.", - arg.name - ); - - assert!( - self.is_set(AppSettings::TrailingVarArg), - "Positional argument '{}' has hint CommandWithArguments, so App must have TrailingVarArg set.", - arg.name - ); - } - } - - for group in &self.groups { - // Name conflicts - assert!( - self.groups.iter().filter(|x| x.id == group.id).count() < 2, - "Argument group name must be unique\n\n\t'{}' is already in use", - group.name, - ); - - // Groups should not have naming conflicts with Args - assert!( - !self.args.args.iter().any(|x| x.id == group.id), - "Argument group name '{}' must not conflict with argument name", - group.name, - ); - - // Args listed inside groups should exist - for arg in &group.args { - assert!( - self.args.args.iter().any(|x| x.id == *arg), - "Argument group '{}' contains non-existent argument '{:?}'", - group.name, - arg - ) - } - } - - self._panic_on_missing_help(self.g_settings.is_set(AppSettings::HelpRequired)); - } - pub(crate) fn _propagate(&mut self, prop: Propagation) { macro_rules! propagate_subcmd { ($_self:expr, $sc:expr) => {{ @@ -2616,11 +2330,28 @@ impl<'help> App<'help> { /// Iterate through all the names of all subcommands (not recursively), including aliases. /// Used for suggestions. - pub(crate) fn all_subcommand_names(&self) -> impl Iterator { - self.get_subcommands().map(|s| s.get_name()).chain( - self.get_subcommands() - .flat_map(|s| s.aliases.iter().map(|&(n, _)| n)), - ) + pub(crate) fn all_subcommand_names<'a>(&'a self) -> impl Iterator + where + 'help: 'a, + { + let a: Vec<_> = self + .get_subcommands() + .flat_map(|sc| { + let name = sc.get_name(); + let aliases = sc.get_all_aliases(); + std::iter::once(name).chain(aliases) + }) + .collect(); + + // Strictly speaking, we don't need this trip through the Vec. + // We should have been able to return FlatMap iter above directly. + // + // Unfortunately, that would trigger + // https://github.com/rust-lang/rust/issues/34511#issuecomment-373423999 + // + // I think this "collect to vec" solution is better then the linked one + // because it's simpler and it doesn't really matter performance-wise. + a.into_iter() } pub(crate) fn unroll_args_in_group(&self, group: &Id) -> Vec { diff --git a/tests/flag_subcommands.rs b/tests/flag_subcommands.rs index 40c2f49b80c..c6a602d278f 100644 --- a/tests/flag_subcommands.rs +++ b/tests/flag_subcommands.rs @@ -281,7 +281,7 @@ fn flag_subcommand_multiple() { #[cfg(debug_assertions)] #[test] -#[should_panic = "Short option names must be unique for each argument, but \'-f\' is used by both an App named \'some\' and an Arg named \'test\'"] +#[should_panic = "the \'-f\' short flag for the \'test\' argument conflicts with the short flag for \'some\' subcommand"] fn flag_subcommand_short_conflict_with_arg() { let _ = App::new("test") .subcommand(App::new("some").short_flag('f').long_flag("some")) @@ -291,7 +291,7 @@ fn flag_subcommand_short_conflict_with_arg() { #[cfg(debug_assertions)] #[test] -#[should_panic = "Short option names must be unique for each argument, but \'-f\' is used by both an App named \'some\' and an App named \'result\'"] +#[should_panic = "the \'-f\' short flag is specified for both \'some\' and \'result\' subcommands"] fn flag_subcommand_short_conflict_with_alias() { let _ = App::new("test") .subcommand(App::new("some").short_flag('f').long_flag("some")) @@ -301,7 +301,7 @@ fn flag_subcommand_short_conflict_with_alias() { #[cfg(debug_assertions)] #[test] -#[should_panic = "Long option names must be unique for each argument, but \'--flag\' is used by both an App named \'some\' and an App named \'result\'"] +#[should_panic = "the \'--flag\' long flag is specified for both \'some\' and \'result\' subcommands"] fn flag_subcommand_long_conflict_with_alias() { let _ = App::new("test") .subcommand(App::new("some").long_flag("flag")) @@ -311,7 +311,7 @@ fn flag_subcommand_long_conflict_with_alias() { #[cfg(debug_assertions)] #[test] -#[should_panic = "Short option names must be unique for each argument, but \'-f\' is used by both an App named \'some\' and an Arg named \'test\'"] +#[should_panic = "the \'-f\' short flag for the \'test\' argument conflicts with the short flag for \'some\' subcommand"] fn flag_subcommand_short_conflict_with_arg_alias() { let _ = App::new("test") .subcommand(App::new("some").short_flag('f').long_flag("some")) @@ -321,7 +321,7 @@ fn flag_subcommand_short_conflict_with_arg_alias() { #[cfg(debug_assertions)] #[test] -#[should_panic = "Long option names must be unique for each argument, but \'--some\' is used by both an App named \'some\' and an Arg named \'test\'"] +#[should_panic = "the \'--some\' long flag for the \'test\' argument conflicts with the short flag for \'some\' subcommand"] fn flag_subcommand_long_conflict_with_arg_alias() { let _ = App::new("test") .subcommand(App::new("some").short_flag('f').long_flag("some")) @@ -331,7 +331,7 @@ fn flag_subcommand_long_conflict_with_arg_alias() { #[cfg(debug_assertions)] #[test] -#[should_panic = "Long option names must be unique for each argument, but \'--flag\' is used by both an App named \'some\' and an Arg named \'flag\'"] +#[should_panic = "the \'--flag\' long flag for the \'flag\' argument conflicts with the short flag for \'some\' subcommand"] fn flag_subcommand_long_conflict_with_arg() { let _ = App::new("test") .subcommand(App::new("some").short_flag('a').long_flag("flag")) From 6169bb8e7977fda52b600b9e978d3329e20dbb16 Mon Sep 17 00:00:00 2001 From: CreepySkeleton Date: Sun, 2 Aug 2020 19:52:12 +0300 Subject: [PATCH 5/6] Fix buggy tests --- tests/arg_aliases.rs | 2 +- tests/arg_aliases_short.rs | 2 +- tests/fixtures/app.yaml | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/arg_aliases.rs b/tests/arg_aliases.rs index ef68252850b..46e179ba45f 100644 --- a/tests/arg_aliases.rs +++ b/tests/arg_aliases.rs @@ -167,7 +167,7 @@ fn invisible_arg_aliases_help_output() { .takes_value(true) .aliases(&["invisible", "als1", "more"]), ) - .arg(Arg::from("-f, --flag").aliases(&["invisible", "flg1", "anyway"])), + .arg(Arg::from("-f, --flag").aliases(&["unseeable", "flg1", "anyway"])), ); assert!(utils::compare_output( app, diff --git a/tests/arg_aliases_short.rs b/tests/arg_aliases_short.rs index 65d0f5649bb..865dc8bcdfc 100644 --- a/tests/arg_aliases_short.rs +++ b/tests/arg_aliases_short.rs @@ -163,7 +163,7 @@ fn invisible_short_arg_aliases_help_output() { .takes_value(true) .short_aliases(&['a', 'b', 'c']), ) - .arg(Arg::from("-f, --flag").short_aliases(&['a', 'b', 'c'])), + .arg(Arg::from("-f, --flag").short_aliases(&['x', 'y', 'z'])), ); assert!(utils::compare_output( app, diff --git a/tests/fixtures/app.yaml b/tests/fixtures/app.yaml index feef7c99c40..86b59552783 100644 --- a/tests/fixtures/app.yaml +++ b/tests/fixtures/app.yaml @@ -92,7 +92,7 @@ args: - multshortaliases: long: multshortaliases about: Tests multiple short aliases - short_aliases: [a, b, c] + short_aliases: [b, c] - minvals2: long: minvals2 multiple: true From 39fadbf7c8d19f7fea2b3c51d785fccba829c704 Mon Sep 17 00:00:00 2001 From: CreepySkeleton Date: Wed, 12 Aug 2020 16:40:19 +0300 Subject: [PATCH 6/6] Rebase --- src/build/app/debug_asserts.rs | 4 ++-- src/build/app/mod.rs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/build/app/debug_asserts.rs b/src/build/app/debug_asserts.rs index ee68ea3fd02..c4e52ca51a4 100644 --- a/src/build/app/debug_asserts.rs +++ b/src/build/app/debug_asserts.rs @@ -1,4 +1,4 @@ -use crate::{App, AppSettings, ArgSettings}; +use crate::{App, AppSettings, ArgSettings, ValueHint}; use std::cmp::Ordering; #[derive(Eq)] @@ -197,7 +197,7 @@ pub(crate) fn assert_app(app: &App) { ); assert!( - self.is_set(AppSettings::TrailingVarArg), + app.is_set(AppSettings::TrailingVarArg), "Positional argument '{}' has hint CommandWithArguments, so App must have TrailingVarArg set.", arg.name ); diff --git a/src/build/app/mod.rs b/src/build/app/mod.rs index 6ba037fec42..cd408fee8dd 100644 --- a/src/build/app/mod.rs +++ b/src/build/app/mod.rs @@ -28,7 +28,7 @@ use crate::{ output::{fmt::Colorizer, Help, HelpWriter, Usage}, parse::{ArgMatcher, ArgMatches, Input, Parser}, util::{safe_exit, termcolor::ColorChoice, ArgStr, Id, Key}, - Result as ClapResult, ValueHint, INTERNAL_ERROR_MSG, + Result as ClapResult, INTERNAL_ERROR_MSG, }; // FIXME (@CreepySkeleton): some of these variants are never constructed