From 78d7252b034ad6bd10512a00dd0cc797d1f35670 Mon Sep 17 00:00:00 2001 From: ldm0 Date: Sun, 14 Feb 2021 09:48:59 +0000 Subject: [PATCH 1/5] Remove Parser::need_more_vals --- src/parse/parser.rs | 32 +++++++++++++++----------------- 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/src/parse/parser.rs b/src/parse/parser.rs index 65db23ab..052a3ded 100644 --- a/src/parse/parser.rs +++ b/src/parse/parser.rs @@ -1256,14 +1256,16 @@ impl<'help, 'app> Parser<'help, 'app> { let vals = vals.into_iter().map(|x| x.to_os_string()).collect(); self.add_multiple_vals_to_arg(arg, vals, matcher, ty, append); // If there was a delimiter used or we must use the delimiter to - // separate the values, we're not looking for more values. - let parse_result = - if val.contains_char(delim) || arg.is_set(ArgSettings::RequireDelimiter) { - ParseResult::ValuesDone - } else { - self.need_more_vals(matcher, arg) - }; - return parse_result; + // separate the values or no more vals is needed, we're not + // looking for more values. + return if val.contains_char(delim) + || arg.is_set(ArgSettings::RequireDelimiter) + || !matcher.needs_more_vals(arg) + { + ParseResult::ValuesDone + } else { + ParseResult::Opt(arg.id.clone()) + }; } } if let Some(t) = arg.terminator { @@ -1272,7 +1274,11 @@ impl<'help, 'app> Parser<'help, 'app> { } } self.add_single_val_to_arg(arg, val.to_os_string(), matcher, ty, append); - self.need_more_vals(matcher, arg) + if matcher.needs_more_vals(arg) { + ParseResult::Opt(arg.id.clone()) + } else { + ParseResult::ValuesDone + } } fn add_multiple_vals_to_arg( @@ -1314,14 +1320,6 @@ impl<'help, 'app> Parser<'help, 'app> { matcher.add_index_to(&arg.id, self.cur_idx.get(), ty); } - fn need_more_vals(&self, matcher: &mut ArgMatcher, arg: &Arg<'help>) -> ParseResult { - if matcher.needs_more_vals(arg) { - ParseResult::Opt(arg.id.clone()) - } else { - ParseResult::ValuesDone - } - } - fn arg_have_val(&self, matcher: &mut ArgMatcher, arg: &Arg<'help>) -> bool { matcher.arg_have_val(&arg.id) } From 5cff16b6f959d94953c4f3f4cf849ca58c3a53cc Mon Sep 17 00:00:00 2001 From: ldm0 Date: Sun, 14 Feb 2021 11:20:33 +0000 Subject: [PATCH 2/5] Limit usage of inc_occurence_of --- src/parse/parser.rs | 30 ++++++++++++------------------ 1 file changed, 12 insertions(+), 18 deletions(-) diff --git a/src/parse/parser.rs b/src/parse/parser.rs index 052a3ded..2d214ed6 100644 --- a/src/parse/parser.rs +++ b/src/parse/parser.rs @@ -525,10 +525,7 @@ impl<'help, 'app> Parser<'help, 'app> { let append = self.arg_have_val(matcher, p); self.add_val_to_arg(p, &arg_os, matcher, ValueType::CommandLine, append); - matcher.inc_occurrence_of(&p.id); - for grp in self.app.groups_for_arg(&p.id) { - matcher.inc_occurrence_of(&grp); - } + self.inc_occurrence_of_arg(matcher, p); // Only increment the positional counter if it doesn't allow multiples if !p.settings.is_set(ArgSettings::MultipleValues) { @@ -1197,11 +1194,7 @@ impl<'help, 'app> Parser<'help, 'app> { debug!("None"); } - matcher.inc_occurrence_of(&opt.id); - // Increment or create the group "args" - for grp in self.app.groups_for_arg(&opt.id) { - matcher.inc_occurrence_of(&grp); - } + self.inc_occurrence_of_arg(matcher, opt); let needs_delimiter = opt.is_set(ArgSettings::RequireDelimiter); let multiple = opt.is_set(ArgSettings::MultipleValues); @@ -1327,12 +1320,8 @@ impl<'help, 'app> Parser<'help, 'app> { fn parse_flag(&self, flag: &Arg<'help>, matcher: &mut ArgMatcher) -> ParseResult { debug!("Parser::parse_flag"); - matcher.inc_occurrence_of(&flag.id); matcher.add_index_to(&flag.id, self.cur_idx.get(), ValueType::CommandLine); - // Increment or create the group "args" - for grp in self.app.groups_for_arg(&flag.id) { - matcher.inc_occurrence_of(&grp); - } + self.inc_occurrence_of_arg(matcher, flag); ParseResult::Flag } @@ -1508,6 +1497,14 @@ impl<'help, 'app> Parser<'help, 'app> { } Ok(()) } + + fn inc_occurrence_of_arg(&self, matcher: &mut ArgMatcher, arg: &Arg<'help>) { + matcher.inc_occurrence_of(&arg.id); + // Increment or create the group "args" + for group in self.app.groups_for_arg(&arg.id) { + matcher.inc_occurrence_of(&group); + } + } } // Error, Help, and Version Methods @@ -1542,10 +1539,7 @@ impl<'help, 'app> Parser<'help, 'app> { // 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.app.args.get(&name.as_ref()) { - for g in self.app.groups_for_arg(&opt.id) { - matcher.inc_occurrence_of(&g); - } - matcher.inc_occurrence_of(&opt.id); + self.inc_occurrence_of_arg(matcher, opt); } } From 73c682b65280bfb77b0a714e5236c5fd23711bde Mon Sep 17 00:00:00 2001 From: ldm0 Date: Sun, 14 Feb 2021 13:07:21 +0000 Subject: [PATCH 3/5] Small code shrinking --- src/parse/parser.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/parse/parser.rs b/src/parse/parser.rs index 2d214ed6..7b489994 100644 --- a/src/parse/parser.rs +++ b/src/parse/parser.rs @@ -348,11 +348,8 @@ impl<'help, 'app> Parser<'help, 'app> { // Has the user already passed '--'? Meaning only positional args follow if !self.is_set(AS::TrailingValues) { - let can_be_subcommand = if self.is_set(AS::SubcommandPrecedenceOverArg) { - true - } else { - !matches!(needs_val_of, ParseResult::Opt(_) | ParseResult::Pos(_)) - }; + let can_be_subcommand = self.is_set(AS::SubcommandPrecedenceOverArg) + || !matches!(needs_val_of, ParseResult::Opt(_) | ParseResult::Pos(_)); if can_be_subcommand { // Does the arg match a subcommand name, or any of its aliases (if defined) @@ -1498,6 +1495,7 @@ impl<'help, 'app> Parser<'help, 'app> { Ok(()) } + /// Increase occurrence of specific argument and the grouped arg it's in. fn inc_occurrence_of_arg(&self, matcher: &mut ArgMatcher, arg: &Arg<'help>) { matcher.inc_occurrence_of(&arg.id); // Increment or create the group "args" From 28b58af63bc98270d0fa32b66726972388139da8 Mon Sep 17 00:00:00 2001 From: ldm0 Date: Tue, 16 Feb 2021 03:45:20 +0000 Subject: [PATCH 4/5] Change to num_vals, min_vals, max_vals interacts with Multi* --- src/build/arg/mod.rs | 17 ++++++----------- src/parse/arg_matcher.rs | 4 ++-- src/parse/matches/matched_arg.rs | 2 ++ src/parse/parser.rs | 12 ++++++++++++ src/parse/validator.rs | 17 ++++++++++------- tests/grouped_values.rs | 7 ++++--- 6 files changed, 36 insertions(+), 23 deletions(-) diff --git a/src/build/arg/mod.rs b/src/build/arg/mod.rs index e731ab86..8d1ae3df 100644 --- a/src/build/arg/mod.rs +++ b/src/build/arg/mod.rs @@ -1867,7 +1867,7 @@ impl<'help> Arg<'help> { #[inline] pub fn number_of_values(mut self, qty: usize) -> Self { self.num_vals = Some(qty); - self.takes_value(true) + self.takes_value(true).multiple_values(true) } /// Allows one to perform a custom validation on the argument value. You provide a closure @@ -2152,7 +2152,7 @@ impl<'help> Arg<'help> { #[inline] pub fn min_values(mut self, qty: usize) -> Self { self.min_vals = Some(qty); - self.takes_value(true) + self.takes_value(true).multiple_values(true) } /// Specifies the separator to use when values are clumped together, defaults to `,` (comma). @@ -4347,15 +4347,10 @@ impl<'help> Arg<'help> { { self.val_delim = Some(','); } - if self.index.is_some() || (self.short.is_none() && self.long.is_none()) { - if self.max_vals.is_some() - || self.min_vals.is_some() - || (self.num_vals.is_some() && self.num_vals.unwrap() > 1) - { - self.settings.set(ArgSettings::MultipleValues); - self.settings.set(ArgSettings::MultipleOccurrences); - } - } else if self.is_set(ArgSettings::TakesValue) && self.val_names.len() > 1 { + if !(self.index.is_some() || (self.short.is_none() && self.long.is_none())) + && self.is_set(ArgSettings::TakesValue) + && self.val_names.len() > 1 + { self.num_vals = Some(self.val_names.len()); } } diff --git a/src/parse/arg_matcher.rs b/src/parse/arg_matcher.rs index b12c598c..48d1521b 100644 --- a/src/parse/arg_matcher.rs +++ b/src/parse/arg_matcher.rs @@ -165,10 +165,10 @@ 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_last_group(); + 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_set(ArgSettings::MultipleValues) { + return if o.is_set(ArgSettings::MultipleOccurrences) { (current_num % num) != 0 } else { num != current_num diff --git a/src/parse/matches/matched_arg.rs b/src/parse/matches/matched_arg.rs index 808166d9..2259ef69 100644 --- a/src/parse/matches/matched_arg.rs +++ b/src/parse/matches/matched_arg.rs @@ -81,6 +81,8 @@ impl MatchedArg { self.vals.iter().flatten().count() } + // Will be used later + #[allow(dead_code)] pub(crate) fn num_vals_last_group(&self) -> usize { self.vals.last().map(|x| x.len()).unwrap_or(0) } diff --git a/src/parse/parser.rs b/src/parse/parser.rs index 7b489994..1255cf5a 100644 --- a/src/parse/parser.rs +++ b/src/parse/parser.rs @@ -519,9 +519,15 @@ impl<'help, 'app> Parser<'help, 'app> { } self.seen.push(p.id.clone()); + // 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 continiously. let append = self.arg_have_val(matcher, p); self.add_val_to_arg(p, &arg_os, matcher, ValueType::CommandLine, append); + // Increase occurence no matter if we are appending, Occurences + // of positional argument equals to number of values rather than + // the number of value groups. self.inc_occurrence_of_arg(matcher, p); // Only increment the positional counter if it doesn't allow multiples @@ -1282,6 +1288,9 @@ impl<'help, 'app> Parser<'help, 'app> { // 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.app.groups_for_arg(&arg.id) { + matcher.new_val_group(&group); + } } for val in vals { self.add_single_val_to_arg(arg, val, matcher, ty, true); @@ -1341,6 +1350,9 @@ impl<'help, 'app> Parser<'help, 'app> { arg_overrides.push((overridee.clone(), &overrider.id)); } } + // Only do self override for argument that is not positional + // argument or flag with one of the Multiple* setting + // enabled(which is a feature). if (self.is_set(AS::AllArgsOverrideSelf) || override_self) && !overrider.is_set(ArgSettings::MultipleValues) && !overrider.is_set(ArgSettings::MultipleOccurrences) diff --git a/src/parse/validator.rs b/src/parse/validator.rs index b89c12a3..6b30a269 100644 --- a/src/parse/validator.rs +++ b/src/parse/validator.rs @@ -428,7 +428,9 @@ impl<'help, 'app, 'parser> Validator<'help, 'app, 'parser> { "Validator::validate_arg_num_occurs: {:?}={}", a.name, ma.occurs ); - if ma.occurs > 1 && !a.is_set(ArgSettings::MultipleOccurrences) { + // Occurence of positional argument equals to number of values rather + // than number of grouped values. + if ma.occurs > 1 && !a.is_set(ArgSettings::MultipleOccurrences) && !a.is_positional() { // Not the first time, and we don't allow multiples return Err(Error::unexpected_multiple_usage( a, @@ -442,21 +444,22 @@ impl<'help, 'app, 'parser> Validator<'help, 'app, 'parser> { fn validate_arg_num_vals(&self, a: &Arg, ma: &MatchedArg) -> ClapResult<()> { debug!("Validator::validate_arg_num_vals"); if let Some(num) = a.num_vals { + let total_num = ma.num_vals(); debug!("Validator::validate_arg_num_vals: num_vals set...{}", num); - let should_err = if a.is_set(ArgSettings::MultipleValues) { - ma.num_vals() % num != 0 + let should_err = if a.is_set(ArgSettings::MultipleOccurrences) { + total_num % num != 0 } else { - num != ma.num_vals() + num != total_num }; if should_err { debug!("Validator::validate_arg_num_vals: Sending error WrongNumberOfValues"); return Err(Error::wrong_number_of_values( a, num, - if a.is_set(ArgSettings::MultipleValues) { - ma.num_vals() % num + if a.is_set(ArgSettings::MultipleOccurrences) { + total_num % num } else { - ma.num_vals() + total_num }, Usage::new(self.p).create_usage_with_title(&[]), self.p.app.color(), diff --git a/tests/grouped_values.rs b/tests/grouped_values.rs index 4544f72d..4a7984f2 100644 --- a/tests/grouped_values.rs +++ b/tests/grouped_values.rs @@ -153,18 +153,19 @@ fn issue_1374() { .takes_value(true) .long("input") .overrides_with("input") - .min_values(0), + .min_values(0) + .multiple_occurrences(true), ); let matches = app .clone() .get_matches_from(&["MyApp", "--input", "a", "b", "c", "--input", "d"]); let vs = matches.values_of("input").unwrap(); - assert_eq!(vs.collect::>(), vec!["d"]); + assert_eq!(vs.collect::>(), vec!["a", "b", "c", "d"]); let matches = app .clone() .get_matches_from(&["MyApp", "--input", "a", "b", "--input", "c", "d"]); let vs = matches.values_of("input").unwrap(); - assert_eq!(vs.collect::>(), vec!["c", "d"]); + assert_eq!(vs.collect::>(), vec!["a", "b", "c", "d"]); } #[test] From 3873b647d123f427b519662cb8d7714727dcf3c8 Mon Sep 17 00:00:00 2001 From: ldm0 Date: Tue, 16 Feb 2021 03:58:58 +0000 Subject: [PATCH 5/5] Add tests for 2229 --- tests/multiple_values.rs | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/tests/multiple_values.rs b/tests/multiple_values.rs index 1285fa58..f1f0f54b 100644 --- a/tests/multiple_values.rs +++ b/tests/multiple_values.rs @@ -1192,3 +1192,19 @@ fn issue_1480_max_values_consumes_extra_arg_3() { assert!(res.is_err()); assert_eq!(res.unwrap_err().kind, ErrorKind::UnknownArgument); } + +#[test] +fn issue_2229() { + let m = App::new("multiple_values") + .arg( + Arg::new("pos") + .about("multiple positionals") + .number_of_values(3), + ) + .try_get_matches_from(vec![ + "myprog", "val1", "val2", "val3", "val4", "val5", "val6", + ]); + + assert!(m.is_err()); // This panics, because `m.is_err() == false`. + assert_eq!(m.unwrap_err().kind, ErrorKind::WrongNumberOfValues); +}