From 5a8e2046af832808883be358a25d8e7a1a56fa2e Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 25 Jul 2022 12:05:56 -0500 Subject: [PATCH 1/2] fix(assert)!: Ensure overrides_with IDs are valid --- CHANGELOG.md | 1 + src/builder/debug_asserts.rs | 11 +++++++++++ tests/builder/posix_compatible.rs | 9 +++++++++ 3 files changed, 21 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 137b9eb8bef..55aa2b31842 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,6 +36,7 @@ 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 - *(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) diff --git a/src/builder/debug_asserts.rs b/src/builder/debug_asserts.rs index a555ffa76f0..7b1e284978e 100644 --- a/src/builder/debug_asserts.rs +++ b/src/builder/debug_asserts.rs @@ -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(), diff --git a/tests/builder/posix_compatible.rs b/tests/builder/posix_compatible.rs index 12d64dd8d92..f7544597745 100644 --- a/tests/builder/posix_compatible.rs +++ b/tests/builder/posix_compatible.rs @@ -501,3 +501,12 @@ fn incremental_override() { ); assert!(!*m.get_one::("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"]); +} From ec38212dcb8f112d5d953d8930ddeb71653cbbde Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 25 Jul 2022 12:33:15 -0500 Subject: [PATCH 2/2] fix(assert)!: Disallow self-overrides This will make it easier to drop support for multiple occurrences --- CHANGELOG.md | 1 + src/builder/arg.rs | 83 +-------------- src/builder/debug_asserts.rs | 5 + tests/builder/grouped_values.rs | 30 ------ tests/builder/posix_compatible.rs | 166 +----------------------------- 5 files changed, 10 insertions(+), 275 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 55aa2b31842..98c96535cbb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,6 +37,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - *(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) diff --git a/src/builder/arg.rs b/src/builder/arg.rs index c4b87265742..beb2815d564 100644 --- a/src/builder/arg.rs +++ b/src/builder/arg.rs @@ -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 /// @@ -3891,73 +3887,6 @@ impl<'help> Arg<'help> { /// // was never used because it was overridden with color /// assert!(!*m.get_one::("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::("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::("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 "some option").overrides_with("opt")) - /// .get_matches_from(vec!["", "--opt=some", "--opt=other"]); - /// assert_eq!(m.get_one::("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::("opt").unwrap().collect::>(), &["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 ... "some option") - /// .multiple_values(true) - /// .overrides_with("opt")) - /// .get_matches_from(vec!["", "--opt", "first", "over", "--opt", "other", "val"]); - /// assert_eq!(m.get_many::("opt").unwrap().collect::>(), &["first", "over", "other", "val"]); - /// ``` #[must_use] pub fn overrides_with(mut self, arg_id: T) -> Self { self.overrides.push(arg_id.into()); @@ -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 { diff --git a/src/builder/debug_asserts.rs b/src/builder/debug_asserts.rs index 7b1e284978e..0e8a8212eeb 100644 --- a/src/builder/debug_asserts.rs +++ b/src/builder/debug_asserts.rs @@ -622,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(), diff --git a/tests/builder/grouped_values.rs b/tests/builder/grouped_values.rs index c44f0dd257b..f7db11873aa 100644 --- a/tests/builder/grouped_values.rs +++ b/tests/builder/grouped_values.rs @@ -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::("input") - .unwrap() - .map(|v| v.as_str()); - assert_eq!(vs.collect::>(), 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::("input") - .unwrap() - .map(|v| v.as_str()); - assert_eq!(vs.collect::>(), vec!["a", "b", "c", "d"]); -} - #[test] fn issue_2171() { let schema = Command::new("ripgrep#1701 reproducer") diff --git a/tests/builder/posix_compatible.rs b/tests/builder/posix_compatible.rs index f7544597745..dcfc92ef7a9 100644 --- a/tests/builder/posix_compatible.rs +++ b/tests/builder/posix_compatible.rs @@ -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::("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::("flag").expect("defaulted by clap")); -} - -#[test] -fn option_overrides_itself() { - let res = Command::new("posix") - .arg( - arg!(--opt "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::("opt").map(|v| v.as_str()), - Some("other") - ); -} - -#[test] -fn mult_option_require_delim_overrides_itself() { - let res = Command::new("posix") - .arg( - arg!(--opt ... "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::("opt") - .unwrap() - .map(|v| v.as_str()) - .collect::>(), - &["some", "other", "one", "two"] - ); -} - -#[test] -fn mult_option_overrides_itself() { - let res = Command::new("posix") - .arg( - arg!(--opt ... "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::("opt") - .unwrap() - .map(|v| v.as_str()) - .collect::>(), - &["first", "overrides", "some", "other", "val"] - ); + .build(); } -#[test] -fn option_use_delim_false_override_itself() { - let m = Command::new("posix") - .arg( - arg!(--opt "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::("opt") - .unwrap() - .map(|v| v.as_str()) - .collect::>(), - &["one,two"] - ); -} - -#[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::("val") - .unwrap() - .map(|v| v.as_str()) - .collect::>(), - &["some", "other", "value"] - ); -} #[test] fn posix_compatible_flags_long() { let m = Command::new("posix") @@ -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::("input") - .unwrap() - .map(|v| v.as_str()) - .collect::>(), - &["d"] - ); - let m = cmd - .clone() - .try_get_matches_from(&["test", "--input", "a", "b", "--input", "c", "d"]) - .unwrap(); - assert_eq!( - m.get_many::("input") - .unwrap() - .map(|v| v.as_str()) - .collect::>(), - &["c", "d"] - ); -} - #[test] fn incremental_override() { let mut cmd = Command::new("test")