Repository: clap-rs/clap
Pull request #3998 (2022-07-28 01:24:29)

fix(parser)!: Apply default_missing_value per occurrence
This both simplifies the code and the model we present to the user,
making more sense.

There is room for further exploration of tying flag actions into this.

<!--
Thanks for helping out!

Please link the appropriate issue from your PR.

If you don't have an issue, we'd recommend starting with one first so the PR can focus on the
implementation (unless its an obvious bug or documentation fix that will have
little conversation).
-->



Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index c14ae71f..eead80e8 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -37,6 +37,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 - Disallow more `value_names` than `number_of_values` (#2695)
 - Replaced `cmd.allow_invalid_for_utf8_external_subcommands` with `cmd.external_subcommand_value_parser` (#3733)
 - Changed the default type of `allow_external_subcommands` from `String` to `OsString`
+- `Arg::default_missing_value` now applies per occurrence rather than if a value is missing across all occurrences
 - *(assert)* Always enforce that version is specified when the `ArgAction::Version` is used
 - *(assert)* Add missing `#[track_caller]`s to make it easier to debug asserts
 - *(assert)* Ensure `overrides_with` IDs are valid
diff --git a/src/parser/parser.rs b/src/parser/parser.rs
index b3936dc8..79cf4445 100644
--- a/src/parser/parser.rs
+++ b/src/parser/parser.rs
@@ -964,25 +964,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
         if arg.is_require_equals_set() && !has_eq {
             if arg.min_vals == Some(0) {
                 debug!("Requires equals, but min_vals == 0");
-                let mut arg_values = Vec::new();
-                // We assume this case is valid: require equals, but min_vals == 0.
-                if !arg.default_missing_vals.is_empty() {
-                    debug!("Parser::parse_opt_value: has default_missing_vals");
-                    for v in arg.default_missing_vals.iter() {
-                        let trailing_values = false; // CLI should not be affecting default_missing_values
-                        let _parse_result = self.split_arg_values(
-                            arg,
-                            &RawOsStr::new(v),
-                            trailing_values,
-                            &mut arg_values,
-                        );
-                        if let Some(_parse_result) = _parse_result {
-                            if _parse_result != ParseResult::ValuesDone {
-                                debug!("Parser::parse_opt_value: Ignoring state {:?}; no values accepted after default_missing_values", _parse_result);
-                            }
-                        }
-                    }
-                };
+                let arg_values = Vec::new();
                 let react_result = self.react(
                     Some(ident),
                     ValueSource::CommandLine,
@@ -1133,11 +1115,32 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
         ident: Option<Identifier>,
         source: ValueSource,
         arg: &Arg<'help>,
-        raw_vals: Vec<OsString>,
+        mut raw_vals: Vec<OsString>,
         matcher: &mut ArgMatcher,
     ) -> ClapResult<ParseResult> {
         self.resolve_pending(matcher)?;
 
+        if raw_vals.is_empty() {
+            // We assume this case is valid: require equals, but min_vals == 0.
+            if !arg.default_missing_vals.is_empty() {
+                debug!("Parser::react: has default_missing_vals");
+                for v in arg.default_missing_vals.iter() {
+                    let trailing_values = false; // CLI should not be affecting default_missing_values
+                    let _parse_result = self.split_arg_values(
+                        arg,
+                        &RawOsStr::new(v),
+                        trailing_values,
+                        &mut raw_vals,
+                    );
+                    if let Some(_parse_result) = _parse_result {
+                        if _parse_result != ParseResult::ValuesDone {
+                            debug!("Parser::parse_opt_value: Ignoring state {:?}; no values accepted after default_missing_values", _parse_result);
+                        }
+                    }
+                }
+            };
+        }
+
         debug!(
             "Parser::react action={:?}, identifier={:?}, source={:?}",
             arg.get_action(),
@@ -1350,55 +1353,6 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
     fn add_default_value(&self, arg: &Arg<'help>, matcher: &mut ArgMatcher) -> ClapResult<()> {
         let trailing_values = false; // defaults are independent of the commandline
 
-        if !arg.default_missing_vals.is_empty() {
-            debug!(
-                "Parser::add_default_value:iter:{}: has default missing vals",
-                arg.name
-            );
-            match matcher.get(&arg.id) {
-                Some(ma) if ma.all_val_groups_empty() => {
-                    debug!(
-                        "Parser::add_default_value:iter:{}: has no user defined vals",
-                        arg.name
-                    );
-                    // The flag occurred, we just want to add the val groups
-                    let mut arg_values = Vec::new();
-                    for v in arg.default_missing_vals.iter() {
-                        let _parse_result = self.split_arg_values(
-                            arg,
-                            &RawOsStr::new(v),
-                            trailing_values,
-                            &mut arg_values,
-                        );
-                        if let Some(_parse_result) = _parse_result {
-                            if _parse_result != ParseResult::ValuesDone {
-                                debug!("Parser::add_default_value: Ignoring state {:?}; defaults are outside of the parse loop", _parse_result);
-                            }
-                        }
-                    }
-                    self.start_custom_arg(matcher, arg, ValueSource::CommandLine);
-                    self.push_arg_values(arg, arg_values, matcher)?;
-                }
-                None => {
-                    debug!("Parser::add_default_value:iter:{}: wasn't used", arg.name);
-                    // do nothing
-                }
-                _ => {
-                    debug!(
-                        "Parser::add_default_value:iter:{}: has user defined vals",
-                        arg.name
-                    );
-                    // do nothing
-                }
-            }
-        } else {
-            debug!(
-                "Parser::add_default_value:iter:{}: doesn't have default missing vals",
-                arg.name
-            );
-            // do nothing
-        }
-
         if !arg.default_vals_ifs.is_empty() {
             debug!("Parser::add_default_value: has conditional defaults");
             if !matcher.contains(&arg.id) {


Test patch:
diff --git a/tests/builder/default_missing_vals.rs b/tests/builder/default_missing_vals.rs
index 7f3b6c66..dad62f2e 100644
--- a/tests/builder/default_missing_vals.rs
+++ b/tests/builder/default_missing_vals.rs
@@ -143,6 +143,28 @@ fn opt_default_user_override() {
     );
 }
 
+#[test]
+fn default_missing_value_per_occurrence() {
+    // assert no change to usual argument handling when adding default_missing_value()
+    let r = Command::new("cmd")
+        .arg(
+            arg!(o: -o <opt> ... "some opt")
+                .min_values(0)
+                .default_value("default")
+                .default_missing_value("default_missing"),
+        )
+        .try_get_matches_from(vec!["", "-o", "-o=value", "-o"]);
+    assert!(r.is_ok(), "{}", r.unwrap_err());
+    let m = r.unwrap();
+    assert_eq!(
+        m.get_many::<String>("o")
+            .unwrap()
+            .map(|v| v.as_str())
+            .collect::<Vec<_>>(),
+        vec!["default_missing", "value", "default_missing"]
+    );
+}
+
 #[test]
 #[allow(clippy::bool_assert_comparison)]
 fn default_missing_value_flag_value() {