Merge pull request #3225 from epage/override

fix: Remove overriden occurrences as we go

src/build/app/mod.rs

@@ -2601,6 +2601,7 @@ impl<'help> App<'help> {
             self._derive_display_order();
 
             let mut pos_counter = 1;
+            let self_override = self.is_set(AppSettings::AllArgsOverrideSelf);
             for a in self.args.args_mut() {
                 // Fill in the groups
                 for g in &a.groups {
@@ -2619,6 +2620,10 @@ impl<'help> App<'help> {
                     // in the usage string don't get confused or left out.
                     self.settings.set(AppSettings::DontCollapseArgsInUsage);
                 }
+                if self_override {
+                    let self_id = a.id.clone();
+                    a.overrides.push(self_id);
+                }
                 a._build();
                 if a.is_positional() && a.index.is_none() {
                     a.index = Some(pos_counter);

src/build/arg/mod.rs

@@ -4808,6 +4808,16 @@ impl<'help> Arg<'help> {
                 self.num_vals = Some(val_names_len);
             }
         }
+
+        let self_id = self.id.clone();
+        if self.is_positional() || self.is_set(ArgSettings::MultipleOccurrences) {
+            // Remove self-overrides where they don't make sense.
+            //
+            // We can evaluate switching this to a debug assert at a later time (though it will
+            // require changing propagation of `AllArgsOverrideSelf`).  Being conservative for now
+            // due to where we are at in the release.
+            self.overrides.retain(|e| *e != self_id);
+        }
     }
 
     pub(crate) fn generated(mut self) -> Self {

src/parse/arg_matcher.rs

@@ -176,8 +176,11 @@ impl ArgMatcher {
         ma.push_index(idx);
     }
 
-    pub(crate) fn arg_have_val(&mut self, arg: &Id) -> bool {
+    pub(crate) fn has_val_groups(&mut self, arg: &Id) -> bool {
-        matches!(self.entry(arg), Entry::Occupied(_))
+        match self.entry(arg) {
+            Entry::Occupied(e) => e.get().has_val_groups(),
+            Entry::Vacant(_) => false,
+        }
     }
 
     pub(crate) fn needs_more_vals(&self, o: &Arg) -> bool {

src/parse/matches/matched_arg.rs

@@ -77,10 +77,14 @@ impl MatchedArg {
         self.vals.last().map(|x| x.len()).unwrap_or(0)
     }
 
-    pub(crate) fn is_vals_empty(&self) -> bool {
+    pub(crate) fn all_val_groups_empty(&self) -> bool {
         self.vals.iter().flatten().count() == 0
     }
 
+    pub(crate) fn has_val_groups(&self) -> bool {
+        !self.vals.is_empty()
+    }
+
     // Will be used later
     #[allow(dead_code)]
     pub(crate) fn remove_vals(&mut self, len: usize) {
@@ -104,13 +108,6 @@ impl MatchedArg {
         }
     }
 
-    pub(crate) fn override_vals(&mut self) {
-        let len = self.vals.len();
-        if len > 1 {
-            self.vals.drain(0..len - 1);
-        }
-    }
-
     pub(crate) fn contains_val(&self, val: &str) -> bool {
         self.vals_flatten().any(|v| {
             if self.ignore_case {
@@ -158,47 +155,6 @@ pub(crate) enum ValueType {
 mod tests {
     use super::*;
 
-    #[test]
-    fn test_vals_override() {
-        let mut m = MatchedArg::new();
-        m.push_val("aaa".into());
-        m.new_val_group();
-        m.append_val("bbb".into());
-        m.append_val("ccc".into());
-        m.new_val_group();
-        m.append_val("ddd".into());
-        m.push_val("eee".into());
-        m.new_val_group();
-        m.append_val("fff".into());
-        m.append_val("ggg".into());
-        m.append_val("hhh".into());
-        m.append_val("iii".into());
-
-        m.override_vals();
-        let vals: Vec<&Vec<OsString>> = m.vals().collect();
-        assert_eq!(
-            vals,
-            vec![&vec![
-                OsString::from("fff"),
-                OsString::from("ggg"),
-                OsString::from("hhh"),
-                OsString::from("iii"),
-            ]]
-        );
-        m.override_vals();
-
-        let vals: Vec<&Vec<OsString>> = m.vals().collect();
-        assert_eq!(
-            vals,
-            vec![&vec![
-                OsString::from("fff"),
-                OsString::from("ggg"),
-                OsString::from("hhh"),
-                OsString::from("iii"),
-            ]]
-        );
-    }
-
     #[test]
     fn test_grouped_vals_first() {
         let mut m = MatchedArg::new();

src/parse/parser.rs

@@ -1,6 +1,6 @@
 // Std
 use std::{
-    cell::Cell,
+    cell::{Cell, RefCell},
     ffi::{OsStr, OsString},
 };
 
@@ -26,7 +26,7 @@ use crate::{
 pub(crate) struct Parser<'help, 'app> {
     pub(crate) app: &'app mut App<'help>,
     pub(crate) required: ChildGraph<Id>,
-    pub(crate) overridden: Vec<Id>,
+    pub(crate) overridden: RefCell<Vec<Id>>,
     pub(crate) seen: Vec<Id>,
     pub(crate) cur_idx: Cell<usize>,
     /// Index of the previous flag subcommand in a group of flags.
@@ -51,7 +51,7 @@ impl<'help, 'app> Parser<'help, 'app> {
         Parser {
             app,
             required: reqs,
-            overridden: Vec::new(),
+            overridden: Default::default(),
             seen: Vec::new(),
             cur_idx: Cell::new(0),
             flag_subcmd_at: None,
@@ -591,10 +591,14 @@ impl<'help, 'app> Parser<'help, 'app> {
                 }
 
                 self.seen.push(p.id.clone());
+                // Increase occurrence no matter if we are appending, occurrences
+                // of positional argument equals to number of values rather than
+                // the number of value groups.
+                self.inc_occurrence_of_arg(matcher, p);
                 // 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 continuously.
-                let append = self.arg_have_val(matcher, p);
+                let append = self.has_val_groups(matcher, p);
                 self.add_val_to_arg(
                     p,
                     &arg_os,
@@ -604,11 +608,6 @@ impl<'help, 'app> Parser<'help, 'app> {
                     trailing_values,
                 );
 
-                // Increase occurrence no matter if we are appending, occurrences
-                // 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
                 if !p.is_multiple() {
                     pos_counter += 1;
@@ -658,8 +657,6 @@ impl<'help, 'app> Parser<'help, 'app> {
                     matches: sc_m.into_inner(),
                 });
 
-                self.remove_overrides(matcher);
-
                 return Validator::new(self).validate(
                     parse_state,
                     subcmd_name.is_some(),
@@ -697,8 +694,6 @@ impl<'help, 'app> Parser<'help, 'app> {
             ));
         }
 
-        self.remove_overrides(matcher);
-
         Validator::new(self).validate(parse_state, subcmd_name.is_some(), matcher, trailing_values)
     }
 
@@ -1302,6 +1297,7 @@ impl<'help, 'app> Parser<'help, 'app> {
         if opt.is_set(ArgSettings::RequireEquals) && !has_eq {
             if opt.min_vals == Some(0) {
                 debug!("Requires equals, but min_vals == 0");
+                self.inc_occurrence_of_arg(matcher, opt);
                 // We assume this case is valid: require equals, but min_vals == 0.
                 if !opt.default_missing_vals.is_empty() {
                     debug!("Parser::parse_opt: has default_missing_vals");
@@ -1313,7 +1309,6 @@ impl<'help, 'app> Parser<'help, 'app> {
                         false,
                     );
                 };
-                self.inc_occurrence_of_arg(matcher, opt);
                 if attached_value.is_some() {
                     ParseResult::AttachedValueNotConsumed
                 } else {
@@ -1333,6 +1328,7 @@ impl<'help, 'app> Parser<'help, 'app> {
                 fv,
                 fv.starts_with("=")
             );
+            self.inc_occurrence_of_arg(matcher, opt);
             self.add_val_to_arg(
                 opt,
                 v,
@@ -1341,7 +1337,6 @@ impl<'help, 'app> Parser<'help, 'app> {
                 false,
                 trailing_values,
             );
-            self.inc_occurrence_of_arg(matcher, opt);
             ParseResult::ValuesDone
         } else {
             debug!("Parser::parse_opt: More arg vals required...");
@@ -1463,78 +1458,40 @@ impl<'help, 'app> Parser<'help, 'app> {
         matcher.add_index_to(&arg.id, self.cur_idx.get(), ty);
     }
 
-    fn arg_have_val(&self, matcher: &mut ArgMatcher, arg: &Arg<'help>) -> bool {
+    fn has_val_groups(&self, matcher: &mut ArgMatcher, arg: &Arg<'help>) -> bool {
-        matcher.arg_have_val(&arg.id)
+        matcher.has_val_groups(&arg.id)
     }
 
     fn parse_flag(&self, flag: &Arg<'help>, matcher: &mut ArgMatcher) -> ParseResult {
         debug!("Parser::parse_flag");
 
-        matcher.add_index_to(&flag.id, self.cur_idx.get(), ValueType::CommandLine);
         self.inc_occurrence_of_arg(matcher, flag);
+        matcher.add_index_to(&flag.id, self.cur_idx.get(), ValueType::CommandLine);
 
         ParseResult::ValuesDone
     }
 
-    fn remove_overrides(&mut self, matcher: &mut ArgMatcher) {
+    fn remove_overrides(&self, arg: &Arg<'help>, matcher: &mut ArgMatcher) {
-        debug!("Parser::remove_overrides");
+        debug!("Parser::remove_overrides: id={:?}", arg.id);
-        let mut to_rem: Vec<Id> = Vec::new();
+        for override_id in &arg.overrides {
-        let mut self_override: Vec<Id> = Vec::new();
+            debug!("Parser::remove_overrides:iter:{:?}: removing", override_id);
-        let mut arg_overrides = Vec::new();
+            matcher.remove(override_id);
-        for name in matcher.arg_names() {
+            self.overridden.borrow_mut().push(override_id.clone());
-            debug!("Parser::remove_overrides:iter:{:?}", name);
+        }
-            if let Some(overrider) = self.app.find(name) {
+
-                let mut override_self = false;
+        // Override anything that can override us
-                for overridee in &overrider.overrides {
+        let mut transitive = Vec::new();
-                    debug!(
+        for arg_id in matcher.arg_names() {
-                        "Parser::remove_overrides:iter:{:?} => {:?}",
+            if let Some(overrider) = self.app.find(arg_id) {
-                        name, overrider
+                if overrider.overrides.contains(&arg.id) {
-                    );
+                    transitive.push(&overrider.id);
-                    if *overridee == overrider.id {
-                        override_self = true;
-                    } else {
-                        arg_overrides.push((overrider.id.clone(), overridee));
-                        arg_overrides.push((overridee.clone(), &overrider.id));
-                    }
-                }
-                // Only do self override for argument that is not positional
-                // argument or flag with MultipleOccurrences setting enabled.
-                if (self.is_set(AS::AllArgsOverrideSelf) || override_self)
-                    && !overrider.is_set(ArgSettings::MultipleOccurrences)
-                    && !overrider.is_positional()
-                {
-                    debug!(
-                        "Parser::remove_overrides:iter:{:?}:iter:{:?}: self override",
-                        name, overrider
-                    );
-                    self_override.push(overrider.id.clone());
                 }
             }
         }
-
+        for overrider_id in transitive {
-        // remove future overrides in reverse seen order
+            debug!("Parser::remove_overrides:iter:{:?}: removing", overrider_id);
-        for arg in self.seen.iter().rev() {
+            matcher.remove(overrider_id);
-            for (a, overr) in arg_overrides.iter().filter(|(a, _)| a == arg) {
+            self.overridden.borrow_mut().push(overrider_id.clone());
-                if !to_rem.contains(a) {
-                    to_rem.push((*overr).clone());
-                }
-            }
-        }
-
-        // Do self overrides
-        for name in &self_override {
-            debug!("Parser::remove_overrides:iter:self:{:?}: resetting", name);
-            if let Some(ma) = matcher.get_mut(name) {
-                ma.occurs = 1;
-                ma.override_vals();
-            }
-        }
-
-        // Finally remove conflicts
-        for name in &to_rem {
-            debug!("Parser::remove_overrides:iter:{:?}: removing", name);
-            matcher.remove(name);
-            self.overridden.push(name.clone());
         }
     }
 
@@ -1638,7 +1595,7 @@ impl<'help, 'app> Parser<'help, 'app> {
                 arg.name
             );
             match matcher.get(&arg.id) {
-                Some(ma) if ma.is_vals_empty() => {
+                Some(ma) if ma.all_val_groups_empty() => {
                     debug!(
                         "Parser::add_value:iter:{}: has no user defined vals",
                         arg.name
@@ -1732,6 +1689,9 @@ impl<'help, 'app> Parser<'help, 'app> {
 
     /// Increase occurrence of specific argument and the grouped arg it's in.
     fn inc_occurrence_of_arg(&self, matcher: &mut ArgMatcher, arg: &Arg<'help>) {
+        // With each new occurrence, remove overrides from prior occurrences
+        self.remove_overrides(arg, matcher);
+
         matcher.inc_occurrence_of_arg(arg);
         // Increment or create the group "args"
         for group in self.app.groups_for_arg(&arg.id) {

src/parse/validator.rs

@@ -42,7 +42,7 @@ impl<'help, 'app, 'parser> Validator<'help, 'app, 'parser> {
             let o = &self.p.app[&a];
             reqs_validated = true;
             let should_err = if let Some(v) = matcher.0.args.get(&o.id) {
-                v.is_vals_empty() && !(o.min_vals.is_some() && o.min_vals.unwrap() == 0)
+                v.all_val_groups_empty() && !(o.min_vals.is_some() && o.min_vals.unwrap() == 0)
             } else {
                 true
             };
@@ -396,7 +396,7 @@ impl<'help, 'app, 'parser> Validator<'help, 'app, 'parser> {
         };
         // Issue 665 (https://github.com/clap-rs/clap/issues/665)
         // Issue 1105 (https://github.com/clap-rs/clap/issues/1105)
-        if a.is_set(ArgSettings::TakesValue) && !min_vals_zero && ma.is_vals_empty() {
+        if a.is_set(ArgSettings::TakesValue) && !min_vals_zero && ma.all_val_groups_empty() {
             return Err(Error::empty_value(
                 self.p.app,
                 a,
@@ -457,7 +457,7 @@ impl<'help, 'app, 'parser> Validator<'help, 'app, 'parser> {
 
     fn is_missing_required_ok(&self, a: &Arg<'help>, matcher: &ArgMatcher) -> bool {
         debug!("Validator::is_missing_required_ok: {}", a.name);
-        self.validate_arg_conflicts(a, matcher) || self.p.overridden.contains(&a.id)
+        self.validate_arg_conflicts(a, matcher) || self.p.overridden.borrow().contains(&a.id)
     }
 
     fn validate_arg_conflicts(&self, a: &Arg<'help>, matcher: &ArgMatcher) -> bool {

tests/builder/posix_compatible.rs

@@ -387,3 +387,15 @@ fn issue_1374_overrides_self_with_multiple_values() {
         &["c", "d"]
     );
 }
+
+#[test]
+fn incremental_override() {
+    let mut app = App::new("test")
+        .arg(arg!(--name <NAME>).multiple_occurrences(true))
+        .arg(arg!(--"no-name").overrides_with("name"));
+    let m = app
+        .try_get_matches_from_mut(&["test", "--name=ahmed", "--no-name", "--name=ali"])
+        .unwrap();
+    assert_eq!(m.values_of("name").unwrap().collect::<Vec<_>>(), &["ali"]);
+    assert!(!m.is_present("no-name"));
+}