From e4271d09d914aaf9f4c3a5b8cce820097ddcd348 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 27 May 2022 15:51:35 -0500 Subject: [PATCH 01/19] refactor(parser): Normalize Arg variable names --- src/parser/parser.rs | 94 ++++++++++++++++++++++---------------------- 1 file changed, 47 insertions(+), 47 deletions(-) diff --git a/src/parser/parser.rs b/src/parser/parser.rs index 3e2b22827ba..17a48eab052 100644 --- a/src/parser/parser.rs +++ b/src/parser/parser.rs @@ -130,17 +130,17 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { if low_index_mults || missing_pos { let skip_current = if let Some(n) = raw_args.peek(&args_cursor) { - if let Some(p) = self + if let Some(arg) = self .cmd .get_positionals() - .find(|p| p.index == Some(pos_counter)) + .find(|a| a.index == Some(pos_counter)) { // If next value looks like a new_arg or it's a // subcommand, skip positional argument under current // pos_counter(which means current value cannot be a // positional argument with a value next to it), assume // current value matches the next arg. - self.is_new_arg(&n, p) + self.is_new_arg(&n, arg) || self .possible_subcommand(n.to_value(), valid_arg_found) .is_some() @@ -361,8 +361,8 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { } } - if let Some(p) = self.cmd.get_keymap().get(&pos_counter) { - if p.is_last_set() && !trailing_values { + if let Some(arg) = self.cmd.get_keymap().get(&pos_counter) { + if arg.is_last_set() && !trailing_values { return Err(ClapError::unknown_argument( self.cmd, arg_os.display().to_string(), @@ -375,22 +375,22 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { trailing_values = true; } - if !p.is_multiple_values_set() || !matcher.contains(&p.id) { - self.start_occurrence_of_arg(matcher, p); + if !arg.is_multiple_values_set() || !matcher.contains(&arg.id) { + self.start_occurrence_of_arg(matcher, arg); } let _ignored_result = - self.add_val_to_arg(p, arg_os.to_value_os(), matcher, trailing_values)?; + self.add_val_to_arg(arg, 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() { + if !arg.is_multiple() { pos_counter += 1; parse_state = ParseState::ValuesDone; } else { - parse_state = ParseState::Pos(p.id.clone()); + parse_state = ParseState::Pos(arg.id.clone()); } valid_arg_found = true; } else if let Some(external_parser) = @@ -721,12 +721,12 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { return Ok(ParseResult::NoArg); } - let opt = if let Some(opt) = self.cmd.get_keymap().get(long_arg) { + let arg = if let Some(arg) = self.cmd.get_keymap().get(long_arg) { debug!( - "Parser::parse_long_arg: Found valid opt or flag '{}'", - opt.to_string() + "Parser::parse_long_arg: Found valid arg or flag '{}'", + arg.to_string() ); - Some((long_arg, opt)) + Some((long_arg, arg)) } else if self.cmd.is_infer_long_args_set() { self.cmd.get_arguments().find_map(|a| { if let Some(long) = a.long { @@ -742,15 +742,15 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { None }; - if let Some((long_arg, opt)) = opt { + if let Some((long_arg, arg)) = arg { *valid_arg_found = true; - if opt.is_takes_value_set() { + if arg.is_takes_value_set() { debug!( - "Parser::parse_long_arg({:?}): Found an opt with value '{:?}'", + "Parser::parse_long_arg({:?}): Found an arg with value '{:?}'", long_arg, &long_value ); let has_eq = long_value.is_some(); - self.parse_opt_value(long_value, opt, matcher, trailing_values, has_eq) + self.parse_opt_value(long_value, arg, matcher, trailing_values, has_eq) } else if let Some(rest) = long_value { let required = self.cmd.required_graph(); debug!( @@ -770,11 +770,11 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { Ok(ParseResult::UnneededAttachedValue { rest: rest.to_str_lossy().into_owned(), used, - arg: opt.to_string(), + arg: arg.to_string(), }) } else { debug!("Parser::parse_long_arg({:?}): Presence validated", long_arg); - self.parse_flag(Identifier::Long(long_arg), opt, matcher) + self.parse_flag(Identifier::Long(long_arg), arg, matcher) } } else if let Some(sc_name) = self.possible_long_flag_subcommand(long_arg) { Ok(ParseResult::FlagSubCommand(sc_name.to_string())) @@ -864,14 +864,14 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { // concatenated value: -oval // Option: -o // Value: val - if let Some(opt) = self.cmd.get_keymap().get(&c) { + if let Some(arg) = self.cmd.get_keymap().get(&c) { debug!( "Parser::parse_short_arg:iter:{}: Found valid opt or flag", c ); *valid_arg_found = true; - if !opt.is_takes_value_set() { - ret = self.parse_flag(Identifier::Short(c), opt, matcher)?; + if !arg.is_takes_value_set() { + ret = self.parse_flag(Identifier::Short(c), arg, matcher)?; continue; } @@ -897,7 +897,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { } else { (val, false) }; - match self.parse_opt_value(val, opt, matcher, trailing_values, has_eq)? { + match self.parse_opt_value(val, arg, matcher, trailing_values, has_eq)? { ParseResult::AttachedValueNotConsumed => continue, x => return Ok(x), } @@ -928,29 +928,29 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { fn parse_opt_value( &self, attached_value: Option<&RawOsStr>, - opt: &Arg<'help>, + arg: &Arg<'help>, matcher: &mut ArgMatcher, trailing_values: bool, has_eq: bool, ) -> ClapResult { debug!( - "Parser::parse_opt_value; opt={}, val={:?}, has_eq={:?}", - opt.name, attached_value, has_eq + "Parser::parse_opt_value; arg={}, val={:?}, has_eq={:?}", + arg.name, attached_value, has_eq ); - debug!("Parser::parse_opt_value; opt.settings={:?}", opt.settings); + debug!("Parser::parse_opt_value; arg.settings={:?}", arg.settings); 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) { + if arg.is_require_equals_set() && !has_eq { + if arg.min_vals == Some(0) { debug!("Requires equals, but min_vals == 0"); - self.start_occurrence_of_arg(matcher, opt); + self.start_occurrence_of_arg(matcher, arg); // We assume this case is valid: require equals, but min_vals == 0. - if !opt.default_missing_vals.is_empty() { + if !arg.default_missing_vals.is_empty() { debug!("Parser::parse_opt_value: has default_missing_vals"); - for v in opt.default_missing_vals.iter() { + for v in arg.default_missing_vals.iter() { let _ignored_result = - self.add_val_to_arg(opt, &RawOsStr::new(v), matcher, trailing_values)?; + self.add_val_to_arg(arg, &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); } @@ -964,12 +964,12 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { } else { debug!("Requires equals but not provided. Error."); Ok(ParseResult::EqualsNotProvided { - arg: opt.to_string(), + arg: arg.to_string(), }) } } else if let Some(v) = attached_value { - self.start_occurrence_of_arg(matcher, opt); - let mut val_result = self.add_val_to_arg(opt, v, matcher, trailing_values)?; + self.start_occurrence_of_arg(matcher, arg); + let mut val_result = self.add_val_to_arg(arg, v, matcher, trailing_values)?; if val_result != ParseResult::ValuesDone { debug!("Parser::parse_opt_value: Overriding state {:?}; no values accepted after attached", val_result); val_result = ParseResult::ValuesDone; @@ -977,8 +977,8 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { Ok(val_result) } else { debug!("Parser::parse_opt_value: More arg vals required..."); - self.start_occurrence_of_arg(matcher, opt); - Ok(ParseResult::Opt(opt.id.clone())) + self.start_occurrence_of_arg(matcher, arg); + Ok(ParseResult::Opt(arg.id.clone())) } } @@ -1169,14 +1169,14 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { 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)?; + for arg in self.cmd.get_opts() { + debug!("Parser::add_defaults:iter:{}:", arg.name); + self.add_default_value(arg, matcher)?; } - for p in self.cmd.get_positionals() { - debug!("Parser::add_defaults:iter:{}:", p.name); - self.add_default_value(p, matcher)?; + for arg in self.cmd.get_positionals() { + debug!("Parser::add_defaults:iter:{}:", arg.name); + self.add_default_value(arg, matcher)?; } Ok(()) @@ -1345,8 +1345,8 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { // Add the arg to the matches to build a proper usage string if let Some((name, _)) = did_you_mean.as_ref() { - if let Some(opt) = self.cmd.get_keymap().get(&name.as_ref()) { - self.start_occurrence_of_arg(matcher, opt); + if let Some(arg) = self.cmd.get_keymap().get(&name.as_ref()) { + self.start_occurrence_of_arg(matcher, arg); } } From 7b6bb3298577bc83aa1a5c17887604f80f930f7e Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 27 May 2022 16:27:35 -0500 Subject: [PATCH 02/19] refactor(parser): Separate delimiting from storing --- src/parser/parser.rs | 204 ++++++++++++++++++++++++++++--------------- 1 file changed, 133 insertions(+), 71 deletions(-) diff --git a/src/parser/parser.rs b/src/parser/parser.rs index 17a48eab052..c9ce5a03b28 100644 --- a/src/parser/parser.rs +++ b/src/parser/parser.rs @@ -345,12 +345,22 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { // Assume this is a value of a previous arg. // get the option so we can check the settings - let parse_result = self.add_val_to_arg( - &self.cmd[id], + let mut arg_values = Vec::new(); + let arg = &self.cmd[id]; + let parse_result = self.push_arg_values( + arg, arg_os.to_value_os(), - matcher, trailing_values, - )?; + &mut arg_values, + ); + self.store_arg_values(arg, arg_values, matcher)?; + let parse_result = parse_result.unwrap_or_else(|| { + if matcher.needs_more_vals(arg) { + ParseResult::Opt(arg.id.clone()) + } else { + ParseResult::ValuesDone + } + }); parse_state = match parse_result { ParseResult::Opt(id) => ParseState::Opt(id), ParseResult::ValuesDone => ParseState::ValuesDone, @@ -375,15 +385,25 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { trailing_values = true; } + let mut arg_values = Vec::new(); + let _parse_result = self.push_arg_values( + arg, + arg_os.to_value_os(), + trailing_values, + &mut arg_values, + ); if !arg.is_multiple_values_set() || !matcher.contains(&arg.id) { self.start_occurrence_of_arg(matcher, arg); } - let _ignored_result = - self.add_val_to_arg(arg, arg_os.to_value_os(), matcher, trailing_values)?; - debug!( - "Parser::get_matches_with: Ignoring state {:?}; positionals do their own thing", - _ignored_result - ); + self.store_arg_values(arg, arg_values, matcher)?; + if let Some(_parse_result) = _parse_result { + if _parse_result != ParseResult::ValuesDone { + debug!( + "Parser::get_matches_with: Ignoring state {:?}; positionals do their own thing", + _parse_result + ); + } + } // Only increment the positional counter if it doesn't allow multiples if !arg.is_multiple() { @@ -948,13 +968,21 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { // We assume this case is valid: require equals, but min_vals == 0. if !arg.default_missing_vals.is_empty() { debug!("Parser::parse_opt_value: has default_missing_vals"); + let mut arg_values = Vec::new(); for v in arg.default_missing_vals.iter() { - let _ignored_result = - self.add_val_to_arg(arg, &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); + let _parse_result = self.push_arg_values( + arg, + &RawOsStr::new(v), + trailing_values, + &mut arg_values, + ); + if let Some(_parse_result) = _parse_result { + if _parse_result != ParseResult::ValuesDone { + debug!("Parser::parse_opt_value: Ignoring state {:?}; no values accepted after default_missing_values", _parse_result); + } } } + self.store_arg_values(arg, arg_values, matcher)?; }; if attached_value.is_some() { Ok(ParseResult::AttachedValueNotConsumed) @@ -968,13 +996,22 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { }) } } else if let Some(v) = attached_value { + let mut arg_values = Vec::new(); + let parse_result = self.push_arg_values(arg, v, trailing_values, &mut arg_values); self.start_occurrence_of_arg(matcher, arg); - let mut val_result = self.add_val_to_arg(arg, v, matcher, trailing_values)?; - if val_result != ParseResult::ValuesDone { - debug!("Parser::parse_opt_value: Overriding state {:?}; no values accepted after attached", val_result); - val_result = ParseResult::ValuesDone; + self.store_arg_values(arg, arg_values, matcher)?; + let mut parse_result = parse_result.unwrap_or_else(|| { + if matcher.needs_more_vals(arg) { + ParseResult::Opt(arg.id.clone()) + } else { + ParseResult::ValuesDone + } + }); + if parse_result != ParseResult::ValuesDone { + debug!("Parser::parse_opt_value: Overriding state {:?}; no values accepted after attached", parse_result); + parse_result = ParseResult::ValuesDone; } - Ok(val_result) + Ok(parse_result) } else { debug!("Parser::parse_opt_value: More arg vals required..."); self.start_occurrence_of_arg(matcher, arg); @@ -982,16 +1019,16 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { } } - fn add_val_to_arg( + fn push_arg_values( &self, arg: &Arg<'help>, val: &RawOsStr, - matcher: &mut ArgMatcher, trailing_values: bool, - ) -> ClapResult { - debug!("Parser::add_val_to_arg; arg={}, val={:?}", arg.name, val); + output: &mut Vec, + ) -> Option { + debug!("Parser::push_arg_values; arg={}, val={:?}", arg.name, val); debug!( - "Parser::add_val_to_arg; trailing_values={:?}, DontDelimTrailingVals={:?}", + "Parser::push_arg_values; trailing_values={:?}, DontDelimTrailingVals={:?}", trailing_values, self.cmd.is_dont_delimit_trailing_values_set() ); @@ -1005,51 +1042,53 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { 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); + return Some(ParseResult::ValuesDone); } - self.add_single_val_to_arg(arg, raw_val, matcher)?; + output.push(raw_val); } // Delimited values are always considered the final value - Ok(ParseResult::ValuesDone) + Some(ParseResult::ValuesDone) + } + _ if Some(val) == arg.terminator.map(RawOsStr::from_str) => { + Some(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)?; + output.push(val.to_os_str().into_owned()); if arg.is_require_value_delimiter_set() { - Ok(ParseResult::ValuesDone) - } else if matcher.needs_more_vals(arg) { - Ok(ParseResult::Opt(arg.id.clone())) + Some(ParseResult::ValuesDone) } else { - Ok(ParseResult::ValuesDone) + None } } } } - fn add_single_val_to_arg( + fn store_arg_values( &self, arg: &Arg<'help>, - raw_val: OsString, + raw_vals: Vec, matcher: &mut ArgMatcher, ) -> ClapResult<()> { - debug!("Parser::add_single_val_to_arg: adding val...{:?}", raw_val); + debug!("Parser::store_arg_values: {:?}", raw_vals); - // update the current index because each value is a distinct index to clap - self.cur_idx.set(self.cur_idx.get() + 1); - debug!( - "Parser::add_single_val_to_arg: cur_idx:={}", - self.cur_idx.get() - ); - let value_parser = arg.get_value_parser(); - let val = value_parser.parse_ref(self.cmd, Some(arg), &raw_val)?; + for raw_val in raw_vals { + // update the current index because each value is a distinct index to clap + self.cur_idx.set(self.cur_idx.get() + 1); + debug!( + "Parser::add_single_val_to_arg: cur_idx:={}", + self.cur_idx.get() + ); + let value_parser = arg.get_value_parser(); + let val = value_parser.parse_ref(self.cmd, Some(arg), &raw_val)?; - // 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()); - } + // 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()); + } - matcher.add_val_to(&arg.id, val, raw_val); - matcher.add_index_to(&arg.id, self.cur_idx.get()); + matcher.add_val_to(&arg.id, val, raw_val); + matcher.add_index_to(&arg.id, self.cur_idx.get()); + } Ok(()) } @@ -1133,11 +1172,15 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { "Parser::add_env: Found an opt with value={:?}, trailing={:?}", val, trailing_values ); + let mut arg_values = Vec::new(); + let _parse_result = + self.push_arg_values(arg, &val, trailing_values, &mut arg_values); self.start_custom_arg(matcher, arg, ValueSource::EnvVariable); - 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); + self.store_arg_values(arg, arg_values, matcher)?; + if let Some(_parse_result) = _parse_result { + if _parse_result != ParseResult::ValuesDone { + debug!("Parser::add_env: Ignoring state {:?}; env variables are outside of the parse loop", _parse_result); + } } } else { match arg.get_action() { @@ -1197,14 +1240,22 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { arg.name ); // The flag occurred, we just want to add the val groups - self.start_custom_arg(matcher, arg, ValueSource::CommandLine); + let mut arg_values = Vec::new(); for v in arg.default_missing_vals.iter() { - 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); + let _parse_result = self.push_arg_values( + arg, + &RawOsStr::new(v), + trailing_values, + &mut arg_values, + ); + if let Some(_parse_result) = _parse_result { + if _parse_result != ParseResult::ValuesDone { + debug!("Parser::add_default_value: Ignoring state {:?}; defaults are outside of the parse loop", _parse_result); + } } } + self.start_custom_arg(matcher, arg, ValueSource::CommandLine); + self.store_arg_values(arg, arg_values, matcher)?; } None => { debug!("Parser::add_default_value:iter:{}: wasn't used", arg.name); @@ -1243,15 +1294,19 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { if add { if let Some(default) = default { - self.start_custom_arg(matcher, arg, ValueSource::DefaultValue); - let _ignored_result = self.add_val_to_arg( + let mut arg_values = Vec::new(); + let _parse_result = self.push_arg_values( 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); + &mut arg_values, + ); + self.start_custom_arg(matcher, arg, ValueSource::DefaultValue); + self.store_arg_values(arg, arg_values, matcher)?; + if let Some(_parse_result) = _parse_result { + if _parse_result != ParseResult::ValuesDone { + debug!("Parser::add_default_value: Ignoring state {:?}; defaults are outside of the parse loop", _parse_result); + } } } return Ok(()); @@ -1272,15 +1327,22 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { // do nothing } else { debug!("Parser::add_default_value:iter:{}: wasn't used", arg.name); - - self.start_custom_arg(matcher, arg, ValueSource::DefaultValue); + let mut arg_values = Vec::new(); for v in arg.default_vals.iter() { - 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); + let _parse_result = self.push_arg_values( + arg, + &RawOsStr::new(v), + trailing_values, + &mut arg_values, + ); + if let Some(_parse_result) = _parse_result { + if _parse_result != ParseResult::ValuesDone { + debug!("Parser::add_default_value: Ignoring state {:?}; defaults are outside of the parse loop", _parse_result); + } } } + self.start_custom_arg(matcher, arg, ValueSource::DefaultValue); + self.store_arg_values(arg, arg_values, matcher)?; } } else { debug!( From b862fe227a53b97b13dcd33892e08319e2914708 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 27 May 2022 16:30:38 -0500 Subject: [PATCH 03/19] refactor(parser): Generalize the performing of actions --- src/parser/parser.rs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/parser/parser.rs b/src/parser/parser.rs index c9ce5a03b28..165b580c7fe 100644 --- a/src/parser/parser.rs +++ b/src/parser/parser.rs @@ -794,7 +794,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { }) } else { debug!("Parser::parse_long_arg({:?}): Presence validated", long_arg); - self.parse_flag(Identifier::Long(long_arg), arg, matcher) + self.react(Identifier::Long(long_arg), arg, matcher) } } else if let Some(sc_name) = self.possible_long_flag_subcommand(long_arg) { Ok(ParseResult::FlagSubCommand(sc_name.to_string())) @@ -891,7 +891,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { ); *valid_arg_found = true; if !arg.is_takes_value_set() { - ret = self.parse_flag(Identifier::Short(c), arg, matcher)?; + ret = self.react(Identifier::Short(c), arg, matcher)?; continue; } @@ -1093,13 +1093,16 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { Ok(()) } - fn parse_flag( + fn react( &self, ident: Identifier, arg: &Arg<'help>, matcher: &mut ArgMatcher, ) -> ClapResult { - debug!("Parser::parse_flag"); + debug!( + "Parser::react action={:?}, identifier={:?}", + arg.get_action, ident + ); match arg.get_action() { Action::StoreValue => unreachable!("{:?} is not a flag", arg.get_id()), Action::Flag => { From e1c5cba5f9d4b4ac4ad3cba983e7f3bd7f8b5683 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 27 May 2022 16:50:28 -0500 Subject: [PATCH 04/19] refactor(parser): Extract an occurrence into a reaction --- src/parser/parser.rs | 31 +++++++++++++++++++++++-------- 1 file changed, 23 insertions(+), 8 deletions(-) diff --git a/src/parser/parser.rs b/src/parser/parser.rs index 165b580c7fe..44eaebfa0fc 100644 --- a/src/parser/parser.rs +++ b/src/parser/parser.rs @@ -763,6 +763,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { }; if let Some((long_arg, arg)) = arg { + let ident = Identifier::Long(long_arg); *valid_arg_found = true; if arg.is_takes_value_set() { debug!( @@ -770,7 +771,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { long_arg, &long_value ); let has_eq = long_value.is_some(); - self.parse_opt_value(long_value, arg, matcher, trailing_values, has_eq) + self.parse_opt_value(ident, long_value, arg, matcher, trailing_values, has_eq) } else if let Some(rest) = long_value { let required = self.cmd.required_graph(); debug!( @@ -794,7 +795,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { }) } else { debug!("Parser::parse_long_arg({:?}): Presence validated", long_arg); - self.react(Identifier::Long(long_arg), arg, matcher) + self.react(ident, arg, vec![], matcher) } } else if let Some(sc_name) = self.possible_long_flag_subcommand(long_arg) { Ok(ParseResult::FlagSubCommand(sc_name.to_string())) @@ -885,13 +886,14 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { // Option: -o // Value: val if let Some(arg) = self.cmd.get_keymap().get(&c) { + let ident = Identifier::Short(c); debug!( "Parser::parse_short_arg:iter:{}: Found valid opt or flag", c ); *valid_arg_found = true; if !arg.is_takes_value_set() { - ret = self.react(Identifier::Short(c), arg, matcher)?; + ret = self.react(ident, arg, vec![], matcher)?; continue; } @@ -917,7 +919,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { } else { (val, false) }; - match self.parse_opt_value(val, arg, matcher, trailing_values, has_eq)? { + match self.parse_opt_value(ident, val, arg, matcher, trailing_values, has_eq)? { ParseResult::AttachedValueNotConsumed => continue, x => return Ok(x), } @@ -947,6 +949,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { fn parse_opt_value( &self, + ident: Identifier, attached_value: Option<&RawOsStr>, arg: &Arg<'help>, matcher: &mut ArgMatcher, @@ -998,8 +1001,8 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { } else if let Some(v) = attached_value { let mut arg_values = Vec::new(); let parse_result = self.push_arg_values(arg, v, trailing_values, &mut arg_values); - self.start_occurrence_of_arg(matcher, arg); - self.store_arg_values(arg, arg_values, matcher)?; + let react_result = self.react(ident, arg, arg_values, matcher)?; + debug_assert_eq!(react_result, ParseResult::ValuesDone); let mut parse_result = parse_result.unwrap_or_else(|| { if matcher.needs_more_vals(arg) { ParseResult::Opt(arg.id.clone()) @@ -1097,6 +1100,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { &self, ident: Identifier, arg: &Arg<'help>, + raw_vals: Vec, matcher: &mut ArgMatcher, ) -> ClapResult { debug!( @@ -1104,14 +1108,24 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { arg.get_action, ident ); match arg.get_action() { - Action::StoreValue => unreachable!("{:?} is not a flag", arg.get_id()), + Action::StoreValue => { + self.start_occurrence_of_arg(matcher, arg); + self.store_arg_values(arg, raw_vals, matcher)?; + if cfg!(debug_assertions) && matcher.needs_more_vals(arg) { + debug!( + "Parser::react not enough values passed in, leaving it to the validator to complain", + ); + } + Ok(ParseResult::ValuesDone) + } Action::Flag => { + debug_assert_eq!(raw_vals, Vec::::new()); self.start_occurrence_of_arg(matcher, arg); matcher.add_index_to(&arg.id, self.cur_idx.get()); - Ok(ParseResult::ValuesDone) } Action::Help => { + debug_assert_eq!(raw_vals, Vec::::new()); let use_long = match ident { Identifier::Long(_) => true, Identifier::Short(_) => false, @@ -1120,6 +1134,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { Err(self.help_err(use_long, Stream::Stdout)) } Action::Version => { + debug_assert_eq!(raw_vals, Vec::::new()); let use_long = match ident { Identifier::Long(_) => true, Identifier::Short(_) => false, From f0b2924f36173c7a0d55cdc86006c5954e8f30bc Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 27 May 2022 16:54:48 -0500 Subject: [PATCH 05/19] refactor(parser): Match default_missing_vals to rest --- src/parser/parser.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/parser/parser.rs b/src/parser/parser.rs index 44eaebfa0fc..629678d54c6 100644 --- a/src/parser/parser.rs +++ b/src/parser/parser.rs @@ -967,11 +967,10 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { if arg.is_require_equals_set() && !has_eq { if arg.min_vals == Some(0) { debug!("Requires equals, but min_vals == 0"); - self.start_occurrence_of_arg(matcher, arg); + let mut arg_values = Vec::new(); // We assume this case is valid: require equals, but min_vals == 0. if !arg.default_missing_vals.is_empty() { debug!("Parser::parse_opt_value: has default_missing_vals"); - let mut arg_values = Vec::new(); for v in arg.default_missing_vals.iter() { let _parse_result = self.push_arg_values( arg, @@ -985,8 +984,9 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { } } } - self.store_arg_values(arg, arg_values, matcher)?; }; + self.start_occurrence_of_arg(matcher, arg); + self.store_arg_values(arg, arg_values, matcher)?; if attached_value.is_some() { Ok(ParseResult::AttachedValueNotConsumed) } else { From a98075e9cd037ff8e04a3288ff38e301c79d004c Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 27 May 2022 16:56:52 -0500 Subject: [PATCH 06/19] refactor(parser); Switch default_missing_vals to actions --- src/parser/parser.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/parser/parser.rs b/src/parser/parser.rs index 629678d54c6..c3e942ecb36 100644 --- a/src/parser/parser.rs +++ b/src/parser/parser.rs @@ -985,8 +985,8 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { } } }; - self.start_occurrence_of_arg(matcher, arg); - self.store_arg_values(arg, arg_values, matcher)?; + let react_result = self.react(ident, arg, arg_values, matcher)?; + debug_assert_eq!(react_result, ParseResult::ValuesDone); if attached_value.is_some() { Ok(ParseResult::AttachedValueNotConsumed) } else { From dc8a7d420e45839d42aedcb7fb40c20d9f29c070 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 27 May 2022 19:33:27 -0500 Subject: [PATCH 07/19] refactor(parser): Switch defaults/envs to actions There is a default_missing_vals case which is slightly different because its not actually a default but closing out the remaining argument that was started in last iteration. --- src/parser/parser.rs | 84 ++++++++++++++++++++++++++++++-------------- 1 file changed, 58 insertions(+), 26 deletions(-) diff --git a/src/parser/parser.rs b/src/parser/parser.rs index c3e942ecb36..7dfb7b07e52 100644 --- a/src/parser/parser.rs +++ b/src/parser/parser.rs @@ -795,7 +795,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { }) } else { debug!("Parser::parse_long_arg({:?}): Presence validated", long_arg); - self.react(ident, arg, vec![], matcher) + self.react(Some(ident), ValueSource::CommandLine, arg, vec![], matcher) } } else if let Some(sc_name) = self.possible_long_flag_subcommand(long_arg) { Ok(ParseResult::FlagSubCommand(sc_name.to_string())) @@ -893,7 +893,8 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { ); *valid_arg_found = true; if !arg.is_takes_value_set() { - ret = self.react(ident, arg, vec![], matcher)?; + ret = + self.react(Some(ident), ValueSource::CommandLine, arg, vec![], matcher)?; continue; } @@ -985,7 +986,13 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { } } }; - let react_result = self.react(ident, arg, arg_values, matcher)?; + let react_result = self.react( + Some(ident), + ValueSource::CommandLine, + arg, + arg_values, + matcher, + )?; debug_assert_eq!(react_result, ParseResult::ValuesDone); if attached_value.is_some() { Ok(ParseResult::AttachedValueNotConsumed) @@ -1001,7 +1008,13 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { } else if let Some(v) = attached_value { let mut arg_values = Vec::new(); let parse_result = self.push_arg_values(arg, v, trailing_values, &mut arg_values); - let react_result = self.react(ident, arg, arg_values, matcher)?; + let react_result = self.react( + Some(ident), + ValueSource::CommandLine, + arg, + arg_values, + matcher, + )?; debug_assert_eq!(react_result, ParseResult::ValuesDone); let mut parse_result = parse_result.unwrap_or_else(|| { if matcher.needs_more_vals(arg) { @@ -1098,18 +1111,25 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { fn react( &self, - ident: Identifier, + ident: Option, + source: ValueSource, arg: &Arg<'help>, raw_vals: Vec, matcher: &mut ArgMatcher, ) -> ClapResult { debug!( - "Parser::react action={:?}, identifier={:?}", - arg.get_action, ident + "Parser::react action={:?}, identifier={:?}, souce={:?}", + arg.get_action(), + ident, + source ); match arg.get_action() { Action::StoreValue => { - self.start_occurrence_of_arg(matcher, arg); + if source == ValueSource::CommandLine { + self.start_occurrence_of_arg(matcher, arg); + } else { + self.start_custom_arg(matcher, arg, source); + } self.store_arg_values(arg, raw_vals, matcher)?; if cfg!(debug_assertions) && matcher.needs_more_vals(arg) { debug!( @@ -1120,15 +1140,20 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { } Action::Flag => { debug_assert_eq!(raw_vals, Vec::::new()); - self.start_occurrence_of_arg(matcher, arg); + if source == ValueSource::CommandLine { + self.start_occurrence_of_arg(matcher, arg); + } else { + self.start_custom_arg(matcher, arg, source); + } matcher.add_index_to(&arg.id, self.cur_idx.get()); Ok(ParseResult::ValuesDone) } Action::Help => { debug_assert_eq!(raw_vals, Vec::::new()); let use_long = match ident { - Identifier::Long(_) => true, - Identifier::Short(_) => false, + Some(Identifier::Long(_)) => true, + Some(Identifier::Short(_)) => false, + None => true, }; debug!("Help: use_long={}", use_long); Err(self.help_err(use_long, Stream::Stdout)) @@ -1136,8 +1161,9 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { Action::Version => { debug_assert_eq!(raw_vals, Vec::::new()); let use_long = match ident { - Identifier::Long(_) => true, - Identifier::Short(_) => false, + Some(Identifier::Long(_)) => true, + Some(Identifier::Short(_)) => false, + None => true, }; debug!("Version: use_long={}", use_long); Err(self.version_err(use_long)) @@ -1193,8 +1219,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { let mut arg_values = Vec::new(); let _parse_result = self.push_arg_values(arg, &val, trailing_values, &mut arg_values); - self.start_custom_arg(matcher, arg, ValueSource::EnvVariable); - self.store_arg_values(arg, arg_values, matcher)?; + let _ = self.react(None, ValueSource::EnvVariable, arg, arg_values, matcher)?; if let Some(_parse_result) = _parse_result { if _parse_result != ParseResult::ValuesDone { debug!("Parser::add_env: Ignoring state {:?}; env variables are outside of the parse loop", _parse_result); @@ -1208,16 +1233,19 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { let predicate = str_to_bool(val.to_str_lossy()); debug!("Parser::add_env: Found boolean literal `{:?}`", predicate); if predicate.unwrap_or(true) { - self.start_custom_arg(matcher, arg, ValueSource::EnvVariable); - matcher.add_index_to(&arg.id, self.cur_idx.get()); + let _ = self.react( + None, + ValueSource::EnvVariable, + arg, + vec![], + matcher, + )?; } } // Early return on `Help` or `Version`. - Action::Help => { - return Err(self.help_err(true, Stream::Stdout)); - } - Action::Version => { - return Err(self.version_err(true)); + Action::Help | Action::Version => { + let _ = + self.react(None, ValueSource::EnvVariable, arg, vec![], matcher)?; } } } @@ -1319,8 +1347,13 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { trailing_values, &mut arg_values, ); - self.start_custom_arg(matcher, arg, ValueSource::DefaultValue); - self.store_arg_values(arg, arg_values, matcher)?; + let _ = self.react( + None, + ValueSource::DefaultValue, + arg, + arg_values, + matcher, + )?; if let Some(_parse_result) = _parse_result { if _parse_result != ParseResult::ValuesDone { debug!("Parser::add_default_value: Ignoring state {:?}; defaults are outside of the parse loop", _parse_result); @@ -1359,8 +1392,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { } } } - self.start_custom_arg(matcher, arg, ValueSource::DefaultValue); - self.store_arg_values(arg, arg_values, matcher)?; + let _ = self.react(None, ValueSource::DefaultValue, arg, arg_values, matcher)?; } } else { debug!( From 2d8a15453edf9c4df5800497af7565ae5c764fc3 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 27 May 2022 19:40:49 -0500 Subject: [PATCH 08/19] refactor(parser): Be more explicit that default missing values is not escaped --- src/parser/parser.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/parser/parser.rs b/src/parser/parser.rs index 7dfb7b07e52..bb413b1db85 100644 --- a/src/parser/parser.rs +++ b/src/parser/parser.rs @@ -973,6 +973,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { if !arg.default_missing_vals.is_empty() { debug!("Parser::parse_opt_value: has default_missing_vals"); for v in arg.default_missing_vals.iter() { + let trailing_values = false; // CLI should not be affecting default_missing_values let _parse_result = self.push_arg_values( arg, &RawOsStr::new(v), From 0b5de2198ee932742f655ccca87afedd99610f79 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 27 May 2022 19:50:47 -0500 Subject: [PATCH 09/19] refactor(parser): Don't track the actual identifier When creating `PendingValues`, I can't have the lifetime. I could make it a `Cow` but decided to hold off instead since we don't need this right now. Maybe by the time we do need it, we'll have another way of doing this. --- src/parser/parser.rs | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/parser/parser.rs b/src/parser/parser.rs index bb413b1db85..d53468ae4a8 100644 --- a/src/parser/parser.rs +++ b/src/parser/parser.rs @@ -762,8 +762,8 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { None }; - if let Some((long_arg, arg)) = arg { - let ident = Identifier::Long(long_arg); + if let Some((_long_arg, arg)) = arg { + let ident = Identifier::Long; *valid_arg_found = true; if arg.is_takes_value_set() { debug!( @@ -886,7 +886,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { // Option: -o // Value: val if let Some(arg) = self.cmd.get_keymap().get(&c) { - let ident = Identifier::Short(c); + let ident = Identifier::Short; debug!( "Parser::parse_short_arg:iter:{}: Found valid opt or flag", c @@ -1152,8 +1152,8 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { Action::Help => { debug_assert_eq!(raw_vals, Vec::::new()); let use_long = match ident { - Some(Identifier::Long(_)) => true, - Some(Identifier::Short(_)) => false, + Some(Identifier::Long) => true, + Some(Identifier::Short) => false, None => true, }; debug!("Help: use_long={}", use_long); @@ -1162,8 +1162,8 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { Action::Version => { debug_assert_eq!(raw_vals, Vec::::new()); let use_long = match ident { - Some(Identifier::Long(_)) => true, - Some(Identifier::Short(_)) => false, + Some(Identifier::Long) => true, + Some(Identifier::Short) => false, None => true, }; debug!("Version: use_long={}", use_long); @@ -1542,7 +1542,7 @@ enum ParseResult { } #[derive(Copy, Clone, Debug, PartialEq, Eq)] -enum Identifier<'f> { - Short(char), - Long(&'f str), +enum Identifier { + Short, + Long, } From 6f5aaab44313b5924c0fca4e95a51f2e29571b90 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 27 May 2022 20:10:53 -0500 Subject: [PATCH 10/19] refactor(parser): Allow more match-state --- src/parser/arg_matcher.rs | 78 +++++++++++++++++++++------------------ 1 file changed, 42 insertions(+), 36 deletions(-) diff --git a/src/parser/arg_matcher.rs b/src/parser/arg_matcher.rs index e162a16f14f..7e79eafca37 100644 --- a/src/parser/arg_matcher.rs +++ b/src/parser/arg_matcher.rs @@ -12,35 +12,39 @@ use crate::util::Id; use crate::INTERNAL_ERROR_MSG; #[derive(Debug, Default)] -pub(crate) struct ArgMatcher(ArgMatches); +pub(crate) struct ArgMatcher { + matches: ArgMatches, +} impl ArgMatcher { pub(crate) fn new(_cmd: &Command) -> Self { - ArgMatcher(ArgMatches { - #[cfg(debug_assertions)] - valid_args: { - let args = _cmd.get_arguments().map(|a| a.id.clone()); - let groups = _cmd.get_groups().map(|g| g.id.clone()); - args.chain(groups).collect() + ArgMatcher { + matches: ArgMatches { + #[cfg(debug_assertions)] + valid_args: { + let args = _cmd.get_arguments().map(|a| a.id.clone()); + let groups = _cmd.get_groups().map(|g| g.id.clone()); + args.chain(groups).collect() + }, + #[cfg(debug_assertions)] + valid_subcommands: _cmd.get_subcommands().map(|sc| sc.get_id()).collect(), + // HACK: Allow an external subcommand's ArgMatches be a stand-in for any ArgMatches + // since users can't detect it and avoid the asserts. + // + // See clap-rs/clap#3263 + #[cfg(debug_assertions)] + #[cfg(not(feature = "unstable-v4"))] + disable_asserts: _cmd.is_allow_external_subcommands_set(), + #[cfg(debug_assertions)] + #[cfg(feature = "unstable-v4")] + disable_asserts: false, + ..Default::default() }, - #[cfg(debug_assertions)] - valid_subcommands: _cmd.get_subcommands().map(|sc| sc.get_id()).collect(), - // HACK: Allow an external subcommand's ArgMatches be a stand-in for any ArgMatches - // since users can't detect it and avoid the asserts. - // - // See clap-rs/clap#3263 - #[cfg(debug_assertions)] - #[cfg(not(feature = "unstable-v4"))] - disable_asserts: _cmd.is_allow_external_subcommands_set(), - #[cfg(debug_assertions)] - #[cfg(feature = "unstable-v4")] - disable_asserts: false, - ..Default::default() - }) + } } pub(crate) fn into_inner(self) -> ArgMatches { - self.0 + self.matches } pub(crate) fn propagate_globals(&mut self, global_arg_vec: &[Id]) { @@ -78,51 +82,53 @@ impl ArgMatcher { vals_map.insert(global_arg.clone(), to_update); } } - if let Some(ref mut sc) = self.0.subcommand { - let mut am = ArgMatcher(mem::take(&mut sc.matches)); + if let Some(ref mut sc) = self.matches.subcommand { + let mut am = ArgMatcher { + matches: mem::take(&mut sc.matches), + }; am.fill_in_global_values(global_arg_vec, vals_map); - mem::swap(&mut am.0, &mut sc.matches); + mem::swap(&mut am.matches, &mut sc.matches); } for (name, matched_arg) in vals_map.iter_mut() { - self.0.args.insert(name.clone(), matched_arg.clone()); + self.matches.args.insert(name.clone(), matched_arg.clone()); } } pub(crate) fn get(&self, arg: &Id) -> Option<&MatchedArg> { - self.0.args.get(arg) + self.matches.args.get(arg) } pub(crate) fn get_mut(&mut self, arg: &Id) -> Option<&mut MatchedArg> { - self.0.args.get_mut(arg) + self.matches.args.get_mut(arg) } pub(crate) fn remove(&mut self, arg: &Id) { - self.0.args.swap_remove(arg); + self.matches.args.swap_remove(arg); } pub(crate) fn contains(&self, arg: &Id) -> bool { - self.0.args.contains_key(arg) + self.matches.args.contains_key(arg) } pub(crate) fn arg_names(&self) -> indexmap::map::Keys { - self.0.args.keys() + self.matches.args.keys() } pub(crate) fn entry(&mut self, arg: &Id) -> indexmap::map::Entry { - self.0.args.entry(arg.clone()) + self.matches.args.entry(arg.clone()) } pub(crate) fn subcommand(&mut self, sc: SubCommand) { - self.0.subcommand = Some(Box::new(sc)); + self.matches.subcommand = Some(Box::new(sc)); } pub(crate) fn subcommand_name(&self) -> Option<&str> { - self.0.subcommand_name() + self.matches.subcommand_name() } pub(crate) fn iter(&self) -> indexmap::map::Iter { - self.0.args.iter() + self.matches.args.iter() } pub(crate) fn check_explicit<'a>(&self, arg: &Id, predicate: ArgPredicate<'a>) -> bool { @@ -226,6 +232,6 @@ impl Deref for ArgMatcher { type Target = ArgMatches; fn deref(&self) -> &Self::Target { - &self.0 + &self.matches } } From bf7259d6441573bbc8cefc86913b16e1e76b456f Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 30 May 2022 18:39:36 -0500 Subject: [PATCH 11/19] refactor(parser): Switch positionals to actions --- src/parser/arg_matcher.rs | 28 ++++++++++++++++++++ src/parser/mod.rs | 2 ++ src/parser/parser.rs | 56 ++++++++++++++++++++++++++++++--------- 3 files changed, 73 insertions(+), 13 deletions(-) diff --git a/src/parser/arg_matcher.rs b/src/parser/arg_matcher.rs index 7e79eafca37..2e818ad4152 100644 --- a/src/parser/arg_matcher.rs +++ b/src/parser/arg_matcher.rs @@ -7,6 +7,8 @@ use std::ops::Deref; // Internal use crate::builder::{Arg, ArgPredicate, Command}; use crate::parser::AnyValue; +use crate::parser::Identifier; +use crate::parser::PendingArg; use crate::parser::{ArgMatches, MatchedArg, SubCommand, ValueSource}; use crate::util::Id; use crate::INTERNAL_ERROR_MSG; @@ -14,6 +16,7 @@ use crate::INTERNAL_ERROR_MSG; #[derive(Debug, Default)] pub(crate) struct ArgMatcher { matches: ArgMatches, + pending: Option, } impl ArgMatcher { @@ -40,6 +43,7 @@ impl ArgMatcher { disable_asserts: false, ..Default::default() }, + pending: None, } } @@ -85,6 +89,7 @@ impl ArgMatcher { if let Some(ref mut sc) = self.matches.subcommand { let mut am = ArgMatcher { matches: mem::take(&mut sc.matches), + pending: None, }; am.fill_in_global_values(global_arg_vec, vals_map); mem::swap(&mut am.matches, &mut sc.matches); @@ -226,6 +231,29 @@ impl ArgMatcher { } true } + + pub(crate) fn pending_arg_id(&self) -> Option<&Id> { + self.pending.as_ref().map(|p| &p.id) + } + + pub(crate) fn pending_values_mut( + &mut self, + id: &Id, + ident: Option, + ) -> &mut Vec { + let pending = self.pending.get_or_insert_with(|| PendingArg { + id: id.clone(), + ident, + raw_vals: Default::default(), + }); + debug_assert_eq!(pending.id, *id, "{}", INTERNAL_ERROR_MSG); + debug_assert_eq!(pending.ident, ident, "{}", INTERNAL_ERROR_MSG); + &mut pending.raw_vals + } + + pub(crate) fn take_pending(&mut self) -> Option { + self.pending.take() + } } impl Deref for ArgMatcher { diff --git a/src/parser/mod.rs b/src/parser/mod.rs index e53f9ecfde3..da81648e1fc 100644 --- a/src/parser/mod.rs +++ b/src/parser/mod.rs @@ -13,6 +13,8 @@ pub(crate) use self::arg_matcher::ArgMatcher; pub(crate) use self::matches::AnyValue; pub(crate) use self::matches::AnyValueId; pub(crate) use self::matches::{MatchedArg, SubCommand}; +pub(crate) use self::parser::Identifier; +pub(crate) use self::parser::PendingArg; pub(crate) use self::parser::{ParseState, Parser}; pub(crate) use self::validator::Validator; diff --git a/src/parser/parser.rs b/src/parser/parser.rs index d53468ae4a8..09174843e29 100644 --- a/src/parser/parser.rs +++ b/src/parser/parser.rs @@ -385,17 +385,12 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { trailing_values = true; } - let mut arg_values = Vec::new(); - let _parse_result = self.push_arg_values( - arg, - arg_os.to_value_os(), - trailing_values, - &mut arg_values, - ); - if !arg.is_multiple_values_set() || !matcher.contains(&arg.id) { - self.start_occurrence_of_arg(matcher, arg); + if matcher.pending_arg_id() != Some(&arg.id) || !arg.is_multiple_values_set() { + self.resolve_pending(matcher)?; } - self.store_arg_values(arg, arg_values, matcher)?; + let arg_values = matcher.pending_values_mut(&arg.id, Some(Identifier::Index)); + let _parse_result = + self.push_arg_values(arg, arg_os.to_value_os(), trailing_values, arg_values); if let Some(_parse_result) = _parse_result { if _parse_result != ParseResult::ValuesDone { debug!( @@ -445,6 +440,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { matches: sc_m.into_inner(), }); + self.resolve_pending(matcher)?; #[cfg(feature = "env")] self.add_env(matcher)?; self.add_defaults(matcher)?; @@ -465,6 +461,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { self.parse_subcommand(&sc_name, matcher, raw_args, args_cursor, keep_state)?; } + self.resolve_pending(matcher)?; #[cfg(feature = "env")] self.add_env(matcher)?; self.add_defaults(matcher)?; @@ -1110,6 +1107,27 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { Ok(()) } + fn resolve_pending(&self, matcher: &mut ArgMatcher) -> ClapResult<()> { + let pending = match matcher.take_pending() { + Some(pending) => pending, + None => { + return Ok(()); + } + }; + + debug!("Parser::resolve_pending: id={:?}", pending.id); + let arg = self.cmd.find(&pending.id).expect(INTERNAL_ERROR_MSG); + let _ = self.react( + pending.ident, + ValueSource::CommandLine, + arg, + pending.raw_vals, + matcher, + )?; + + Ok(()) + } + fn react( &self, ident: Option, @@ -1118,6 +1136,8 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { raw_vals: Vec, matcher: &mut ArgMatcher, ) -> ClapResult { + self.resolve_pending(matcher)?; + debug!( "Parser::react action={:?}, identifier={:?}, souce={:?}", arg.get_action(), @@ -1154,6 +1174,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { let use_long = match ident { Some(Identifier::Long) => true, Some(Identifier::Short) => false, + Some(Identifier::Index) => true, None => true, }; debug!("Help: use_long={}", use_long); @@ -1164,6 +1185,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { let use_long = match ident { Some(Identifier::Long) => true, Some(Identifier::Short) => false, + Some(Identifier::Index) => true, None => true, }; debug!("Version: use_long={}", use_long); @@ -1256,7 +1278,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { Ok(()) } - fn add_defaults(&mut self, matcher: &mut ArgMatcher) -> ClapResult<()> { + fn add_defaults(&self, matcher: &mut ArgMatcher) -> ClapResult<()> { debug!("Parser::add_defaults"); for arg in self.cmd.get_opts() { @@ -1504,7 +1526,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { } } -#[derive(Debug)] +#[derive(Debug, PartialEq, Eq)] pub(crate) enum ParseState { ValuesDone, Opt(Id), @@ -1541,8 +1563,16 @@ enum ParseResult { NoArg, } +#[derive(Clone, Debug, PartialEq, Eq)] +pub(crate) struct PendingArg { + pub(crate) id: Id, + pub(crate) ident: Option, + pub(crate) raw_vals: Vec, +} + #[derive(Copy, Clone, Debug, PartialEq, Eq)] -enum Identifier { +pub(crate) enum Identifier { Short, Long, + Index, } From bba83cb2afbfc90f3a08ece33956cd27c1b4a4c1 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 31 May 2022 13:52:34 -0500 Subject: [PATCH 12/19] test(parser): Verify interleaved group behavior --- tests/builder/grouped_values.rs | 46 +++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/tests/builder/grouped_values.rs b/tests/builder/grouped_values.rs index 8b284dcdab2..1a2c617053e 100644 --- a/tests/builder/grouped_values.rs +++ b/tests/builder/grouped_values.rs @@ -174,6 +174,52 @@ fn grouped_value_multiple_positional_arg_last_multiple() { ); } +#[test] +fn grouped_interleaved_positional_values() { + let cmd = clap::Command::new("foo") + .arg(clap::Arg::new("pos").multiple_values(true)) + .arg( + clap::Arg::new("flag") + .short('f') + .long("flag") + .takes_value(true) + .multiple_occurrences(true), + ); + + let m = cmd + .try_get_matches_from(["foo", "1", "2", "-f", "a", "3", "-f", "b", "4"]) + .unwrap(); + let pos: Vec<_> = m.grouped_values_of("pos").unwrap().collect(); + assert_eq!(pos, vec![vec!["1", "2", "3", "4"]]); + assert_eq!(m.occurrences_of("pos"), 1); + let flag: Vec<_> = m.grouped_values_of("flag").unwrap().collect(); + assert_eq!(flag, vec![vec!["a"], vec!["b"]]); + assert_eq!(m.occurrences_of("flag"), 2); +} + +#[test] +fn grouped_interleaved_positional_occurrences() { + let cmd = clap::Command::new("foo") + .arg(clap::Arg::new("pos").multiple_occurrences(true)) + .arg( + clap::Arg::new("flag") + .short('f') + .long("flag") + .takes_value(true) + .multiple_occurrences(true), + ); + + let m = cmd + .try_get_matches_from(["foo", "1", "2", "-f", "a", "3", "-f", "b", "4"]) + .unwrap(); + let pos: Vec<_> = m.grouped_values_of("pos").unwrap().collect(); + assert_eq!(pos, vec![vec!["1"], vec!["2"], vec!["3"], vec!["4"]]); + assert_eq!(m.occurrences_of("pos"), 4); + let flag: Vec<_> = m.grouped_values_of("flag").unwrap().collect(); + assert_eq!(flag, vec![vec!["a"], vec!["b"]]); + assert_eq!(m.occurrences_of("flag"), 2); +} + #[test] fn issue_1374() { let cmd = Command::new("MyApp").arg( From f2a219e77d6769dd9089307f2df31d70bbc5060d Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 31 May 2022 12:19:00 -0500 Subject: [PATCH 13/19] refactor(parser)!: Switch flag values to Actions This changes how occurrences and values are grouped for multiple values. Today, it appears as a bug. If we move forward with #3772, then this can make sense. --- src/parser/arg_matcher.rs | 49 ++++++++++++++++++++------------- src/parser/parser.rs | 35 ++++++++++++----------- tests/builder/grouped_values.rs | 4 +-- 3 files changed, 51 insertions(+), 37 deletions(-) diff --git a/src/parser/arg_matcher.rs b/src/parser/arg_matcher.rs index 2e818ad4152..eab4999189f 100644 --- a/src/parser/arg_matcher.rs +++ b/src/parser/arg_matcher.rs @@ -210,26 +210,35 @@ impl ArgMatcher { } 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) { - let current_num = ma.num_vals(); - if let Some(num) = o.num_vals { - debug!("ArgMatcher::needs_more_vals: num_vals...{}", num); - return if o.is_multiple_occurrences_set() { - (current_num % num) != 0 - } else { - num != current_num - }; - } else if let Some(num) = o.max_vals { - debug!("ArgMatcher::needs_more_vals: max_vals...{}", num); - return current_num < num; - } else if o.min_vals.is_some() { - debug!("ArgMatcher::needs_more_vals: min_vals...true"); - return true; + let num_resolved = self.get(&o.id).map(|ma| ma.num_vals()).unwrap_or(0); + let num_pending = self + .pending + .as_ref() + .and_then(|p| (p.id == o.id).then(|| p.raw_vals.len())) + .unwrap_or(0); + let current_num = num_resolved + num_pending; + debug!( + "ArgMatcher::needs_more_vals: o={}, resolved={}, pending={}", + o.name, num_resolved, num_pending + ); + if current_num == 0 { + true + } else if let Some(num) = o.num_vals { + debug!("ArgMatcher::needs_more_vals: num_vals...{}", num); + if o.is_multiple_occurrences_set() { + (current_num % num) != 0 + } else { + num != current_num } - return o.is_multiple_values_set(); + } else if let Some(num) = o.max_vals { + debug!("ArgMatcher::needs_more_vals: max_vals...{}", num); + current_num < num + } else if o.min_vals.is_some() { + debug!("ArgMatcher::needs_more_vals: min_vals...true"); + true + } else { + o.is_multiple_values_set() } - true } pub(crate) fn pending_arg_id(&self) -> Option<&Id> { @@ -247,7 +256,9 @@ impl ArgMatcher { raw_vals: Default::default(), }); debug_assert_eq!(pending.id, *id, "{}", INTERNAL_ERROR_MSG); - debug_assert_eq!(pending.ident, ident, "{}", INTERNAL_ERROR_MSG); + if ident.is_some() { + debug_assert_eq!(pending.ident, ident, "{}", INTERNAL_ERROR_MSG); + } &mut pending.raw_vals } diff --git a/src/parser/parser.rs b/src/parser/parser.rs index 09174843e29..cc6d0e4085f 100644 --- a/src/parser/parser.rs +++ b/src/parser/parser.rs @@ -345,15 +345,14 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { // Assume this is a value of a previous arg. // get the option so we can check the settings - let mut arg_values = Vec::new(); + let arg_values = matcher.pending_values_mut(&id, None); let arg = &self.cmd[id]; let parse_result = self.push_arg_values( arg, arg_os.to_value_os(), trailing_values, - &mut arg_values, + arg_values, ); - self.store_arg_values(arg, arg_values, matcher)?; let parse_result = parse_result.unwrap_or_else(|| { if matcher.needs_more_vals(arg) { ParseResult::Opt(arg.id.clone()) @@ -720,10 +719,6 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { return Ok(ParseResult::MaybeHyphenValue); } - // Update the current index - self.cur_idx.set(self.cur_idx.get() + 1); - debug!("Parser::parse_long_arg: cur_idx:={}", self.cur_idx.get()); - debug!("Parser::parse_long_arg: Does it contain '='..."); let long_arg = match long_arg { Ok(long_arg) => long_arg, @@ -870,14 +865,6 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { }; debug!("Parser::parse_short_arg:iter:{}", c); - // update each index because `-abcd` is four indices to clap - self.cur_idx.set(self.cur_idx.get() + 1); - debug!( - "Parser::parse_short_arg:iter:{}: cur_idx:={}", - c, - self.cur_idx.get() - ); - // Check for matching short options, and return the name if there is no trailing // concatenated value: -oval // Option: -o @@ -925,6 +912,11 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { return if let Some(sc_name) = self.cmd.find_short_subcmd(c) { debug!("Parser::parse_short_arg:iter:{}: subcommand={}", c, sc_name); + // Make sure indices get updated before reading `self.cur_idx` + self.resolve_pending(matcher)?; + self.cur_idx.set(self.cur_idx.get() + 1); + debug!("Parser::parse_short_arg: cur_idx:={}", self.cur_idx.get()); + let name = sc_name.to_string(); // Get the index of the previously saved flag subcommand in the group of flags (if exists). // If it is a new flag subcommand, then the formentioned index should be the current one @@ -1028,7 +1020,8 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { Ok(parse_result) } else { debug!("Parser::parse_opt_value: More arg vals required..."); - self.start_occurrence_of_arg(matcher, arg); + self.resolve_pending(matcher)?; + matcher.pending_values_mut(&arg.id, Some(ident)); Ok(ParseResult::Opt(arg.id.clone())) } } @@ -1147,6 +1140,11 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { match arg.get_action() { Action::StoreValue => { if source == ValueSource::CommandLine { + if matches!(ident, Some(Identifier::Short) | Some(Identifier::Long)) { + // Record flag's index + self.cur_idx.set(self.cur_idx.get() + 1); + debug!("Parser::react: cur_idx:={}", self.cur_idx.get()); + } self.start_occurrence_of_arg(matcher, arg); } else { self.start_custom_arg(matcher, arg, source); @@ -1162,6 +1160,11 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { Action::Flag => { debug_assert_eq!(raw_vals, Vec::::new()); if source == ValueSource::CommandLine { + if matches!(ident, Some(Identifier::Short) | Some(Identifier::Long)) { + // Record flag's index + self.cur_idx.set(self.cur_idx.get() + 1); + debug!("Parser::react: cur_idx:={}", self.cur_idx.get()); + } self.start_occurrence_of_arg(matcher, arg); } else { self.start_custom_arg(matcher, arg, source); diff --git a/tests/builder/grouped_values.rs b/tests/builder/grouped_values.rs index 1a2c617053e..fbb61d04960 100644 --- a/tests/builder/grouped_values.rs +++ b/tests/builder/grouped_values.rs @@ -190,8 +190,8 @@ fn grouped_interleaved_positional_values() { .try_get_matches_from(["foo", "1", "2", "-f", "a", "3", "-f", "b", "4"]) .unwrap(); let pos: Vec<_> = m.grouped_values_of("pos").unwrap().collect(); - assert_eq!(pos, vec![vec!["1", "2", "3", "4"]]); - assert_eq!(m.occurrences_of("pos"), 1); + assert_eq!(pos, vec![vec!["1", "2"], vec!["3"], vec!["4"]]); + assert_eq!(m.occurrences_of("pos"), 3); let flag: Vec<_> = m.grouped_values_of("flag").unwrap().collect(); assert_eq!(flag, vec![vec!["a"], vec!["b"]]); assert_eq!(m.occurrences_of("flag"), 2); From cb6f7b783ac644f2b08b16ab08262425f5a70553 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 31 May 2022 14:15:55 -0500 Subject: [PATCH 14/19] fix(parser): Restore interleaved positional behavior If we felt this was important long-term, we should fix this outside of the Action. Since we might be changing up occurrences (#3772), we can probably get away with a hack. --- src/parser/parser.rs | 23 +++++++++++++++-------- tests/builder/grouped_values.rs | 4 ++-- 2 files changed, 17 insertions(+), 10 deletions(-) diff --git a/src/parser/parser.rs b/src/parser/parser.rs index cc6d0e4085f..d3ed404b820 100644 --- a/src/parser/parser.rs +++ b/src/parser/parser.rs @@ -1139,15 +1139,22 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { ); match arg.get_action() { Action::StoreValue => { - if source == ValueSource::CommandLine { - if matches!(ident, Some(Identifier::Short) | Some(Identifier::Long)) { - // Record flag's index - self.cur_idx.set(self.cur_idx.get() + 1); - debug!("Parser::react: cur_idx:={}", self.cur_idx.get()); - } - self.start_occurrence_of_arg(matcher, arg); + if ident == Some(Identifier::Index) + && arg.is_multiple_values_set() + && matcher.contains(&arg.id) + { + // HACK: Reuse existing occurrence } else { - self.start_custom_arg(matcher, arg, source); + if source == ValueSource::CommandLine { + if matches!(ident, Some(Identifier::Short) | Some(Identifier::Long)) { + // Record flag's index + self.cur_idx.set(self.cur_idx.get() + 1); + debug!("Parser::react: cur_idx:={}", self.cur_idx.get()); + } + self.start_occurrence_of_arg(matcher, arg); + } else { + self.start_custom_arg(matcher, arg, source); + } } self.store_arg_values(arg, raw_vals, matcher)?; if cfg!(debug_assertions) && matcher.needs_more_vals(arg) { diff --git a/tests/builder/grouped_values.rs b/tests/builder/grouped_values.rs index fbb61d04960..1a2c617053e 100644 --- a/tests/builder/grouped_values.rs +++ b/tests/builder/grouped_values.rs @@ -190,8 +190,8 @@ fn grouped_interleaved_positional_values() { .try_get_matches_from(["foo", "1", "2", "-f", "a", "3", "-f", "b", "4"]) .unwrap(); let pos: Vec<_> = m.grouped_values_of("pos").unwrap().collect(); - assert_eq!(pos, vec![vec!["1", "2"], vec!["3"], vec!["4"]]); - assert_eq!(m.occurrences_of("pos"), 3); + assert_eq!(pos, vec![vec!["1", "2", "3", "4"]]); + assert_eq!(m.occurrences_of("pos"), 1); let flag: Vec<_> = m.grouped_values_of("flag").unwrap().collect(); assert_eq!(flag, vec![vec!["a"], vec!["b"]]); assert_eq!(m.occurrences_of("flag"), 2); From c052a976b8ab0e85e75f98a224319fc27c82ee8c Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 31 May 2022 14:22:22 -0500 Subject: [PATCH 15/19] fix(parser): Qualify the type of action My hope is to add group actions as well, so we need to qualify what kind of action this is. --- src/builder/action.rs | 2 +- src/builder/arg.rs | 8 ++++---- src/builder/command.rs | 8 ++++---- src/builder/mod.rs | 2 +- src/parser/parser.rs | 16 ++++++++-------- 5 files changed, 18 insertions(+), 18 deletions(-) diff --git a/src/builder/action.rs b/src/builder/action.rs index 71da9ccc27e..3d1d1627030 100644 --- a/src/builder/action.rs +++ b/src/builder/action.rs @@ -1,6 +1,6 @@ /// Behavior of arguments when they are encountered while parsing #[derive(Clone, Debug, PartialEq, Eq)] -pub(crate) enum Action { +pub(crate) enum ArgAction { StoreValue, Flag, Help, diff --git a/src/builder/arg.rs b/src/builder/arg.rs index 46676adbd94..3b2469f894e 100644 --- a/src/builder/arg.rs +++ b/src/builder/arg.rs @@ -18,7 +18,7 @@ use yaml_rust::Yaml; // Internal use crate::builder::usage_parser::UsageParser; -use crate::builder::Action; +use crate::builder::ArgAction; use crate::builder::ArgPredicate; use crate::util::{Id, Key}; use crate::PossibleValue; @@ -64,7 +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) action: Option, pub(crate) value_parser: Option, pub(crate) blacklist: Vec, pub(crate) settings: ArgFlags, @@ -4531,8 +4531,8 @@ impl<'help> Arg<'help> { } /// Behavior when parsing the argument - pub(crate) fn get_action(&self) -> &super::Action { - const DEFAULT: super::Action = super::Action::StoreValue; + pub(crate) fn get_action(&self) -> &super::ArgAction { + const DEFAULT: super::ArgAction = super::ArgAction::StoreValue; self.action.as_ref().unwrap_or(&DEFAULT) } diff --git a/src/builder/command.rs b/src/builder/command.rs index 0a7ce146bc4..8fbe5bc0622 100644 --- a/src/builder/command.rs +++ b/src/builder/command.rs @@ -4150,16 +4150,16 @@ impl<'help> App<'help> { // 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; + let action = super::ArgAction::Help; a.action = Some(action); } else if a.get_id() == "version" && auto_version { - let action = super::Action::Version; + let action = super::ArgAction::Version; a.action = Some(action); } else if a.is_takes_value_set() { - let action = super::Action::StoreValue; + let action = super::ArgAction::StoreValue; a.action = Some(action); } else { - let action = super::Action::Flag; + let action = super::ArgAction::Flag; a.action = Some(action); } } diff --git a/src/builder/mod.rs b/src/builder/mod.rs index 10e8bdff980..ad74e4020d8 100644 --- a/src/builder/mod.rs +++ b/src/builder/mod.rs @@ -54,7 +54,7 @@ pub use command::App; #[cfg(feature = "regex")] pub use self::regex::RegexRef; -pub(crate) use action::Action; +pub(crate) use action::ArgAction; 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 d3ed404b820..a5676db266d 100644 --- a/src/parser/parser.rs +++ b/src/parser/parser.rs @@ -8,8 +8,8 @@ use std::{ use clap_lex::RawOsStr; // Internal -use crate::builder::Action; use crate::builder::AppSettings as AS; +use crate::builder::ArgAction; use crate::builder::{Arg, Command}; use crate::error::Error as ClapError; use crate::error::Result as ClapResult; @@ -1138,7 +1138,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { source ); match arg.get_action() { - Action::StoreValue => { + ArgAction::StoreValue => { if ident == Some(Identifier::Index) && arg.is_multiple_values_set() && matcher.contains(&arg.id) @@ -1164,7 +1164,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { } Ok(ParseResult::ValuesDone) } - Action::Flag => { + ArgAction::Flag => { debug_assert_eq!(raw_vals, Vec::::new()); if source == ValueSource::CommandLine { if matches!(ident, Some(Identifier::Short) | Some(Identifier::Long)) { @@ -1179,7 +1179,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { matcher.add_index_to(&arg.id, self.cur_idx.get()); Ok(ParseResult::ValuesDone) } - Action::Help => { + ArgAction::Help => { debug_assert_eq!(raw_vals, Vec::::new()); let use_long = match ident { Some(Identifier::Long) => true, @@ -1190,7 +1190,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { debug!("Help: use_long={}", use_long); Err(self.help_err(use_long, Stream::Stdout)) } - Action::Version => { + ArgAction::Version => { debug_assert_eq!(raw_vals, Vec::::new()); let use_long = match ident { Some(Identifier::Long) => true, @@ -1260,8 +1260,8 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { } } else { match arg.get_action() { - Action::StoreValue => unreachable!("{:?} is not a flag", arg.get_id()), - Action::Flag => { + ArgAction::StoreValue => unreachable!("{:?} is not a flag", arg.get_id()), + ArgAction::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); @@ -1276,7 +1276,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { } } // Early return on `Help` or `Version`. - Action::Help | Action::Version => { + ArgAction::Help | ArgAction::Version => { let _ = self.react(None, ValueSource::EnvVariable, arg, vec![], matcher)?; } From 5a55f4a8635a95956029046397dfd9c84ff15d0a Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 31 May 2022 14:41:17 -0500 Subject: [PATCH 16/19] fix(parser): Restore positional occurrence behavior This fixes a compatibility issue introduced in 9805fdad1b17fde2f5860969de84d7eb4d8f2b51 --- src/parser/matches/matched_arg.rs | 4 ++++ src/parser/parser.rs | 5 +++++ tests/builder/multiple_values.rs | 22 +++++++++++----------- 3 files changed, 20 insertions(+), 11 deletions(-) diff --git a/src/parser/matches/matched_arg.rs b/src/parser/matches/matched_arg.rs index b2c527eaab0..c82f1b116d2 100644 --- a/src/parser/matches/matched_arg.rs +++ b/src/parser/matches/matched_arg.rs @@ -71,6 +71,10 @@ impl MatchedArg { self.occurs += 1; } + pub(crate) fn set_occurrences(&mut self, occurs: u64) { + self.occurs = occurs + } + pub(crate) fn get_occurrences(&self) -> u64 { self.occurs } diff --git a/src/parser/parser.rs b/src/parser/parser.rs index a5676db266d..4b00ced29aa 100644 --- a/src/parser/parser.rs +++ b/src/parser/parser.rs @@ -1157,6 +1157,11 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { } } self.store_arg_values(arg, raw_vals, matcher)?; + if ident == Some(Identifier::Index) && arg.is_multiple_values_set() { + // HACK: Maintain existing occurrence behavior + let matched = matcher.get_mut(&arg.id).unwrap(); + matched.set_occurrences(matched.num_vals() as u64); + } if cfg!(debug_assertions) && matcher.needs_more_vals(arg) { debug!( "Parser::react not enough values passed in, leaving it to the validator to complain", diff --git a/tests/builder/multiple_values.rs b/tests/builder/multiple_values.rs index c775e359dd6..6f33b65f229 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"), 1); + assert_eq!(m.occurrences_of("pos"), 3); 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"), 1); + assert_eq!(m.occurrences_of("pos"), 3); 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"), 1); + assert_eq!(m.occurrences_of("pos"), 3); 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"), 1); + assert_eq!(m.occurrences_of("pos"), 4); 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"), 1); + assert_eq!(m.occurrences_of("pos"), 3); 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"), 1); + assert_eq!(m.occurrences_of("pos"), 2); 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"), 1); + assert_eq!(m.occurrences_of("files"), 3); 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"), 1); + assert_eq!(sm.occurrences_of("files"), 3); 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"), 1); + assert_eq!(m.occurrences_of("files"), 3); 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"), 1); + assert_eq!(m.occurrences_of("files"), 3); assert!(m.is_present("target")); assert_eq!(m.occurrences_of("target"), 1); assert_eq!( @@ -1537,7 +1537,7 @@ fn values_per_occurrence_positional() { .collect::>(), ["val1", "val2"] ); - assert_eq!(m.occurrences_of("pos"), 1); + assert_eq!(m.occurrences_of("pos"), 2); let m = a.try_get_matches_from_mut(vec!["myprog", "val1", "val2", "val3", "val4"]); let m = match m { From 70b633b0ea5f78fe0eca2f4c14b7ffee4b4f7171 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 31 May 2022 14:44:28 -0500 Subject: [PATCH 17/19] refactor(parser): Be explicit about not not iterating over every value --- src/parser/matches/matched_arg.rs | 2 +- tests/builder/grouped_values.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/parser/matches/matched_arg.rs b/src/parser/matches/matched_arg.rs index c82f1b116d2..558e3098ca8 100644 --- a/src/parser/matches/matched_arg.rs +++ b/src/parser/matches/matched_arg.rs @@ -132,7 +132,7 @@ impl MatchedArg { } pub(crate) fn num_vals(&self) -> usize { - self.vals.iter().flatten().count() + self.vals.iter().map(|v| v.len()).sum() } // Will be used later diff --git a/tests/builder/grouped_values.rs b/tests/builder/grouped_values.rs index 1a2c617053e..acb750923c6 100644 --- a/tests/builder/grouped_values.rs +++ b/tests/builder/grouped_values.rs @@ -191,7 +191,7 @@ fn grouped_interleaved_positional_values() { .unwrap(); let pos: Vec<_> = m.grouped_values_of("pos").unwrap().collect(); assert_eq!(pos, vec![vec!["1", "2", "3", "4"]]); - assert_eq!(m.occurrences_of("pos"), 1); + assert_eq!(m.occurrences_of("pos"), 4); let flag: Vec<_> = m.grouped_values_of("flag").unwrap().collect(); assert_eq!(flag, vec![vec!["a"], vec!["b"]]); assert_eq!(m.occurrences_of("flag"), 2); From 12d145c60de163fd8c9d41c28a53fa3afc9f88a2 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 31 May 2022 14:23:03 -0500 Subject: [PATCH 18/19] style: Fix debug typoe --- 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 4b00ced29aa..9514ca190e7 100644 --- a/src/parser/parser.rs +++ b/src/parser/parser.rs @@ -1132,7 +1132,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { self.resolve_pending(matcher)?; debug!( - "Parser::react action={:?}, identifier={:?}, souce={:?}", + "Parser::react action={:?}, identifier={:?}, source={:?}", arg.get_action(), ident, source From 8af7294a26c3100d392fe9c25ecae85584169dcb Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 31 May 2022 14:50:56 -0500 Subject: [PATCH 19/19] style: Make clippy happy --- src/parser/parser.rs | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/src/parser/parser.rs b/src/parser/parser.rs index 9514ca190e7..35a3501ddd8 100644 --- a/src/parser/parser.rs +++ b/src/parser/parser.rs @@ -345,7 +345,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { // Assume this is a value of a previous arg. // get the option so we can check the settings - let arg_values = matcher.pending_values_mut(&id, None); + let arg_values = matcher.pending_values_mut(id, None); let arg = &self.cmd[id]; let parse_result = self.push_arg_values( arg, @@ -1144,17 +1144,15 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { && matcher.contains(&arg.id) { // HACK: Reuse existing occurrence - } else { - if source == ValueSource::CommandLine { - if matches!(ident, Some(Identifier::Short) | Some(Identifier::Long)) { - // Record flag's index - self.cur_idx.set(self.cur_idx.get() + 1); - debug!("Parser::react: cur_idx:={}", self.cur_idx.get()); - } - self.start_occurrence_of_arg(matcher, arg); - } else { - self.start_custom_arg(matcher, arg, source); + } else if source == ValueSource::CommandLine { + if matches!(ident, Some(Identifier::Short) | Some(Identifier::Long)) { + // Record flag's index + self.cur_idx.set(self.cur_idx.get() + 1); + debug!("Parser::react: cur_idx:={}", self.cur_idx.get()); } + self.start_occurrence_of_arg(matcher, arg); + } else { + self.start_custom_arg(matcher, arg, source); } self.store_arg_values(arg, raw_vals, matcher)?; if ident == Some(Identifier::Index) && arg.is_multiple_values_set() {