number_of_values(n) incorrectly allows argument counts that are integer multiples of n

[elldritch]

Reproducing the bug

Okay, this one is a bit of a headscratcher. I've been digging into this with Rust 1.48.0 and Clap 3.0.0-beta.2, although I bet 2.x.x suffers from the same problem given its definition of struct MatchedArg.

First, a test case:

use clap::*;

fn main() {
    let m = App::new("multiple_values")
        .arg(
            Arg::new("pos")
                .about("multiple positionals")
                .number_of_values(3),
        )
        .try_get_matches_from(vec![
            "myprog", "val1", "val2", "val3", "val4", "val5", "val6",
        ]);

    assert!(m.is_err()); // This panics, because `m.is_err() == false`.
    assert_eq!(m.unwrap_err().kind, ErrorKind::WrongNumberOfValues);
}

This test case should obviously fail: specifying number_of_values(3) should not allow a successful parse if I pass 6 values. If you play around with this, you find a couple of other interesting symptoms. In general, if I call number_of_values(x) and pass N values:

• When N == 0, the parse incorrectly succeeds. (This is because N % x == 0 - see below where I explain what's going on here).
• When x > 1:
  • When 0 < N < x, the parse correctly fails.
  • When N == x, the parse correctly succeeds.
  • When N > x && N % x == 0, the parse incorrectly succeeds.
• When x == 1 && N > 1, the parse correctly fails, but returns an UnknownArgument error instead of the WrongNumberOfValues error, which is interesting but actually I think correct (because the parser is trying to interpret the extra value as a new, unknown argument rather than an additional value to the previous argument).

This is also different from setting max_values(3) and min_values(3), which does what I want (only allow exactly three argument values), but also does so incorrectly (by ignoring multiple occurrences altogether!) and renders a different error message (max_values returns TooManyValues and min_values returns TooFewValues instead of WrongNumberOfValues).

What's going on?

The issue is that validation of argument counts for multiple values and multiple occurrences of arguments is fundamentally broken because Clap doesn't store which argument values were matched in which occurrences.

I have some failing test cases set up in my fork at master...liftM:bug/number-of-values-multioccurrence. I tried to put together a PR patch, but I'm not sure I understand all the implications for how multiple values and occurrences are handled.

For some reason, calling number_of_values sends us down the path to

clap/src/build/arg/mod.rs

Lines 4219 to 4239 in 08b2f4d

	// FIXME: (@CreepySkeleton)
	#[doc(hidden)]
	pub fn _build(&mut self) {
	if (self.is_set(ArgSettings::UseValueDelimiter)
	|| self.is_set(ArgSettings::RequireDelimiter))
	&& self.val_delim.is_none()
	{
	self.val_delim = Some(',');
	}
	if self.index.is_some() || (self.short.is_none() && self.long.is_none()) {
	if self.max_vals.is_some()
	|| self.min_vals.is_some()
	|| (self.num_vals.is_some() && self.num_vals.unwrap() > 1)
	{
	self.set_mut(ArgSettings::MultipleValues);
	self.set_mut(ArgSettings::MultipleOccurrences);
	}
	} else if self.is_set(ArgSettings::TakesValue) && self.val_names.len() > 1 {
	self.num_vals = Some(self.val_names.len() as u64);
	}
	}

and sets ArgSettings::MultipleValues and ArgSettings::MultipleOccurrences. When the number_of_values validator then runs, it goes down the code path at

clap/src/parse/validator.rs

Lines 445 to 466 in 08b2f4d

	if let Some(num) = a.num_vals {
	debug!("Validator::validate_arg_num_vals: num_vals set...{}", num);
	let should_err = if a.is_set(ArgSettings::MultipleValues) {
	((ma.vals.len() as u64) % num) != 0
	} else {
	num != (ma.vals.len() as u64)
	};
	if should_err {
	debug!("Validator::validate_arg_num_vals: Sending error WrongNumberOfValues");
	return Err(Error::wrong_number_of_values(
	a,
	num,
	if a.is_set(ArgSettings::MultipleValues) {
	ma.vals.len() % num as usize
	} else {
	ma.vals.len()
	},
	Usage::new(self.p).create_usage_with_title(&[]),
	self.p.app.color(),
	));
	}
	}

which checks that ((ma.vals.len() as u64) % num) != 0.

I assume this is happening as a hack in order to support multiple values and multiple occurrences. Unfortunately, this doesn't work:

• Documentation on the meaning of MultipleValues seems to be incorrect (see multiple_values() documentation wrong #1828), but I think the intention of this setting is to allow multiple values to be passed into a single occurrence of an argument.
• The documentation for MultipleOccurrences says that this setting is intended to allow multiple occurrences of an argument.
• Therefore, I think the correct, intended behavior for validating argument counts should be: (1) if MultipleOccurrences is set, allow any number of occurrences per argument, and (2) if MultipleValues is set, allow a validated number of values per argument occurrence.

Unfortunately, the current definition of MatchedArg at

clap/src/parse/matches/matched_arg.rs

Lines 12 to 18 in 08b2f4d

	#[derive(Debug, Clone)]
	pub(crate) struct MatchedArg {
	pub(crate) occurs: u64,
	pub(crate) ty: ValueType,
	pub(crate) indices: Vec<usize>,
	pub(crate) vals: Vec<OsString>,
	}

makes this impossible, because we cannot determine which values were passed through which occurrence. I believe the correct fix here is to refactor MatchedArg so that we not only count occurrences, but we in fact store each vector of occurrences (so that we can validate the correct count of values per occurrence).

Alternatively, another option is to make the design call to drop support for either multiple values or multiple occurrences. I think #2031 makes a pretty compelling argument that supporting both of them is confusing for both developers and users.

[elldritch]

Oh no, I accidentally hit shift-enter. Give me a moment to fix up the issue.

Issue is ready now. Sorry for the extra noise!

[pksunkara]

Yeah, I still need to go through all the multiple related issues and do a proper design for v3. Just short of time. If someone wants to pick this up and do a proper design and implement it, I would really appreciate it.