Mixing multiplicity or required + non-required options in an exclusive ArgGroup gives unexpected results

[scottjasso]

Related to #870

Tested with latest master. Using multiplicity="0..*" and exclusive = true with an ArgGroup, or exclusive = true with an ArgGroup that has both optional and required parameters gives unexpected results.

class Group {
    @Option(names = "--name", required = true) String name;
    @Option(names = "--id", required = true) String id;
    @Option(names = "--opt") String opt;
}

@ArgGroup(exclusive = true, multiplicity = "0..*") List<Group> groups;

1. cli --opt=1 results in one group: {name=null, opt=1}. Even though no required options are provided, validation passes.

2. cli --name=foo --opt=1 gives Error: --name=<name>, --opt=<opt> are mutually exclusive (specify only one). Even though --opt is optional, it's still exclusive with --name.

3. cli --opt=1 --name=foo works and results in [{name=null, opt=1},{name=foo, opt=null}]. Whoa, switching the arg order makes it work? It still seems to be exclusive, but for some reason now it knows to create a second group.

4. cli --name=foo --id=bar gives Error: --name=<name>, --id=<id> are mutually exclusive (specify only one). This seems like a bug. Those should be two valid groups, right? {name=foo, id=null} and {name=null, id=bar}? Why doesn't it create a second group now?

I'd think, either (1) only the required arguments are exclusive, and it's still an error to specify an ArgGroup option without exactly one of the required args, as in #870, or maybe (2) picocli should just immediately error out if using non-required options w/ exclusive = True. I'm leaning heavily toward (1).

In general, I think the logic should be:

(1) Attempt to fit an arg into the previous group, unless it makes the group invalid. *
(2) If a new arg cannot be fit into the previous group, start a new group with it, and validate the (now completed) previous group.
(3) Now that a new group is started, check if the new number of groups > max multiplicity.
(4) exclusive simply changes the validation of required options in a group. exclusive=true means exactly 1 of the required options per group, and exclusive=false means all of the required options per group. Not specifying any required options should always be invalid.

Max multiplicity=1 can be a special case, where we know not to start a new group so the error message is clearer: "--foo, --bar are mutually exclusive"

* Consider making it so that only required args can start a new group. Optional args would always be pushed into the previous group, triggering a validation error if, for example, providing too many of that optional arg, and it would be illegal to start with an optional arg. This makes it always obvious which group an optional arg belongs to:
cli --req --opt --opt --req --req --opt --req -> always {req, opt, opt}, {req}, {req, opt}, {req}. Currently, it's {req, opt}, {opt, req}, {req, opt}, {req} if opt is singular, but {req, opt, opt}, {req}, {req, opt}, {req} if opt is repeatable.

[remkop]

Thanks again for raising this. I will need some time for this.

[scottjasso]

No problem at all, please take your time! Again I really appreciate your work here :)

[hanslovsky]

• Consider making it so that only required args can start a new group. Optional args would always be pushed into the previous group, triggering a validation error if, for example, providing too many of that optional arg, and it would be illegal to start with an optional arg

I would really like to see that or, even more restrictive, (optionally) specify a single argument that starts a new group (all other args would be pushed into the previous group). An example use-case is our connectomics visualization and proof-reading tool Paintera. N5 datasets can be added through the picocli CLI by specifying a container and one ore more datasets inside that container:

paintera --add-n5-container /data/hanslovskyp/sample_A_20160501.n5 -d volumes/raw volumes/labels/neuron_ids

It would make sense here to start a new group only with the --add-n5-container option, but this command (currently) is the same:

paintera -d volumes/raw volumes/labels/neuron_ids --add-n5-container /data/hanslovskyp/sample_A_20160501.n5

In practice, this will not happen (a lot) because users will probably intuitively specify the container first, but I think it may be confusing if they accidentally do so.

[remkop]

@hanslovsky I suspect that Paintera is a good use case for repeating subcommands. #454 is on the todo list for picocli 4.2.

@wjohnson5 sorry for my late reply, I had other commitments that took my time.
Looking at it again now, I have trouble wrapping my head around it all...

In the context of mutually dependent groups (where all options must co-occur), the notion of optional and required options makes sense. It is useful to be able to define a group as (-a -b [-c])... so that both -a -b -c and -a -b are valid on the command line, but not -a -c for example.

However, what does it mean if an option is not required in an exclusive group? Using the corresponding example: (-a | -b | [-c])..., what behaviour are we trying to achieve by defining the group this way?

I think the simplest thing to do, both from an implementation/maintenance perspective, but also from a usability perspective, is to make all options required in exclusive groups. To ease migration, we can silently make the default to be required=true in exclusive groups, and throw an InitializationException for options that are explicitly defined as required=false in exclusive groups. (The annotation processor can make this a compilation error.)

The example you mention in example 4 looks like a bug. If the max multiplicity is greater than 1, a new group should be created for each option in the exclusive group.

[scottjasso]

That makes sense to me.
I was thinking that optional args in an exclusive group would be something like ((-a | -b) [-c]), but combining two arg groups could likely accomplish the same thing and be more obvious in intention.

[remkop]

Fixed and included in the 4.1.2 release.

After enforcing all options to be required in exclusive groups, I could no longer reproduce the issue you mentioned in use case 4: if the group has max multiplicity > 1, a new group is correctly created when one of the options in the exclusive group is recognized.

Please verify when you get a chance.

I also updated the documentation for exclusive and non-exclusive groups to note the use of required options. Feedback welcome!