fix: Always respect positional occurrences

When supporting multiple occurrences for positional arguments in #2804,
I added some tests to cover this but apparently simpler cases fail
despite those more complicated cases being tested.

This adds more multiple-occurrences tests for positional arguments,
fixes them, and in general equates multiple values with occurrences for
positional arguments as part of #2692.  There are a couple more points
for consideration for #2692 for us to decide on once this unblocks them
(usage special case in #2977 and how subcommand help should be handled).

I fully admit I have not fully quantified the impact of all of these
changes and am heavily relying on the quality of our tests to carry this
forward.

clap_generate/src/generators/shells/zsh.rs

@@ -613,7 +613,9 @@ fn write_positionals_of(p: &App) -> String {
     for arg in p.get_positionals() {
         debug!("write_positionals_of:iter: arg={}", arg.get_name());
 
-        let cardinality = if arg.is_set(ArgSettings::MultipleValues) {
+        let cardinality = if arg.is_set(ArgSettings::MultipleValues)
+            || arg.is_set(ArgSettings::MultipleOccurrences)
+        {
             "*:"
         } else if !arg.is_set(ArgSettings::Required) {
             ":"

src/build/arg/mod.rs

@@ -4912,6 +4912,11 @@ impl<'help> Arg<'help> {
             Cow::Borrowed(self.name)
         }
     }
+
+    /// Either multiple values or occurrences
+    pub(crate) fn is_multiple(&self) -> bool {
+        self.is_set(ArgSettings::MultipleValues) | self.is_set(ArgSettings::MultipleOccurrences)
+    }
 }
 
 #[cfg(feature = "yaml")]
@@ -5109,7 +5114,7 @@ where
                         write(&delim.to_string(), false)?;
                     }
                 }
-                if num_val_names == 1 && mult_val {
+                if (num_val_names == 1 && mult_val) || (arg.is_positional() && mult_occ) {
                     write("...", true)?;
                 }
             }

src/parse/parser.rs

@@ -178,10 +178,8 @@ impl<'help, 'app> Parser<'help, 'app> {
             num_p
         );
 
-        // Next we verify that only the highest index has a .multiple_values(true) (if any)
+        // Next we verify that only the highest index has takes multiple arguments (if any)
-        let only_highest = |a: &Arg| {
+        let only_highest = |a: &Arg| a.is_multiple() && (a.index.unwrap_or(0) != highest_idx);
-            a.is_set(ArgSettings::MultipleValues) && (a.index.unwrap_or(0) != highest_idx)
-        };
         if self.app.get_positionals().any(only_highest) {
             // First we make sure if there is a positional that allows multiple values
             // the one before it (second to last) has one of these:
@@ -208,8 +206,7 @@ impl<'help, 'app> Parser<'help, 'app> {
             );
 
             // We make sure if the second to last is Multiple the last is ArgSettings::Last
-            let ok = second_to_last.is_set(ArgSettings::MultipleValues)
+            let ok = second_to_last.is_multiple() || last.is_set(ArgSettings::Last);
-                || last.is_set(ArgSettings::Last);
             assert!(
                 ok,
                 "Only the last positional argument, or second to last positional \
@@ -220,12 +217,15 @@ impl<'help, 'app> Parser<'help, 'app> {
             let count = self
                 .app
                 .get_positionals()
-                .filter(|p| p.settings.is_set(ArgSettings::MultipleValues) && p.num_vals.is_none())
+                .filter(|p| {
+                    p.settings.is_set(ArgSettings::MultipleOccurrences)
+                        || (p.settings.is_set(ArgSettings::MultipleValues) && p.num_vals.is_none())
+                })
                 .count();
             let ok = count <= 1
                 || (last.is_set(ArgSettings::Last)
-                    && last.is_set(ArgSettings::MultipleValues)
+                    && last.is_multiple()
-                    && second_to_last.is_set(ArgSettings::MultipleValues)
+                    && second_to_last.is_multiple()
                     && count == 2);
             assert!(
                 ok,
@@ -390,12 +390,12 @@ impl<'help, 'app> Parser<'help, 'app> {
                 let is_second_to_last = pos_counter + 1 == positional_count;
 
                 // The last positional argument, or second to last positional
-                // argument may be set to .multiple_values(true)
+                // argument may be set to .multiple_values(true) or `.multiple_occurrences(true)`
                 let low_index_mults = is_second_to_last
-                    && self.app.get_positionals().any(|a| {
+                    && self
-                        a.is_set(ArgSettings::MultipleValues)
+                        .app
-                            && (positional_count != a.index.unwrap_or(0))
+                        .get_positionals()
-                    })
+                        .any(|a| a.is_multiple() && (positional_count != a.index.unwrap_or(0)))
                     && self
                         .app
                         .get_positionals()
@@ -683,7 +683,7 @@ impl<'help, 'app> Parser<'help, 'app> {
                 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) {
+                if !p.is_multiple() {
                     pos_counter += 1;
                     parse_state = ParseState::ValuesDone;
                 } else {

tests/help.rs

@@ -2503,6 +2503,106 @@ OPTIONS:
     ));
 }
 
+#[test]
+fn positional_multiple_values_is_dotted() {
+    let app = App::new("test").arg(
+        Arg::new("foo")
+            .required(true)
+            .takes_value(true)
+            .multiple_values(true),
+    );
+    assert!(utils::compare_output(
+        app,
+        "test --help",
+        "test 
+
+USAGE:
+    test <foo>...
+
+ARGS:
+    <foo>...    
+
+OPTIONS:
+    -h, --help    Print help information
+",
+        false
+    ));
+
+    let app = App::new("test").arg(
+        Arg::new("foo")
+            .required(true)
+            .takes_value(true)
+            .value_name("BAR")
+            .multiple_values(true),
+    );
+    assert!(utils::compare_output(
+        app,
+        "test --help",
+        "test 
+
+USAGE:
+    test <BAR>...
+
+ARGS:
+    <BAR>...    
+
+OPTIONS:
+    -h, --help    Print help information
+",
+        false
+    ));
+}
+
+#[test]
+fn positional_multiple_occurrences_is_dotted() {
+    let app = App::new("test").arg(
+        Arg::new("foo")
+            .required(true)
+            .takes_value(true)
+            .multiple_occurrences(true),
+    );
+    assert!(utils::compare_output(
+        app,
+        "test --help",
+        "test 
+
+USAGE:
+    test <foo>...
+
+ARGS:
+    <foo>...    
+
+OPTIONS:
+    -h, --help    Print help information
+",
+        false
+    ));
+
+    let app = App::new("test").arg(
+        Arg::new("foo")
+            .required(true)
+            .takes_value(true)
+            .value_name("BAR")
+            .multiple_occurrences(true),
+    );
+    assert!(utils::compare_output(
+        app,
+        "test --help",
+        "test 
+
+USAGE:
+    test <BAR>...
+
+ARGS:
+    <BAR>...    
+
+OPTIONS:
+    -h, --help    Print help information
+",
+        false
+    ));
+}
+
 #[test]
 fn disabled_help_flag() {
     let res = App::new("foo")

tests/multiple_occurrences.rs

@@ -59,6 +59,38 @@ fn multiple_occurrences_of_flags_mixed() {
     assert_eq!(m.occurrences_of("flag"), 1);
 }
 
+#[test]
+fn multiple_occurrences_of_positional() {
+    let app = App::new("test").arg(Arg::new("multi").setting(ArgSettings::MultipleOccurrences));
+
+    let m = app
+        .clone()
+        .try_get_matches_from(&["test"])
+        .expect("zero occurrences work");
+    assert!(!m.is_present("multi"));
+    assert_eq!(m.occurrences_of("multi"), 0);
+    assert!(m.values_of("multi").is_none());
+
+    let m = app
+        .clone()
+        .try_get_matches_from(&["test", "one"])
+        .expect("single occurrence work");
+    assert!(m.is_present("multi"));
+    assert_eq!(m.occurrences_of("multi"), 1);
+    assert_eq!(m.values_of("multi").unwrap().collect::<Vec<_>>(), ["one"]);
+
+    let m = app
+        .clone()
+        .try_get_matches_from(&["test", "one", "two", "three", "four"])
+        .expect("many occurrences work");
+    assert!(m.is_present("multi"));
+    assert_eq!(m.occurrences_of("multi"), 4);
+    assert_eq!(
+        m.values_of("multi").unwrap().collect::<Vec<_>>(),
+        ["one", "two", "three", "four"]
+    );
+}
+
 #[test]
 fn multiple_occurrences_of_flags_large_quantity() {
     let args: Vec<&str> = vec![""]
@@ -194,3 +226,27 @@ fn max_occurrences_try_inputs() {
     assert!(m.is_err());
     assert_eq!(m.unwrap_err().kind, ErrorKind::TooManyOccurrences);
 }
+
+#[test]
+fn max_occurrences_positional() {
+    let app = App::new("prog").arg(Arg::new("verbose").max_occurrences(3));
+    let m = app.clone().try_get_matches_from(vec!["prog", "v"]);
+    assert!(m.is_ok(), "{}", m.unwrap_err());
+    assert_eq!(m.unwrap().occurrences_of("verbose"), 1);
+
+    let m = app.clone().try_get_matches_from(vec!["prog", "v", "v"]);
+    assert!(m.is_ok(), "{}", m.unwrap_err());
+    assert_eq!(m.unwrap().occurrences_of("verbose"), 2);
+
+    let m = app
+        .clone()
+        .try_get_matches_from(vec!["prog", "v", "v", "v"]);
+    assert!(m.is_ok(), "{}", m.unwrap_err());
+    assert_eq!(m.unwrap().occurrences_of("verbose"), 3);
+
+    let m = app
+        .clone()
+        .try_get_matches_from(vec!["prog", "v", "v", "v", "v"]);
+    assert!(m.is_err());
+    assert_eq!(m.unwrap_err().kind, ErrorKind::TooManyOccurrences);
+}