From 771a44bcef145e61a2d5bbc9cefd18d9561d5570 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 27 Dec 2021 12:46:55 -0600 Subject: [PATCH] fix: Remove overriden occurrences as we go This allows two overriding args to interact with each other mid-line. This was broken in 7673dfc / #1154. Before that, we duplicated the override logic in several places. Now we plug into the start of a new arg which allows us to do this incrementally without making the logic complex or inefficient, thanks to prior refactors. Fixes #3217 --- src/parse/matches/matched_arg.rs | 48 ------------------ src/parse/parser.rs | 82 +++++++++---------------------- src/parse/validator.rs | 2 +- tests/builder/posix_compatible.rs | 12 +++++ 4 files changed, 37 insertions(+), 107 deletions(-) diff --git a/src/parse/matches/matched_arg.rs b/src/parse/matches/matched_arg.rs index 90752fb4..e453b1e1 100644 --- a/src/parse/matches/matched_arg.rs +++ b/src/parse/matches/matched_arg.rs @@ -108,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 { @@ -162,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 86a45b36..d490f851 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, @@ -657,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(), @@ -696,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) } @@ -1475,60 +1471,27 @@ impl<'help, 'app> Parser<'help, 'app> { 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)); - } - } - if override_self { - 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()); + } + + // 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); } } } - - // 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()); - } - } - } - - // 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()); } } @@ -1726,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 3098b879..7e92ab20 100644 --- a/src/parse/validator.rs +++ b/src/parse/validator.rs @@ -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 f207d82b..ea2437df 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")); +}