fix(clap-rs#1374): Remove all values from previous occurrences on sel…

…f-override

When an argument gets self-overriden, at most a single value[1] gets
removed from the existing values list.  This is incorrect when the
argument has multiple values per occurrence, and makes for "funny" bugs
such as `--input --input a b` being parsed as `--input b` and
`--input a b c --input d` being parsed as `--input c d`.

This patch fixes the issue by keeping track of how many values were
already present when we started parsing each occurrence, and removing
all the values but the ones for the last occurrence when a self-override
occurs.

[1]: Actually at most two values due to clap handling the overrides
twice, which is a separate bug fixed in v3.

src/app/parser.rs

@@ -1071,12 +1071,15 @@ where
                     }
                     self.cache = Some(p.b.name);
                 }
-                let _ = self.add_val_to_arg(p, &arg_os, matcher)?;
 
                 matcher.inc_occurrence_of(p.b.name);
                 let _ = self.groups_for_arg(p.b.name)
                     .and_then(|vec| Some(matcher.inc_occurrences_of(&*vec)));
 
+                // Add value after increasing the occurrence count to ensure it is associated with
+                // the new occurence.
+                let _ = self.add_val_to_arg(p, &arg_os, matcher)?;
+
                 self.settings.set(AS::ValidArgFound);
                 // Only increment the positional counter if it doesn't allow multiples
                 if !p.b.settings.is_set(ArgSettings::Multiple) {
@@ -1740,6 +1743,7 @@ where
         let needs_eq = opt.is_set(ArgSettings::RequireEquals);
 
         debug!("Parser::parse_opt; Checking for val...");
+        let mut parsed_val = None;
         if let Some(fv) = val {
             has_eq = fv.starts_with(&[b'=']) || had_eq;
             let v = fv.trim_left_matches(b'=');
@@ -1757,7 +1761,8 @@ where
                 fv,
                 fv.starts_with(&[b'='])
             );
-            self.add_val_to_arg(opt, v, matcher)?;
+
+            parsed_val = Some(v);
         } else if needs_eq && !(empty_vals || min_vals_zero) {
             sdebugln!("None, but requires equals...Error");
             return Err(Error::empty_value(
@@ -1774,6 +1779,12 @@ where
         self.groups_for_arg(opt.b.name)
             .and_then(|vec| Some(matcher.inc_occurrences_of(&*vec)));
 
+        // Add value after increasing the occurrence count to ensure it is associated with the new
+        // occurence.
+        if let Some(v) = parsed_val {
+            self.add_val_to_arg(opt, v, matcher)?;
+        }
+
         let needs_delim = opt.is_set(ArgSettings::RequireDelimiter);
         let mult = opt.is_set(ArgSettings::Multiple);
         if no_val && min_vals_zero && !has_eq && needs_eq {

src/args/arg_matcher.rs

@@ -64,13 +64,18 @@ impl<'a> ArgMatcher<'a> {
                 return;
             }
             if let Some(ma) = self.get_mut(aa.name()) {
-                if ma.vals.len() > 1 {
-                    // swap_remove(0) would be O(1) but does not preserve order, which
-                    // we need
-                    ma.vals.remove(0);
-                    ma.occurs = 1;
-                } else if !aa.takes_value() && ma.occurs > 1 {
+                if ma.occurs > 1 {
+                    if aa.takes_value() {
+                        // Only keep values for the last occurrence
+                        let len = ma.vals.len();
+                        let keep = len - ma.occurrences.pop().unwrap_or(len);
+
+                        ma.vals.rotate_right(keep);
+                        ma.vals.truncate(keep);
+                    }
+
                     ma.occurs = 1;
+                    ma.occurrences.clear();
                 }
             }
         }
@@ -155,6 +160,11 @@ impl<'a> ArgMatcher<'a> {
     pub fn inc_occurrence_of(&mut self, arg: &'a str) {
         debugln!("ArgMatcher::inc_occurrence_of: arg={}", arg);
         if let Some(a) = self.get_mut(arg) {
+            if a.occurs > 0 {
+                // If not the first occurrence, we need to record the position in the vals vector
+                // at which this occurrence starts
+                a.occurrences.push(a.vals.len());
+            }
             a.occurs += 1;
             return;
         }
@@ -174,6 +184,7 @@ impl<'a> ArgMatcher<'a> {
             occurs: 0,
             indices: Vec::with_capacity(1),
             vals: Vec::with_capacity(1),
+            occurrences: Vec::new(),
         });
         ma.vals.push(val.to_owned());
     }
@@ -183,6 +194,7 @@ impl<'a> ArgMatcher<'a> {
             occurs: 0,
             indices: Vec::with_capacity(1),
             vals: Vec::new(),
+            occurrences: Vec::new(),
         });
         ma.indices.push(idx);
     }

src/args/matched_arg.rs

@@ -7,6 +7,10 @@ pub struct MatchedArg {
     #[doc(hidden)] pub occurs: u64,
     #[doc(hidden)] pub indices: Vec<usize>,
     #[doc(hidden)] pub vals: Vec<OsString>,
+    // This contains the positions in `vals` at which each occurrence starts.  The first occurrence
+    // is implicitely starting at position `0` so that `occurrences` is empty in the common case of
+    // a single occurrence.
+    #[doc(hidden)] pub occurrences: Vec<usize>,
 }
 
 impl Default for MatchedArg {
@@ -15,6 +19,7 @@ impl Default for MatchedArg {
             occurs: 1,
             indices: Vec::new(),
             vals: Vec::new(),
+            occurrences: Vec::new(),
         }
     }
 }