ArgGroup with optional positional parameters

[mattjlewis]

Multiple positional parameters, some of which have arity "0..1" work outside of an ArgGroup but not within. Example:

	@Parameters(index = "0", arity = "1", description = "parameter 0")
	String param0;
	@Parameters(index = "1", arity = "0..1", description = "parameter 1")
	String param1;
	@Parameters(index = "2", arity = "0..1", description = "parameter 2")
	String param2;

Test is run with e.g. "a b c".

I appreciate that I could use String[] with arity "1..3", but that doesn't fit my scenario - I have simplified the test case to reproduce the issue.

Things get even stranger when param0 is change to int (as per my scenario) - the numeric command line parameter gets dropped altogether when running with, e.g. "1 a b" with the ArgGroup example, but not the non-ArgGroup one.

Note I've read the limitations.

Test case attached.
test.zip

[kurtkaiser]

I am looking into this issue. @mattjlewis being relatively new to picoCLI, to clarify, is this a bug or new functionality? I want to make sure I understand as I dig into this. Currently both test files fail for me, is that expected?

[remkop]

CliTest1 runs without issues for me. For CliTest2, I am getting the following error message:

Arguments: 1, a, b
Error: expected only one match but got (<param0> [<param1>] [<param2>])={params[0]=1} and (<param0> [<param1>] [<param2>])={params[0]=a} and (<param0> [<param1>] [<param2>])={params[0]=b}
Usage: CLI Test 2 [-hV] (<param0> [<param1>] [<param2>])
      <param0>     parameter 0
      [<param1>]   parameter 1
      [<param2>]   parameter 2
  -h, --help       Show this help message and exit.
  -V, --version    Print version information and exit.

@mattjlewis, @kurtkaiser This looks like a bug; when all required elements of a group have been matched, there is some parser logic to determine whether a newly encountered parameter should start a new group or should be added to the group that is "in progress". It looks like the parser makes the wrong decision and creates a new group. When all args have been parsed, the parser thinks 3 separate occurrences of the group have been matched and decides that the input is invalid because the group is defined as multiplicity=1 (max one occurrence).

The logic in this method: picocli.CommandLine.ParseResult.GroupMatchContainer#canMatchPositionalParam looks suspicious, but I need to spend more time to dig into this. All help is welcome, of course! 😁

I added a test src/test/java/picocli/ArgGroupTest.java#testIssue1384 to facilitate the investigation.

[kurtkaiser]

Thanks @remkop, I have modified how I am running the application and can now replicate what you see. It has taken a bit to understand the application, even watched a few YouTube videos in foreign languages. I am trying to understand the different behavior ocurring between the two test files. Not sure I comprehend the matching and so that is what I am looking at now. Hopefully I can get this bug taken care of, tips and good vibes are appreciated.

[remkop]

@kurtkaiser Thanks for spending the time to look into this. Good vibes are going your way! 📡

Not sure if this constitutes as a tip, but here is some background: 😅

Groups can have required and optional elements. The parser will "start a new group" and add arguments to it when the previous group did not exist or was fully populated.
Otherwise, the intention is for the parser to keep adding args to the existing group until it is fully populated.

The logic for determining whether a group is "fully populated" is different for options and positional parameters.

For positional parameters, the intention was to allow groups containing positional parameters.
(Originally my goal was to allow multiple groups to have positional parameters, or have commands with positional parameters both in a group as well as in the command (outside a group) but that proved too difficult...)

I don't think I considered the case where multiple positional parameters are optional or created tests for that prior to this issue.

[kurtkaiser]

@remkop My understanding is when a positional parameter is optional and no value is passed it is given a fallback value, correct? Therefore it is expected that fallback values are used for each optional positional parameter? Or am I getting my wires crossed? Trying to nail down what I should expect versus the data I am seeing. By the way, this project/repo/picoCLI is ambitious, impressive.

[remkop]

@kurtkaiser There is a difference between fallback values and default values. Fallback values pretty much only apply to named options (not to positional parameters): they are the value that is applied when the option name is specified on the command line, without an option parameter following the option name.
(I may be nitpicking here, but I thought it was important to clarify this point to avoid confusion later on.) 😅

So, assuming we are talking about default values, yes, if positional parameters have a default value, then that default value is applied when no value is specified for that positional parameter.

However, argument groups are somewhat exceptional: if no element of the group was matched on the command line, then picocli will not instantiate the @ArgGroup-annotated element and will not assign any defaults. To say this again in a different way, default values for options/positional parameters in an argument group are only applied if at least one option/positional parameter of that group was matched on the command line (which forces picocli to instantiate that group).

Does that answer your question?

[mattjlewis]

Thank you for looking into this.

[kurtkaiser]

Been able to go through more of the documentation and spend time debugging this weekend. The idea of parsing and new group creation along with matching could be tripping me up. If "myCreateNewMatch" that will impact if a new group is created, correct? The idea of a match space is being evalulated and if sufficient it can match and be added. However, if last minimum element, if the arg block is full then a group must be created?

If that is the case, group creation is not being referenced directly in canMatchPositionalParam, correct? IYou original and super helpful response referenced that method.

@remkop thanks for all your input thus far.

Also, if anyone curious, some of the debug logs I'm going through.

[remkop]

Hi @kurtkaiser thanks for continuing the investigation!
I see the screenshots you posted but it is hard for me to understand what you are trying to convey.

