Repository: clap-rs/clap
Pull request #2172 (2020-10-13 21:29:05)

Don't use default value when option is passed
This PR changes `default_value` so that it does not apply for an option when the user passes the flag without a corresponding value. This case is now handled by `default_missing_value`.

(See also my discussion question https://github.com/clap-rs/clap/discussions/2083 where I understood that this is the expected behaviour.)

Consider this sample program:

```
#[derive(Clap, Debug)]
struct Opts {
    #[clap(long, default_value = "5")]
    batch_size: usize
}
```

With the current master, I get the following results of execution:

```
cargo run                    // OK
batch_size = 5

cargo run -- --batch-size    // <---This should not succeed, as default_missing_value is not set
batch_size = 5

cargo run -- --batch-size 6  // OK
batch_size = 6
```

After this patch, everything works as expected:

```
cargo run
batch_size = 5

cargo run -- --batch-size
error: The argument '--batch-size <batch-size>' requires a value but none was supplied

cargo run -- --batch-size 6
batch_size = 6
```




Patch:
diff --git a/clap_derive/examples/keyvalue.rs b/clap_derive/examples/keyvalue.rs
index c4724ca6..5bf952fb 100644
--- a/clap_derive/examples/keyvalue.rs
+++ b/clap_derive/examples/keyvalue.rs
@@ -4,12 +4,12 @@ use clap::Clap;
 use std::error::Error;
 
 /// Parse a single key-value pair
-fn parse_key_val<T, U>(s: &str) -> Result<(T, U), Box<dyn Error + Send + Sync + 'static>>
+fn parse_key_val<T, U>(s: &str) -> Result<(T, U), Box<dyn Error>>
 where
     T: std::str::FromStr,
-    T::Err: Error + Send + Sync + 'static,
+    T::Err: Error + 'static,
     U: std::str::FromStr,
-    U::Err: Error + Send + Sync + 'static,
+    U::Err: Error + 'static,
 {
     let pos = s
         .find('=')
diff --git a/clap_derive/src/derives/into_app.rs b/clap_derive/src/derives/into_app.rs
index 01baca22..4570f749 100644
--- a/clap_derive/src/derives/into_app.rs
+++ b/clap_derive/src/derives/into_app.rs
@@ -227,6 +227,7 @@ pub fn gen_app_augmentation(
                         .validator(|s| {
                             #func(s)
                             .map(|_: #convert_type| ())
+                            .map_err(|e| e.to_string())
                         })
                     },
                     ParserKind::TryFromOsStr => quote_spanned! { func.span()=>
diff --git a/src/build/arg/mod.rs b/src/build/arg/mod.rs
index 1c555444..a1cfd98e 100644
--- a/src/build/arg/mod.rs
+++ b/src/build/arg/mod.rs
@@ -11,7 +11,6 @@ use std::{
     borrow::Cow,
     cmp::{Ord, Ordering},
     env,
-    error::Error,
     ffi::{OsStr, OsString},
     fmt::{self, Display, Formatter},
     str,
@@ -39,8 +38,8 @@ use crate::{
 #[cfg(feature = "yaml")]
 use yaml_rust::Yaml;
 
-type Validator<'a> = dyn FnMut(&str) -> Result<(), Box<dyn Error + Send + Sync>> + Send + 'a;
-type ValidatorOs<'a> = dyn FnMut(&OsStr) -> Result<(), Box<dyn Error + Send + Sync>> + Send + 'a;
+type Validator<'a> = dyn FnMut(&str) -> Result<(), String> + Send + 'a;
+type ValidatorOs<'a> = dyn FnMut(&OsStr) -> Result<(), String> + Send + 'a;
 
 /// The abstract representation of a command line argument. Used to set all the options and
 /// relationships that define a valid argument for the program.
@@ -1880,10 +1879,10 @@ impl<'help> Arg<'help> {
     pub fn validator<F, O, E>(mut self, mut f: F) -> Self
     where
         F: FnMut(&str) -> Result<O, E> + Send + 'help,
-        E: Into<Box<dyn Error + Send + Sync + 'static>>,
+        E: ToString,
     {
         self.validator = Some(Arc::new(Mutex::new(move |s: &str| {
-            f(s).map(|_| ()).map_err(|e| e.into())
+            f(s).map(|_| ()).map_err(|e| e.to_string())
         })));
         self
     }
@@ -1918,14 +1917,11 @@ impl<'help> Arg<'help> {
     /// [`Result`]: https://doc.rust-lang.org/std/result/enum.Result.html
     /// [`Err(String)`]: https://doc.rust-lang.org/std/result/enum.Result.html#variant.Err
     /// [`Rc`]: https://doc.rust-lang.org/std/rc/struct.Rc.html
-    pub fn validator_os<F, O, E>(mut self, mut f: F) -> Self
+    pub fn validator_os<F, O>(mut self, mut f: F) -> Self
     where
-        F: FnMut(&OsStr) -> Result<O, E> + Send + 'help,
-        E: Into<Box<dyn Error + Send + Sync + 'static>>,
+        F: FnMut(&OsStr) -> Result<O, String> + Send + 'help,
     {
-        self.validator_os = Some(Arc::new(Mutex::new(move |s: &OsStr| {
-            f(s).map(|_| ()).map_err(|e| e.into())
-        })));
+        self.validator_os = Some(Arc::new(Mutex::new(move |s: &OsStr| f(s).map(|_| ()))));
         self
     }
 
diff --git a/src/parse/errors.rs b/src/parse/errors.rs
index dfd01a7c..e44b6807 100644
--- a/src/parse/errors.rs
+++ b/src/parse/errors.rs
@@ -385,7 +385,6 @@ pub struct Error {
     /// Additional information depending on the error kind, like values and argument names.
     /// Useful when you want to render an error of your own.
     pub info: Vec<String>,
-    pub(crate) source: Option<Box<dyn error::Error + Send + Sync>>,
 }
 
 impl Display for Error {
@@ -479,7 +478,6 @@ impl Error {
             message: c,
             kind: ErrorKind::ArgumentConflict,
             info,
-            source: None,
         }
     }
 
@@ -497,7 +495,6 @@ impl Error {
             message: c,
             kind: ErrorKind::EmptyValue,
             info: vec![arg],
-            source: None,
         }
     }
 
@@ -560,7 +557,6 @@ impl Error {
             message: c,
             kind: ErrorKind::InvalidValue,
             info: vec![],
-            source: None,
         }
     }
 
@@ -591,7 +587,6 @@ impl Error {
             message: c,
             kind: ErrorKind::InvalidSubcommand,
             info: vec![subcmd],
-            source: None,
         }
     }
 
@@ -613,7 +608,6 @@ impl Error {
             message: c,
             kind: ErrorKind::UnrecognizedSubcommand,
             info: vec![subcmd],
-            source: None,
         }
     }
 
@@ -643,7 +637,6 @@ impl Error {
             message: c,
             kind: ErrorKind::MissingRequiredArgument,
             info,
-            source: None,
         }
     }
 
@@ -660,7 +653,6 @@ impl Error {
             message: c,
             kind: ErrorKind::MissingSubcommand,
             info: vec![],
-            source: None,
         }
     }
 
@@ -678,7 +670,6 @@ impl Error {
             message: c,
             kind: ErrorKind::InvalidUtf8,
             info: vec![],
-            source: None,
         }
     }
 
@@ -702,7 +693,6 @@ impl Error {
             message: c,
             kind: ErrorKind::TooManyValues,
             info: vec![arg.to_string(), val],
-            source: None,
         }
     }
 
