Merge pull request #2350 from ldm0/num_vals

Change how num_vals, min_vals, max_vals interacts with Multiple*

src/build/arg/mod.rs

@@ -1871,7 +1871,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
@@ -2156,7 +2156,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).
@@ -4351,15 +4351,10 @@ impl<'help> Arg<'help> {
         {
             self.val_delim = Some(',');
         }
-        if self.index.is_some() || (self.short.is_none() && self.long.is_none()) {
+        if !(self.index.is_some() || (self.short.is_none() && self.long.is_none()))
-            if self.max_vals.is_some()
+            && self.is_set(ArgSettings::TakesValue)
-                || self.min_vals.is_some()
+            && self.val_names.len() > 1
-                || (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 {
             self.num_vals = Some(self.val_names.len());
         }
     }

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

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)
     }

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) {
+                let can_be_subcommand = self.is_set(AS::SubcommandPrecedenceOverArg)
-                    true
+                    || !matches!(needs_val_of, ParseResult::Opt(_) | ParseResult::Pos(_));
-                } else {
-                    !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)
@@ -522,13 +519,16 @@ 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);
 
-                matcher.inc_occurrence_of(&p.id);
+                // Increase occurence no matter if we are appending, Occurences
-                for grp in self.app.groups_for_arg(&p.id) {
+                // of positional argument equals to number of values rather than
-                    matcher.inc_occurrence_of(&grp);
+                // the number of value groups.
-                }
+                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 +1197,7 @@ impl<'help, 'app> Parser<'help, 'app> {
             debug!("None");
         }
 
-        matcher.inc_occurrence_of(&opt.id);
+        self.inc_occurrence_of_arg(matcher, opt);
-        // Increment or create the group "args"
-        for grp in self.app.groups_for_arg(&opt.id) {
-            matcher.inc_occurrence_of(&grp);
-        }
 
         let needs_delimiter = opt.is_set(ArgSettings::RequireDelimiter);
         let multiple = opt.is_set(ArgSettings::MultipleValues);
@@ -1256,14 +1252,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.
+                // separate the values or no more vals is needed, we're not
-                let parse_result =
+                // looking for more values.
-                    if val.contains_char(delim) || arg.is_set(ArgSettings::RequireDelimiter) {
+                return if val.contains_char(delim)
-                        ParseResult::ValuesDone
+                    || arg.is_set(ArgSettings::RequireDelimiter)
-                    } else {
+                    || !matcher.needs_more_vals(arg)
-                        self.need_more_vals(matcher, arg)
+                {
-                    };
+                    ParseResult::ValuesDone
-                return parse_result;
+                } else {
+                    ParseResult::Opt(arg.id.clone())
+                };
             }
         }
         if let Some(t) = arg.terminator {
@@ -1272,7 +1270,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(
@@ -1286,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);
@@ -1314,14 +1319,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)
     }
@@ -1329,12 +1326,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"
+        self.inc_occurrence_of_arg(matcher, flag);
-        for grp in self.app.groups_for_arg(&flag.id) {
-            matcher.inc_occurrence_of(&grp);
-        }
 
         ParseResult::Flag
     }
@@ -1360,6 +1353,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)
@@ -1513,6 +1509,15 @@ 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"
+        for group in self.app.groups_for_arg(&arg.id) {
+            matcher.inc_occurrence_of(&group);
+        }
+    }
 }
 
 // Error, Help, and Version Methods
@@ -1547,10 +1552,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) {
+                self.inc_occurrence_of_arg(matcher, opt);
-                    matcher.inc_occurrence_of(&g);
-                }
-                matcher.inc_occurrence_of(&opt.id);
             }
         }
 

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) {
+            let should_err = if a.is_set(ArgSettings::MultipleOccurrences) {
-                ma.num_vals() % num != 0
+                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) {
+                    if a.is_set(ArgSettings::MultipleOccurrences) {
-                        ma.num_vals() % num
+                        total_num % num
                     } else {
-                        ma.num_vals()
+                        total_num
                     },
                     Usage::new(self.p).create_usage_with_title(&[]),
                     self.p.app.color(),

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<_>>(), vec!["d"]);
+    assert_eq!(vs.collect::<Vec<_>>(), 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<_>>(), vec!["c", "d"]);
+    assert_eq!(vs.collect::<Vec<_>>(), vec!["a", "b", "c", "d"]);
 }
 
 #[test]

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);
+}