Custom Type Converters are missing for repeated subcommands

[diba1013]

Description

When using the newly introduced repeatable subcommands, I encountered that all custom registered type converters for the current CommandLine goes missing when trying to parse a subcommand with a custom type.

While this is fine for only one command, it will fail when using repeated subcommands (see example below).

I first thought that this is might be related to the fact that the converters are not registered for subcommands which are added afterwards. However, while debugging, all subcommands are correctly identified and parsed prior to adding the converter. Thus I think, that this is not the intended behavior.

Demo

Note: This uses lombok to get rid of some boilerplate.

@Slf4j
@Command( name = "git", mixinStandardHelpOptions = true, subcommandsRepeatable = true )
public class Git {

	@Command( name = "add" )
	public void add( @Option( names = "--stage" ) final Stage stage, @Parameters final Path file ) {
		log.info( "Adding file '{}' to stage '{}'.", file, stage.getName() );
	}

	public static void main( final String[] args ) {
		final CommandLine line = new CommandLine( new Git() );

		line.registerConverter( Stage.class, Stage::new );

		System.exit( line.execute( args ) );
	}

	@Value
	public static class Stage {

		private final String name;
	}
}

In my original code, I use a @Mixin to provide the options and parameters.

Example

Disclaimer: These have only been executed within the IDE.

Working

Input:

git add --stage=foo path/to/file1

Output:

[main] INFO Git - Adding file 'path/to/file' to stage 'foo'.

Failing

Input:

git add --stage=foo path/to/file1 add --stage=bar path/to/file2

Output:

No TypeConverter registered for Git$Stage of MethodParam void Git.add(Git$Stage,java.nio.file.Path):arg0
Usage: git add [--stage=<arg0>] <arg1>
      <arg1>
      --stage=<arg0>

Workaround

Simply add the converter on the option.

-public void add( @Option( names = "--stage" ) final Stage stage, @Parameters final Path file ) {
+public void add( @Option( names = "--stage", converter = StageConverter.class ) final Stage stage, @Parameters final Path file ) {

Potential Fix

While the initial subcommand retrieved in CommandLine:L11590 holds all custom converters, they are not copied over when a result is present (i.e. the subcommand has already been parsed) and a new a new CommandLine is forked: CommandLine:L11595. Thus it seems necessary to simply copy over the converterRegistry or introduce a proper CommandLine#copy-method.

Specs

picocli-version: 4.2.0
java-version: OpenJDK14

[remkop]

Thank you for raising this!
Looking into it...

[remkop]

Good catch!
Work is in progress on a proper CommandLine.copy() method...

[remkop]

It turns out that the scope of this issue is quite large and not limited to just custom type converter registration. Any programmatic change to a CommandLine object was not reflected in the CommandLine copy that is created on demand for repeatable subcommands.

Fixed so far

With the fixes I made so far, repeatable subcommands will now correctly pick up any programmatic changes made via public CommandLine methods after the CommandLine instance was constructed:

• registerConverter
• setAdjustLineBreaksForWideCJKCharacters
• setAtFileCommentChar
• setCaseInsensitiveEnumValuesAllowed
• setColorScheme
• setCommandName
• setDefaultValueProvider
• setEndOfOptionsDelimiter
• setErr
• setExecutionExceptionHandler
• setExecutionStrategy
• setExitCodeExceptionMapper
• setExpandAtFiles
• setHelpFactory
• setHelpSectionKeys
• setHelpSectionMap
• setInterpolateVariables
• setNegatableOptionTransformer
• setOut
• setOverwrittenOptionsAllowed
• setParameterExceptionHandler
• setPosixClusteredShortOptionsAllowed
• setResourceBundle
• setSeparator
• setSplitQuotedStrings
• setStopAtPositional
• setStopAtUnmatched
• setToggleBooleanFlags
• setTrimQuotes
• setUnmatchedArgumentsAllowed
• setUnmatchedOptionsArePositionalParams
• setUsageHelpAutoWidth
• setUsageHelpLongOptionsMaxWidth
• setUsageHelpWidth
• setUseSimplifiedAtFiles

Remaining

What is remaining is that repeatable subcommands will not correctly pick up CommandSpec objects that were created programmatically or modified programmatically.

Applications that programmatically add subcommands or mixins, or build CommandSpec by programmatically creating and adding OptionSpec and PositionalParamSpec objects will not work correctly in repeatable subcommands.

I created #1010 as a separate ticket for that follow-up work.

[diba1013]

Thanks a lot for the fast fix.

Any programmatic change to a CommandLine object was not reflected in the CommandLine copy that is created on demand for repeatable subcommands.

I was realizing that on your first commit.

I'm assuming that you want to create a new copy to have a new command-object available to set the fields on, in order for it to be executed with the IParseResultHandler.

Does it make sense to split the configuration and internal representation from the state (which might need a larger refactoring and thus nothing to consider before 5.0)? I am thinking to postpone the population of the command fields/parameters just before the execution through the IParseResultHandler (i.e. into executeUserObject)? Some kind of Decorator could be useful, which is able to set the fields of the command-object or parameters in case of a command-method (thus holds the parsed values, similar to CommandSpec#argValues). The CommandLine then does not set the values directly anymore but saves the accessors within the Decorator. Then the IParseResultHandler would be responsible for populating the command (which might break some implementations?) and executing it afterwards.

I am basing this on code for picocli:4.2.0 as presented by the IDE, thus it might be out of date. Secondly, I am only skimming over the code and might not fully understand each implication.

[remkop]

@diba1013 If you don't mind, I propose we continue discussion about the remaining work on the #1010 ticket.

I plan to close this ticket as soon as I get a chance to add some unit tests for the part I fixed so far.

[diba1013]

Sure, let's keep the scope of this ticket minimal. It already blew the expected amount of work.