Merge pull request clap-rs#4030 from epage/take

fix!: Remove bookkeeping getters from API

clap_complete/src/generator/utils.rs

@@ -121,14 +121,14 @@ pub fn longs_and_visible_aliases(p: &Command) -> Vec<String> {
 pub fn flags<'help>(p: &Command<'help>) -> Vec<Arg<'help>> {
     debug!("flags: name={}", p.get_name());
     p.get_arguments()
-        .filter(|a| !a.is_takes_value_set() && !a.is_positional())
+        .filter(|a| !a.get_num_args().expect("built").takes_values() && !a.is_positional())
         .cloned()
         .collect()
 }
 
 /// Get the possible values for completion
 pub fn possible_values<'help>(a: &Arg<'help>) -> Option<Vec<clap::builder::PossibleValue<'help>>> {
-    if !a.is_takes_value_set() {
+    if !a.get_num_args().expect("built").takes_values() {
         None
     } else {
         a.get_value_parser()

clap_complete/src/shells/fish.rs

@@ -148,7 +148,7 @@ fn gen_fish_inner(
 }
 
 fn value_completion(option: &Arg) -> String {
-    if !option.is_takes_value_set() {
+    if !option.get_num_args().expect("built").takes_values() {
         return "".to_string();
     }
 

clap_complete/src/shells/zsh.rs

@@ -462,10 +462,7 @@ fn write_opts_of(p: &Command, p_global: Option<&Command>) -> String {
             Some(val) => format!(":{}:{}", vn, val),
             None => format!(":{}: ", vn),
         };
-        let vc = match o.get_num_args() {
-            Some(num_vals) => vc.repeat(num_vals.min_values()),
-            None => vc,
-        };
+        let vc = vc.repeat(o.get_num_args().expect("built").min_values());
 
         if let Some(shorts) = o.get_short_and_visible_aliases() {
             for short in shorts {
@@ -625,7 +622,8 @@ fn write_positionals_of(p: &Command) -> String {
     for arg in p.get_positionals() {
         debug!("write_positionals_of:iter: arg={}", arg.get_id());
 
-        let cardinality = if arg.is_multiple_values_set() {
+        let num_args = arg.get_num_args().expect("built");
+        let cardinality = if num_args != 1.into() && num_args != 0.into() {
             "*:"
         } else if !arg.is_required_set() {
             ":"

clap_complete_fig/src/fig.rs

@@ -321,7 +321,7 @@ fn gen_options(cmd: &Command, indent: usize) -> String {
 }
 
 fn gen_args(arg: &Arg, indent: usize) -> String {
-    if !arg.is_takes_value_set() {
+    if !arg.get_num_args().expect("built").takes_values() {
         return "".to_string();
     }
 
@@ -334,7 +334,8 @@ fn gen_args(arg: &Arg, indent: usize) -> String {
         indent = indent
     ));
 
-    if arg.is_multiple_values_set() {
+    let num_args = arg.get_num_args().expect("built");
+    if num_args != 0.into() && num_args != 1.into() {
         buffer.push_str(&format!(
             "{:indent$}isVariadic: true,\n",
             "",

src/builder/arg.rs

@@ -3709,8 +3709,8 @@ impl<'help> Arg<'help> {
     }
 
     #[inline]
-    pub(crate) fn get_min_vals(&self) -> Option<usize> {
-        self.get_num_args().map(|r| r.min_values())
+    pub(crate) fn get_min_vals(&self) -> usize {
+        self.get_num_args().expect(INTERNAL_ERROR_MSG).min_values()
     }
 
     /// Get the delimiter between multiple values
@@ -3790,13 +3790,11 @@ impl<'help> Arg<'help> {
         self.is_set(ArgSettings::Required)
     }
 
-    /// Report whether [`Arg::num_args`] allows multiple values
-    pub fn is_multiple_values_set(&self) -> bool {
+    pub(crate) fn is_multiple_values_set(&self) -> bool {
         self.is_set(ArgSettings::MultipleValues)
     }
 
-    /// Report whether [`Arg::is_takes_value_set`] is set
-    pub fn is_takes_value_set(&self) -> bool {
+    pub(crate) fn is_takes_value_set(&self) -> bool {
         self.is_set(ArgSettings::TakesValue)
     }
 
@@ -4060,7 +4058,7 @@ impl<'help> Display for Arg<'help> {
             write!(f, "-{}", s)?;
         }
         let mut need_closing_bracket = false;
-        let is_optional_val = self.get_min_vals() == Some(0);
+        let is_optional_val = self.get_min_vals() == 0;
         if self.is_positional() {
             if is_optional_val {
                 let sep = "[";
@@ -4171,13 +4169,7 @@ pub(crate) fn render_arg_val(arg: &Arg) -> String {
 
     let mut extra_values = false;
     debug_assert!(arg.is_takes_value_set());
-    let num_vals = arg.get_num_args().unwrap_or_else(|| {
-        if arg.is_multiple_values_set() {
-            (1..).into()
-        } else {
-            1.into()
-        }
-    });
+    let num_vals = arg.get_num_args().expect(INTERNAL_ERROR_MSG);
     if val_names.len() == 1 {
         let arg_name = format!("<{}>", val_names[0]);
         let min = num_vals.min_values().max(1);
@@ -4187,7 +4179,6 @@ pub(crate) fn render_arg_val(arg: &Arg) -> String {
             }
             rendered.push_str(&arg_name);
         }
-        extra_values |= arg.get_num_args().is_none() && arg.is_multiple_values_set();
         extra_values |= min < num_vals.max_values();
     } else {
         debug_assert!(1 < val_names.len());

src/builder/debug_asserts.rs

@@ -5,6 +5,7 @@ use clap_lex::RawOsStr;
 use crate::builder::arg::ArgProvider;
 use crate::mkeymap::KeyType;
 use crate::ArgAction;
+use crate::INTERNAL_ERROR_MSG;
 use crate::{Arg, Command, ValueHint};
 
 pub(crate) fn assert_app(cmd: &Command) {
@@ -535,7 +536,7 @@ fn _verify_positionals(cmd: &Command) -> bool {
             .get_positionals()
             .filter(|p| {
                 p.is_multiple_values_set()
-                    && !p.get_num_args().map(|r| r.is_fixed()).unwrap_or(false)
+                    && !p.get_num_args().expect(INTERNAL_ERROR_MSG).is_fixed()
             })
             .count();
         let ok = count <= 1
@@ -690,44 +691,44 @@ fn assert_arg(arg: &Arg) {
         );
     }
 
-    if let Some(num_vals) = arg.get_num_args() {
-        // This can be the cause of later asserts, so put this first
-        if num_vals != 0.into() {
-            // HACK: Don't check for flags to make the derive easier
-            let num_val_names = arg.get_value_names().unwrap_or(&[]).len();
-            if num_vals.max_values() < num_val_names {
-                panic!(
-                    "Argument {}: Too many value names ({}) compared to `num_args` ({})",
-                    arg.name, num_val_names, num_vals
-                );
-            }
+    let num_vals = arg.get_num_args().expect(INTERNAL_ERROR_MSG);
+    // This can be the cause of later asserts, so put this first
+    if num_vals != 0.into() {
+        // HACK: Don't check for flags to make the derive easier
+        let num_val_names = arg.get_value_names().unwrap_or(&[]).len();
+        if num_vals.max_values() < num_val_names {
+            panic!(
+                "Argument {}: Too many value names ({}) compared to `num_args` ({})",
+                arg.name, num_val_names, num_vals
+            );
         }
+    }
 
-        assert_eq!(
-            num_vals.takes_values(),
-            arg.is_takes_value_set(),
-            "Argument {}: mismatch between `num_args` ({}) and `takes_value`",
-            arg.name,
-            num_vals,
-        );
-        assert_eq!(
-            num_vals.is_multiple(),
-            arg.is_multiple_values_set(),
-            "Argument {}: mismatch between `num_args` ({}) and `multiple_values`",
+    assert_eq!(
+        num_vals.takes_values(),
+        arg.is_takes_value_set(),
+        "Argument {}: mismatch between `num_args` ({}) and `takes_value`",
+        arg.name,
+        num_vals,
+    );
+    assert_eq!(
+        num_vals.is_multiple(),
+        arg.is_multiple_values_set(),
+        "Argument {}: mismatch between `num_args` ({}) and `multiple_values`",
+        arg.name,
+        num_vals,
+    );
+
+    if 1 < num_vals.min_values() {
+        assert!(
+            !arg.is_require_equals_set(),
+            "Argument {}: cannot accept more than 1 arg (num_args={}) with require_equals",
             arg.name,
-            num_vals,
+            num_vals
         );
-
-        if 1 < num_vals.min_values() {
-            assert!(
-                !arg.is_require_equals_set(),
-                "Argument {}: cannot accept more than 1 arg (num_args={}) with require_equals",
-                arg.name,
-                num_vals
-            );
-        }
     }
-    if arg.get_num_args() == Some(1.into()) {
+
+    if num_vals == 1.into() {
         assert!(
             !arg.is_multiple_values_set(),
             "Argument {}: mismatch between `num_args` and `multiple_values`",

src/macros.rs

@@ -403,14 +403,22 @@ macro_rules! arg_impl {
         $crate::arg_impl! {
             @arg
             ({
-                if $arg.get_long().is_none() && $arg.get_short().is_none() {
-                    $arg.num_args(1..)
-                        // Allow collecting arguments interleaved with flags
-                        .action($crate::ArgAction::Append)
-                } else if $arg.is_takes_value_set() {
-                    $arg.action($crate::ArgAction::Append)
-                } else {
-                    $arg.action($crate::ArgAction::Count)
+                match $arg.get_action() {
+                    $crate::ArgAction::Set => {
+                        if $arg.get_long().is_none() && $arg.get_short().is_none() {
+                            $arg.num_args(1..)
+                                // Allow collecting arguments interleaved with flags
+                                .action($crate::ArgAction::Append)
+                        } else {
+                            $arg.action($crate::ArgAction::Append)
+                        }
+                    },
+                    $crate::ArgAction::SetTrue => {
+                        $arg.action($crate::ArgAction::Count)
+                    }
+                    action => {
+                        panic!("Unexpected action {:?}", action)
+                    }
                 }
             })
             $($tail)*

src/output/help.rs

@@ -297,7 +297,7 @@ impl<'help, 'cmd, 'writer> Help<'help, 'cmd, 'writer> {
         debug!("Help::val: arg={}", arg.name);
         let mut need_closing_bracket = false;
         if arg.is_takes_value_set() && !arg.is_positional() {
-            let is_optional_val = arg.get_min_vals() == Some(0);
+            let is_optional_val = arg.get_min_vals() == 0;
             let sep = if arg.is_require_equals_set() {
                 if is_optional_val {
                     need_closing_bracket = true;

src/parser/arg_matcher.rs

@@ -202,15 +202,12 @@ impl ArgMatcher {
             "ArgMatcher::needs_more_vals: o={}, pending={}",
             o.name, num_pending
         );
-        if let Some(expected) = o.get_num_args() {
-            debug!(
-                "ArgMatcher::needs_more_vals: expected={}, actual={}",
-                expected, num_pending
-            );
-            expected.accepts_more(num_pending)
-        } else {
-            o.is_multiple_values_set()
-        }
+        let expected = o.get_num_args().expect(INTERNAL_ERROR_MSG);
+        debug!(
+            "ArgMatcher::needs_more_vals: expected={}, actual={}",
+            expected, num_pending
+        );
+        expected.accepts_more(num_pending)
     }
 
     pub(crate) fn pending_arg_id(&self) -> Option<&Id> {

src/parser/parser.rs

@@ -978,7 +978,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
         debug!("Parser::parse_opt_value; Checking for val...");
         // require_equals is set, but no '=' is provided, try throwing error.
         if arg.is_require_equals_set() && !has_eq {
-            if arg.get_min_vals() == Some(0) {
+            if arg.get_min_vals() == 0 {
                 debug!("Requires equals, but min_vals == 0");
                 let arg_values = Vec::new();
                 let trailing_idx = None;
@@ -1252,9 +1252,9 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
         }
 
         let actual = raw_vals.len();
+        let expected = arg.get_num_args().expect(INTERNAL_ERROR_MSG);
 
-        let min_vals = arg.get_min_vals().unwrap_or(1);
-        if arg.is_takes_value_set() && 0 < min_vals && actual == 0 {
+        if 0 < expected.min_values() && actual == 0 {
             // Issue 665 (https://github.com/clap-rs/clap/issues/665)
             // Issue 1105 (https://github.com/clap-rs/clap/issues/1105)
             return Err(ClapError::empty_value(
@@ -1266,42 +1266,39 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
                     .collect::<Vec<_>>(),
                 arg.to_string(),
             ));
-        }
-
-        if let Some(expected) = arg.get_num_args() {
-            if let Some(expected) = expected.num_values() {
-                if expected != actual {
-                    debug!("Validator::validate_arg_num_vals: Sending error WrongNumberOfValues");
-                    return Err(ClapError::wrong_number_of_values(
-                        self.cmd,
-                        arg.to_string(),
-                        expected,
-                        actual,
-                        Usage::new(self.cmd).create_usage_with_title(&[]),
-                    ));
-                }
-            } else if actual < expected.min_values() {
-                return Err(ClapError::too_few_values(
+        } else if let Some(expected) = expected.num_values() {
+            if expected != actual {
+                debug!("Validator::validate_arg_num_vals: Sending error WrongNumberOfValues");
+                return Err(ClapError::wrong_number_of_values(
                     self.cmd,
                     arg.to_string(),
-                    expected.min_values(),
+                    expected,
                     actual,
                     Usage::new(self.cmd).create_usage_with_title(&[]),
                 ));
-            } else if expected.max_values() < actual {
-                debug!("Validator::validate_arg_num_vals: Sending error TooManyValues");
-                return Err(ClapError::too_many_values(
-                    self.cmd,
-                    raw_vals
-                        .last()
-                        .expect(INTERNAL_ERROR_MSG)
-                        .to_string_lossy()
-                        .into_owned(),
-                    arg.to_string(),
-                    Usage::new(self.cmd).create_usage_with_title(&[]),
-                ));
             }
+        } else if actual < expected.min_values() {
+            return Err(ClapError::too_few_values(
+                self.cmd,
+                arg.to_string(),
+                expected.min_values(),
+                actual,
+                Usage::new(self.cmd).create_usage_with_title(&[]),
+            ));
+        } else if expected.max_values() < actual {
+            debug!("Validator::validate_arg_num_vals: Sending error TooManyValues");
+            return Err(ClapError::too_many_values(
+                self.cmd,
+                raw_vals
+                    .last()
+                    .expect(INTERNAL_ERROR_MSG)
+                    .to_string_lossy()
+                    .into_owned(),
+                arg.to_string(),
+                Usage::new(self.cmd).create_usage_with_title(&[]),
+            ));
         }
+
         Ok(())
     }
 

src/parser/validator.rs

@@ -33,7 +33,7 @@ impl<'help, 'cmd> Validator<'help, 'cmd> {
 
             let o = &self.cmd[&a];
             let should_err = if let Some(v) = matcher.args.get(&o.id) {
-                v.all_val_groups_empty() && o.get_min_vals() != Some(0)
+                v.all_val_groups_empty() && o.get_min_vals() != 0
             } else {
                 true
             };