fix!: Make ArgAction::Set the default

This removes the need for `TakesValue` bookkeeping for when we knew we
took values but didn't know how many we should take.

Fixes clap-rs#2687

CHANGELOG.md

@@ -20,6 +20,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 - `arg!` now sets `ArgAction::SetTrue`, `ArgAction::Count`, `ArgAction::Set`, or `ArgAction::Append` as appropriate (#3795)
 - Default actions are now `Set` and `SetTrue`
 - Removed `PartialEq` and `Eq` from `Command`
+- By default, an `Arg`s default action is `ArgAction::Set`, rather than `ArgAction::SetTrue`
 - Replace `Arg::number_of_values` (average across occurrences) with `Arg::num_args` (per occurrence, raw CLI args, not parsed values)
   - `num_args(0)` no longer implies `takes_value(true).multiple_values(true)`
   - `num_args(1)` no longer implies `multiple_values(true)`

src/builder/arg.rs

@@ -778,16 +778,6 @@ impl<'help> Arg<'help> {
 
 /// # Value Handling
 impl<'help> Arg<'help> {
-    #[inline]
-    #[must_use]
-    fn takes_value(self, yes: bool) -> Self {
-        if yes {
-            self.setting(ArgSettings::TakesValue)
-        } else {
-            self.unset_setting(ArgSettings::TakesValue)
-        }
-    }
-
     /// Specify the behavior when parsing an argument
     ///
     /// # Examples
@@ -1024,7 +1014,7 @@ impl<'help> Arg<'help> {
     pub fn num_args(mut self, qty: impl Into<ValueRange>) -> Self {
         let qty = qty.into();
         self.num_vals = Some(qty);
-        self.takes_value(qty.takes_values())
+        self
     }
 
     /// Placeholder for the argument's value in the help message / usage.
@@ -1134,7 +1124,7 @@ impl<'help> Arg<'help> {
     #[must_use]
     pub fn value_names(mut self, names: &[&'help str]) -> Self {
         self.val_names = names.to_vec();
-        self.takes_value(true)
+        self
     }
 
     /// Provide the shell a hint about how to complete this argument.
@@ -1169,7 +1159,7 @@ impl<'help> Arg<'help> {
     #[must_use]
     pub fn value_hint(mut self, value_hint: ValueHint) -> Self {
         self.value_hint = Some(value_hint);
-        self.takes_value(true)
+        self
     }
 
     /// Match values against [`PossibleValuesParser`][crate::builder::PossibleValuesParser] without matching case.
@@ -1370,7 +1360,7 @@ impl<'help> Arg<'help> {
     #[must_use]
     pub fn value_delimiter(mut self, d: impl Into<Option<char>>) -> Self {
         self.val_delim = d.into();
-        self.takes_value(true)
+        self
     }
 
     /// Sentinel to **stop** parsing multiple values of a given argument.
@@ -1423,7 +1413,7 @@ impl<'help> Arg<'help> {
     #[must_use]
     pub fn value_terminator(mut self, term: &'help str) -> Self {
         self.terminator = Some(term);
-        self.takes_value(true)
+        self
     }
 
     /// Consume all following arguments.
@@ -1451,8 +1441,10 @@ impl<'help> Arg<'help> {
     #[inline]
     #[must_use]
     pub fn raw(mut self, yes: bool) -> Self {
-        self.num_vals.get_or_insert_with(|| (1..).into());
-        self.takes_value(yes).allow_hyphen_values(yes).last(yes)
+        if yes {
+            self.num_vals.get_or_insert_with(|| (1..).into());
+        }
+        self.allow_hyphen_values(yes).last(yes)
     }
 
     /// Value for the argument when not present.
@@ -1549,7 +1541,7 @@ impl<'help> Arg<'help> {
     #[must_use]
     pub fn default_values_os(mut self, vals: &[&'help OsStr]) -> Self {
         self.default_vals = vals.to_vec();
-        self.takes_value(true)
+        self
     }
 
     /// Value for the argument when the flag is present but no value is specified.
@@ -1681,7 +1673,7 @@ impl<'help> Arg<'help> {
     #[must_use]
     pub fn default_missing_values_os(mut self, vals: &[&'help OsStr]) -> Self {
         self.default_missing_vals = vals.to_vec();
-        self.takes_value(true)
+        self
     }
 
     /// Read from `name` environment variable when argument is not present.
@@ -1733,7 +1725,7 @@ impl<'help> Arg<'help> {
     ///
     /// ```rust
     /// # use std::env;
-    /// # use clap::{Command, Arg};
+    /// # use clap::{Command, Arg, ArgAction};
     /// # use clap::builder::FalseyValueParser;
     ///
     /// env::set_var("TRUE_FLAG", "true");
@@ -1742,14 +1734,17 @@ impl<'help> Arg<'help> {
     /// let m = Command::new("prog")
     ///     .arg(Arg::new("true_flag")
     ///         .long("true_flag")
+    ///         .action(ArgAction::SetTrue)
     ///         .value_parser(FalseyValueParser::new())
     ///         .env("TRUE_FLAG"))
     ///     .arg(Arg::new("false_flag")
     ///         .long("false_flag")
+    ///         .action(ArgAction::SetTrue)
     ///         .value_parser(FalseyValueParser::new())
     ///         .env("FALSE_FLAG"))
     ///     .arg(Arg::new("absent_flag")
     ///         .long("absent_flag")
+    ///         .action(ArgAction::SetTrue)
     ///         .value_parser(FalseyValueParser::new())
     ///         .env("ABSENT_FLAG"))
     ///     .get_matches_from(vec![
@@ -2621,7 +2616,7 @@ impl<'help> Arg<'help> {
     ) -> Self {
         self.default_vals_ifs
             .push((arg_id.into(), val.into(), default));
-        self.takes_value(true)
+        self
     }
 
     /// Specifies multiple values and conditions in the same manner as [`Arg::default_value_if`].
@@ -3401,7 +3396,8 @@ impl<'help> Arg<'help> {
     ///         .conflicts_with("debug")
     ///         .long("config"))
     ///     .arg(Arg::new("debug")
-    ///         .long("debug"))
+    ///         .long("debug")
+    ///         .action(ArgAction::SetTrue))
     ///     .try_get_matches_from(vec![
     ///         "prog", "--debug", "--config", "file.conf"
     ///     ]);
@@ -3782,7 +3778,7 @@ impl<'help> Arg<'help> {
     }
 
     pub(crate) fn is_takes_value_set(&self) -> bool {
-        self.is_set(ArgSettings::TakesValue)
+        self.get_action().takes_values()
     }
 
     /// Report whether [`Arg::allow_hyphen_values`] is set
@@ -3891,27 +3887,28 @@ impl<'help> Arg<'help> {
 /// # Internally used only
 impl<'help> Arg<'help> {
     pub(crate) fn _build(&mut self) {
-        if self.is_positional() {
-            self.settings.set(ArgSettings::TakesValue);
-        }
         if self.action.is_none() {
-            if self.get_id() == "help" && !self.is_takes_value_set() {
+            if self.get_id() == "help"
+                && matches!(self.get_num_args(), Some(ValueRange::EMPTY) | None)
+            {
                 let action = super::ArgAction::Help;
                 self.action = Some(action);
-            } else if self.get_id() == "version" && !self.is_takes_value_set() {
+            } else if self.get_id() == "version"
+                && matches!(self.get_num_args(), Some(ValueRange::EMPTY) | None)
+            {
                 let action = super::ArgAction::Version;
                 self.action = Some(action);
-            } else if self.is_takes_value_set() {
+            } else if self.num_vals == Some(ValueRange::EMPTY) {
+                let action = super::ArgAction::SetTrue;
+                self.action = Some(action);
+            } else {
                 let action = if self.is_positional() && self.is_multiple_values_set() {
                     // Allow collecting arguments interleaved with flags
                     super::ArgAction::Append
                 } else {
                     super::ArgAction::Set
                 };
                 self.action = Some(action);
-            } else {
-                let action = super::ArgAction::SetTrue;
-                self.action = Some(action);
             }
         }
         if let Some(action) = self.action.as_ref() {
@@ -3925,11 +3922,6 @@ impl<'help> Arg<'help> {
                     self.default_missing_vals = vec![default_value];
                 }
             }
-            if action.takes_values() {
-                self.settings.set(ArgSettings::TakesValue);
-            } else {
-                self.settings.unset(ArgSettings::TakesValue);
-            }
         }
 
         if self.value_parser.is_none() {
@@ -3944,11 +3936,12 @@ impl<'help> Arg<'help> {
         if val_names_len > 1 {
             self.num_vals.get_or_insert(val_names_len.into());
         } else {
-            if self.is_takes_value_set() {
-                self.num_vals.get_or_insert(ValueRange::SINGLE);
+            let nargs = if self.get_action().takes_values() {
+                ValueRange::SINGLE
             } else {
-                self.num_vals.get_or_insert(ValueRange::EMPTY);
-            }
+                ValueRange::EMPTY
+            };
+            self.num_vals.get_or_insert(nargs);
         }
     }
 

src/builder/arg_settings.rs

@@ -30,7 +30,6 @@ pub(crate) enum ArgSettings {
     Required,
     Global,
     Hidden,
-    TakesValue,
     NextLineHelp,
     HidePossibleValues,
     AllowHyphenValues,
@@ -52,7 +51,6 @@ bitflags! {
         const REQUIRED         = 1;
         const GLOBAL           = 1 << 3;
         const HIDDEN           = 1 << 4;
-        const TAKES_VAL        = 1 << 5;
         const NEXT_LINE_HELP   = 1 << 7;
         const DELIM_NOT_SET    = 1 << 10;
         const HIDE_POS_VALS    = 1 << 11;
@@ -76,7 +74,6 @@ impl_settings! { ArgSettings, ArgFlags,
     Required => Flags::REQUIRED,
     Global => Flags::GLOBAL,
     Hidden => Flags::HIDDEN,
-    TakesValue => Flags::TAKES_VAL,
     NextLineHelp => Flags::NEXT_LINE_HELP,
     HidePossibleValues => Flags::HIDE_POS_VALS,
     AllowHyphenValues => Flags::ALLOW_TAC_VALS,

src/builder/range.rs

@@ -108,7 +108,7 @@ impl std::ops::RangeBounds<usize> for ValueRange {
 
 impl Default for ValueRange {
     fn default() -> Self {
-        Self::EMPTY
+        Self::SINGLE
     }
 }
 

tests/builder/help.rs

@@ -401,21 +401,25 @@ OPTIONS:
             Arg::new("all")
                 .short('a')
                 .long("all")
+                .action(ArgAction::SetTrue)
                 .help("Also do versioning for private crates (will not be published)"),
         )
         .arg(
             Arg::new("exact")
                 .long("exact")
+                .action(ArgAction::SetTrue)
                 .help("Specify inter dependency version numbers exactly with `=`"),
         )
         .arg(
             Arg::new("no_git_commit")
                 .long("no-git-commit")
+                .action(ArgAction::SetTrue)
                 .help("Do not commit version changes"),
         )
         .arg(
             Arg::new("no_git_push")
                 .long("no-git-push")
+                .action(ArgAction::SetTrue)
                 .help("Do not push generated commit and tags to git remote"),
         );
     utils::assert_output(cmd, "test --help", WRAPPED_HELP, false);
@@ -445,21 +449,25 @@ OPTIONS:
             Arg::new("all")
                 .short('a')
                 .long("all")
+                .action(ArgAction::SetTrue)
                 .help("Also do versioning for private crates (will not be published)"),
         )
         .arg(
             Arg::new("exact")
                 .long("exact")
+                .action(ArgAction::SetTrue)
                 .help("Specify inter dependency version numbers exactly with `=`"),
         )
         .arg(
             Arg::new("no_git_commit")
                 .long("no-git-commit")
+                .action(ArgAction::SetTrue)
                 .help("Do not commit version changes"),
         )
         .arg(
             Arg::new("no_git_push")
                 .long("no-git-push")
+                .action(ArgAction::SetTrue)
                 .help("Do not push generated commit and tags to git remote"),
         );
     utils::assert_output(cmd, "test --help", UNWRAPPED_HELP, false);

tests/builder/help_env.rs

@@ -172,6 +172,7 @@ fn hide_env_flag() {
         Arg::new("cafe")
             .short('c')
             .long("cafe")
+            .action(ArgAction::SetTrue)
             .hide_env(true)
             .env("ENVVAR")
             .help("A coffeehouse, coffee shop, or café."),
@@ -188,6 +189,7 @@ fn show_env_flag() {
         Arg::new("cafe")
             .short('c')
             .long("cafe")
+            .action(ArgAction::SetTrue)
             .env("ENVVAR")
             .help("A coffeehouse, coffee shop, or café."),
     );
@@ -203,6 +205,7 @@ fn hide_env_vals_flag() {
         Arg::new("cafe")
             .short('c')
             .long("cafe")
+            .action(ArgAction::SetTrue)
             .hide_env_values(true)
             .env("ENVVAR")
             .help("A coffeehouse, coffee shop, or café."),
@@ -219,6 +222,7 @@ fn show_env_vals_flag() {
         Arg::new("cafe")
             .short('c')
             .long("cafe")
+            .action(ArgAction::SetTrue)
             .env("ENVVAR")
             .help("A coffeehouse, coffee shop, or café."),
     );