fix(parser): Conflict-with-self is back on by default

CHANGELOG.md

@@ -183,6 +183,9 @@ Subtle changes (i.e. compiler won't catch):
   - `ArgAction::Count`, requiring `ArgMatches::get_count` instead of `ArgMatches::occurrences_of`
   - `ArgAction::Set`, requiring `ArgMatches::get_one` instead of `ArgMatches::value_of`
   - `ArgAction::Append`, requiring `ArgMatches::get_many` instead of `ArgMatches::values_of`
+- `ArgAction::Set`, `ArgAction::SetTrue`, and `Arg::Action::SetFalse` now
+  conflict by default to be like `ArgAction::StoreValue` and
+  `ArgAction::IncOccurrences`, requiring `cmd.args_override_self(true)` to override instead (#4261)
 - By default, an `Arg`s default action is `ArgAction::Set`, rather than `ArgAction::IncOccurrence` to reduce confusing magic through consistency (#2687, #4032, see also #3977)
 - `mut_arg` can no longer be used to customize help and version arguments, instead disable them (`Command::disable_help_flag`, `Command::disable_version_flag`) and provide your own (#4056)
 - Removed lifetimes from `Command`, `Arg`, `ArgGroup`, and `PossibleValue`, assuming `'static`.  `string` feature flag will enable support for `String`s (#1041, #2150, #4223)

examples/tutorial_builder/03_01_flag_bool.md

@@ -16,6 +16,11 @@ $ 03_01_flag_bool --verbose
 verbose: true
 
 $ 03_01_flag_bool --verbose --verbose
-verbose: true
+? failed
+error: The argument '--verbose' cannot be used with '--verbose'
+
+Usage: 03_01_flag_bool[EXE] [OPTIONS]
+
+For more information try '--help'
 
 ```

examples/tutorial_derive/03_01_flag_bool.md

@@ -16,6 +16,11 @@ $ 03_01_flag_bool_derive --verbose
 verbose: true
 
 $ 03_01_flag_bool_derive --verbose --verbose
-verbose: true
+? failed
+error: The argument '--verbose' cannot be used with '--verbose'
+
+Usage: 03_01_flag_bool_derive[EXE] [OPTIONS]
+
+For more information try '--help'
 
 ```

src/builder/action.rs

@@ -27,6 +27,10 @@
 pub enum ArgAction {
     /// When encountered, store the associated value(s) in [`ArgMatches`][crate::ArgMatches]
     ///
+    /// **NOTE:** If the argument has previously been seen, it will result in a
+    /// [`ArgumentConflict`][crate::error::ErrorKind::ArgumentConflict] unless
+    /// [`Command::args_override_self(true)`][crate::Command::args_override_self] is set.
+    ///
     /// # Examples
     ///
     /// ```rust
@@ -76,6 +80,10 @@ pub enum ArgAction {
     /// No value is allowed. To optionally accept a value, see
     /// [`Arg::default_missing_value`][super::Arg::default_missing_value]
     ///
+    /// **NOTE:** If the argument has previously been seen, it will result in a
+    /// [`ArgumentConflict`][crate::error::ErrorKind::ArgumentConflict] unless
+    /// [`Command::args_override_self(true)`][crate::Command::args_override_self] is set.
+    ///
     /// # Examples
     ///
     /// ```rust
@@ -88,7 +96,7 @@ pub enum ArgAction {
     ///             .action(clap::ArgAction::SetTrue)
     ///     );
     ///
-    /// let matches = cmd.clone().try_get_matches_from(["mycmd", "--flag", "--flag"]).unwrap();
+    /// let matches = cmd.clone().try_get_matches_from(["mycmd", "--flag"]).unwrap();
     /// assert!(matches.contains_id("flag"));
     /// assert_eq!(
     ///     matches.get_one::<bool>("flag").copied(),
@@ -123,7 +131,7 @@ pub enum ArgAction {
     ///             )
     ///     );
     ///
-    /// let matches = cmd.clone().try_get_matches_from(["mycmd", "--flag", "--flag"]).unwrap();
+    /// let matches = cmd.clone().try_get_matches_from(["mycmd", "--flag"]).unwrap();
     /// assert!(matches.contains_id("flag"));
     /// assert_eq!(
     ///     matches.get_one::<usize>("flag").copied(),
@@ -145,6 +153,10 @@ pub enum ArgAction {
     /// No value is allowed. To optionally accept a value, see
     /// [`Arg::default_missing_value`][super::Arg::default_missing_value]
     ///
+    /// **NOTE:** If the argument has previously been seen, it will result in a
+    /// [`ArgumentConflict`][crate::error::ErrorKind::ArgumentConflict] unless
+    /// [`Command::args_override_self(true)`][crate::Command::args_override_self] is set.
+    ///
     /// # Examples
     ///
     /// ```rust

src/builder/app_settings.rs

@@ -31,6 +31,7 @@ pub(crate) enum AppSettings {
     IgnoreErrors,
     AllowHyphenValues,
     AllowNegativeNumbers,
+    AllArgsOverrideSelf,
     AllowMissingPositional,
     TrailingVarArg,
     DontDelimitTrailingValues,
@@ -93,6 +94,7 @@ bitflags! {
         const VALID_ARG_FOUND                = 1 << 35;
         const INFER_SUBCOMMANDS              = 1 << 36;
         const CONTAINS_LAST                  = 1 << 37;
+        const ARGS_OVERRIDE_SELF             = 1 << 38;
         const HELP_REQUIRED                  = 1 << 39;
         const SUBCOMMAND_PRECEDENCE_OVER_ARG = 1 << 40;
         const DISABLE_HELP_FLAG              = 1 << 41;
@@ -163,6 +165,8 @@ impl_settings! { AppSettings, AppFlags,
         => Flags::BIN_NAME_BUILT,
     InferSubcommands
         => Flags::INFER_SUBCOMMANDS,
+    AllArgsOverrideSelf
+        => Flags::ARGS_OVERRIDE_SELF,
     InferLongArgs
         => Flags::INFER_LONG_ARGS
 }

src/builder/command.rs

@@ -978,6 +978,23 @@ impl Command {
         }
     }
 
+    /// Specifies that all arguments override themselves.
+    ///
+    /// This is the equivalent to saying the `foo` arg using [`Arg::overrides_with("foo")`] for all
+    /// defined arguments.
+    ///
+    /// **NOTE:** This choice is propagated to all child subcommands.
+    ///
+    /// [`Arg::overrides_with("foo")`]: crate::Arg::overrides_with()
+    #[inline]
+    pub fn args_override_self(self, yes: bool) -> Self {
+        if yes {
+            self.global_setting(AppSettings::AllArgsOverrideSelf)
+        } else {
+            self.unset_global_setting(AppSettings::AllArgsOverrideSelf)
+        }
+    }
+
     /// Disables the automatic delimiting of values after `--` or when [`Command::trailing_var_arg`]
     /// was used.
     ///
@@ -3671,6 +3688,11 @@ impl Command {
         self.is_set(AppSettings::ArgsNegateSubcommands)
     }
 
+    #[doc(hidden)]
+    pub fn is_args_override_self(&self) -> bool {
+        self.is_set(AppSettings::AllArgsOverrideSelf)
+    }
+
     /// Report whether [`Command::subcommand_precedence_over_arg`] is set
     pub fn is_subcommand_precedence_over_arg_set(&self) -> bool {
         self.is_set(AppSettings::SubcommandPrecedenceOverArg)

src/builder/value_parser.rs

@@ -657,7 +657,7 @@ pub trait TypedValueParser: Clone + Send + Sync + 'static {
     ///             )
     ///     );
     ///
-    /// let matches = cmd.clone().try_get_matches_from(["mycmd", "--flag", "--flag"]).unwrap();
+    /// let matches = cmd.clone().try_get_matches_from(["mycmd", "--flag"]).unwrap();
     /// assert!(matches.contains_id("flag"));
     /// assert_eq!(
     ///     matches.get_one::<usize>("flag").copied(),

src/parser/arg_matcher.rs

@@ -101,8 +101,8 @@ impl ArgMatcher {
         self.matches.args.get_mut(arg)
     }
 
-    pub(crate) fn remove(&mut self, arg: &Id) {
-        self.matches.args.remove(arg);
+    pub(crate) fn remove(&mut self, arg: &Id) -> bool {
+        self.matches.args.remove(arg).is_some()
     }
 
     pub(crate) fn contains(&self, arg: &Id) -> bool {

src/parser/matches/arg_matches.rs

@@ -162,7 +162,7 @@ impl ArgMatches {
     ///             .action(clap::ArgAction::SetTrue)
     ///     );
     ///
-    /// let matches = cmd.clone().try_get_matches_from(["mycmd", "--flag", "--flag"]).unwrap();
+    /// let matches = cmd.clone().try_get_matches_from(["mycmd", "--flag"]).unwrap();
     /// assert!(matches.contains_id("flag"));
     /// assert_eq!(
     ///     matches.get_flag("flag"),

src/parser/parser.rs

@@ -1179,7 +1179,14 @@ impl<'cmd> Parser<'cmd> {
                     self.cur_idx.set(self.cur_idx.get() + 1);
                     debug!("Parser::react: cur_idx:={}", self.cur_idx.get());
                 }
-                matcher.remove(&arg.id);
+                if matcher.remove(&arg.id) && !self.cmd.is_args_override_self() {
+                    return Err(ClapError::argument_conflict(
+                        self.cmd,
+                        arg.to_string(),
+                        vec![arg.to_string()],
+                        Usage::new(self.cmd).create_usage_with_title(&[]),
+                    ));
+                }
                 self.start_custom_arg(matcher, arg, source);
                 self.push_arg_values(arg, raw_vals, matcher)?;
                 if cfg!(debug_assertions) && matcher.needs_more_vals(arg) {
@@ -1213,7 +1220,14 @@ impl<'cmd> Parser<'cmd> {
                     raw_vals
                 };
 
-                matcher.remove(&arg.id);
+                if matcher.remove(&arg.id) && !self.cmd.is_args_override_self() {
+                    return Err(ClapError::argument_conflict(
+                        self.cmd,
+                        arg.to_string(),
+                        vec![arg.to_string()],
+                        Usage::new(self.cmd).create_usage_with_title(&[]),
+                    ));
+                }
                 self.start_custom_arg(matcher, arg, source);
                 self.push_arg_values(arg, raw_vals, matcher)?;
                 Ok(ParseResult::ValuesDone)
@@ -1225,7 +1239,14 @@ impl<'cmd> Parser<'cmd> {
                     raw_vals
                 };
 
-                matcher.remove(&arg.id);
+                if matcher.remove(&arg.id) && self.cmd.is_args_override_self() {
+                    return Err(ClapError::argument_conflict(
+                        self.cmd,
+                        arg.to_string(),
+                        vec![arg.to_string()],
+                        Usage::new(self.cmd).create_usage_with_title(&[]),
+                    ));
+                }
                 self.start_custom_arg(matcher, arg, source);
                 self.push_arg_values(arg, raw_vals, matcher)?;
                 Ok(ParseResult::ValuesDone)

tests/builder/action.rs

@@ -1,6 +1,7 @@
 #![allow(clippy::bool_assert_comparison)]
 
 use clap::builder::ArgPredicate;
+use clap::error::ErrorKind;
 use clap::Arg;
 use clap::ArgAction;
 use clap::Command;
@@ -22,8 +23,15 @@ fn set() {
     assert_eq!(matches.contains_id("mammal"), true);
     assert_eq!(matches.index_of("mammal"), Some(2));
 
+    let result = cmd
+        .clone()
+        .try_get_matches_from(["test", "--mammal", "dog", "--mammal", "cat"]);
+    let err = result.err().unwrap();
+    assert_eq!(err.kind(), ErrorKind::ArgumentConflict);
+
     let matches = cmd
         .clone()
+        .args_override_self(true)
         .try_get_matches_from(["test", "--mammal", "dog", "--mammal", "cat"])
         .unwrap();
     assert_eq!(matches.get_one::<String>("mammal").unwrap(), "cat");
@@ -88,8 +96,15 @@ fn set_true() {
     assert_eq!(matches.contains_id("mammal"), true);
     assert_eq!(matches.index_of("mammal"), Some(1));
 
+    let result = cmd
+        .clone()
+        .try_get_matches_from(["test", "--mammal", "--mammal"]);
+    let err = result.err().unwrap();
+    assert_eq!(err.kind(), ErrorKind::ArgumentConflict);
+
     let matches = cmd
         .clone()
+        .args_override_self(true)
         .try_get_matches_from(["test", "--mammal", "--mammal"])
         .unwrap();
     assert_eq!(*matches.get_one::<bool>("mammal").unwrap(), true);

tests/builder/app_settings.rs

@@ -986,65 +986,11 @@ fn built_in_subcommand_escaped() {
     }
 }
 
-#[test]
-fn aaos_flags() {
-    // flags
-    let cmd = Command::new("posix").arg(arg!(--flag  "some flag").action(ArgAction::SetTrue));
-
-    let res = cmd.clone().try_get_matches_from(vec!["", "--flag"]);
-    assert!(res.is_ok(), "{}", res.unwrap_err());
-    let m = res.unwrap();
-    assert!(m.contains_id("flag"));
-    assert!(*m.get_one::<bool>("flag").expect("defaulted by clap"));
-
-    let res = cmd.try_get_matches_from(vec!["", "--flag", "--flag", "--flag", "--flag"]);
-    assert!(res.is_ok(), "{}", res.unwrap_err());
-    let m = res.unwrap();
-    assert!(m.contains_id("flag"));
-    assert!(*m.get_one::<bool>("flag").expect("defaulted by clap"));
-}
-
-#[test]
-fn aaos_flags_mult() {
-    // flags with multiple
-    let cmd = Command::new("posix").arg(arg!(--flag  "some flag").action(ArgAction::Count));
-
-    let res = cmd.clone().try_get_matches_from(vec!["", "--flag"]);
-    assert!(res.is_ok(), "{}", res.unwrap_err());
-    let m = res.unwrap();
-    assert!(m.contains_id("flag"));
-    assert_eq!(*m.get_one::<u8>("flag").expect("defaulted by clap"), 1);
-
-    let res = cmd.try_get_matches_from(vec!["", "--flag", "--flag", "--flag", "--flag"]);
-    assert!(res.is_ok(), "{}", res.unwrap_err());
-    let m = res.unwrap();
-    assert!(m.contains_id("flag"));
-    assert_eq!(*m.get_one::<u8>("flag").expect("defaulted by clap"), 4);
-}
-
-#[test]
-fn aaos_opts() {
-    // opts
-    let res = Command::new("posix")
-        .arg(
-            arg!(--opt <val> "some option")
-                .required(true)
-                .action(ArgAction::Set),
-        )
-        .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 aaos_opts_w_other_overrides() {
     // opts with other overrides
     let res = Command::new("posix")
+        .args_override_self(true)
         .arg(arg!(--opt <val> "some option").action(ArgAction::Set))
         .arg(
             arg!(--other <val> "some other option")
@@ -1066,6 +1012,7 @@ fn aaos_opts_w_other_overrides() {
 fn aaos_opts_w_other_overrides_rev() {
     // opts with other overrides, rev
     let res = Command::new("posix")
+        .args_override_self(true)
         .arg(
             arg!(--opt <val> "some option")
                 .required(true)
@@ -1092,6 +1039,7 @@ fn aaos_opts_w_other_overrides_rev() {
 fn aaos_opts_w_other_overrides_2() {
     // opts with other overrides
     let res = Command::new("posix")
+        .args_override_self(true)
         .arg(
             arg!(--opt <val> "some option")
                 .overrides_with("other")
@@ -1113,6 +1061,7 @@ fn aaos_opts_w_other_overrides_2() {
 fn aaos_opts_w_other_overrides_rev_2() {
     // opts with other overrides, rev
     let res = Command::new("posix")
+        .args_override_self(true)
         .arg(
             arg!(--opt <val> "some option")
                 .required(true)
@@ -1259,6 +1208,7 @@ fn aaos_pos_mult() {
 #[test]
 fn aaos_option_use_delim_false() {
     let m = Command::new("posix")
+        .args_override_self(true)
         .arg(
             arg!(--opt <val> "some option")
                 .required(true)

tests/builder/flags.rs

@@ -19,6 +19,7 @@ fn flag_using_short() {
 #[test]
 fn lots_o_flags_sep() {
     let r = Command::new("opts")
+        .args_override_self(true)
         .arg(arg!(o: -o ... "some flag").action(ArgAction::SetTrue))
         .try_get_matches_from(vec![
             "", "-o", "-o", "-o", "-o", "-o", "-o", "-o", "-o", "-o", "-o", "-o", "-o", "-o", "-o",
@@ -53,6 +54,7 @@ fn lots_o_flags_sep() {
 #[test]
 fn lots_o_flags_combined() {
     let r = Command::new("opts")
+        .args_override_self(true)
         .arg(arg!(o: -o ... "some flag").action(ArgAction::SetTrue))
         .try_get_matches_from(vec![
             "",

tests/builder/grouped_values.rs

@@ -223,6 +223,7 @@ fn grouped_interleaved_positional_occurrences() {
 #[test]
 fn issue_2171() {
     let schema = Command::new("ripgrep#1701 reproducer")
+        .args_override_self(true)
         .arg(
             Arg::new("pretty")
                 .short('p')