diff --git a/src/build/app/mod.rs b/src/build/app/mod.rs index b73ce2a7495..8abb8e35d48 100644 --- a/src/build/app/mod.rs +++ b/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); diff --git a/src/build/arg/mod.rs b/src/build/arg/mod.rs index ff9d05ed7ae..ec887f44049 100644 --- a/src/build/arg/mod.rs +++ b/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 { diff --git a/src/parse/arg_matcher.rs b/src/parse/arg_matcher.rs index 1d2ce6cac8d..58a94059c46 100644 --- a/src/parse/arg_matcher.rs +++ b/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 { - matches!(self.entry(arg), Entry::Occupied(_)) + pub(crate) fn has_val_groups(&mut self, arg: &Id) -> bool { + 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 { diff --git a/src/parse/matches/matched_arg.rs b/src/parse/matches/matched_arg.rs index cd06c4718a1..e453b1e18ef 100644 --- a/src/parse/matches/matched_arg.rs +++ b/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> = 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> = 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(); diff --git a/src/parse/parser.rs b/src/parse/parser.rs index fd6e2243990..d490f851c25 100644 --- a/src/parse/parser.rs +++ b/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, - pub(crate) overridden: Vec, + pub(crate) overridden: RefCell>, pub(crate) seen: Vec, pub(crate) cur_idx: Cell, /// 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 { - matcher.arg_have_val(&arg.id) + fn has_val_groups(&self, matcher: &mut ArgMatcher, arg: &Arg<'help>) -> bool { + 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) { - debug!("Parser::remove_overrides"); - let mut to_rem: Vec = Vec::new(); - let mut self_override: Vec = Vec::new(); - let mut arg_overrides = Vec::new(); - for name in matcher.arg_names() { - debug!("Parser::remove_overrides:iter:{:?}", name); - if let Some(overrider) = self.app.find(name) { - let mut override_self = false; - for overridee in &overrider.overrides { - debug!( - "Parser::remove_overrides:iter:{:?} => {:?}", - name, overrider - ); - 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()); - } - } + fn remove_overrides(&self, arg: &Arg<'help>, matcher: &mut ArgMatcher) { + debug!("Parser::remove_overrides: id={:?}", arg.id); + for override_id in &arg.overrides { + debug!("Parser::remove_overrides:iter:{:?}: removing", override_id); + matcher.remove(override_id); + self.overridden.borrow_mut().push(override_id.clone()); } - // remove future overrides in reverse seen order - for arg in self.seen.iter().rev() { - for (a, overr) in arg_overrides.iter().filter(|(a, _)| a == arg) { - if !to_rem.contains(a) { - to_rem.push((*overr).clone()); + // Override anything that can override us + let mut transitive = Vec::new(); + for arg_id in matcher.arg_names() { + if let Some(overrider) = self.app.find(arg_id) { + if overrider.overrides.contains(&arg.id) { + transitive.push(&overrider.id); } } } - - // 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()); + for overrider_id in transitive { + debug!("Parser::remove_overrides:iter:{:?}: removing", overrider_id); + matcher.remove(overrider_id); + self.overridden.borrow_mut().push(overrider_id.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) { diff --git a/src/parse/validator.rs b/src/parse/validator.rs index 48dd46715f6..7e92ab206fc 100644 --- a/src/parse/validator.rs +++ b/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 { diff --git a/tests/builder/posix_compatible.rs b/tests/builder/posix_compatible.rs index f207d82b49a..ea2437df8de 100644 --- a/tests/builder/posix_compatible.rs +++ b/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 ).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::>(), &["ali"]); + assert!(!m.is_present("no-name")); +}