Skip to content

Commit

Permalink
fix: Clarify that get_num_args is safe to call
Browse files Browse the repository at this point in the history 
The only time it won't be initialized is before `_build`.  This is possible because
of clap-rs#4027

I wish I could just put the `expect` inside the call but I'm worried
about allowing people to build stuff on top of clap.
  • Loading branch information
@epage
epage committed Aug 4, 2022
1 parent 91b1055 commit 52ec1f9
Show file tree
Hide file tree
Showing 7 changed files with 77 additions and 92 deletions.
5 changes: 1 addition & 4 deletions clap_complete/src/shells/zsh.rs
View file
Expand Up @@ -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 {
Expand Down
15 changes: 4 additions & 11 deletions src/builder/arg.rs
View file
Expand Up @@ -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
Expand Down Expand Up @@ -4060,7 +4060,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 = "[";
Expand Down Expand Up @@ -4171,13 +4171,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);
Expand All @@ -4187,7 +4181,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());
Expand Down
69 changes: 35 additions & 34 deletions src/builder/debug_asserts.rs
View file
Expand Up @@ -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) {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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`",
Expand Down
2 changes: 1 addition & 1 deletion src/output/help.rs
View file
Expand Up @@ -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;
Expand Down
15 changes: 6 additions & 9 deletions src/parser/arg_matcher.rs
View file
Expand Up @@ -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> {
Expand Down
61 changes: 29 additions & 32 deletions src/parser/parser.rs
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -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(
Expand All @@ -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(())
}

Expand Down
2 changes: 1 addition & 1 deletion src/parser/validator.rs
View file
Expand Up @@ -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
};
Expand Down

0 comments on commit 52ec1f9

Please sign in to comment.