diff --git a/clap_derive/tests/non_literal_attributes.rs b/clap_derive/tests/non_literal_attributes.rs index 72e8b865594b..5a342c2c0e14 100644 --- a/clap_derive/tests/non_literal_attributes.rs +++ b/clap_derive/tests/non_literal_attributes.rs @@ -12,7 +12,7 @@ // commit#ea76fa1b1b273e65e3b0b1046643715b49bec51f which is licensed under the // MIT/Apache 2.0 license. -use clap::{AppSettings, Clap}; +use clap::{AppSettings, Clap, ErrorKind}; use std::num::ParseIntError; pub const DISPLAY_ORDER: usize = 2; @@ -121,6 +121,7 @@ fn test_bool() { ); let result = Opt::try_parse_from(&["test", "-l", "1", "--x", "1"]); assert!(result.is_err()); + assert_eq!(result.unwrap_err().kind, ErrorKind::NoEquals); } fn parse_hex(input: &str) -> Result { diff --git a/src/build/app/settings.rs b/src/build/app/settings.rs index 249d2b57d411..659ac598b70f 100644 --- a/src/build/app/settings.rs +++ b/src/build/app/settings.rs @@ -1041,6 +1041,7 @@ pub enum AppSettings { NoAutoVersion, #[doc(hidden)] + /// If the user already passed '--'. Meaning only positional args follow. TrailingValues, #[doc(hidden)] diff --git a/src/build/arg/mod.rs b/src/build/arg/mod.rs index e64d79d8f6c7..2dece162671d 100644 --- a/src/build/arg/mod.rs +++ b/src/build/arg/mod.rs @@ -3245,8 +3245,8 @@ impl<'help> Arg<'help> { /// assert!(res.is_ok()); /// ``` /// - /// Setting [`RequireEquals`] and *not* supplying the equals will cause an error - /// unless [`ArgSettings::EmptyValues`] is set. + /// Setting [`RequireEquals`] and *not* supplying the equals, the arg will emits + /// an error. /// /// ```rust /// # use clap::{App, Arg, ErrorKind, ArgSettings}; @@ -3259,16 +3259,13 @@ impl<'help> Arg<'help> { /// ]); /// /// assert!(res.is_err()); - /// assert_eq!(res.unwrap_err().kind, ErrorKind::EmptyValue); + /// assert_eq!(res.unwrap_err().kind, ErrorKind::NoEquals); /// ``` /// [`RequireEquals`]: ./enum.ArgSettings.html#variant.RequireEquals - /// [`ArgSettings::EmptyValues`]: ./enum.ArgSettings.html#variant.EmptyValues - /// [`ArgSettings::EmptyValues`]: ./enum.ArgSettings.html#variant.TakesValue #[inline] pub fn require_equals(self, r: bool) -> Self { if r { - self.unset_setting(ArgSettings::AllowEmptyValues) - .setting(ArgSettings::RequireEquals) + self.setting(ArgSettings::RequireEquals) } else { self.unset_setting(ArgSettings::RequireEquals) } @@ -3946,55 +3943,7 @@ impl<'help> Arg<'help> { self.multiple_occurrences(multi).multiple_values(multi) } - /// Allows an argument to accept explicitly empty values. An empty value must be specified at - /// the command line with an explicit `""`, `''`, or `--option=` - /// - /// **NOTE:** By default empty values are *not* allowed - /// - /// **NOTE:** Implicitly sets [`ArgSettings::TakesValue`] - /// - /// # Examples - /// - /// ```rust - /// # use clap::{App, Arg, ArgSettings}; - /// Arg::new("file") - /// .long("file") - /// .setting(ArgSettings::AllowEmptyValues) - /// # ; - /// ``` - /// The default is to *not* allow empty values. - /// - /// ```rust - /// # use clap::{App, Arg, ErrorKind, ArgSettings}; - /// let res = App::new("prog") - /// .arg(Arg::new("cfg") - /// .long("config") - /// .short('v') - /// .setting(ArgSettings::TakesValue)) - /// .try_get_matches_from(vec![ - /// "prog", "--config=" - /// ]); - /// - /// assert!(res.is_err()); - /// assert_eq!(res.unwrap_err().kind, ErrorKind::EmptyValue); - /// ``` - /// By adding this setting, we can allow empty values - /// - /// ```rust - /// # use clap::{App, Arg, ArgSettings}; - /// let res = App::new("prog") - /// .arg(Arg::new("cfg") - /// .long("config") - /// .short('v') - /// .setting(ArgSettings::AllowEmptyValues)) // implies TakesValue - /// .try_get_matches_from(vec![ - /// "prog", "--config=" - /// ]); - /// - /// assert!(res.is_ok()); - /// assert_eq!(res.unwrap().value_of("config"), None); - /// ``` - /// [`ArgSettings::TakesValue`]: ./enum.ArgSettings.html#variant.TakesValue + /// Placeholder documentation #[inline] pub fn multiple_values(self, multi: bool) -> Self { if multi { diff --git a/src/build/arg/settings.rs b/src/build/arg/settings.rs index 6b722b880c41..faea51238a97 100644 --- a/src/build/arg/settings.rs +++ b/src/build/arg/settings.rs @@ -8,7 +8,7 @@ bitflags! { struct Flags: u32 { const REQUIRED = 1; const MULTIPLE_OCC = 1 << 1; - const EMPTY_VALS = 1 << 2 | Self::TAKES_VAL.bits; + const NO_EMPTY_VALS = 1 << 2 | Self::TAKES_VAL.bits; const GLOBAL = 1 << 3; const HIDDEN = 1 << 4; const TAKES_VAL = 1 << 5; @@ -39,7 +39,7 @@ impl_settings! { ArgSettings, ArgFlags, Required("required") => Flags::REQUIRED, MultipleOccurrences("multipleoccurrences") => Flags::MULTIPLE_OCC, MultipleValues("multiplevalues") => Flags::MULTIPLE_VALS, - AllowEmptyValues("allowemptyvalues") => Flags::EMPTY_VALS, + NoEmptyValues("noemptyvalues") => Flags::NO_EMPTY_VALS, Hidden("hidden") => Flags::HIDDEN, TakesValue("takesvalue") => Flags::TAKES_VAL, UseValueDelimiter("usevaluedelimiter") => Flags::USE_DELIM, @@ -80,8 +80,57 @@ pub enum ArgSettings { MultipleValues, /// Allows an arg to appear multiple times MultipleOccurrences, - /// Allows an arg accept empty values such as `""` - AllowEmptyValues, + /// Don't allow an argument to accept explicitly empty values. An empty value + /// must be specified at the command line with an explicit `""`, `''`, or + /// `--option=` + /// + /// **NOTE:** By default empty values are allowed. + /// + /// **NOTE:** Implicitly sets [`ArgSettings::TakesValue`] + /// + /// # Examples + /// + /// ```rust + /// # use clap::{App, Arg, ArgSettings}; + /// Arg::new("file") + /// .long("file") + /// .setting(ArgSettings::NoEmptyValues) + /// # ; + /// ``` + /// The default is allowing empty values. + /// + /// ```rust + /// # use clap::{App, Arg, ErrorKind, ArgSettings}; + /// let res = App::new("prog") + /// .arg(Arg::new("cfg") + /// .long("config") + /// .short('v') + /// .setting(ArgSettings::TakesValue)) + /// .try_get_matches_from(vec![ + /// "prog", "--config=" + /// ]); + /// + /// assert!(res.is_ok()); + /// assert_eq!(res.unwrap().value_of("config"), None); + /// ``` + /// By adding this setting, we can allow empty values + /// + /// ```rust + /// # use clap::{App, Arg, ErrorKind, ArgSettings}; + /// let res = App::new("prog") + /// .arg(Arg::new("cfg") + /// .long("config") + /// .short('v') + /// .setting(ArgSettings::NoEmptyValues)) // implies TakesValue + /// .try_get_matches_from(vec![ + /// "prog", "--config=" + /// ]); + /// + /// assert!(res.is_err()); + /// assert_eq!(res.unwrap_err().kind, ErrorKind::EmptyValue); + /// ``` + /// [`ArgSettings::TakesValue`]: ./enum.ArgSettings.html#variant.TakesValue + NoEmptyValues, /// Hides an arg from the help message Hidden, /// Allows an argument to take a value (such as `--option value`) @@ -130,8 +179,8 @@ mod test { ArgSettings::AllowHyphenValues ); assert_eq!( - "allowemptyvalues".parse::().unwrap(), - ArgSettings::AllowEmptyValues + "noemptyvalues".parse::().unwrap(), + ArgSettings::NoEmptyValues ); assert_eq!( "hidepossiblevalues".parse::().unwrap(), diff --git a/src/parse/errors.rs b/src/parse/errors.rs index f472a9845f71..391cf12978bb 100644 --- a/src/parse/errors.rs +++ b/src/parse/errors.rs @@ -116,6 +116,7 @@ pub enum ErrorKind { /// let res = App::new("prog") /// .arg(Arg::new("color") /// .setting(ArgSettings::TakesValue) + /// .setting(ArgSettings::NoEmptyValues) /// .long("color")) /// .try_get_matches_from(vec!["prog", "--color="]); /// assert!(res.is_err()); @@ -123,6 +124,22 @@ pub enum ErrorKind { /// ``` EmptyValue, + /// Occurs when the user doesn't use equals for an option that requres equal + /// sign to provide values. + /// + /// ```rust + /// # use clap::{App, Arg, ErrorKind, ArgSettings}; + /// let res = App::new("prog") + /// .arg(Arg::new("color") + /// .setting(ArgSettings::TakesValue) + /// .setting(ArgSettings::RequireEquals) + /// .long("color")) + /// .try_get_matches_from(vec!["prog", "--color", "red"]); + /// assert!(res.is_err()); + /// assert_eq!(res.unwrap_err().kind, ErrorKind::NoEquals); + /// ``` + NoEquals, + /// Occurs when the user provides a value for an argument with a custom validation and the /// value fails that validation. /// @@ -504,6 +521,24 @@ impl Error { } } + pub(crate) fn no_equals(arg: &Arg, usage: String, color: ColorChoice) -> Self { + let mut c = Colorizer::new(true, color); + let arg = arg.to_string(); + + start_error(&mut c, "The argument '"); + c.warning(arg.clone()); + c.none("' requires equals on value providing."); + put_usage(&mut c, usage); + try_help(&mut c); + + Error { + message: c, + kind: ErrorKind::NoEquals, + info: vec![arg], + source: None, + } + } + pub(crate) fn invalid_value( bad_val: String, good_vals: &[G], diff --git a/src/parse/parser.rs b/src/parse/parser.rs index b1bf32364cb0..9c2ad7a1a443 100644 --- a/src/parse/parser.rs +++ b/src/parse/parser.rs @@ -1152,7 +1152,7 @@ impl<'help, 'app> Parser<'help, 'app> { // has_eq: --flag=value let mut has_eq = false; let no_val = val.is_none(); - let empty_vals = opt.is_set(ArgSettings::AllowEmptyValues); + let no_empty_vals = opt.is_set(ArgSettings::NoEmptyValues); let min_vals_zero = opt.min_vals.unwrap_or(1) == 0; let require_equals = opt.is_set(ArgSettings::RequireEquals); @@ -1160,7 +1160,7 @@ impl<'help, 'app> Parser<'help, 'app> { if let Some(fv) = val { has_eq = fv.starts_with("="); let v = fv.trim_start_n_matches(1, b'='); - if !empty_vals && (v.is_empty() || (require_equals && !has_eq)) { + if no_empty_vals && (v.is_empty() || (require_equals && !has_eq)) { debug!("Found Empty - Error"); return Err(ClapError::empty_value( opt, @@ -1175,24 +1175,32 @@ impl<'help, 'app> Parser<'help, 'app> { fv.starts_with("=") ); self.add_val_to_arg(opt, &v, matcher, ValueType::CommandLine, false); - } else if require_equals && !empty_vals && !min_vals_zero { - debug!("None, but requires equals...Error"); - return Err(ClapError::empty_value( - opt, - Usage::new(self).create_usage_with_title(&[]), - self.app.color(), - )); - } else if require_equals && min_vals_zero { - debug!("None and requires equals, but min_vals == 0"); - if !opt.default_missing_vals.is_empty() { - debug!("Parser::parse_opt: has default_missing_vals"); - let vals = opt - .default_missing_vals - .iter() - .map(OsString::from) - .collect(); - self.add_multiple_vals_to_arg(opt, vals, matcher, ValueType::CommandLine, false); - }; + } else if require_equals { + if min_vals_zero { + debug!("Requires equals, but min_vals == 0"); + if !opt.default_missing_vals.is_empty() { + debug!("Parser::parse_opt: has default_missing_vals"); + let vals = opt + .default_missing_vals + .iter() + .map(OsString::from) + .collect(); + self.add_multiple_vals_to_arg( + opt, + vals, + matcher, + ValueType::CommandLine, + false, + ); + }; + } else { + debug!("Requires equals but not provided. Error."); + return Err(ClapError::no_equals( + opt, + Usage::new(self).create_usage_with_title(&[]), + self.app.color(), + )); + } } else { debug!("None"); } diff --git a/src/parse/validator.rs b/src/parse/validator.rs index 6b30a2692fac..f1a43d77c507 100644 --- a/src/parse/validator.rs +++ b/src/parse/validator.rs @@ -125,9 +125,7 @@ impl<'help, 'app, 'parser> Validator<'help, 'app, 'parser> { )); } } - if !arg.is_set(ArgSettings::AllowEmptyValues) - && val.is_empty() - && matcher.contains(&arg.id) + if arg.is_set(ArgSettings::NoEmptyValues) && val.is_empty() && matcher.contains(&arg.id) { debug!("Validator::validate_arg_values: illegal empty val found"); return Err(Error::empty_value( diff --git a/tests/default_vals.rs b/tests/default_vals.rs index c361f16d7331..da6674a89a80 100644 --- a/tests/default_vals.rs +++ b/tests/default_vals.rs @@ -1,4 +1,4 @@ -use clap::{App, Arg, ErrorKind}; +use clap::{App, Arg, ArgSettings, ErrorKind}; #[test] fn opts() { @@ -14,7 +14,11 @@ fn opts() { #[test] fn opt_without_value_fail() { let r = App::new("df") - .arg(Arg::from("-o [opt] 'some opt'").default_value("default")) + .arg( + Arg::from("-o [opt] 'some opt'") + .default_value("default") + .setting(ArgSettings::NoEmptyValues), + ) .try_get_matches_from(vec!["", "-o"]); assert!(r.is_err()); let err = r.unwrap_err(); diff --git a/tests/empty_values.rs b/tests/empty_values.rs new file mode 100644 index 000000000000..dd951d14117a --- /dev/null +++ b/tests/empty_values.rs @@ -0,0 +1,123 @@ +mod utils; + +use clap::{App, Arg, ArgSettings, ErrorKind}; + +#[test] +fn empty_values() { + let m = App::new("config") + .arg(Arg::new("config").long("config").takes_value(true)) + .get_matches_from(&["config", "--config", ""]); + assert_eq!(m.value_of("config"), Some("")); +} + +#[test] +fn empty_values_with_equals() { + let m = App::new("config") + .arg(Arg::new("config").long("config").takes_value(true)) + .get_matches_from(&["config", "--config="]); + assert_eq!(m.value_of("config"), Some("")); + + let m = App::new("config") + .arg(Arg::new("config").short('c').takes_value(true)) + .get_matches_from(&["config", "-c="]); + assert_eq!(m.value_of("config"), Some("")) +} + +#[test] +fn no_empty_values() { + let m = App::new("config") + .arg( + Arg::new("config") + .long("config") + .takes_value(true) + .setting(ArgSettings::NoEmptyValues), + ) + .try_get_matches_from(&["config", "--config", ""]); + assert!(m.is_err()); + assert_eq!(m.unwrap_err().kind, ErrorKind::EmptyValue); + let m = App::new("config") + .arg( + Arg::new("config") + .short('c') + .takes_value(true) + .setting(ArgSettings::NoEmptyValues), + ) + .try_get_matches_from(&["config", "-c", ""]); + assert!(m.is_err()); + assert_eq!(m.unwrap_err().kind, ErrorKind::EmptyValue) +} + +#[test] +fn no_empty_values_with_equals() { + let m = App::new("config") + .arg( + Arg::new("config") + .long("config") + .takes_value(true) + .setting(ArgSettings::NoEmptyValues), + ) + .try_get_matches_from(&["config", "--config="]); + assert!(m.is_err()); + assert_eq!(m.unwrap_err().kind, ErrorKind::EmptyValue); + + let m = App::new("config") + .arg( + Arg::new("config") + .short('c') + .takes_value(true) + .setting(ArgSettings::NoEmptyValues), + ) + .try_get_matches_from(&["config", "-c="]); + assert!(m.is_err()); + assert_eq!(m.unwrap_err().kind, ErrorKind::EmptyValue); +} + +#[test] +fn no_empty_values_without_equals() { + let m = App::new("config") + .arg( + Arg::new("config") + .long("config") + .takes_value(true) + .setting(ArgSettings::NoEmptyValues), + ) + .try_get_matches_from(&["config", "--config"]); + assert!(m.is_err()); + assert_eq!(m.unwrap_err().kind, ErrorKind::EmptyValue); + + let m = App::new("config") + .arg( + Arg::new("config") + .short('c') + .takes_value(true) + .setting(ArgSettings::NoEmptyValues), + ) + .try_get_matches_from(&["config", "-c"]); + assert!(m.is_err()); + assert_eq!(m.unwrap_err().kind, ErrorKind::EmptyValue) +} + +#[test] +fn no_empty_values_without_equals_but_requires_equals() { + let app = App::new("config").arg( + Arg::new("config") + .long("config") + .takes_value(true) + .setting(ArgSettings::NoEmptyValues) + .setting(ArgSettings::RequireEquals), + ); + let m = app.clone().try_get_matches_from(&["config", "--config"]); + // Should error on no equals rather than empty value. + assert!(m.is_err()); + assert_eq!(m.unwrap_err().kind, ErrorKind::NoEquals); + + static NO_EUQALS_ERROR: &str = + "error: The argument '--config=' requires equals on value providing. + + USAGE: + config [OPTIONS] + + For more information try --help"; + + utils::compare_output(app, "config --config", NO_EUQALS_ERROR, true); +} diff --git a/tests/opts.rs b/tests/opts.rs index 0218f93feb1e..8c1f2314e651 100644 --- a/tests/opts.rs +++ b/tests/opts.rs @@ -34,12 +34,13 @@ fn require_equals_fail() { .arg( Arg::new("cfg") .setting(ArgSettings::RequireEquals) + .setting(ArgSettings::NoEmptyValues) .setting(ArgSettings::TakesValue) .long("config"), ) .try_get_matches_from(vec!["prog", "--config", "file.conf"]); assert!(res.is_err()); - assert_eq!(res.unwrap_err().kind, ErrorKind::EmptyValue); + assert_eq!(res.unwrap_err().kind, ErrorKind::NoEquals); } #[test] @@ -78,6 +79,7 @@ fn require_equals_no_empty_values_fail() { .arg( Arg::new("cfg") .setting(ArgSettings::RequireEquals) + .setting(ArgSettings::NoEmptyValues) .long("config"), ) .arg(Arg::new("some")) @@ -92,7 +94,6 @@ fn require_equals_empty_vals_pass() { .arg( Arg::new("cfg") .setting(ArgSettings::RequireEquals) - .setting(ArgSettings::AllowEmptyValues) .long("config"), ) .try_get_matches_from(vec!["prog", "--config="]); @@ -385,8 +386,10 @@ fn did_you_mean() { fn issue_665() { let res = App::new("tester") .arg("-v, --reroll-count=[N] 'Mark the patch series as PATCH vN'") - .arg(Arg::from( -"--subject-prefix [Subject-Prefix] 'Use [Subject-Prefix] instead of the standard [PATCH] prefix'") ) + .arg( + Arg::from("--subject-prefix [Subject-Prefix] 'Use [Subject-Prefix] instead of the standard [PATCH] prefix'") + .setting(ArgSettings::NoEmptyValues) + ) .try_get_matches_from(vec!["test", "--subject-prefix", "-v", "2"]); assert!(res.is_err()); @@ -411,7 +414,7 @@ fn issue_1047_min_zero_vals_default_val() { fn issue_1105_setup(argv: Vec<&'static str>) -> Result { App::new("opts") - .arg(Arg::from("-o, --option [opt] 'some option'").setting(ArgSettings::AllowEmptyValues)) + .arg(Arg::from("-o, --option [opt] 'some option'")) .arg(Arg::from("--flag 'some flag'")) .try_get_matches_from(argv) }