Inherited positional parameter gets overridden by default value if placed after subcommand

[danielthegray]

If a positional parameter is defined on a root command, with an INHERIT scope, and with a default value, the positional parameter value is only preserved correctly if it is written before the subcommand. If written after the subcommand, the default value gets overwritten. This should not happen, as it has already been initialized in the call.

I have prepared a pull request with a test which showcases this failure.

[danielthegray]

Another problem is that the help message itself puts the inherited positional parameter at the end, even though this is the only way that it does NOT work. So another option would be to move it back to before the subcommand in the usage/help message.

[remkop]

Thanks for raising this!
Away from my PC now, I will look into this in more detail when I get a chance.

[danielthegray]

If it's at all helpful, I'll share the findings from my debugging process:

In the case where the positional parameter is set before the subcommand, the processArguments() call in CommandLine.java:12547 processes it and correctly places the ArgSpec into the initialized set. Therefore, in the applyDefaultValues() call in CommandLine.java:12548 (and the condition check in CommandLIne.java:12623) the default value of the parameter is not set, since it is found in the initialized.

In the second case, the subcommand creates its own initialized set (in the .parse() call inside processSubcommand on CommandLine.java:12811) and thus processes the parameter value correctly, but then gets overwritten in the applyDefaultValues() call of the root command parse() call.

I initially tried to fix it by sharing one single initialized set between subcommand calls (by adding it as a parameter), but unfortunately, the ArgSpec object was different for inside the subcommand, so the applyDefaultValues() check failed and the value was overwritten anyway.

[remkop]

@danielthegray Yes, that is helpful and your analysis makes a lot of sense.
I just need to find the time to sit down and work through it to get to a solution, but I have not been able to do so yet.
Maybe (hopefully) this weekend.

Thank you again for the test case, that is very helpful. (I've marked it as @ignored for now to allow other people to build the project.)

[remkop]

I had a chance to look at this.
One idea is to, in addition to sharing a single initialized set between subcommands (by adding it as a parameter to parse), to also ensure that this initialized set contains both the actual ArgSpec object, but also, if ArgSpec.inherited() == true, then also to add the corresponding ArgSpec in the parent command that this ArgSpec was inherited from.

So, instead of calling initialized.add(argSpec) directly in applyValuesToArrayField, applyValuesToCollectionField, applyValueToSingleValuedField and applyValuesToMapField, we create a wrapper method to do this. Something like this:

private void addToInitialized(ArgSpec argSpec, Set<ArgSpec> initialized) {
    initialized.add(argSpec);
    CommandSpec parent = argSpec.command().parent();
    while (argSpec.inherited() && parent != null) {
        initialized.add(parent.findOriginal(argSpec));
        parent = parent.parent();
    }
}

Then we still need to implement CommandSpec::findOriginal. For options we can match on the longest option name, for positional args it is more tricky; we could use PositionalParamSpec::toString, but that may not be reliable for applications that use the programmatic API. Perhaps a combination of toString, paramLabel, description, descriptionKey, index and arity can be safely considered to produce a unique key that is equal for inherited positional params.

Come to think of it, I wonder why this problem does not occur for named options, or did I miss this in testing too?

@danielthegray Would you be interested in providing a pull request for this?

[danielthegray]

@remkop Sounds good! I will give this a try and submit a pull request as soon as I can. Thanks for your input!

I'll also look into what happens with named options :)

[danielthegray]

@remkop I implemented the described solution, but some other unit tests now fail. The one that most clearly shows it is:

@Test
public void testAnnotatedSubcommandWithDoubleMixin() throws Exception {
    AnnotatedClassWithMixinParameters command = new AnnotatedClassWithMixinParameters();
    CommandLine commandLine = new CommandLine(command);
    List<CommandLine> parsed = commandLine.parse("-a 3 -b 5 sum -y foo -y bar -a 7 -b 11 13 42".split(" "));
    // ^^^^^^this line throws an exception

which throws the following exception:

option '-a' (<a>) should be specified only once
picocli.CommandLine$OverwrittenOptionException: option '-a' (<a>) should be specified only once
	at picocli.CommandLine$Interpreter.applyValueToSingleValuedField(CommandLine.java:13216)
	at picocli.CommandLine$Interpreter.applyOption(CommandLine.java:13084)
	at picocli.CommandLine$Interpreter.processStandaloneOption(CommandLine.java:12972)
	at picocli.CommandLine$Interpreter.processArguments(CommandLine.java:12810)
	at picocli.CommandLine$Interpreter.parse(CommandLine.java:12582)
	at picocli.CommandLine$Interpreter.processSubcommand(CommandLine.java:12846)
	at picocli.CommandLine$Interpreter.processArguments(CommandLine.java:12764)
	at picocli.CommandLine$Interpreter.parse(CommandLine.java:12582)
	at picocli.CommandLine$Interpreter.parse(CommandLine.java:12563)
	at picocli.CommandLine$Interpreter.parse(CommandLine.java:12464)
	at picocli.CommandLine.parse(CommandLine.java:1440)
	at picocli.CommandMethodTest.testAnnotatedSubcommandWithDoubleMixin(CommandMethodTest.java:303

which is because of this check inside applyValueToSingleValuedField():

if (argSpec.group() == null && initialized.contains(argSpec)) {
    if (!isOverwrittenOptionsAllowed()) {
        throw new OverwrittenOptionException(CommandLine.this, argSpec, optionDescription("", argSpec, 0) +  " should be specified only once");
    }
    traceMessage = overwriteValueMessage;
}

So, the initialized is actually used as well to prevent overwriting the option twice inside the same evaluation context (of the subcommand I assume).

Should I separate this into two cases perhaps? Will a mixin with a positional parameter clash with this?

Another option I can think of is to change the applyDefaultValues() call itself... should it only apply the default value if the value is something not-null? I assume that this frees us from needing to share the initialized set, which other functionality seems to be based on. I'll submit the work done so far as a "bad" PR, to see if it's valuable or just worth discarding.

[remkop]

At first glance, that exception looks correct: -a on the subcommand causes the inherited -a option on the parent command to be overwritten. This needs to be explicitly enabled.

I need to go over the old tickets related to inherited options to see if this scenario was discussed. If not, your PR may be fine as is.
If inherited options are “special” and should allow overwriting then we’ll need to tune this a bit.

[remkop]

Potentially related older tickets:

• Order of configured global options should not be enforced
• Can required options be inherited?
• Default values should not be applied to inherited options
• Allow making inherited options hidden on subcommands
• How do I configure global options for all subcommands?

None of these issue discuss this scenario of end users setting an inherited option in the top-level command and then specifying it again on a subcommand. I think it is fine to treat this as a bug. I will raise a separate ticket for that, and your PR will fix both these issues.