@@ -730,14 +720,13 @@ impl Error {
             message: c,
             kind: ErrorKind::TooFewValues,
             info: vec![arg.to_string(), curr_vals.to_string(), min_vals.to_string()],
-            source: None,
         }
     }
 
     pub(crate) fn value_validation(
         arg: String,
         val: String,
-        err: Box<dyn error::Error + Send + Sync>,
+        err: String,
         color: ColorChoice,
     ) -> Self {
         let mut c = Colorizer::new(true, color);
@@ -754,8 +743,7 @@ impl Error {
         Error {
             message: c,
             kind: ErrorKind::ValueValidation,
-            info: vec![arg, val, err.to_string()],
-            source: Some(err),
+            info: vec![arg, val, err],
         }
     }
 
@@ -783,7 +771,6 @@ impl Error {
             message: c,
             kind: ErrorKind::WrongNumberOfValues,
             info: vec![arg.to_string(), curr_vals.to_string(), num_vals.to_string()],
-            source: None,
         }
     }
 
@@ -801,7 +788,6 @@ impl Error {
             message: c,
             kind: ErrorKind::UnexpectedMultipleUsage,
             info: vec![arg],
-            source: None,
         }
     }
 
@@ -850,7 +836,6 @@ impl Error {
             message: c,
             kind: ErrorKind::UnknownArgument,
             info: vec![arg],
-            source: None,
         }
     }
 
