Repository: clap-rs/clap
Pull request #3986 (2022-07-25 17:35:02)

fix(assert)!: Disallow self-overrides 
This will make it easier to drop support for multiple occurrences


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 137b9eb8..98c96535 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -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)
diff --git a/src/builder/arg.rs b/src/builder/arg.rs
index c4b87265..beb2815d 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::<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());
@@ -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 a555ffa7..0e8a8212 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(),
@@ -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(),


Test patch:
diff --git a/tests/builder/grouped_values.rs b/tests/builder/grouped_values.rs
index c44f0dd2..f7db1187 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::<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")
diff --git a/tests/builder/posix_compatible.rs b/tests/builder/posix_compatible.rs
index 12d64dd8..dcfc92ef 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::<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")
@@ -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")
@@ -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"]);
+}