Are you interested in doing some pair debugging session, maybe using Code With Me?
I live in Japan, and can be available between 5-8am JST Mon-Sat. Email me if you want to schedule something.

[remkop]

(Note to self)

While doing a pair debugging session, @kurtkaiser and I noticed that the problem does not occur when the last positional parameter in the group is required:

class OkGroup { // the problem does not occur with this group 
    @Parameters(index = "0", arity = "0..1") String param0; // optional
    @Parameters(index = "1", arity = "0..1") String param1; // optional
    @Parameters(index = "2", arity = "1")    String param2; // required
}

Also, we noticed that the cause of the problem seems related to the way positional parameters are processed in CommandLine.Interpreter#processPositionalParameter: for each argument encountered on the command line, this method loops over all PositionalParamSpec objects in the CommandSpec, seeing if the argument can be assigned to one or more of these PositionalParamSpec objects.

When the group is defined like this:

class ProblemGroup { // the problem *does* occur with this group 
    @Parameters(index = "0", arity = "1")    String param0; // required
    @Parameters(index = "1", arity = "0..1") String param1; // optional
    @Parameters(index = "2", arity = "0..1") String param2; // optional
}

Suppose the input provided is a b c.

Starting with the first argument, a:
The code in CommandLine.Interpreter#processPositionalParameter loops over all PositionalParamSpec objects, and finds the first one (associated with the param0 field). It then notices that this is in a group and calls parseResultBuilder.groupMatchContainer.findOrCreateMatchingGroup to find the associated GroupMatchContainer for this group. If no such group exists, a new one is created. So far so good, a new groupMatchContainer is created for a, and this container returns true for canMatchPositionalParam(positionalParam).
The code also loops over the remaining PositionalParamSpec objects, param1 and param2, but the same container returns false for canMatchPositionalParam(positionalParam) for these objects.

Next, b. Again, #processPositionalParameter loops over all PositionalParamSpec objects again. parseResultBuilder.groupMatchContainer.findOrCreateMatchingGroup returns the same groupMatchContainer that already contains a match for the first argument a. This container again returns true for canMatchPositionalParam(positionalParam) for param0. This is a problem. Picocli will create a new instance of the group, with the b argument assigned to param0 at index 0 of the second group. This way we will end up with 3 group matches, all with only the first slot filled, instead of one group match with all three slots filled.
Instead, what we want is that canMatchPositionalParam returns false for param0 (and param2), but returns true for param1.

During our debugging session we were running out of time at this point and did not look at possible solutions.
Looking at this again now I think the logic in canMatchPositionalParam is suspect, especially the logic that is trying to calculate what the index is of the current argument within the current group.

[remkop]

The bug may be in ArgGroupSpec#localPositionalParamCount.
The idea of this method is to determine how many positional parameters could maximally fit into a group.
It is currently implemented by taking the total sum of the minimum of the capacity range of the positional parameters in the group.

However, for our group with only 1 required param and 2 optional params, this results in a total sum of 1 for this group. This is incorrect: we should be able to fit 3 positional parameters in this group.

Changing the localPositionalParamCount implementation to taking the total sum of the maximum of the capacity ranges of the parameters in the group seems to fix the issue.

Index: src/main/java/picocli/CommandLine.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/main/java/picocli/CommandLine.java b/src/main/java/picocli/CommandLine.java
--- a/src/main/java/picocli/CommandLine.java	(revision df24ead53e6fdb6b919973639ef64afafe78502a)
+++ b/src/main/java/picocli/CommandLine.java	(date 1637743422797)
@@ -10228,7 +10228,7 @@
                 int result = 0;
                 for (ArgSpec arg : args) {
                     if (arg.isPositional()) {
-                        result += ((PositionalParamSpec) arg).capacity().min();
+                        result += ((PositionalParamSpec) arg).capacity().max();
                     }
                 }
                 return result;

[kurtkaiser]

Great details, I took last week off and plan to work more on the issue this week.

[kurtkaiser]

When the bug appears, if the arg group has the last parameter as optional, the following line evaluates to true. Having done some debugging, boolean mayCreateNewMatch = !matches.isEmpty() && lastMatch().matchedMinElements(); The last match matchedMinElements evaluating to true is the issue, I am trying to hunt down how that gets set.

[kurtkaiser]

Well, I spent a few hours on this. For the second parameter, when when required this will return false however, when option it does not and proceeds on to have an error.

if (matchedValues.get(arg) == null && (arg.required() || allRequired)) {
return false;
}

)

[remkop]

@kurtkaiser I actually thought (maybe being overly optimistic), that the code change to the localPositionalParamCount method that I suggested in my previous comment would solve the issue altogether. (That is, changing the localPositionalParamCount method to return the total sum of the capacity range maximum (instead of the minimum) of all parameters in the group.)

I was going to ask you to also take a look and see if you could find any edge case(s) where that fix would be insufficient.

If I follow what you are saying correctly, you are saying that, even with that change, you have found a scenario that gives incorrect result? Can you give more details about that scenario (like a failing test)?

[kurtkaiser]

Oh, I will test this, interesting. I had some concerns about edge cases and then got into the weeds as I went.

[kurtkaiser]

I threw a bunch of stuff at this and didnt see edge cases. Not sure if needed but I just pr'ed with the branch I created to do this new round of testing.

[remkop]

Perfect, thank you!