From 868320097ee0bc708070eedf5efbaaf6e6bff251 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 26 May 2022 09:36:42 -0500 Subject: [PATCH 01/12] refactor(parser): Consolidate help/version handling This is a step towards user-visible actions --- src/builder/action.rs | 8 +++ src/builder/arg.rs | 8 +++ src/builder/command.rs | 22 ++++++ src/builder/mod.rs | 2 + src/parser/parser.rs | 156 +++++++++++++---------------------------- 5 files changed, 89 insertions(+), 107 deletions(-) create mode 100644 src/builder/action.rs diff --git a/src/builder/action.rs b/src/builder/action.rs new file mode 100644 index 00000000000..71da9ccc27e --- /dev/null +++ b/src/builder/action.rs @@ -0,0 +1,8 @@ +/// Behavior of arguments when they are encountered while parsing +#[derive(Clone, Debug, PartialEq, Eq)] +pub(crate) enum Action { + StoreValue, + Flag, + Help, + Version, +} diff --git a/src/builder/arg.rs b/src/builder/arg.rs index 49cb8a96819..46676adbd94 100644 --- a/src/builder/arg.rs +++ b/src/builder/arg.rs @@ -18,6 +18,7 @@ use yaml_rust::Yaml; // Internal use crate::builder::usage_parser::UsageParser; +use crate::builder::Action; use crate::builder::ArgPredicate; use crate::util::{Id, Key}; use crate::PossibleValue; @@ -63,6 +64,7 @@ pub struct Arg<'help> { pub(crate) name: &'help str, pub(crate) help: Option<&'help str>, pub(crate) long_help: Option<&'help str>, + pub(crate) action: Option, pub(crate) value_parser: Option, pub(crate) blacklist: Vec, pub(crate) settings: ArgFlags, @@ -4528,6 +4530,12 @@ impl<'help> Arg<'help> { self.is_set(ArgSettings::AllowInvalidUtf8) } + /// Behavior when parsing the argument + pub(crate) fn get_action(&self) -> &super::Action { + const DEFAULT: super::Action = super::Action::StoreValue; + self.action.as_ref().unwrap_or(&DEFAULT) + } + /// Configured parser for argument values /// /// # Example diff --git a/src/builder/command.rs b/src/builder/command.rs index b54012bd3dc..0a7ce146bc4 100644 --- a/src/builder/command.rs +++ b/src/builder/command.rs @@ -4115,6 +4115,10 @@ impl<'help> App<'help> { let mut pos_counter = 1; let self_override = self.is_set(AppSettings::AllArgsOverrideSelf); let hide_pv = self.is_set(AppSettings::HidePossibleValues); + let auto_help = + !self.is_set(AppSettings::NoAutoHelp) && !self.is_disable_help_flag_set(); + let auto_version = + !self.is_set(AppSettings::NoAutoVersion) && !self.is_disable_version_flag_set(); for a in self.args.args_mut() { // Fill in the groups for g in &a.groups { @@ -4141,6 +4145,24 @@ impl<'help> App<'help> { a.overrides.push(self_id); } a._build(); + // HACK: Setting up action at this level while auto-help / disable help flag is + // required. Otherwise, most of this won't be needed because when we can break + // compat, actions will reign supreme (default to `Store`) + if a.action.is_none() { + if a.get_id() == "help" && auto_help { + let action = super::Action::Help; + a.action = Some(action); + } else if a.get_id() == "version" && auto_version { + let action = super::Action::Version; + a.action = Some(action); + } else if a.is_takes_value_set() { + let action = super::Action::StoreValue; + a.action = Some(action); + } else { + let action = super::Action::Flag; + a.action = Some(action); + } + } if a.is_positional() && a.index.is_none() { a.index = Some(pos_counter); pos_counter += 1; diff --git a/src/builder/mod.rs b/src/builder/mod.rs index 9b009ea31fe..10e8bdff980 100644 --- a/src/builder/mod.rs +++ b/src/builder/mod.rs @@ -3,6 +3,7 @@ #[macro_use] mod macros; +mod action; mod app_settings; mod arg; mod arg_group; @@ -53,6 +54,7 @@ pub use command::App; #[cfg(feature = "regex")] pub use self::regex::RegexRef; +pub(crate) use action::Action; pub(crate) use arg::display_arg_val; pub(crate) use arg_predicate::ArgPredicate; pub(crate) use value_parser::ValueParserInner; diff --git a/src/parser/parser.rs b/src/parser/parser.rs index 14e4c223e7f..a2750a98273 100644 --- a/src/parser/parser.rs +++ b/src/parser/parser.rs @@ -8,6 +8,7 @@ use std::{ use clap_lex::RawOsStr; // Internal +use crate::builder::Action; use crate::builder::AppSettings as AS; use crate::builder::{Arg, Command}; use crate::error::Error as ClapError; @@ -253,12 +254,6 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { Usage::new(self.cmd).create_usage_no_title(&used), )) } - ParseResult::HelpFlag => { - return Err(self.help_err(true, Stream::Stdout)); - } - ParseResult::VersionFlag => { - return Err(self.version_err(true)); - } ParseResult::MaybeHyphenValue => { // Maybe a hyphen value, do nothing. } @@ -336,12 +331,6 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { Usage::new(self.cmd).create_usage_with_title(&[]), )); } - ParseResult::HelpFlag => { - return Err(self.help_err(false, Stream::Stdout)); - } - ParseResult::VersionFlag => { - return Err(self.version_err(false)); - } ParseResult::MaybeHyphenValue => { // Maybe a hyphen value, do nothing. } @@ -695,74 +684,6 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { Ok(()) } - // Retrieves the names of all args the user has supplied thus far, except required ones - // because those will be listed in self.required - fn check_for_help_and_version_str(&self, arg: &RawOsStr) -> Option { - debug!("Parser::check_for_help_and_version_str"); - debug!( - "Parser::check_for_help_and_version_str: Checking if --{:?} is help or version...", - arg - ); - - if let Some(help) = self.cmd.find(&Id::help_hash()) { - if let Some(h) = help.long { - if arg == h && !self.is_set(AS::NoAutoHelp) && !self.cmd.is_disable_help_flag_set() - { - debug!("Help"); - return Some(ParseResult::HelpFlag); - } - } - } - - if let Some(version) = self.cmd.find(&Id::version_hash()) { - if let Some(v) = version.long { - if arg == v - && !self.is_set(AS::NoAutoVersion) - && !self.cmd.is_disable_version_flag_set() - { - debug!("Version"); - return Some(ParseResult::VersionFlag); - } - } - } - - debug!("Neither"); - None - } - - fn check_for_help_and_version_char(&self, arg: char) -> Option { - debug!("Parser::check_for_help_and_version_char"); - debug!( - "Parser::check_for_help_and_version_char: Checking if -{} is help or version...", - arg - ); - - if let Some(help) = self.cmd.find(&Id::help_hash()) { - if let Some(h) = help.short { - if arg == h && !self.is_set(AS::NoAutoHelp) && !self.cmd.is_disable_help_flag_set() - { - debug!("Help"); - return Some(ParseResult::HelpFlag); - } - } - } - - if let Some(version) = self.cmd.find(&Id::version_hash()) { - if let Some(v) = version.short { - if arg == v - && !self.is_set(AS::NoAutoVersion) - && !self.cmd.is_disable_version_flag_set() - { - debug!("Version"); - return Some(ParseResult::VersionFlag); - } - } - } - - debug!("Neither"); - None - } - fn parse_long_arg( &mut self, matcher: &mut ArgMatcher, @@ -843,13 +764,9 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { used, arg: opt.to_string(), }) - } else if let Some(parse_result) = - self.check_for_help_and_version_str(RawOsStr::from_str(long_arg)) - { - Ok(parse_result) } else { debug!("Parser::parse_long_arg: Presence validated"); - Ok(self.parse_flag(opt, matcher)) + self.parse_flag(Identifier::Long(long_arg), opt, matcher) } } else if let Some(sc_name) = self.possible_long_flag_subcommand(long_arg) { Ok(ParseResult::FlagSubCommand(sc_name.to_string())) @@ -946,10 +863,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { ); *valid_arg_found = true; if !opt.is_takes_value_set() { - if let Some(parse_result) = self.check_for_help_and_version_char(c) { - return Ok(parse_result); - } - ret = self.parse_flag(opt, matcher); + ret = self.parse_flag(Identifier::Short(c), opt, matcher)?; continue; } @@ -1161,13 +1075,38 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { matcher.has_val_groups(&arg.id) } - fn parse_flag(&self, flag: &Arg<'help>, matcher: &mut ArgMatcher) -> ParseResult { + fn parse_flag( + &self, + ident: Identifier, + arg: &Arg<'help>, + matcher: &mut ArgMatcher, + ) -> ClapResult { debug!("Parser::parse_flag"); + match arg.get_action() { + Action::StoreValue => unreachable!("{:?} is not a flag", arg.get_id()), + Action::Flag => { + self.start_occurrence_of_arg(matcher, arg); + matcher.add_index_to(&arg.id, self.cur_idx.get()); - self.start_occurrence_of_arg(matcher, flag); - matcher.add_index_to(&flag.id, self.cur_idx.get()); - - ParseResult::ValuesDone + Ok(ParseResult::ValuesDone) + } + Action::Help => { + let use_long = match ident { + Identifier::Long(_) => true, + Identifier::Short(_) => false, + }; + debug!("Help: use_long={}", use_long); + Err(self.help_err(use_long, Stream::Stdout)) + } + Action::Version => { + let use_long = match ident { + Identifier::Long(_) => true, + Identifier::Short(_) => false, + }; + debug!("Version: use_long={}", use_long); + Err(self.version_err(use_long)) + } + } } fn remove_overrides(&self, arg: &Arg<'help>, matcher: &mut ArgMatcher) { @@ -1220,15 +1159,9 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { self.start_custom_arg(matcher, arg, ValueSource::EnvVariable); self.add_val_to_arg(arg, &val, matcher, false, trailing_values)?; } else { - // Early return on `HelpFlag` or `VersionFlag`. - match self.check_for_help_and_version_str(&val) { - Some(ParseResult::HelpFlag) => { - return Err(self.help_err(true, Stream::Stdout)); - } - Some(ParseResult::VersionFlag) => { - return Err(self.version_err(true)); - } - _ => { + match arg.get_action() { + Action::StoreValue => unreachable!("{:?} is not a flag", arg.get_id()), + Action::Flag => { debug!("Parser::add_env: Found a flag with value `{:?}`", val); let predicate = str_to_bool(val.to_str_lossy()); debug!("Parser::add_env: Found boolean literal `{:?}`", predicate); @@ -1237,6 +1170,13 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { matcher.add_index_to(&arg.id, self.cur_idx.get()); } } + // Early return on `Help` or `Version`. + Action::Help => { + return Err(self.help_err(true, Stream::Stdout)); + } + Action::Version => { + return Err(self.version_err(true)); + } } } } @@ -1518,8 +1458,10 @@ enum ParseResult { }, /// No argument found e.g. parser is given `-` when parsing a flag. NoArg, - /// This is a Help flag. - HelpFlag, - /// This is a version flag. - VersionFlag, +} + +#[derive(Copy, Clone, Debug, PartialEq, Eq)] +enum Identifier<'f> { + Short(char), + Long(&'f str), } From 28cb71cdddb1beb50d70a78bf0060403b378c152 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 26 May 2022 16:29:14 -0500 Subject: [PATCH 02/12] fix(parser): Pass the intended flag to the action Inferred flags can make it hard for a future action to trigger behavior off of the selected alias, like we might want to do for negations, so we are now translating to the intended arg. This will also help for debugging. --- src/parser/parser.rs | 29 ++++++++++++++++++----------- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/src/parser/parser.rs b/src/parser/parser.rs index a2750a98273..0116074c9af 100644 --- a/src/parser/parser.rs +++ b/src/parser/parser.rs @@ -725,30 +725,37 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { "Parser::parse_long_arg: Found valid opt or flag '{}'", opt.to_string() ); - Some(opt) + Some((long_arg, opt)) } else if self.cmd.is_infer_long_args_set() { - self.cmd.get_arguments().find(|a| { - a.long.map_or(false, |long| long.starts_with(long_arg)) - || a.aliases - .iter() - .any(|(alias, _)| alias.starts_with(long_arg)) + self.cmd.get_arguments().find_map(|a| { + if let Some(long) = a.long { + if long.starts_with(long_arg) { + return Some((long, a)); + } + } + a.aliases + .iter() + .find_map(|(alias, _)| alias.starts_with(long_arg).then(|| (*alias, a))) }) } else { None }; - if let Some(opt) = opt { + if let Some((long_arg, opt)) = opt { *valid_arg_found = true; if opt.is_takes_value_set() { debug!( - "Parser::parse_long_arg: Found an opt with value '{:?}'", - &long_value + "Parser::parse_long_arg({:?}): Found an opt with value '{:?}'", + long_arg, &long_value ); let has_eq = long_value.is_some(); self.parse_opt(long_value, opt, matcher, trailing_values, has_eq) } else if let Some(rest) = long_value { let required = self.cmd.required_graph(); - debug!("Parser::parse_long_arg: Got invalid literal `{:?}`", rest); + debug!( + "Parser::parse_long_arg({:?}): Got invalid literal `{:?}`", + long_arg, rest + ); let used: Vec = matcher .arg_names() .filter(|&n| { @@ -765,7 +772,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { arg: opt.to_string(), }) } else { - debug!("Parser::parse_long_arg: Presence validated"); + debug!("Parser::parse_long_arg({:?}): Presence validated", long_arg); self.parse_flag(Identifier::Long(long_arg), opt, matcher) } } else if let Some(sc_name) = self.possible_long_flag_subcommand(long_arg) { From 9805fdad1b17fde2f5860969de84d7eb4d8f2b51 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 26 May 2022 19:14:56 -0500 Subject: [PATCH 03/12] refactor(parser)!: Consolidate group/occurrence logic We were independently starting occurrences and starting value groups. Now we do them at the same time. COMPATIBILITY: This changes us from counting occurrences per positional when using `multiple_values` to one occurrence. This is user visible and tests were written against it but it goes against the documentation and doesn't quite make sense. --- src/parser/arg_matcher.rs | 38 ++++----------------- src/parser/matches/matched_arg.rs | 48 --------------------------- src/parser/parser.rs | 52 +++++++---------------------- tests/builder/multiple_values.rs | 55 +++++++++++++++++++++++++------ 4 files changed, 63 insertions(+), 130 deletions(-) diff --git a/src/parser/arg_matcher.rs b/src/parser/arg_matcher.rs index bb26bc3f261..e162a16f14f 100644 --- a/src/parser/arg_matcher.rs +++ b/src/parser/arg_matcher.rs @@ -11,9 +11,6 @@ use crate::parser::{ArgMatches, MatchedArg, SubCommand, ValueSource}; use crate::util::Id; use crate::INTERNAL_ERROR_MSG; -// Third party -use indexmap::map::Entry; - #[derive(Debug, Default)] pub(crate) struct ArgMatcher(ArgMatches); @@ -141,6 +138,7 @@ impl ArgMatcher { let ma = self.entry(id).or_insert(MatchedArg::new_arg(arg)); debug_assert_eq!(ma.type_id(), Some(arg.get_value_parser().type_id())); ma.set_source(source); + ma.new_val_group(); } pub(crate) fn start_custom_group(&mut self, id: &Id, source: ValueSource) { @@ -151,6 +149,7 @@ impl ArgMatcher { let ma = self.entry(id).or_insert(MatchedArg::new_group()); debug_assert_eq!(ma.type_id(), None); ma.set_source(source); + ma.new_val_group(); } pub(crate) fn start_occurrence_of_arg(&mut self, arg: &Arg) { @@ -160,6 +159,7 @@ impl ArgMatcher { debug_assert_eq!(ma.type_id(), Some(arg.get_value_parser().type_id())); ma.set_source(ValueSource::CommandLine); ma.inc_occurrences(); + ma.new_val_group(); } pub(crate) fn start_occurrence_of_group(&mut self, id: &Id) { @@ -168,6 +168,7 @@ impl ArgMatcher { debug_assert_eq!(ma.type_id(), None); ma.set_source(ValueSource::CommandLine); ma.inc_occurrences(); + ma.new_val_group(); } pub(crate) fn start_occurrence_of_external(&mut self, cmd: &crate::Command) { @@ -184,46 +185,19 @@ impl ArgMatcher { ); ma.set_source(ValueSource::CommandLine); ma.inc_occurrences(); + ma.new_val_group(); } - pub(crate) fn add_val_to(&mut self, arg: &Id, val: AnyValue, raw_val: OsString, append: bool) { - if append { - self.append_val_to(arg, val, raw_val); - } else { - self.push_val_to(arg, val, raw_val); - } - } - - fn push_val_to(&mut self, arg: &Id, val: AnyValue, raw_val: OsString) { - // We will manually inc occurrences later(for flexibility under - // specific circumstances, like only add one occurrence for flag - // when we met: `--flag=one,two`). - let ma = self.get_mut(arg).expect(INTERNAL_ERROR_MSG); - ma.push_val(val, raw_val); - } - - fn append_val_to(&mut self, arg: &Id, val: AnyValue, raw_val: OsString) { + pub(crate) fn add_val_to(&mut self, arg: &Id, val: AnyValue, raw_val: OsString) { let ma = self.get_mut(arg).expect(INTERNAL_ERROR_MSG); ma.append_val(val, raw_val); } - pub(crate) fn new_val_group(&mut self, arg: &Id) { - let ma = self.get_mut(arg).expect(INTERNAL_ERROR_MSG); - ma.new_val_group(); - } - pub(crate) fn add_index_to(&mut self, arg: &Id, idx: usize) { let ma = self.get_mut(arg).expect(INTERNAL_ERROR_MSG); ma.push_index(idx); } - pub(crate) fn has_val_groups(&mut self, arg: &Id) -> bool { - match self.entry(arg) { - Entry::Occupied(e) => e.get().has_val_groups(), - Entry::Vacant(_) => false, - } - } - pub(crate) fn needs_more_vals(&self, o: &Arg) -> bool { debug!("ArgMatcher::needs_more_vals: o={}", o.name); if let Some(ma) = self.get(&o.id) { diff --git a/src/parser/matches/matched_arg.rs b/src/parser/matches/matched_arg.rs index fa9f7664cac..b2c527eaab0 100644 --- a/src/parser/matches/matched_arg.rs +++ b/src/parser/matches/matched_arg.rs @@ -87,11 +87,6 @@ impl MatchedArg { self.indices.push(index) } - #[cfg(test)] - pub(crate) fn raw_vals(&self) -> Iter> { - self.raw_vals.iter() - } - #[cfg(feature = "unstable-grouped")] pub(crate) fn vals(&self) -> Iter> { self.vals.iter() @@ -118,11 +113,6 @@ impl MatchedArg { self.raw_vals_flatten().next() } - pub(crate) fn push_val(&mut self, val: AnyValue, raw_val: OsString) { - self.vals.push(vec![val]); - self.raw_vals.push(vec![raw_val]); - } - pub(crate) fn new_val_group(&mut self) { self.vals.push(vec![]); self.raw_vals.push(vec![]); @@ -151,10 +141,6 @@ impl MatchedArg { self.vals.iter().flatten().count() == 0 } - pub(crate) fn has_val_groups(&self) -> bool { - !self.vals.is_empty() - } - pub(crate) fn check_explicit(&self, predicate: ArgPredicate) -> bool { if self.source == Some(ValueSource::DefaultValue) { return false; @@ -244,38 +230,4 @@ mod tests { m.append_val(AnyValue::new(String::from("ccc")), "ccc".into()); assert_eq!(m.first_raw(), Some(&OsString::from("bbb"))); } - - #[test] - fn test_grouped_vals_push_and_append() { - let mut m = MatchedArg::new_group(); - m.push_val(AnyValue::new(String::from("aaa")), "aaa".into()); - m.new_val_group(); - m.append_val(AnyValue::new(String::from("bbb")), "bbb".into()); - m.append_val(AnyValue::new(String::from("ccc")), "ccc".into()); - m.new_val_group(); - m.append_val(AnyValue::new(String::from("ddd")), "ddd".into()); - m.push_val(AnyValue::new(String::from("eee")), "eee".into()); - m.new_val_group(); - m.append_val(AnyValue::new(String::from("fff")), "fff".into()); - m.append_val(AnyValue::new(String::from("ggg")), "ggg".into()); - m.append_val(AnyValue::new(String::from("hhh")), "hhh".into()); - m.append_val(AnyValue::new(String::from("iii")), "iii".into()); - - let vals: Vec<&Vec> = m.raw_vals().collect(); - assert_eq!( - vals, - vec![ - &vec![OsString::from("aaa")], - &vec![OsString::from("bbb"), OsString::from("ccc"),], - &vec![OsString::from("ddd")], - &vec![OsString::from("eee")], - &vec![ - OsString::from("fff"), - OsString::from("ggg"), - OsString::from("hhh"), - OsString::from("iii"), - ] - ] - ) - } } diff --git a/src/parser/parser.rs b/src/parser/parser.rs index 0116074c9af..dc4dac3e19e 100644 --- a/src/parser/parser.rs +++ b/src/parser/parser.rs @@ -347,7 +347,6 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { &self.cmd[id], arg_os.to_value_os(), matcher, - true, trailing_values, )?; parse_state = match parse_result { @@ -374,15 +373,10 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { trailing_values = true; } - // Increase occurrence no matter if we are appending, occurrences - // of positional argument equals to number of values rather than - // the number of value groups. - self.start_occurrence_of_arg(matcher, p); - // Creating new value group rather than appending when the arg - // doesn't have any value. This behaviour is right because - // positional arguments are always present continuously. - let append = self.has_val_groups(matcher, p); - self.add_val_to_arg(p, arg_os.to_value_os(), matcher, append, trailing_values)?; + if !p.is_multiple_values_set() || !matcher.contains(&p.id) { + self.start_occurrence_of_arg(matcher, p); + } + self.add_val_to_arg(p, arg_os.to_value_os(), matcher, trailing_values)?; // Only increment the positional counter if it doesn't allow multiples if !p.is_multiple() { @@ -415,7 +409,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { for raw_val in raw_args.remaining(&mut args_cursor) { let val = external_parser.parse_ref(self.cmd, None, raw_val)?; let external_id = &Id::empty_hash(); - sc_m.add_val_to(external_id, val, raw_val.to_os_string(), false); + sc_m.add_val_to(external_id, val, raw_val.to_os_string()); } matcher.subcommand(SubCommand { @@ -951,7 +945,6 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { opt, opt.default_missing_vals.iter().map(OsString::from), matcher, - false, )?; }; if attached_value.is_some() { @@ -967,15 +960,11 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { } } else if let Some(v) = attached_value { self.start_occurrence_of_arg(matcher, opt); - self.add_val_to_arg(opt, v, matcher, false, trailing_values)?; + self.add_val_to_arg(opt, v, matcher, trailing_values)?; Ok(ParseResult::ValuesDone) } else { debug!("Parser::parse_opt: More arg vals required..."); self.start_occurrence_of_arg(matcher, opt); - matcher.new_val_group(&opt.id); - for group in self.cmd.groups_for_arg(&opt.id) { - matcher.new_val_group(&group); - } Ok(ParseResult::Opt(opt.id.clone())) } } @@ -985,7 +974,6 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { arg: &Arg<'help>, val: &RawOsStr, matcher: &mut ArgMatcher, - append: bool, trailing_values: bool, ) -> ClapResult { debug!("Parser::add_val_to_arg; arg={}, val={:?}", arg.name, val); @@ -1001,7 +989,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { .split(delim) .map(|x| x.to_os_str().into_owned()) .take_while(|val| Some(val.as_os_str()) != terminator); - self.add_multiple_vals_to_arg(arg, vals, matcher, append)?; + self.add_multiple_vals_to_arg(arg, vals, matcher)?; // If there was a delimiter used or we must use the delimiter to // separate the values or no more vals is needed, we're not // looking for more values. @@ -1020,7 +1008,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { return Ok(ParseResult::ValuesDone); } } - self.add_single_val_to_arg(arg, val.to_os_str().into_owned(), matcher, append)?; + self.add_single_val_to_arg(arg, val.to_os_str().into_owned(), matcher)?; if matcher.needs_more_vals(arg) { Ok(ParseResult::Opt(arg.id.clone())) } else { @@ -1033,17 +1021,9 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { arg: &Arg<'help>, raw_vals: impl Iterator, matcher: &mut ArgMatcher, - append: bool, ) -> ClapResult<()> { - // If not appending, create a new val group and then append vals in. - if !append { - matcher.new_val_group(&arg.id); - for group in self.cmd.groups_for_arg(&arg.id) { - matcher.new_val_group(&group); - } - } for raw_val in raw_vals { - self.add_single_val_to_arg(arg, raw_val, matcher, true)?; + self.add_single_val_to_arg(arg, raw_val, matcher)?; } Ok(()) @@ -1054,7 +1034,6 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { arg: &Arg<'help>, raw_val: OsString, matcher: &mut ArgMatcher, - append: bool, ) -> ClapResult<()> { debug!("Parser::add_single_val_to_arg: adding val...{:?}", raw_val); @@ -1069,19 +1048,15 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { // Increment or create the group "args" for group in self.cmd.groups_for_arg(&arg.id) { - matcher.add_val_to(&group, val.clone(), raw_val.clone(), append); + matcher.add_val_to(&group, val.clone(), raw_val.clone()); } - matcher.add_val_to(&arg.id, val, raw_val, append); + matcher.add_val_to(&arg.id, val, raw_val); matcher.add_index_to(&arg.id, self.cur_idx.get()); Ok(()) } - fn has_val_groups(&self, matcher: &mut ArgMatcher, arg: &Arg<'help>) -> bool { - matcher.has_val_groups(&arg.id) - } - fn parse_flag( &self, ident: Identifier, @@ -1164,7 +1139,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { val, trailing_values ); self.start_custom_arg(matcher, arg, ValueSource::EnvVariable); - self.add_val_to_arg(arg, &val, matcher, false, trailing_values)?; + self.add_val_to_arg(arg, &val, matcher, trailing_values)?; } else { match arg.get_action() { Action::StoreValue => unreachable!("{:?} is not a flag", arg.get_id()), @@ -1236,7 +1211,6 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { arg, &RawOsStr::new(default), matcher, - false, trailing_values, )?; } @@ -1279,7 +1253,6 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { arg, process_default_vals(arg, &arg.default_vals).into_iter(), matcher, - false, )?; } } else { @@ -1307,7 +1280,6 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { arg, process_default_vals(arg, &arg.default_missing_vals).into_iter(), matcher, - false, )?; } None => { diff --git a/tests/builder/multiple_values.rs b/tests/builder/multiple_values.rs index 2029a4df356..c775e359dd6 100644 --- a/tests/builder/multiple_values.rs +++ b/tests/builder/multiple_values.rs @@ -385,7 +385,7 @@ fn positional() { let m = m.unwrap(); assert!(m.is_present("pos")); - assert_eq!(m.occurrences_of("pos"), 3); + assert_eq!(m.occurrences_of("pos"), 1); assert_eq!( m.get_many::("pos") .unwrap() @@ -409,7 +409,7 @@ fn positional_exact_exact() { let m = m.unwrap(); assert!(m.is_present("pos")); - assert_eq!(m.occurrences_of("pos"), 3); + assert_eq!(m.occurrences_of("pos"), 1); assert_eq!( m.get_many::("pos") .unwrap() @@ -457,7 +457,7 @@ fn positional_min_exact() { let m = m.unwrap(); assert!(m.is_present("pos")); - assert_eq!(m.occurrences_of("pos"), 3); + assert_eq!(m.occurrences_of("pos"), 1); assert_eq!( m.get_many::("pos") .unwrap() @@ -487,7 +487,7 @@ fn positional_min_more() { let m = m.unwrap(); assert!(m.is_present("pos")); - assert_eq!(m.occurrences_of("pos"), 4); + assert_eq!(m.occurrences_of("pos"), 1); assert_eq!( m.get_many::("pos") .unwrap() @@ -507,7 +507,7 @@ fn positional_max_exact() { let m = m.unwrap(); assert!(m.is_present("pos")); - assert_eq!(m.occurrences_of("pos"), 3); + assert_eq!(m.occurrences_of("pos"), 1); assert_eq!( m.get_many::("pos") .unwrap() @@ -527,7 +527,7 @@ fn positional_max_less() { let m = m.unwrap(); assert!(m.is_present("pos")); - assert_eq!(m.occurrences_of("pos"), 2); + assert_eq!(m.occurrences_of("pos"), 1); assert_eq!( m.get_many::("pos") .unwrap() @@ -1139,7 +1139,7 @@ fn low_index_positional() { let m = m.unwrap(); assert!(m.is_present("files")); - assert_eq!(m.occurrences_of("files"), 3); + assert_eq!(m.occurrences_of("files"), 1); assert!(m.is_present("target")); assert_eq!(m.occurrences_of("target"), 1); assert_eq!( @@ -1176,7 +1176,7 @@ fn low_index_positional_in_subcmd() { let sm = m.subcommand_matches("test").unwrap(); assert!(sm.is_present("files")); - assert_eq!(sm.occurrences_of("files"), 3); + assert_eq!(sm.occurrences_of("files"), 1); assert!(sm.is_present("target")); assert_eq!(sm.occurrences_of("target"), 1); assert_eq!( @@ -1212,7 +1212,7 @@ fn low_index_positional_with_option() { let m = m.unwrap(); assert!(m.is_present("files")); - assert_eq!(m.occurrences_of("files"), 3); + assert_eq!(m.occurrences_of("files"), 1); assert!(m.is_present("target")); assert_eq!(m.occurrences_of("target"), 1); assert_eq!( @@ -1250,7 +1250,7 @@ fn low_index_positional_with_flag() { let m = m.unwrap(); assert!(m.is_present("files")); - assert_eq!(m.occurrences_of("files"), 3); + assert_eq!(m.occurrences_of("files"), 1); assert!(m.is_present("target")); assert_eq!(m.occurrences_of("target"), 1); assert_eq!( @@ -1498,6 +1498,7 @@ fn values_per_occurrence_named() { .collect::>(), ["val1", "val2"] ); + assert_eq!(m.occurrences_of("pos"), 1); let m = a.try_get_matches_from_mut(vec![ "myprog", "--pos", "val1", "val2", "--pos", "val3", "val4", @@ -1513,6 +1514,7 @@ fn values_per_occurrence_named() { .collect::>(), ["val1", "val2", "val3", "val4"] ); + assert_eq!(m.occurrences_of("pos"), 2); } #[test] @@ -1535,6 +1537,7 @@ fn values_per_occurrence_positional() { .collect::>(), ["val1", "val2"] ); + assert_eq!(m.occurrences_of("pos"), 1); let m = a.try_get_matches_from_mut(vec!["myprog", "val1", "val2", "val3", "val4"]); let m = match m { @@ -1548,4 +1551,36 @@ fn values_per_occurrence_positional() { .collect::>(), ["val1", "val2", "val3", "val4"] ); + //assert_eq!(m.occurrences_of("pos"), 2); // Fails, we don't recognize this as two occurrences +} + +// Theoretically we could support this but we aren't tracking occurrence boundaries for positionals +#[test] +#[should_panic = "When using a positional argument with .multiple_values(true) that is *not the last* positional argument, the last positional argument (i.e. the one with the highest index) *must* have .required(true) or .last(true) set."] +fn positional_parser_advances() { + let m = Command::new("multiple_values") + .arg(Arg::new("pos1").number_of_values(2)) + .arg(Arg::new("pos2").number_of_values(2)) + .try_get_matches_from(vec!["myprog", "val1", "val2", "val3", "val4"]); + + assert!(m.is_ok(), "{}", m.unwrap_err()); + let m = m.unwrap(); + + assert_eq!(m.occurrences_of("pos1"), 1); + assert_eq!( + m.get_many::("pos1") + .unwrap() + .map(|v| v.as_str()) + .collect::>(), + ["val1", "val2"] + ); + + assert_eq!(m.occurrences_of("pos2"), 1); + assert_eq!( + m.get_many::("pos2") + .unwrap() + .map(|v| v.as_str()) + .collect::>(), + ["val3", "val4"] + ); } From ccc809a9df18541297a3be758200ddb277e5cd19 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 26 May 2022 20:36:49 -0500 Subject: [PATCH 04/12] fix(parser): Allow delimiting default_missing_values Fixes #3761 --- src/parser/parser.rs | 56 ++++++--------------------- tests/builder/default_missing_vals.rs | 33 ++++++++++++++++ 2 files changed, 45 insertions(+), 44 deletions(-) diff --git a/src/parser/parser.rs b/src/parser/parser.rs index dc4dac3e19e..5a7151c65c3 100644 --- a/src/parser/parser.rs +++ b/src/parser/parser.rs @@ -941,11 +941,9 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { // We assume this case is valid: require equals, but min_vals == 0. if !opt.default_missing_vals.is_empty() { debug!("Parser::parse_opt: has default_missing_vals"); - self.add_multiple_vals_to_arg( - opt, - opt.default_missing_vals.iter().map(OsString::from), - matcher, - )?; + for v in opt.default_missing_vals.iter() { + self.add_val_to_arg(opt, &RawOsStr::new(v), matcher, trailing_values)?; + } }; if attached_value.is_some() { Ok(ParseResult::AttachedValueNotConsumed) @@ -989,7 +987,9 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { .split(delim) .map(|x| x.to_os_str().into_owned()) .take_while(|val| Some(val.as_os_str()) != terminator); - self.add_multiple_vals_to_arg(arg, vals, matcher)?; + for raw_val in vals { + self.add_single_val_to_arg(arg, raw_val, matcher)?; + } // If there was a delimiter used or we must use the delimiter to // separate the values or no more vals is needed, we're not // looking for more values. @@ -1016,19 +1016,6 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { } } - fn add_multiple_vals_to_arg( - &self, - arg: &Arg<'help>, - raw_vals: impl Iterator, - matcher: &mut ArgMatcher, - ) -> ClapResult<()> { - for raw_val in raw_vals { - self.add_single_val_to_arg(arg, raw_val, matcher)?; - } - - Ok(()) - } - fn add_single_val_to_arg( &self, arg: &Arg<'help>, @@ -1222,21 +1209,6 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { debug!("Parser::add_default_value: doesn't have conditional defaults"); } - fn process_default_vals(arg: &Arg<'_>, default_vals: &[&OsStr]) -> Vec { - if let Some(delim) = arg.val_delim { - let mut vals = vec![]; - for val in default_vals { - let val = RawOsStr::new(val); - for val in val.split(delim) { - vals.push(val.to_os_str().into_owned()); - } - } - vals - } else { - default_vals.iter().map(OsString::from).collect() - } - } - if !arg.default_vals.is_empty() { debug!( "Parser::add_default_value:iter:{}: has default vals", @@ -1249,11 +1221,9 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { debug!("Parser::add_default_value:iter:{}: wasn't used", arg.name); self.start_custom_arg(matcher, arg, ValueSource::DefaultValue); - self.add_multiple_vals_to_arg( - arg, - process_default_vals(arg, &arg.default_vals).into_iter(), - matcher, - )?; + for v in arg.default_vals.iter() { + self.add_val_to_arg(arg, &RawOsStr::new(v), matcher, trailing_values)?; + } } } else { debug!( @@ -1276,11 +1246,9 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { arg.name ); self.start_custom_arg(matcher, arg, ValueSource::DefaultValue); - self.add_multiple_vals_to_arg( - arg, - process_default_vals(arg, &arg.default_missing_vals).into_iter(), - matcher, - )?; + for v in arg.default_missing_vals.iter() { + self.add_val_to_arg(arg, &RawOsStr::new(v), matcher, trailing_values)?; + } } None => { debug!("Parser::add_default_value:iter:{}: wasn't used", arg.name); diff --git a/tests/builder/default_missing_vals.rs b/tests/builder/default_missing_vals.rs index 0700df6712f..11879f56d1c 100644 --- a/tests/builder/default_missing_vals.rs +++ b/tests/builder/default_missing_vals.rs @@ -174,6 +174,39 @@ fn default_missing_value_flag_value() { ); } +#[test] +fn delimited_missing_value() { + let cmd = Command::new("test").arg( + Arg::new("flag") + .long("flag") + .default_value("one,two") + .default_missing_value("three,four") + .min_values(0) + .value_delimiter(',') + .require_equals(true), + ); + + let m = cmd.clone().try_get_matches_from(["test"]).unwrap(); + assert_eq!( + m.get_many::("flag") + .unwrap() + .map(|s| s.as_str()) + .collect::>(), + vec!["one", "two"] + ); + assert_eq!(m.occurrences_of("flag"), 0); + + let m = cmd.try_get_matches_from(["test", "--flag"]).unwrap(); + assert_eq!( + m.get_many::("flag") + .unwrap() + .map(|s| s.as_str()) + .collect::>(), + vec!["three", "four"] + ); + assert_eq!(m.occurrences_of("flag"), 1); +} + #[cfg(debug_assertions)] #[test] #[should_panic = "Argument `arg`'s default_missing_value=\"value\" failed validation: error: \"value\" isn't a valid value for ''"] From 302bf63678b63b028fdae569a3c06ed1d9d2ba7b Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 26 May 2022 20:50:41 -0500 Subject: [PATCH 05/12] fix(parser): Always use delimiter on defaults/env Doesn't make sense to respect how the command line ended --- src/parser/parser.rs | 26 +++++++++----------- tests/builder/default_vals.rs | 46 +++++++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+), 14 deletions(-) diff --git a/src/parser/parser.rs b/src/parser/parser.rs index 5a7151c65c3..faca4e3ed4f 100644 --- a/src/parser/parser.rs +++ b/src/parser/parser.rs @@ -419,8 +419,8 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { }); #[cfg(feature = "env")] - self.add_env(matcher, trailing_values)?; - self.add_defaults(matcher, trailing_values)?; + self.add_env(matcher)?; + self.add_defaults(matcher)?; return Validator::new(self.cmd).validate(parse_state, matcher); } else { // Start error processing @@ -439,8 +439,8 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { } #[cfg(feature = "env")] - self.add_env(matcher, trailing_values)?; - self.add_defaults(matcher, trailing_values)?; + self.add_env(matcher)?; + self.add_defaults(matcher)?; Validator::new(self.cmd).validate(parse_state, matcher) } @@ -1101,10 +1101,11 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { } #[cfg(feature = "env")] - fn add_env(&mut self, matcher: &mut ArgMatcher, trailing_values: bool) -> ClapResult<()> { + fn add_env(&mut self, matcher: &mut ArgMatcher) -> ClapResult<()> { debug!("Parser::add_env"); use crate::util::str_to_bool; + let trailing_values = false; // defaults are independent of the commandline for arg in self.cmd.get_arguments() { // Use env only if the arg was absent among command line args, // early return if this is not the case. @@ -1154,28 +1155,25 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { Ok(()) } - fn add_defaults(&mut self, matcher: &mut ArgMatcher, trailing_values: bool) -> ClapResult<()> { + fn add_defaults(&mut self, matcher: &mut ArgMatcher) -> ClapResult<()> { debug!("Parser::add_defaults"); for o in self.cmd.get_opts() { debug!("Parser::add_defaults:iter:{}:", o.name); - self.add_default_value(o, matcher, trailing_values)?; + self.add_default_value(o, matcher)?; } for p in self.cmd.get_positionals() { debug!("Parser::add_defaults:iter:{}:", p.name); - self.add_default_value(p, matcher, trailing_values)?; + self.add_default_value(p, matcher)?; } Ok(()) } - fn add_default_value( - &self, - arg: &Arg<'help>, - matcher: &mut ArgMatcher, - trailing_values: bool, - ) -> ClapResult<()> { + fn add_default_value(&self, arg: &Arg<'help>, matcher: &mut ArgMatcher) -> ClapResult<()> { + let trailing_values = false; // defaults are independent of the commandline + if !arg.default_vals_ifs.is_empty() { debug!("Parser::add_default_value: has conditional defaults"); if matcher.get(&arg.id).is_none() { diff --git a/tests/builder/default_vals.rs b/tests/builder/default_vals.rs index 678484737d0..501ffcd386a 100644 --- a/tests/builder/default_vals.rs +++ b/tests/builder/default_vals.rs @@ -831,3 +831,49 @@ fn missing_with_value_delimiter() { ["value1", "value2", "value3", "value4", "value5"] ); } + +#[test] +fn default_independent_of_trailing() { + let cmd = Command::new("test") + .dont_delimit_trailing_values(true) + .arg(Arg::new("pos").required(true)) + .arg( + Arg::new("flag") + .long("flag") + .default_value("one,two") + .value_delimiter(','), + ); + + // Baseline behavior + let m = cmd + .clone() + .try_get_matches_from(vec!["program", "here"]) + .unwrap(); + assert_eq!( + m.get_one::("pos").map(|v| v.as_str()).unwrap(), + "here" + ); + assert_eq!( + m.get_many::("flag") + .unwrap() + .map(|v| v.as_str()) + .collect::>(), + ["one", "two"] + ); + + // Trailing-values behavior should match the baseline + let m = cmd + .try_get_matches_from(vec!["program", "--", "here"]) + .unwrap(); + assert_eq!( + m.get_one::("pos").map(|v| v.as_str()).unwrap(), + "here" + ); + assert_eq!( + m.get_many::("flag") + .unwrap() + .map(|v| v.as_str()) + .collect::>(), + ["one", "two"] + ); +} From c72f03e53f6993b4c453d59940887978d700f294 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 27 May 2022 07:04:21 -0500 Subject: [PATCH 06/12] refactor(parser): Be more explicit in default_missing_values I wrote these tests expecting to highlight a bug but it turns out things were structured just right to not exhibit it. The fact that the code looks like its broken is a problem, so I restructured it (put it first, changed the source) so it doesn't look suspicious anymore. --- src/parser/parser.rs | 74 +++++++++++----------- tests/builder/default_missing_vals.rs | 89 +++++++++++++++++++-------- 2 files changed, 99 insertions(+), 64 deletions(-) diff --git a/src/parser/parser.rs b/src/parser/parser.rs index faca4e3ed4f..cfaae49eda6 100644 --- a/src/parser/parser.rs +++ b/src/parser/parser.rs @@ -1174,6 +1174,43 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { fn add_default_value(&self, arg: &Arg<'help>, matcher: &mut ArgMatcher) -> ClapResult<()> { let trailing_values = false; // defaults are independent of the commandline + if !arg.default_missing_vals.is_empty() { + debug!( + "Parser::add_default_value:iter:{}: has default missing vals", + arg.name + ); + match matcher.get(&arg.id) { + Some(ma) if ma.all_val_groups_empty() => { + debug!( + "Parser::add_default_value:iter:{}: has no user defined vals", + arg.name + ); + // The flag occurred, we just want to add the val groups + self.start_custom_arg(matcher, arg, ValueSource::CommandLine); + for v in arg.default_missing_vals.iter() { + self.add_val_to_arg(arg, &RawOsStr::new(v), matcher, trailing_values)?; + } + } + None => { + debug!("Parser::add_default_value:iter:{}: wasn't used", arg.name); + // do nothing + } + _ => { + debug!( + "Parser::add_default_value:iter:{}: has user defined vals", + arg.name + ); + // do nothing + } + } + } else { + debug!( + "Parser::add_default_value:iter:{}: doesn't have default missing vals", + arg.name + ); + // do nothing + } + if !arg.default_vals_ifs.is_empty() { debug!("Parser::add_default_value: has conditional defaults"); if matcher.get(&arg.id).is_none() { @@ -1232,43 +1269,6 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { // do nothing } - if !arg.default_missing_vals.is_empty() { - debug!( - "Parser::add_default_value:iter:{}: has default missing vals", - arg.name - ); - match matcher.get(&arg.id) { - Some(ma) if ma.all_val_groups_empty() => { - debug!( - "Parser::add_default_value:iter:{}: has no user defined vals", - arg.name - ); - self.start_custom_arg(matcher, arg, ValueSource::DefaultValue); - for v in arg.default_missing_vals.iter() { - self.add_val_to_arg(arg, &RawOsStr::new(v), matcher, trailing_values)?; - } - } - None => { - debug!("Parser::add_default_value:iter:{}: wasn't used", arg.name); - // do nothing - } - _ => { - debug!( - "Parser::add_default_value:iter:{}: has user defined vals", - arg.name - ); - // do nothing - } - } - } else { - debug!( - "Parser::add_default_value:iter:{}: doesn't have default missing vals", - arg.name - ); - - // do nothing - } - Ok(()) } diff --git a/tests/builder/default_missing_vals.rs b/tests/builder/default_missing_vals.rs index 11879f56d1c..bacbe1ff8e6 100644 --- a/tests/builder/default_missing_vals.rs +++ b/tests/builder/default_missing_vals.rs @@ -1,4 +1,4 @@ -use clap::{arg, Arg, ArgMatches, Command}; +use clap::{arg, Arg, Command}; #[test] fn opt_missing() { @@ -20,6 +20,10 @@ fn opt_missing() { "auto" ); assert_eq!(m.occurrences_of("color"), 0); + assert_eq!( + m.value_source("color").unwrap(), + clap::ValueSource::DefaultValue + ); } #[test] @@ -42,6 +46,10 @@ fn opt_present_with_missing_value() { "always" ); assert_eq!(m.occurrences_of("color"), 1); + assert_eq!( + m.value_source("color").unwrap(), + clap::ValueSource::CommandLine + ); } #[test] @@ -64,6 +72,10 @@ fn opt_present_with_value() { "never" ); assert_eq!(m.occurrences_of("color"), 1); + assert_eq!( + m.value_source("color").unwrap(), + clap::ValueSource::CommandLine + ); } #[test] @@ -85,6 +97,10 @@ fn opt_present_with_empty_value() { "" ); assert_eq!(m.occurrences_of("color"), 1); + assert_eq!( + m.value_source("color").unwrap(), + clap::ValueSource::CommandLine + ); } //## `default_value`/`default_missing_value` non-interaction checks @@ -134,43 +150,62 @@ fn default_missing_value_flag_value() { Arg::new("flag") .long("flag") .takes_value(true) + .default_value("false") .default_missing_value("true"), ); - fn flag_value(m: ArgMatches) -> bool { - match m.get_one::("flag").map(|v| v.as_str()) { - None => false, - Some(x) => x.parse().expect("non boolean value"), - } - } + let m = cmd.clone().try_get_matches_from(&["test"]).unwrap(); + assert!(m.is_present("flag")); + assert_eq!( + m.get_one::("flag").map(|v| v.as_str()), + Some("false") + ); + assert_eq!(m.occurrences_of("flag"), 0); + assert_eq!( + m.value_source("flag").unwrap(), + clap::ValueSource::DefaultValue + ); + + let m = cmd + .clone() + .try_get_matches_from(&["test", "--flag"]) + .unwrap(); + assert!(m.is_present("flag")); + assert_eq!( + m.get_one::("flag").map(|v| v.as_str()), + Some("true") + ); + assert_eq!(m.occurrences_of("flag"), 1); + assert_eq!( + m.value_source("flag").unwrap(), + clap::ValueSource::CommandLine + ); + let m = cmd + .clone() + .try_get_matches_from(&["test", "--flag=true"]) + .unwrap(); + assert!(m.is_present("flag")); assert_eq!( - flag_value(cmd.clone().try_get_matches_from(&["test"]).unwrap()), - false + m.get_one::("flag").map(|v| v.as_str()), + Some("true") ); + assert_eq!(m.occurrences_of("flag"), 1); assert_eq!( - flag_value( - cmd.clone() - .try_get_matches_from(&["test", "--flag"]) - .unwrap() - ), - true + m.value_source("flag").unwrap(), + clap::ValueSource::CommandLine ); + + let m = cmd.try_get_matches_from(&["test", "--flag=false"]).unwrap(); + assert!(m.is_present("flag")); assert_eq!( - flag_value( - cmd.clone() - .try_get_matches_from(&["test", "--flag=true"]) - .unwrap() - ), - true + m.get_one::("flag").map(|v| v.as_str()), + Some("false") ); + assert_eq!(m.occurrences_of("flag"), 1); assert_eq!( - flag_value( - cmd.clone() - .try_get_matches_from(&["test", "--flag=false"]) - .unwrap() - ), - false + m.value_source("flag").unwrap(), + clap::ValueSource::CommandLine ); } From e268dbfbe9f672f4ce76e8c1e7eada881cc25a63 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 27 May 2022 09:23:21 -0500 Subject: [PATCH 07/12] refactor(parser): Clarify value consumption name --- src/parser/parser.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/parser/parser.rs b/src/parser/parser.rs index cfaae49eda6..0a9b7d4a65b 100644 --- a/src/parser/parser.rs +++ b/src/parser/parser.rs @@ -743,7 +743,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { long_arg, &long_value ); let has_eq = long_value.is_some(); - self.parse_opt(long_value, opt, matcher, trailing_values, has_eq) + self.parse_opt_value(long_value, opt, matcher, trailing_values, has_eq) } else if let Some(rest) = long_value { let required = self.cmd.required_graph(); debug!( @@ -890,7 +890,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { } else { (val, false) }; - match self.parse_opt(val, opt, matcher, trailing_values, has_eq)? { + match self.parse_opt_value(val, opt, matcher, trailing_values, has_eq)? { ParseResult::AttachedValueNotConsumed => continue, x => return Ok(x), } @@ -918,7 +918,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { Ok(ret) } - fn parse_opt( + fn parse_opt_value( &self, attached_value: Option<&RawOsStr>, opt: &Arg<'help>, @@ -927,12 +927,12 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { has_eq: bool, ) -> ClapResult { debug!( - "Parser::parse_opt; opt={}, val={:?}, has_eq={:?}", + "Parser::parse_opt_value; opt={}, val={:?}, has_eq={:?}", opt.name, attached_value, has_eq ); - debug!("Parser::parse_opt; opt.settings={:?}", opt.settings); + debug!("Parser::parse_opt_value; opt.settings={:?}", opt.settings); - debug!("Parser::parse_opt; Checking for val..."); + debug!("Parser::parse_opt_value; Checking for val..."); // require_equals is set, but no '=' is provided, try throwing error. if opt.is_require_equals_set() && !has_eq { if opt.min_vals == Some(0) { @@ -940,7 +940,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { self.start_occurrence_of_arg(matcher, opt); // We assume this case is valid: require equals, but min_vals == 0. if !opt.default_missing_vals.is_empty() { - debug!("Parser::parse_opt: has default_missing_vals"); + debug!("Parser::parse_opt_value: has default_missing_vals"); for v in opt.default_missing_vals.iter() { self.add_val_to_arg(opt, &RawOsStr::new(v), matcher, trailing_values)?; } @@ -961,7 +961,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { self.add_val_to_arg(opt, v, matcher, trailing_values)?; Ok(ParseResult::ValuesDone) } else { - debug!("Parser::parse_opt: More arg vals required..."); + debug!("Parser::parse_opt_value: More arg vals required..."); self.start_occurrence_of_arg(matcher, opt); Ok(ParseResult::Opt(opt.id.clone())) } From 9f4686714ac8b3b263da1fc75873ff863c14de94 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 27 May 2022 10:31:10 -0500 Subject: [PATCH 08/12] fix(parser): Always end on required delimited arg Before, if we were in trailing values that aren't delimite, we wouldn't respect this flag and end processing of the value, now we do. This also has a slight perf benefit of us only splitting the value if the delimiter is present. We checked for the delimiter anyways, so doing it first removes a slight bit of work. I also feel this helps clarify the intended behavior and ooches us towards a unified code path for actions. --- src/parser/parser.rs | 49 ++++++++++++++++++++------------------------ 1 file changed, 22 insertions(+), 27 deletions(-) diff --git a/src/parser/parser.rs b/src/parser/parser.rs index 0a9b7d4a65b..0d8af1b0b23 100644 --- a/src/parser/parser.rs +++ b/src/parser/parser.rs @@ -980,40 +980,35 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { trailing_values, self.cmd.is_dont_delimit_trailing_values_set() ); - if !(trailing_values && self.cmd.is_dont_delimit_trailing_values_set()) { - if let Some(delim) = arg.val_delim { - let terminator = arg.terminator.map(OsStr::new); - let vals = val - .split(delim) - .map(|x| x.to_os_str().into_owned()) - .take_while(|val| Some(val.as_os_str()) != terminator); + + let mut delim = arg.val_delim; + if trailing_values && self.cmd.is_dont_delimit_trailing_values_set() { + delim = None; + } + match delim { + Some(delim) if val.contains(delim) => { + let vals = val.split(delim).map(|x| x.to_os_str().into_owned()); for raw_val in vals { + if Some(raw_val.as_os_str()) == arg.terminator.map(OsStr::new) { + return Ok(ParseResult::ValuesDone); + } self.add_single_val_to_arg(arg, raw_val, matcher)?; } - // If there was a delimiter used or we must use the delimiter to - // separate the values or no more vals is needed, we're not - // looking for more values. - return if val.contains(delim) - || arg.is_require_value_delimiter_set() - || !matcher.needs_more_vals(arg) - { + // Delimited values are always considered the final value + Ok(ParseResult::ValuesDone) + } + _ if Some(val) == arg.terminator.map(RawOsStr::from_str) => Ok(ParseResult::ValuesDone), + _ => { + self.add_single_val_to_arg(arg, val.to_os_str().into_owned(), matcher)?; + if arg.is_require_value_delimiter_set() { Ok(ParseResult::ValuesDone) - } else { + } else if matcher.needs_more_vals(arg) { Ok(ParseResult::Opt(arg.id.clone())) - }; - } - } - if let Some(t) = arg.terminator { - if t == val { - return Ok(ParseResult::ValuesDone); + } else { + Ok(ParseResult::ValuesDone) + } } } - self.add_single_val_to_arg(arg, val.to_os_str().into_owned(), matcher)?; - if matcher.needs_more_vals(arg) { - Ok(ParseResult::Opt(arg.id.clone())) - } else { - Ok(ParseResult::ValuesDone) - } } fn add_single_val_to_arg( From ac8320ddb23b7ce20c0e70607664384fe882d65d Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 27 May 2022 12:57:13 -0500 Subject: [PATCH 09/12] refactor(parser): Make it more clear when we ignore parse results --- src/parser/parser.rs | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/src/parser/parser.rs b/src/parser/parser.rs index 0d8af1b0b23..f81670ebd06 100644 --- a/src/parser/parser.rs +++ b/src/parser/parser.rs @@ -182,7 +182,8 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { && !self.is_set(AS::NoAutoHelp) && !self.cmd.is_disable_help_subcommand_set() { - self.parse_help_subcommand(raw_args.remaining(&mut args_cursor))?; + let _ = + self.parse_help_subcommand(raw_args.remaining(&mut args_cursor))?; } subcmd_name = Some(sc_name.to_owned()); break; @@ -376,7 +377,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { if !p.is_multiple_values_set() || !matcher.contains(&p.id) { self.start_occurrence_of_arg(matcher, p); } - self.add_val_to_arg(p, arg_os.to_value_os(), matcher, trailing_values)?; + let _ = self.add_val_to_arg(p, arg_os.to_value_os(), matcher, trailing_values)?; // Only increment the positional counter if it doesn't allow multiples if !p.is_multiple() { @@ -942,7 +943,8 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { if !opt.default_missing_vals.is_empty() { debug!("Parser::parse_opt_value: has default_missing_vals"); for v in opt.default_missing_vals.iter() { - self.add_val_to_arg(opt, &RawOsStr::new(v), matcher, trailing_values)?; + let _ = + self.add_val_to_arg(opt, &RawOsStr::new(v), matcher, trailing_values)?; } }; if attached_value.is_some() { @@ -958,7 +960,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { } } else if let Some(v) = attached_value { self.start_occurrence_of_arg(matcher, opt); - self.add_val_to_arg(opt, v, matcher, trailing_values)?; + let _ = self.add_val_to_arg(opt, v, matcher, trailing_values)?; Ok(ParseResult::ValuesDone) } else { debug!("Parser::parse_opt_value: More arg vals required..."); @@ -1122,7 +1124,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { val, trailing_values ); self.start_custom_arg(matcher, arg, ValueSource::EnvVariable); - self.add_val_to_arg(arg, &val, matcher, trailing_values)?; + let _ = self.add_val_to_arg(arg, &val, matcher, trailing_values)?; } else { match arg.get_action() { Action::StoreValue => unreachable!("{:?} is not a flag", arg.get_id()), @@ -1183,7 +1185,8 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { // The flag occurred, we just want to add the val groups self.start_custom_arg(matcher, arg, ValueSource::CommandLine); for v in arg.default_missing_vals.iter() { - self.add_val_to_arg(arg, &RawOsStr::new(v), matcher, trailing_values)?; + let _ = + self.add_val_to_arg(arg, &RawOsStr::new(v), matcher, trailing_values)?; } } None => { @@ -1224,7 +1227,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { if add { if let Some(default) = default { self.start_custom_arg(matcher, arg, ValueSource::DefaultValue); - self.add_val_to_arg( + let _ = self.add_val_to_arg( arg, &RawOsStr::new(default), matcher, @@ -1252,7 +1255,8 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { self.start_custom_arg(matcher, arg, ValueSource::DefaultValue); for v in arg.default_vals.iter() { - self.add_val_to_arg(arg, &RawOsStr::new(v), matcher, trailing_values)?; + let _ = + self.add_val_to_arg(arg, &RawOsStr::new(v), matcher, trailing_values)?; } } } else { @@ -1373,6 +1377,7 @@ pub(crate) enum ParseState { /// Recoverable Parsing results. #[derive(Debug, PartialEq, Clone)] +#[must_use] enum ParseResult { FlagSubCommand(String), Opt(Id), From 2a409be7a5ba5a42f8283177c4e10a91fa08e5bc Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 27 May 2022 13:15:34 -0500 Subject: [PATCH 10/12] refactor(parser): Clarify all ignored parse results --- src/parser/parser.rs | 50 +++++++++++++++++++++++++++++++++----------- 1 file changed, 38 insertions(+), 12 deletions(-) diff --git a/src/parser/parser.rs b/src/parser/parser.rs index f81670ebd06..b080ee6e64c 100644 --- a/src/parser/parser.rs +++ b/src/parser/parser.rs @@ -182,10 +182,11 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { && !self.is_set(AS::NoAutoHelp) && !self.cmd.is_disable_help_subcommand_set() { - let _ = - self.parse_help_subcommand(raw_args.remaining(&mut args_cursor))?; + self.parse_help_subcommand(raw_args.remaining(&mut args_cursor))?; + unreachable!("`parse_help_subcommand` always errors"); + } else { + subcmd_name = Some(sc_name.to_owned()); } - subcmd_name = Some(sc_name.to_owned()); break; } } @@ -377,7 +378,12 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { if !p.is_multiple_values_set() || !matcher.contains(&p.id) { self.start_occurrence_of_arg(matcher, p); } - let _ = self.add_val_to_arg(p, arg_os.to_value_os(), matcher, trailing_values)?; + let _ignored_result = + self.add_val_to_arg(p, arg_os.to_value_os(), matcher, trailing_values)?; + debug!( + "Parser::get_matches_with: Ignoring state {:?}; positionals do their own thing", + _ignored_result + ); // Only increment the positional counter if it doesn't allow multiples if !p.is_multiple() { @@ -570,7 +576,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { fn parse_help_subcommand( &self, cmds: impl Iterator, - ) -> ClapResult { + ) -> ClapResult { debug!("Parser::parse_help_subcommand"); let mut cmd = self.cmd.clone(); @@ -943,8 +949,11 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { if !opt.default_missing_vals.is_empty() { debug!("Parser::parse_opt_value: has default_missing_vals"); for v in opt.default_missing_vals.iter() { - let _ = + let _ignored_result = self.add_val_to_arg(opt, &RawOsStr::new(v), matcher, trailing_values)?; + if _ignored_result != ParseResult::ValuesDone { + debug!("Parser::parse_opt_value: Ignoring state {:?}; no values accepted after default_missing_values", _ignored_result); + } } }; if attached_value.is_some() { @@ -960,8 +969,12 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { } } else if let Some(v) = attached_value { self.start_occurrence_of_arg(matcher, opt); - let _ = self.add_val_to_arg(opt, v, matcher, trailing_values)?; - Ok(ParseResult::ValuesDone) + let mut val_result = self.add_val_to_arg(opt, v, matcher, trailing_values)?; + if val_result != ParseResult::ValuesDone { + debug!("Parser::parse_opt_value: Overiding state {:?}; no values accepted after attached", val_result); + val_result = ParseResult::ValuesDone; + } + Ok(val_result) } else { debug!("Parser::parse_opt_value: More arg vals required..."); self.start_occurrence_of_arg(matcher, opt); @@ -1124,7 +1137,11 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { val, trailing_values ); self.start_custom_arg(matcher, arg, ValueSource::EnvVariable); - let _ = self.add_val_to_arg(arg, &val, matcher, trailing_values)?; + let _ignored_result = + self.add_val_to_arg(arg, &val, matcher, trailing_values)?; + if _ignored_result != ParseResult::ValuesDone { + debug!("Parser::add_env: Ignoring state {:?}; env variables are outside of the parse loop", _ignored_result); + } } else { match arg.get_action() { Action::StoreValue => unreachable!("{:?} is not a flag", arg.get_id()), @@ -1185,8 +1202,11 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { // The flag occurred, we just want to add the val groups self.start_custom_arg(matcher, arg, ValueSource::CommandLine); for v in arg.default_missing_vals.iter() { - let _ = + let _ignored_result = self.add_val_to_arg(arg, &RawOsStr::new(v), matcher, trailing_values)?; + if _ignored_result != ParseResult::ValuesDone { + debug!("Parser::add_default_value: Ignoring state {:?}; defaults are outside of the parse loop", _ignored_result); + } } } None => { @@ -1227,12 +1247,15 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { if add { if let Some(default) = default { self.start_custom_arg(matcher, arg, ValueSource::DefaultValue); - let _ = self.add_val_to_arg( + let _ignored_result = self.add_val_to_arg( arg, &RawOsStr::new(default), matcher, trailing_values, )?; + if _ignored_result != ParseResult::ValuesDone { + debug!("Parser::add_default_value: Ignoring state {:?}; defaults are outside of the parse loop", _ignored_result); + } } return Ok(()); } @@ -1255,8 +1278,11 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { self.start_custom_arg(matcher, arg, ValueSource::DefaultValue); for v in arg.default_vals.iter() { - let _ = + let _ignored_result = self.add_val_to_arg(arg, &RawOsStr::new(v), matcher, trailing_values)?; + if _ignored_result != ParseResult::ValuesDone { + debug!("Parser::add_default_value: Ignoring state {:?}; defaults are outside of the parse loop", _ignored_result); + } } } } else { From 8f16f2ea41e6d1097f164b1e0a4f3bc5dcc19893 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 27 May 2022 13:55:52 -0500 Subject: [PATCH 11/12] refactor(parser): Clarify intent for defaults/envs Especially important is not inferring intent from occurrences as hopefully that will change with the introduction of actions. --- src/parser/parser.rs | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/parser/parser.rs b/src/parser/parser.rs index b080ee6e64c..ced90e90345 100644 --- a/src/parser/parser.rs +++ b/src/parser/parser.rs @@ -1119,10 +1119,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { for arg in self.cmd.get_arguments() { // Use env only if the arg was absent among command line args, // early return if this is not the case. - if matcher - .get(&arg.id) - .map_or(false, |arg| arg.get_occurrences() != 0) - { + if matcher.contains(&arg.id) { debug!("Parser::add_env: Skipping existing arg `{}`", arg); continue; } @@ -1231,7 +1228,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { if !arg.default_vals_ifs.is_empty() { debug!("Parser::add_default_value: has conditional defaults"); - if matcher.get(&arg.id).is_none() { + if !matcher.contains(&arg.id) { for (id, val, default) in arg.default_vals_ifs.iter() { let add = if let Some(a) = matcher.get(id) { match val { @@ -1270,7 +1267,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { "Parser::add_default_value:iter:{}: has default vals", arg.name ); - if matcher.get(&arg.id).is_some() { + if matcher.contains(&arg.id) { debug!("Parser::add_default_value:iter:{}: was used", arg.name); // do nothing } else { From e41a65d5407b62ebbd69737a85710e57c5014d59 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 27 May 2022 13:56:38 -0500 Subject: [PATCH 12/12] style: Fix typos --- src/parser/parser.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/parser/parser.rs b/src/parser/parser.rs index ced90e90345..3e2b22827ba 100644 --- a/src/parser/parser.rs +++ b/src/parser/parser.rs @@ -971,7 +971,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { self.start_occurrence_of_arg(matcher, opt); let mut val_result = self.add_val_to_arg(opt, v, matcher, trailing_values)?; if val_result != ParseResult::ValuesDone { - debug!("Parser::parse_opt_value: Overiding state {:?}; no values accepted after attached", val_result); + debug!("Parser::parse_opt_value: Overriding state {:?}; no values accepted after attached", val_result); val_result = ParseResult::ValuesDone; } Ok(val_result)