Repository: clap-rs/clap
Pull request #3793 (2022-06-06 16:20:29)

fix(validator): Ignore defaults for requireds
This is a follow up to https://github.com/clap-rs/clap/pull/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 https://github.com/clap-rs/clap/pull/3420 and this is in line with
those.


Patch:
diff --git a/src/output/usage.rs b/src/output/usage.rs
index 5cd6ba9e..7adaf58c 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 dc4c3598..a7456f91 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(())
@@ -476,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");
@@ -489,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![]);
                 }
@@ -500,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()]);
                 }
@@ -509,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()]);
             }
         }
@@ -537,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())


Test patch:
diff --git a/tests/builder/action.rs b/tests/builder/action.rs
index a6f73d24..5c08590f 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::<bool>("dog").unwrap(), false);
     assert_eq!(*matches.get_one::<bool>("mammal").unwrap(), true);
 
-    let matches = cmd.clone().try_get_matches_from(["test", "--dog"]).unwrap();
-    assert_eq!(*matches.get_one::<bool>("dog").unwrap(), true);
-    assert_eq!(*matches.get_one::<bool>("mammal").unwrap(), false);
+    cmd.clone()
+        .try_get_matches_from(["test", "--dog"])
+        .unwrap_err();
 
     let matches = cmd
         .clone()