@@ -872,7 +857,6 @@ impl Error {
             message: c,
             kind: ErrorKind::UnknownArgument,
             info: vec![arg],
-            source: None,
         }
     }
 
@@ -888,7 +872,6 @@ impl Error {
             message: c,
             kind: ErrorKind::ArgumentNotFound,
             info: vec![arg],
-            source: None,
         }
     }
 
@@ -905,7 +888,6 @@ impl Error {
             message: c,
             kind,
             info: vec![],
-            source: None,
         }
     }
 }
@@ -922,12 +904,4 @@ impl From<fmt::Error> for Error {
     }
 }
 
-impl error::Error for Error {
-    fn source(&self) -> Option<&(dyn error::Error + 'static)> {
-        if let Some(source) = self.source.as_deref() {
-            Some(source)
-        } else {
-            None
-        }
-    }
-}
+impl error::Error for Error {}
diff --git a/src/parse/matches/arg_matches.rs b/src/parse/matches/arg_matches.rs
index c7d06e69..3fc3362b 100644
--- a/src/parse/matches/arg_matches.rs
+++ b/src/parse/matches/arg_matches.rs
@@ -351,12 +351,7 @@ impl ArgMatches {
                     v, name, e
                 );
 
-                Error::value_validation(
-                    name.to_string(),
-                    v.to_string(),
-                    message.into(),
-                    ColorChoice::Auto,
-                )
+                Error::value_validation(name.to_string(), v.to_string(), message, ColorChoice::Auto)
             })
         } else {
             Err(Error::argument_not_found_auto(name.to_string()))
@@ -444,7 +439,7 @@ impl ArgMatches {
                     Error::value_validation(
                         name.to_string(),
                         v.to_string(),
-                        message.into(),
+                        message,
                         ColorChoice::Auto,
                     )
                 })
diff --git a/src/parse/parser.rs b/src/parse/parser.rs
index 47bbc2e0..1d5d9c50 100644
--- a/src/parse/parser.rs
+++ b/src/parse/parser.rs
@@ -752,7 +752,6 @@ impl<'help, 'app> Parser<'help, 'app> {
                     message,
                     kind: ErrorKind::DisplayHelpOnMissingArgumentOrSubcommand,
                     info: vec![],
-                    source: None,
                 });
             }
         }
@@ -1613,23 +1612,8 @@ impl<'help, 'app> Parser<'help, 'app> {
 
         if !arg.default_vals.is_empty() {
             debug!("Parser::add_value:iter:{}: has default vals", arg.name);
-            if matcher
-                .get(&arg.id)
-                .map(|ma| ma.vals.len())
-                .map(|len| len == 0)
-                .unwrap_or(false)
-            {
-                debug!(
-                    "Parser::add_value:iter:{}: has no user defined vals",
-                    arg.name
-                );
-
-                for val in &arg.default_vals {
-                    self.add_val_to_arg(arg, &ArgStr::new(val), matcher, ty)?;
-                }
-            } else if matcher.get(&arg.id).is_some() {
-                debug!("Parser::add_value:iter:{}: has user defined vals", arg.name);
-
+            if matcher.get(&arg.id).is_some() {
+                debug!("Parser::add_value:iter:{}: was used", arg.name);
             // do nothing
             } else {
                 debug!("Parser::add_value:iter:{}: wasn't used", arg.name);
@@ -1647,6 +1631,39 @@ impl<'help, 'app> Parser<'help, 'app> {
             // do nothing
         }
 
+        if !arg.default_missing_vals.is_empty() {
+            debug!(
+                "Parser::add_value:iter:{}: has default missing vals",
+                arg.name
+            );
+            match matcher.get(&arg.id) {
+                Some(ma) if ma.vals.is_empty() => {
+                    debug!(
+                        "Parser::add_value:iter:{}: has no user defined vals",
+                        arg.name
+                    );
+                    for val in &arg.default_missing_vals {
+                        self.add_val_to_arg(arg, &ArgStr::new(val), matcher, ty)?;
+                    }
+                }
+                None => {
+                    debug!("Parser::add_value:iter:{}: wasn't used", arg.name);
+                    // do nothing
+                }
+                _ => {
+                    debug!("Parser::add_value:iter:{}: has user defined vals", arg.name);
+                    // do nothing
+                }
+            }
+        } else {
+            debug!(
+                "Parser::add_value:iter:{}: doesn't have default missing vals",
+                arg.name
+            );
+
+            // do nothing
+        }
+
         Ok(())
     }
 
@@ -1748,7 +1765,6 @@ impl<'help, 'app> Parser<'help, 'app> {
                 message: c,
                 kind: ErrorKind::DisplayHelp,
                 info: vec![],
-                source: None,
             },
         }
     }
@@ -1763,7 +1779,6 @@ impl<'help, 'app> Parser<'help, 'app> {
             message: c,
             kind: ErrorKind::DisplayVersion,
             info: vec![],
-            source: None,
         }
     }
 }
