From ac6439191011a72eb2fab26cf22e2165ac212e0b Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 6 Jun 2022 10:15:28 -0500 Subject: [PATCH 1/2] refactor(parser): Remove dead code --- src/parser/validator.rs | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/src/parser/validator.rs b/src/parser/validator.rs index dc4c3598fd1..7277ff65ed8 100644 --- a/src/parser/validator.rs +++ b/src/parser/validator.rs @@ -88,12 +88,7 @@ impl<'help, 'cmd> Validator<'help, 'cmd> { Ok(()) } - fn validate_arg_values( - &self, - arg: &Arg, - ma: &MatchedArg, - matcher: &ArgMatcher, - ) -> ClapResult<()> { + fn validate_arg_values(&self, arg: &Arg, ma: &MatchedArg) -> ClapResult<()> { debug!("Validator::validate_arg_values: arg={:?}", arg.name); for val in ma.raw_vals_flatten() { if !arg.possible_vals.is_empty() { @@ -121,7 +116,7 @@ impl<'help, 'cmd> Validator<'help, 'cmd> { } { #![allow(deprecated)] - if arg.is_forbid_empty_values_set() && val.is_empty() && matcher.contains(&arg.id) { + if arg.is_forbid_empty_values_set() && val.is_empty() { debug!("Validator::validate_arg_values: illegal empty val found"); return Err(Error::empty_value( self.cmd, @@ -327,7 +322,7 @@ impl<'help, 'cmd> Validator<'help, 'cmd> { ); if let Some(arg) = self.cmd.find(name) { self.validate_arg_num_vals(arg, ma)?; - self.validate_arg_values(arg, ma, matcher)?; + self.validate_arg_values(arg, ma)?; self.validate_arg_num_occurs(arg, ma)?; } Ok(()) From 0f1de7303d82e80499f6293b99157a49719fc4df Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 6 Jun 2022 10:52:41 -0500 Subject: [PATCH 2/2] fix(validator): Ignore defaults for requireds This is a follow up to #3420. Its easy to overlook this because it is only useful for the conditionals (we actually prevent applying unconditional defaults to unconditional requireds). This became apparent with the increased use of defaults with `SetTrue`. As always, there is the question of when is a bug fix a breaking change. I'm going to consider this safe since we prevent some instances of this from even happening and we already did #3420 and this is in line with those. --- src/output/usage.rs | 14 +++++++++++--- src/parser/validator.rs | 17 ++++++++++++----- tests/builder/action.rs | 6 +++--- 3 files changed, 26 insertions(+), 11 deletions(-) diff --git a/src/output/usage.rs b/src/output/usage.rs index 5cd6ba9e984..7adaf58c6eb 100644 --- a/src/output/usage.rs +++ b/src/output/usage.rs @@ -395,7 +395,13 @@ impl<'help, 'cmd> Usage<'help, 'cmd> { .filter(|name| !self.cmd.get_positionals().any(|p| &&p.id == name)) .filter(|name| !self.cmd.get_groups().any(|g| &&g.id == name)) .filter(|name| !args_in_groups.contains(name)) - .filter(|name| !(matcher.is_some() && matcher.as_ref().unwrap().contains(name))) + .filter(|name| { + !(matcher.is_some() + && matcher + .as_ref() + .unwrap() + .check_explicit(name, ArgPredicate::IsPresent)) + }) { debug!("Usage::get_required_usage_from:iter:{:?}", a); let arg = self.cmd.find(a).expect(INTERNAL_ERROR_MSG).to_string(); @@ -412,7 +418,7 @@ impl<'help, 'cmd> Usage<'help, 'cmd> { .cmd .unroll_args_in_group(g) .iter() - .any(|arg| m.contains(arg)); + .any(|arg| m.check_explicit(arg, ArgPredicate::IsPresent)); if have_group_entry { continue; } @@ -429,7 +435,9 @@ impl<'help, 'cmd> Usage<'help, 'cmd> { .iter() .chain(incls.iter()) .filter(|a| self.cmd.get_positionals().any(|p| &&p.id == a)) - .filter(|&pos| matcher.map_or(true, |m| !m.contains(pos))) + .filter(|&pos| { + matcher.map_or(true, |m| !m.check_explicit(pos, ArgPredicate::IsPresent)) + }) .filter_map(|pos| self.cmd.find(pos)) .filter(|&pos| incl_last || !pos.is_last_set()) .filter(|pos| !args_in_groups.contains(&pos.id)) diff --git a/src/parser/validator.rs b/src/parser/validator.rs index 7277ff65ed8..a7456f917df 100644 --- a/src/parser/validator.rs +++ b/src/parser/validator.rs @@ -471,7 +471,11 @@ impl<'help, 'cmd> Validator<'help, 'cmd> { is_exclusive_present ); - for arg_or_group in self.required.iter().filter(|r| !matcher.contains(r)) { + for arg_or_group in self + .required + .iter() + .filter(|r| !matcher.check_explicit(r, ArgPredicate::IsPresent)) + { debug!("Validator::validate_required:iter:aog={:?}", arg_or_group); if let Some(arg) = self.cmd.find(arg_or_group) { debug!("Validator::validate_required:iter: This is an arg"); @@ -484,7 +488,7 @@ impl<'help, 'cmd> Validator<'help, 'cmd> { .cmd .unroll_args_in_group(&group.id) .iter() - .any(|a| matcher.contains(a)) + .any(|a| matcher.check_explicit(a, ArgPredicate::IsPresent)) { return self.missing_required_error(matcher, vec![]); } @@ -495,7 +499,7 @@ impl<'help, 'cmd> Validator<'help, 'cmd> { for a in self.cmd.get_arguments() { for (other, val) in &a.r_ifs { if matcher.check_explicit(other, ArgPredicate::Equals(std::ffi::OsStr::new(*val))) - && !matcher.contains(&a.id) + && !matcher.check_explicit(&a.id, ArgPredicate::IsPresent) { return self.missing_required_error(matcher, vec![a.id.clone()]); } @@ -504,7 +508,10 @@ impl<'help, 'cmd> Validator<'help, 'cmd> { let match_all = a.r_ifs_all.iter().all(|(other, val)| { matcher.check_explicit(other, ArgPredicate::Equals(std::ffi::OsStr::new(*val))) }); - if match_all && !a.r_ifs_all.is_empty() && !matcher.contains(&a.id) { + if match_all + && !a.r_ifs_all.is_empty() + && !matcher.check_explicit(&a.id, ArgPredicate::IsPresent) + { return self.missing_required_error(matcher, vec![a.id.clone()]); } } @@ -532,7 +539,7 @@ impl<'help, 'cmd> Validator<'help, 'cmd> { .get_arguments() .filter(|&a| { (!a.r_unless.is_empty() || !a.r_unless_all.is_empty()) - && !matcher.contains(&a.id) + && !matcher.check_explicit(&a.id, ArgPredicate::IsPresent) && self.fails_arg_required_unless(a, matcher) }) .map(|a| a.id.clone()) diff --git a/tests/builder/action.rs b/tests/builder/action.rs index a6f73d2471e..5c08590f06a 100644 --- a/tests/builder/action.rs +++ b/tests/builder/action.rs @@ -202,9 +202,9 @@ fn set_true_with_required_if_eq() { assert_eq!(*matches.get_one::("dog").unwrap(), false); assert_eq!(*matches.get_one::("mammal").unwrap(), true); - let matches = cmd.clone().try_get_matches_from(["test", "--dog"]).unwrap(); - assert_eq!(*matches.get_one::("dog").unwrap(), true); - assert_eq!(*matches.get_one::("mammal").unwrap(), false); + cmd.clone() + .try_get_matches_from(["test", "--dog"]) + .unwrap_err(); let matches = cmd .clone()