fix(parser): Resolve problems around allow_hyphen_values

CHANGELOG.md

@@ -44,6 +44,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 - `Arg::short_aliases` and other builder functions that took `&[]` need the `&` dropped
 - Changed `Arg::requires_ifs` and `Arg::default_value*_ifs*` to taking an `ArgPredicate`, removing ambiguity with `None` when accepting owned and borrowed types
 - Removed lifetimes from `Command`, `Arg`, `ArgGroup`, and `PossibleValue`
+- *(parser)* Short flags now have higher precedence than hyphen values with `Arg::allow_hyphen_values`, like `Command::allow_hyphen_values`
 - *(help)* Make `DeriveDisplayOrder` the default and removed the setting.  To sort help, set `next_display_order(None)` (#2808)
 - *(help)* Subcommand display order respects `Command::next_display_order` instead of `DeriveDisplayOrder` and using its own initial display order value (#2808)
 - *(help)* Subcommands are now listed before arguments.  To get the old behavior, see `Command::help_template`
@@ -66,6 +67,8 @@ Deprecated
 - `Arg::number_of_values` in favor of `Arg::num_args`
 - `default_value_os`, `default_values_os`, `default_value_if_os`, and `default_value_ifs_os` as the non `_os` variants now accept either a `str` or an `OsStr`
 - `Command::dont_collapse_args_in_usage` is now the default and is deprecated
+- `Command::trailing_var_arg` in favor of `Arg::trailing_var_arg`
+- `Command::allow_hyphen_values` in favor of `Arg::allow_hyphen_values`
 - *(derive)* `structopt` and `clap` attributes in favor of the more specific `command`, `arg`, and `value`
 
 ### Features
@@ -108,6 +111,7 @@ Deprecated
 - *(version)* Use `Command::display_name` rather than `Command::bin_name`
 - *(parser)* Assert on unknown args when using external subcommands (#3703)
 - *(parser)* Always fill in `""` argument for external subcommands (#3263)
+- *(parser)* Short flags now have higher precedence than hyphen values with `Arg::allow_hyphen_values`, like `Command::allow_hyphen_values`
 - *(derive)* Detect escaped external subcommands that look like built-in subcommands (#3703)
 - *(derive)* Leave `Arg::id` as `verbatim` casing (#3282)
 - *(derive)* Default to `#[clap(value_parser, action)]` instead of `#[clap(parse)]` (#3827)

clap_bench/benches/06_rustup.rs

@@ -238,9 +238,13 @@ fn build_cli() -> Command {
             Command::new("run")
                 .about("Run a command with an environment configured for a given toolchain")
                 .after_help(RUN_HELP)
-                .trailing_var_arg(true)
                 .arg(Arg::new("toolchain").required(true))
-                .arg(Arg::new("command").required(true).num_args(1..)),
+                .arg(
+                    Arg::new("command")
+                        .required(true)
+                        .num_args(1..)
+                        .trailing_var_arg(true),
+                ),
         )
         .subcommand(
             Command::new("which")

clap_complete/examples/completion-derive.rs

@@ -19,11 +19,7 @@ use std::io;
 use std::path::PathBuf;
 
 #[derive(Parser, Debug, PartialEq)]
-#[command(
-    name = "value_hints_derive",
-    // Command::trailing_var_ar is required to use ValueHint::CommandWithArguments
-    trailing_var_arg = true,
-)]
+#[command(name = "value_hints_derive")]
 struct Opt {
     /// If provided, outputs the completion file for given shell
     #[arg(long = "generate", value_enum)]
@@ -45,7 +41,8 @@ struct Opt {
     cmd_name: Option<OsString>,
     #[arg(short, long, value_hint = ValueHint::CommandString)]
     cmd: Option<String>,
-    #[arg(value_hint = ValueHint::CommandWithArguments)]
+    // Command::trailing_var_ar is required to use ValueHint::CommandWithArguments
+    #[arg(trailing_var_arg = true, value_hint = ValueHint::CommandWithArguments)]
     command_with_args: Vec<String>,
     #[arg(short, long, value_hint = ValueHint::Username)]
     user: Option<String>,

clap_complete/examples/completion.rs

@@ -18,8 +18,6 @@ use std::io;
 
 fn build_cli() -> Command {
     Command::new("value_hints")
-        // AppSettings::TrailingVarArg is required to use ValueHint::CommandWithArguments
-        .trailing_var_arg(true)
         .arg(
             Arg::new("generator")
                 .long("generate")
@@ -69,6 +67,8 @@ fn build_cli() -> Command {
         .arg(
             Arg::new("command_with_args")
                 .num_args(1..)
+                // AppSettings::TrailingVarArg is required to use ValueHint::CommandWithArguments
+                .trailing_var_arg(true)
                 .value_hint(ValueHint::CommandWithArguments),
         )
         .arg(

clap_complete/tests/common.rs

@@ -169,7 +169,6 @@ pub fn sub_subcommands_command(name: &'static str) -> clap::Command {
 
 pub fn value_hint_command(name: &'static str) -> clap::Command {
     clap::Command::new(name)
-        .trailing_var_arg(true)
         .arg(
             clap::Arg::new("choice")
                 .long("choice")
@@ -225,6 +224,7 @@ pub fn value_hint_command(name: &'static str) -> clap::Command {
             clap::Arg::new("command_with_args")
                 .action(clap::ArgAction::Set)
                 .num_args(1..)
+                .trailing_var_arg(true)
                 .value_hint(clap::ValueHint::CommandWithArguments),
         )
         .arg(

clap_complete_fig/tests/common.rs

@@ -165,7 +165,6 @@ pub fn sub_subcommands_command(name: &'static str) -> clap::Command {
 
 pub fn value_hint_command(name: &'static str) -> clap::Command {
     clap::Command::new(name)
-        .trailing_var_arg(true)
         .arg(
             clap::Arg::new("choice")
                 .long("choice")
@@ -221,6 +220,7 @@ pub fn value_hint_command(name: &'static str) -> clap::Command {
             clap::Arg::new("command_with_args")
                 .action(clap::ArgAction::Set)
                 .num_args(1..)
+                .trailing_var_arg(true)
                 .value_hint(clap::ValueHint::CommandWithArguments),
         )
         .arg(

clap_mangen/tests/common.rs

@@ -163,7 +163,6 @@ pub fn sub_subcommands_command(name: &'static str) -> clap::Command {
 
 pub fn value_hint_command(name: &'static str) -> clap::Command {
     clap::Command::new(name)
-        .trailing_var_arg(true)
         .arg(
             clap::Arg::new("choice")
                 .long("choice")
@@ -219,6 +218,7 @@ pub fn value_hint_command(name: &'static str) -> clap::Command {
             clap::Arg::new("command_with_args")
                 .action(clap::ArgAction::Set)
                 .num_args(1..)
+                .trailing_var_arg(true)
                 .value_hint(clap::ValueHint::CommandWithArguments),
         )
         .arg(
@@ -283,7 +283,6 @@ pub fn assert_matches_path(expected_path: impl AsRef<std::path::Path>, cmd: clap
 
 pub fn possible_values_command(name: &'static str) -> clap::Command {
     clap::Command::new(name)
-        .trailing_var_arg(true)
         .arg(
             clap::Arg::new("choice")
                 .long("choice")

src/builder/arg.rs

@@ -459,6 +459,37 @@ impl Arg {
         self
     }
 
+    /// This is a "VarArg" and everything that follows should be captured by it, as if the user had
+    /// used a `--`.
+    ///
+    /// **NOTE:** To start the trailing "VarArg" on unknown flags (and not just a positional
+    /// value), set [`allow_hyphen_values`][Arg::allow_hyphen_values].  Either way, users still
+    /// have the option to explicitly escape ambiguous arguments with `--`.
+    ///
+    /// **NOTE:** [`Arg::value_delimiter`] still applies if set.
+    ///
+    /// **NOTE:** Setting this requires [`Arg::num_args(..)`].
+    ///
+    /// # Examples
+    ///
+    /// ```rust
+    /// # use clap::{Command, arg};
+    /// let m = Command::new("myprog")
+    ///     .arg(arg!(<cmd> ... "commands to run").trailing_var_arg(true))
+    ///     .get_matches_from(vec!["myprog", "arg1", "-r", "val1"]);
+    ///
+    /// let trail: Vec<_> = m.get_many::<String>("cmd").unwrap().collect();
+    /// assert_eq!(trail, ["arg1", "-r", "val1"]);
+    /// ```
+    /// [`Arg::num_args(..)`]: crate::Arg::num_args()
+    pub fn trailing_var_arg(self, yes: bool) -> Self {
+        if yes {
+            self.setting(ArgSettings::TrailingVarArg)
+        } else {
+            self.unset_setting(ArgSettings::TrailingVarArg)
+        }
+    }
+
     /// This arg is the last, or final, positional argument (i.e. has the highest
     /// index) and is *only* able to be accessed via the `--` syntax (i.e. `$ prog args --
     /// last_arg`).
@@ -1244,18 +1275,16 @@ impl Arg {
     ///
     /// **NOTE:** Setting this requires [taking values][Arg::num_args]
     ///
-    /// **WARNING**: Take caution when using this setting combined with
+    /// **NOTE:** If a positional argument has `allow_hyphen_values` and is followed by a known
+    /// flag, it will be treated as a flag (see [`trailing_var_arg`][Arg::trailing_var_arg] for
+    /// consuming known flags).  If an option has `allow_hyphen_values` and is followed by a known
+    /// flag, it will be treated as the value for the option.
+    ///
+    /// **WARNING**: Take caution when using this setting combined with another argument using
     /// [`Arg::num_args`], as this becomes ambiguous `$ prog --arg -- -- val`. All
     /// three `--, --, val` will be values when the user may have thought the second `--` would
     /// constitute the normal, "Only positional args follow" idiom.
     ///
-    /// **WARNING**: When building your CLIs, consider the effects of allowing leading hyphens and
-    /// the user passing in a value that matches a valid short. For example, `prog -opt -F` where
-    /// `-F` is supposed to be a value, yet `-F` is *also* a valid short for another arg.
-    /// Care should be taken when designing these args. This is compounded by the ability to "stack"
-    /// short args. I.e. if `-val` is supposed to be a value, but `-v`, `-a`, and `-l` are all valid
-    /// shorts.
-    ///
     /// # Examples
     ///
     /// ```rust
@@ -3894,6 +3923,11 @@ impl Arg {
         self.is_set(ArgSettings::Exclusive)
     }
 
+    /// Report whether [`Arg::trailing_var_arg`] is set
+    pub fn is_trailing_var_arg_set(&self) -> bool {
+        self.is_set(ArgSettings::TrailingVarArg)
+    }
+
     /// Reports whether [`Arg::last`] is set
     pub fn is_last_set(&self) -> bool {
         self.is_set(ArgSettings::Last)

src/builder/arg_settings.rs

@@ -35,6 +35,7 @@ pub(crate) enum ArgSettings {
     AllowHyphenValues,
     RequireEquals,
     Last,
+    TrailingVarArg,
     HideDefaultValue,
     IgnoreCase,
     #[cfg(feature = "env")]
@@ -51,6 +52,7 @@ bitflags! {
         const REQUIRED         = 1;
         const GLOBAL           = 1 << 3;
         const HIDDEN           = 1 << 4;
+        const TRAILING_VARARG  = 1 << 5;
         const NEXT_LINE_HELP   = 1 << 7;
         const DELIM_NOT_SET    = 1 << 10;
         const HIDE_POS_VALS    = 1 << 11;
@@ -79,6 +81,7 @@ impl_settings! { ArgSettings, ArgFlags,
     AllowHyphenValues => Flags::ALLOW_TAC_VALS,
     RequireEquals => Flags::REQUIRE_EQUALS,
     Last => Flags::LAST,
+    TrailingVarArg => Flags::TRAILING_VARARG,
     IgnoreCase => Flags::CASE_INSENSITIVE,
     #[cfg(feature = "env")]
     HideEnv => Flags::HIDE_ENV,

src/builder/command.rs

@@ -1938,32 +1938,11 @@ impl Command {
         }
     }
 
-    /// Specifies that leading hyphens are allowed in all argument *values* (e.g. `-10`).
-    ///
-    /// Otherwise they will be parsed as another flag or option.  See also
-    /// [`Command::allow_negative_numbers`].
-    ///
-    /// **NOTE:** Use this setting with caution as it silences certain circumstances which would
-    /// otherwise be an error (such as accidentally forgetting to specify a value for leading
-    /// option). It is preferred to set this on a per argument basis, via [`Arg::allow_hyphen_values`].
-    ///
-    /// # Examples
-    ///
-    /// ```rust
-    /// # use clap::{Arg, Command};
-    /// // Imagine you needed to represent negative numbers as well, such as -10
-    /// let m = Command::new("nums")
-    ///     .allow_hyphen_values(true)
-    ///     .arg(Arg::new("neg"))
-    ///     .get_matches_from(vec![
-    ///         "nums", "-20"
-    ///     ]);
-    ///
-    /// assert_eq!(m.get_one::<String>("neg").unwrap(), "-20");
-    /// # ;
-    /// ```
-    /// [`Arg::allow_hyphen_values`]: crate::Arg::allow_hyphen_values()
-    #[inline]
+    #[doc(hidden)]
+    #[cfg_attr(
+        feature = "deprecated",
+        deprecated(since = "4.0.0", note = "Replaced with `Arg::allow_hyphen_values`")
+    )]
     pub fn allow_hyphen_values(self, yes: bool) -> Self {
         if yes {
             self.setting(AppSettings::AllowHyphenValues)
@@ -2000,26 +1979,11 @@ impl Command {
         }
     }
 
-    /// Specifies that the final positional argument is a "VarArg" and that `clap` should not
-    /// attempt to parse any further args.
-    ///
-    /// The values of the trailing positional argument will contain all args from itself on.
-    ///
-    /// **NOTE:** The final positional argument **must** have [`Arg::num_args(..)`].
-    ///
-    /// # Examples
-    ///
-    /// ```rust
-    /// # use clap::{Command, arg};
-    /// let m = Command::new("myprog")
-    ///     .trailing_var_arg(true)
-    ///     .arg(arg!(<cmd> ... "commands to run"))
-    ///     .get_matches_from(vec!["myprog", "arg1", "-r", "val1"]);
-    ///
-    /// let trail: Vec<_> = m.get_many::<String>("cmd").unwrap().collect();
-    /// assert_eq!(trail, ["arg1", "-r", "val1"]);
-    /// ```
-    /// [`Arg::num_args(..)`]: crate::Arg::num_args()
+    #[doc(hidden)]
+    #[cfg_attr(
+        feature = "deprecated",
+        deprecated(since = "4.0.0", note = "Replaced with `Arg::trailing_var_arg`")
+    )]
     pub fn trailing_var_arg(self, yes: bool) -> Self {
         if yes {
             self.setting(AppSettings::TrailingVarArg)
@@ -3588,7 +3552,14 @@ impl Command {
         self.is_set(AppSettings::ArgRequiredElseHelp)
     }
 
-    /// Report whether [`Command::allow_hyphen_values`] is set
+    #[doc(hidden)]
+    #[cfg_attr(
+        feature = "deprecated",
+        deprecated(
+            since = "4.0.0",
+            note = "Replaced with `Arg::is_allow_hyphen_values_set`"
+        )
+    )]
     pub(crate) fn is_allow_hyphen_values_set(&self) -> bool {
         self.is_set(AppSettings::AllowHyphenValues)
     }
@@ -3598,7 +3569,11 @@ impl Command {
         self.is_set(AppSettings::AllowNegativeNumbers)
     }
 
-    /// Report whether [`Command::trailing_var_arg`] is set
+    #[doc(hidden)]
+    #[cfg_attr(
+        feature = "deprecated",
+        deprecated(since = "4.0.0", note = "Replaced with `Arg::is_trailing_var_arg_set`")
+    )]
     pub fn is_trailing_var_arg_set(&self) -> bool {
         self.is_set(AppSettings::TrailingVarArg)
     }
@@ -3809,6 +3784,35 @@ impl Command {
 
             self.args._build();
 
+            #[allow(deprecated)]
+            if self.is_trailing_var_arg_set() {
+                let highest_idx = self
+                    .get_keymap()
+                    .keys()
+                    .filter_map(|x| {
+                        if let crate::mkeymap::KeyType::Position(n) = x {
+                            Some(*n)
+                        } else {
+                            None
+                        }
+                    })
+                    .max()
+                    .unwrap_or(0);
+                for pos in self.args.args_mut() {
+                    if pos.get_index() == Some(highest_idx) {
+                        pos.settings.insert(ArgSettings::TrailingVarArg.into());
+                    }
+                }
+            }
+            #[allow(deprecated)]
+            if self.is_allow_hyphen_values_set() {
+                for arg in self.args.args_mut() {
+                    if arg.is_takes_value_set() {
+                        arg.settings.insert(ArgSettings::AllowHyphenValues.into());
+                    }
+                }
+            }
+
             #[cfg(debug_assertions)]
             assert_app(self);
             self.settings.set(AppSettings::Built);