Skip to content

Commit

Permalink
Merge pull request #3986 from epage/override
Browse files Browse the repository at this point in the history 
fix(assert)!: Disallow self-overrides
  • Loading branch information
@epage
epage committed Jul 25, 2022
2 parents b47a845 + ec38212 commit 8c7fe8b
Show file tree
Hide file tree
Showing 5 changed files with 31 additions and 275 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Expand Up @@ -36,6 +36,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
- Disallow more `value_names` than `number_of_values` (#2695)
- *(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
- *(assert)* Ensure no self-`overrides_with` now that Actions replace it
- *(help)* Use `Command::display_name` in the help title rather than `Command::bin_name`
- *(version)* Use `Command::display_name` rather than `Command::bin_name`
- *(parser)* Assert on unknown args when using external subcommands (#3703)
Expand Down
83 changes: 1 addition & 82 deletions src/builder/arg.rs
View file
Expand Up @@ -3866,11 +3866,7 @@ impl<'help> Arg<'help> {
///
/// **NOTE:** Overriding an argument implies they [conflict][Arg::conflicts_with`].
///
/// **WARNING:** Positional arguments and options which accept
/// [`Arg::multiple_occurrences`] cannot override themselves (or we
/// would never be able to advance to the next positional). If a positional
/// argument or option with one of the [`Arg::multiple_occurrences`]
/// settings lists itself as an override, it is simply ignored.
/// **NOTE:** All arguments implicitly override themselves.
///
/// # Examples
///
Expand All @@ -3891,73 +3887,6 @@ impl<'help> Arg<'help> {
/// // was never used because it was overridden with color
/// assert!(!*m.get_one::<bool>("flag").unwrap());
/// ```
/// Care must be taken when using this setting, and having an arg override with itself. This
/// is common practice when supporting things like shell aliases, config files, etc.
/// However, when combined with multiple values, it can get dicy.
/// Here is how clap handles such situations:
///
/// When a flag overrides itself, it's as if the flag was only ever used once (essentially
/// preventing a "Unexpected multiple usage" error):
///
/// ```rust
/// # use clap::{Command, arg};
/// let m = Command::new("posix")
/// .arg(arg!(--flag "some flag").overrides_with("flag"))
/// .get_matches_from(vec!["posix", "--flag", "--flag"]);
/// assert!(*m.get_one::<bool>("flag").unwrap());
/// ```
///
/// Making an arg [`Arg::multiple_occurrences`] and override itself
/// is essentially meaningless. Therefore clap ignores an override of self.
///
/// ```
/// # use clap::{Command, arg};
/// let m = Command::new("posix")
/// .arg(arg!(--flag ... "some flag").overrides_with("flag"))
/// .get_matches_from(vec!["", "--flag", "--flag", "--flag", "--flag"]);
/// assert_eq!(*m.get_one::<u8>("flag").unwrap(), 4);
/// ```
///
/// Now notice with options (which *do not* set
/// [`Arg::multiple_occurrences`]), it's as if only the last
/// occurrence happened.
///
/// ```
/// # use clap::{Command, arg};
/// let m = Command::new("posix")
/// .arg(arg!(--opt <val> "some option").overrides_with("opt"))
/// .get_matches_from(vec!["", "--opt=some", "--opt=other"]);
/// assert_eq!(m.get_one::<String>("opt").unwrap(), "other");
/// ```
///
/// This will also work when [`Arg::multiple_values`] is enabled:
///
/// ```
/// # use clap::{Command, Arg};
/// let m = Command::new("posix")
/// .arg(
/// Arg::new("opt")
/// .long("opt")
/// .takes_value(true)
/// .multiple_values(true)
/// .overrides_with("opt")
/// )
/// .get_matches_from(vec!["", "--opt", "1", "2", "--opt", "3", "4", "5"]);
/// assert_eq!(m.get_many::<String>("opt").unwrap().collect::<Vec<_>>(), &["3", "4", "5"]);
/// ```
///
/// Just like flags, options with [`Arg::multiple_occurrences`] set
/// will ignore the "override self" setting.
///
/// ```
/// # use clap::{Command, arg};
/// let m = Command::new("posix")
/// .arg(arg!(--opt <val> ... "some option")
/// .multiple_values(true)
/// .overrides_with("opt"))
/// .get_matches_from(vec!["", "--opt", "first", "over", "--opt", "other", "val"]);
/// assert_eq!(m.get_many::<String>("opt").unwrap().collect::<Vec<_>>(), &["first", "over", "other", "val"]);
/// ```
#[must_use]
pub fn overrides_with<T: Key>(mut self, arg_id: T) -> Self {
self.overrides.push(arg_id.into());
Expand Down Expand Up @@ -4436,16 +4365,6 @@ impl<'help> Arg<'help> {
self.num_vals = Some(val_names_len);
}
}

let self_id = self.id.clone();
if self.is_positional() || self.is_multiple_occurrences_set() {
// Remove self-overrides where they don't make sense.
//
// We can evaluate switching this to a debug assert at a later time (though it will
// require changing propagation of `AllArgsOverrideSelf`). Being conservative for now
// due to where we are at in the release.
self.overrides.retain(|e| *e != self_id);
}
}

pub(crate) fn generated(mut self) -> Self {
Expand Down
16 changes: 16 additions & 0 deletions src/builder/debug_asserts.rs
View file
Expand Up @@ -222,6 +222,17 @@ pub(crate) fn assert_app(cmd: &Command) {
);
}

// overrides
for req in &arg.overrides {
assert!(
cmd.id_exists(req),
"Command {}: Argument or group '{:?}' specified in 'overrides_with*' for '{}' does not exist",
cmd.get_name(),
req,
arg.name,
);
}

if arg.is_last_set() {
assert!(
arg.long.is_none(),
Expand Down Expand Up @@ -611,6 +622,11 @@ fn assert_arg(arg: &Arg) {
"Argument '{}' cannot conflict with itself",
arg.name,
);
assert!(
!arg.overrides.iter().any(|x| *x == arg.id),
"Argument '{}' cannot override itself, its the default",
arg.name,
);

assert_eq!(
arg.get_action().takes_values(),
Expand Down
30 changes: 0 additions & 30 deletions tests/builder/grouped_values.rs
View file
Expand Up @@ -220,36 +220,6 @@ fn grouped_interleaved_positional_occurrences() {
assert_eq!(flag, vec![vec!["a"], vec!["b"]]);
}

#[test]
fn issue_1374() {
let cmd = Command::new("MyApp").arg(
Arg::new("input")
.takes_value(true)
.long("input")
.overrides_with("input")
.min_values(0)
.action(ArgAction::Append),
);
let matches = cmd
.clone()
.try_get_matches_from(&["MyApp", "--input", "a", "b", "c", "--input", "d"])
.unwrap();
let vs = matches
.get_many::<String>("input")
.unwrap()
.map(|v| v.as_str());
assert_eq!(vs.collect::<Vec<_>>(), vec!["a", "b", "c", "d"]);
let matches = cmd
.clone()
.try_get_matches_from(&["MyApp", "--input", "a", "b", "--input", "c", "d"])
.unwrap();
let vs = matches
.get_many::<String>("input")
.unwrap()
.map(|v| v.as_str());
assert_eq!(vs.collect::<Vec<_>>(), vec!["a", "b", "c", "d"]);
}

#[test]
fn issue_2171() {
let schema = Command::new("ripgrep#1701 reproducer")
Expand Down
175 changes: 12 additions & 163 deletions tests/builder/posix_compatible.rs
View file
@@ -1,145 +1,18 @@
use clap::{arg, error::ErrorKind, Arg, ArgAction, Command};

#[test]
#[should_panic = "Argument 'flag' cannot override itself"]
fn flag_overrides_itself() {
let res = Command::new("posix")
Command::new("posix")
.arg(
arg!(--flag "some flag"
)
.action(ArgAction::SetTrue)
.overrides_with("flag"),
)
.try_get_matches_from(vec!["", "--flag", "--flag"]);
assert!(res.is_ok(), "{}", res.unwrap_err());
let m = res.unwrap();
assert!(*m.get_one::<bool>("flag").expect("defaulted by clap"));
}

#[test]
fn mult_flag_overrides_itself() {
let res = Command::new("posix")
.arg(
arg!(--flag ... "some flag")
.overrides_with("flag")
.action(ArgAction::SetTrue),
)
.try_get_matches_from(vec!["", "--flag", "--flag", "--flag", "--flag"]);
assert!(res.is_ok(), "{}", res.unwrap_err());
let m = res.unwrap();
assert!(*m.get_one::<bool>("flag").expect("defaulted by clap"));
}

#[test]
fn option_overrides_itself() {
let res = Command::new("posix")
.arg(
arg!(--opt <val> "some option")
.required(false)
.overrides_with("opt"),
)
.try_get_matches_from(vec!["", "--opt=some", "--opt=other"]);
assert!(res.is_ok(), "{}", res.unwrap_err());
let m = res.unwrap();
assert!(m.contains_id("opt"));
assert_eq!(
m.get_one::<String>("opt").map(|v| v.as_str()),
Some("other")
);
}

#[test]
fn mult_option_require_delim_overrides_itself() {
let res = Command::new("posix")
.arg(
arg!(--opt <val> ... "some option")
.required(false)
.overrides_with("opt")
.number_of_values(1)
.takes_value(true)
.use_value_delimiter(true)
.require_value_delimiter(true),
)
.try_get_matches_from(vec!["", "--opt=some", "--opt=other", "--opt=one,two"]);
assert!(res.is_ok(), "{}", res.unwrap_err());
let m = res.unwrap();
assert!(m.contains_id("opt"));
assert_eq!(
m.get_many::<String>("opt")
.unwrap()
.map(|v| v.as_str())
.collect::<Vec<_>>(),
&["some", "other", "one", "two"]
);
}

#[test]
fn mult_option_overrides_itself() {
let res = Command::new("posix")
.arg(
arg!(--opt <val> ... "some option")
.required(false)
.multiple_values(true)
.overrides_with("opt"),
)
.try_get_matches_from(vec![
"",
"--opt",
"first",
"overrides",
"--opt",
"some",
"other",
"val",
]);
assert!(res.is_ok(), "{}", res.unwrap_err());
let m = res.unwrap();
assert!(m.contains_id("opt"));
assert_eq!(
m.get_many::<String>("opt")
.unwrap()
.map(|v| v.as_str())
.collect::<Vec<_>>(),
&["first", "overrides", "some", "other", "val"]
);
}

#[test]
fn option_use_delim_false_override_itself() {
let m = Command::new("posix")
.arg(
arg!(--opt <val> "some option")
.required(false)
.overrides_with("opt"),
)
.try_get_matches_from(vec!["", "--opt=some,other", "--opt=one,two"])
.unwrap();
assert!(m.contains_id("opt"));
assert_eq!(
m.get_many::<String>("opt")
.unwrap()
.map(|v| v.as_str())
.collect::<Vec<_>>(),
&["one,two"]
);
.build();
}

#[test]
fn pos_mult_overrides_itself() {
// opts with multiple
let res = Command::new("posix")
.arg(arg!([val] ... "some pos").overrides_with("val"))
.try_get_matches_from(vec!["", "some", "other", "value"]);
assert!(res.is_ok(), "{}", res.unwrap_err());
let m = res.unwrap();
assert!(m.contains_id("val"));
assert_eq!(
m.get_many::<String>("val")
.unwrap()
.map(|v| v.as_str())
.collect::<Vec<_>>(),
&["some", "other", "value"]
);
}
#[test]
fn posix_compatible_flags_long() {
let m = Command::new("posix")
Expand Down Expand Up @@ -447,39 +320,6 @@ fn require_overridden_4() {
assert_eq!(err.kind(), ErrorKind::MissingRequiredArgument);
}

#[test]
fn issue_1374_overrides_self_with_multiple_values() {
let cmd = Command::new("test").arg(
Arg::new("input")
.long("input")
.takes_value(true)
.overrides_with("input")
.min_values(0),
);
let m = cmd
.clone()
.try_get_matches_from(&["test", "--input", "a", "b", "c", "--input", "d"])
.unwrap();
assert_eq!(
m.get_many::<String>("input")
.unwrap()
.map(|v| v.as_str())
.collect::<Vec<_>>(),
&["d"]
);
let m = cmd
.clone()
.try_get_matches_from(&["test", "--input", "a", "b", "--input", "c", "d"])
.unwrap();
assert_eq!(
m.get_many::<String>("input")
.unwrap()
.map(|v| v.as_str())
.collect::<Vec<_>>(),
&["c", "d"]
);
}

#[test]
fn incremental_override() {
let mut cmd = Command::new("test")
Expand All @@ -501,3 +341,12 @@ fn incremental_override() {
);
assert!(!*m.get_one::<bool>("no-name").expect("defaulted by clap"));
}

#[cfg(debug_assertions)]
#[test]
#[should_panic = "Argument or group 'extra' specified in 'overrides_with*' for 'config' does not exist"]
fn overrides_with_invalid_arg() {
let _ = Command::new("prog")
.arg(Arg::new("config").long("config").overrides_with("extra"))
.try_get_matches_from(vec!["", "--config"]);
}

0 comments on commit 8c7fe8b

Please sign in to comment.