diff --git a/src/parse/validator.rs b/src/parse/validator.rs
index 2ba7c798..8f63d525 100644
--- a/src/parse/validator.rs
+++ b/src/parse/validator.rs
@@ -62,7 +62,6 @@ impl<'help, 'app, 'parser> Validator<'help, 'app, 'parser> {
                 message,
                 kind: ErrorKind::DisplayHelpOnMissingArgumentOrSubcommand,
                 info: vec![],
-                source: None,
             });
         }
         self.validate_conflicts(matcher)?;


Test patch:
diff --git a/clap_derive/tests/custom-string-parsers.rs b/clap_derive/tests/custom-string-parsers.rs
index 8c6f8ad0..d127cac9 100644
--- a/clap_derive/tests/custom-string-parsers.rs
+++ b/clap_derive/tests/custom-string-parsers.rs
@@ -90,15 +90,7 @@ fn test_parse_hex() {
 fn custom_parser_1(_: &str) -> &'static str {
     "A"
 }
-#[derive(Debug)]
-struct ErrCode(u32);
-impl std::error::Error for ErrCode {}
-impl std::fmt::Display for ErrCode {
-    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        std::fmt::Display::fmt(&self.0, f)
-    }
-}
-fn custom_parser_2(_: &str) -> Result<&'static str, ErrCode> {
+fn custom_parser_2(_: &str) -> Result<&'static str, u32> {
     Ok("B")
 }
 fn custom_parser_3(_: &OsStr) -> &'static str {
diff --git a/tests/default_vals.rs b/tests/default_vals.rs
index 3c1866d0..fd6e20d6 100644
--- a/tests/default_vals.rs
+++ b/tests/default_vals.rs
@@ -11,6 +11,19 @@ fn opts() {
     assert_eq!(m.value_of("o").unwrap(), "default");
 }
 
+#[test]
+fn opt_without_value_fail() {
+    let r = App::new("df")
+        .arg(Arg::from("-o [opt] 'some opt'").default_value("default"))
+        .try_get_matches_from(vec!["", "-o"]);
+    assert!(r.is_err());
+    let err = r.unwrap_err();
+    assert_eq!(err.kind, ErrorKind::EmptyValue);
+    assert!(err
+        .to_string()
+        .contains("The argument '-o <opt>' requires a value but none was supplied"));
+}
+
 #[test]
 fn opt_user_override() {
     let r = App::new("df")
diff --git a/tests/opts.rs b/tests/opts.rs
index fcd4d73e..f726354f 100644
--- a/tests/opts.rs
+++ b/tests/opts.rs
@@ -402,7 +402,7 @@ fn issue_1047_min_zero_vals_default_val() {
                 .long("del")
                 .setting(ArgSettings::RequireEquals)
                 .min_values(0)
-                .default_value("default"),
+                .default_missing_value("default"),
         )
         .get_matches_from(vec!["foo", "-d"]);
     assert_eq!(m.occurrences_of("del"), 1);