clap_derive: Vec/Option<Vec> behavior is inconsistent with other types

[CreepySkeleton]

This is actually a summary of TeXitoi/structopt#364

Current behavior

• Vec<T> and Option<Vec<T>> are not required = true by default.
• Vec<T> is multiple = true by default which allows not only multiple values (--foo 1 2 3) but also multiple occurrences (--foo 1 --foo 2 3).
• Option<Vec<T>> additionally allows zero number of values (--foo).

What's wrong

• The fact that Vec<T> is not required by default is inconsistent with all the other types in clap_derive that are required by default unless they are wrapped in Option (except bool but it's a very special case).
• The fact that Option<Vec<T>> is different from Vec<T>, different not in "not required" sense, confuses newcomers.
• The fact that Vec<T> allows multiple occurrences along with values is misleading.

Proposal

• Use min_values = 1 for both Option<Vec<T>> and Vec<T> instead of multiple = true, allowing only non-zero number of values and disallow multiple occurrences (--foo 1 2 but not --foo nor --foo 1 --foo 2). If a user wants to allow zero values or multiple occurrences as well, they can explicitly specify it via min_values = 0 and multiple = true respectively.
• Use required = true for Vec<T>.

cc @TeXitoi @Dylan-DPC @pksunkara

[TeXitoi]

The fact that Vec is not required by default is inconsistent with all the other types in clap_derive that are required by default unless they are wrapped in Option (except bool but it's a very special case).

I strongly disagree on this. A vector can have 0 elements, and that's the most used case. If you need at least one, you can add min_values(1) yourself.

[CreepySkeleton]

If we use min_values(0), an empty vec would represent --foo <no values>, but "no --foo" would be disallowed due to required.

Option<Vec<T>> would represent just what it does currently:

command	Vec<T> result	Option<Vec<T>> result
app --foo 1 2	[1, 2]	Some([1, 2])
app --foo	[]	Some([])
app	ERRROR	None

[pksunkara]

The min_values and occurrences issue should be fixed in #1026. Let's leave it out of the discussion here.

The fact that Option<Vec> is different from Vec, different not in "not required" sense, confuses newcomers.

Can you explain what this means? I don't understand the sentence.

I think we should have min_values = 0 and multiple = true for both but required = true for Vec<T>.

[TeXitoi]

I really disagree. Most of the time you're using a Vec, you just want the list of parameters, and you don't really care if an empty parameter was provided. Forcing using an option of vec in the most common case would impact a lot the ergonomics.

[cecton]

Here is a use case where I find structopt very confusing: paritytech/substrate@6b0eed4

When I read the struct I see that listen_addr is a required argument and port is optional. But the structopt parameters shows me that both arguments are conflicting. Before that commit, the parameters weren't declared as conflicting and I had no way to know that listen_addr was actually optional. The only way I could know that is by reading the table.

I personally do think that Vec<_> should mean required (the argument must be provided) and Option<Vec<_>> should mean optional (the argument can be omitted).

I invite you to read the lengthy discussion here.

Forcing using an option of vec in the most common case would impact a lot the ergonomics.

Maybe but it will be consistent with the rest of the API.

[CreepySkeleton]

Pff, it took me some time to get back to, sorry for the delays guys.

The min_values and occurrences issue should be fixed in #1026. Let's leave it out of the discussion here.

That issue is related here indeed, but we can't leave it out of this discussion because this is about defaults in derive, not about distinguishing between the two.

Forcing using an option of vec in the most common case would impact a lot the ergonomics.

@TeXitoi I see your point about Option<Vec<T>> vs Vec<T> ergonomics. I agree with you, simple Vec is much more handy than Option<Vec<T>> to work with.

But we wouldn't be forcing anything! Users would still be very much able to do #[clap(required = false)] if they want to keep a raw Vec

struct Opts {
    // This Vec is not option so it's required BY DEFAULT
    // because it's not bool and not Option
    required_vec: Vec<u32>,

    // We can override default behavior 
    // while keeping the raw Vec 
    #[clap(required = false)]
    not_required_vec: Vec<u32>,

    // We can use Option<Vec> instead of overriding
    // We lose some of the ergonomics but acquire the 
    // ability to distinguish between <empty list> an <no option at all>
    opt_vec: Option<Vec<u32>> 
}

In other words, this is question of good defaults, and I believe I've found the answer.

I think that our willingness of changing defaults should depend of what the most widespread usage is in practice. I decided to check via grep.app and this has been eye opening for me:

(IMPORTANT: this research assumes that #[structopt] attributes are one-line which is is quite frequently not true. If somebody has an idea how to cover

#[structopt(
 method = name,
 method2 = expr, method3 = expr)

attributes with regexes, speak up!)

• required Vec or not.

  • Contains required = true and field is Vec - 33 matches.
  • All occurrences of #[structopt(...)] field: Vec - 214 matches. Therefore, the number of not required vecs is 214 - 33 = 181.
  
  Conclusion: current default is exactly what most of our users need (~85%). Changing it would be unwise.
  

• min_values = 0 vs min_values = 1 (behavior of multiple = true)

  • I searched "contains min_values" and there are only 13 results. None of them are min_values = 0. A little bonus: almost all of them are min_values = 1 which is the default behavior, lol.
  
  Conclusion: nobody wants `min_values = 0`. Making it default would be pointless.
  

• Multiple values vs multiple occurrences. Default behavior for now is both. This is kind of thing that is hard to grep for, so I tried to figure out how many users are aware of multiple occurrences in the first place:

  • Contains number_of_values - 11 results.
  • Contains max_values: 1 result.
  • Contains multiple = true - 11 matches.
    
    
    This is all I've managed to come up with. Looks like there are not many people out there who are aware or want multiple occurrences, but the results here are probably very inaccurate. I vote for using multiple_values = true and ask the community after the beta is out.

[pksunkara]

I think that our willingness of changing defaults should depend of what the most widespread usage is in practice.

I don't agree with this. Maybe all of them tried Option<Vec<T>> first and then decided that it didn't make a difference so they optimised it back to Vec<T>.

Also, we are not technically breaking this because this a separate library clap and not structopt. I would opt for it easier to understand w.r.t semantics in how to use the types for the fields and being different with each other rather than existing usage.

min_values should always be 0. As you saw, people saw Vec and with semantics they thought min_values might have been zero and that is why they customised the behaviour to be min_values = 1. (Related to #1682)

Note: multiple means multiple occurrences not values according to clap docs

Type	required	multiple
Vec<T>	true	false
Option<Vec<T>>	false	false
Vec<Vec<T>>	true	true
Option<Vec<Vec<T>>>	false	true

command	Vec<T>	Option<Vec<T>>	Vec<Vec<T>>	Option<Vec<Vec<T>>>
app -f 1 2	[1, 2]	Some([1, 2])	[[1, 2]]	Some([[1, 2]])
app -f	[]	Some([])	[[]]	Some([[]])
app	ERRROR	None	ERROR	None
app -f 1 2 -f 3 4	[3, 4]	Some([3, 4])	[[1, 2], [3, 4]]	Some([[1, 2], [3, 4]])
app -ff	[]	Some([])	[[], []]	Some([[], []])

By default, if one wants to support a -vvv kind of flag, they would have to use Option<Vec<Vec<bool>>>. But they always have the option of customizing stuff just by describing it using methods like how you already proposed.

#[clap(short, multiple = true, required = false, max_values = 0)]
verbose: Vec<bool>

[CreepySkeleton]

Maybe all of them tried Option<Vec> first and then decided that it didn't make a difference so they optimised it back to Vec.

Sounds very unlikely, but maybe. @TeXitoi , has anybody ever told you something like "I tried Option<Vec> first, but settled with Vec because it's handy"? I remember only one such message: TeXitoi/structopt#285 .

Also, we are not technically breaking this because this a separate library clap and not structopt

I agree, absolutely. But we aren't talking just about breaking changes, we're talking about good defaults and whether it's worth to change them. If we find that the current defaults are not optimal, we shall change them, otherwise we shall leave them as is.

As you saw, people saw Vec and with semantics they thought min_values might have been zero and that is why they customised the behaviour to be min_values = 1

Eleven (min_values = 1) people out of 214 (total number of Vecs discovered). Very few people have been confused, so min_values = 1 default is preferable for Vec and min_values = 0 is preferable for Option<Vec>. In my opinion.

---

I like your Vec<Vec<T>> idea. We need to fix #1026 first though. Also, in the light of #1682, we may also specialize Vec<[T; N]> (arrays) and Vec<(T1, T2, ...)> (tuples) along with the Option<...> variants of them.

By default, if one wants to support a -vvv kind of flag, they would have to use Option<Vec<Vec>>

This is exactly what parse(from_occurrences) is for, isn't it?