Feature request: disallow option parameter values resembling option names

[petermr]

Summary: an option value with leading -- (--junk) is not trapped and is added to option values.

This is not a complete program. (see

https://github.com/petermr/normami/ 

code at:

src/test/java/org/contentmine/ami/tools/AMISummaryToolTest.java

The class AMISummaryTool contains:

public class AMISummaryTool extends AbstractAMITool {

    @Option(names = {"--word"},
    		arity = "0",
            description = "analyze word frequencies")
    private boolean word = false;

    @Option(names = {"--dictionary"},
    		arity = "1..*",
            description = "dictionaries to summarize")
    private List<String> dictionaryList = new ArrayList<>();

    @Option(names = {"--gene"},
    		arity = "0..*",
    	    defaultValue = "human",
            description = "genes to summarize")
    private List<String> geneList = new ArrayList<>();

    @Option(names = {"--species"},
    		arity = "0..*",
    		defaultValue = "binomial",
            description = "species to summarize")
    private List<String> speciesList = new ArrayList<>();
    

	public AMISummaryTool(CProject cProject) {
		this.cProject = cProject;
	}
	
	public AMISummaryTool() {
	}
	
    public static void main(String[] args) throws Exception {
    	new AMISummaryTool().runCommands(args);
    }


    @Override
// this echoes the option values after parsing...
	protected void parseSpecifics() {
		System.out.println("dictionaryList       " + dictionaryList);
		System.out.println("geneList             " + geneList);
		System.out.println("speciesList          " + speciesList);
		System.out.println("word                 " + word);
		System.out.println();
	}

The test module

public class AMISummaryToolTest {
// snipped
	@Test
	public void testCommand() {
		File targetDir = new File("target/summary/tigr2ess");
		String args = 
				"-p "+targetDir
				+ " --word"
				+ " --dictionary country drugs --junk "
				+ " --species binomial"
				+ " --output table"
			;
		new AMISummaryTool().runCommands(args);
	}
}

gives:

[SNIPPED]

Specific values (AMISummaryTool)
================================
dictionaryList       [country, drugs, --junk]    <<< incorrect option values
geneList             [human]
speciesList          [binomial]
word                 true

cProject: tigr2ess

[remkop]

Thanks for raising this.

Reproducing the issue

I created a small test to reproduce the problem:

@Test
public void testUnknownOptionInVariableArityValue() {
    class App {
        @Option(names = {"-x", "--xvalues"}, arity = "0..*")
        List<String> x;

        @Option(names = {"-y", "--yvalues"}, arity = "0..*")
        List<String> y;
    }
    CommandLine cmd = new CommandLine(new App());
    try {
        cmd.parseArgs("-x", "0", "1", "-unknown", "-y", "A", "B");
        fail("Expect exception");
    } catch (UnmatchedArgumentException ex) {
        assertEquals("Unknown option: -unknown", ex.getMessage());
    }
}

Analysis

The above test fails because the -unknown parameter is consumed as a value of the variadic -x option.

This only happens when picocli is parsing values for a variadic option (an option with arity = "0..*" or arity = "1..*"). In other contexts, unknown options are rejected by default.

The problem is in Interpreter::varargCanConsumeNextValue
and Interpreter::isOption: currently only "real" options and the -- end-of-options delimiter cause the parser to stop consuming arguments for a variadic option.

Unknown Option Handling

I need to think about what the best way is to resolve this, in a way that is consistent with current behaviour for unknown options and other configuration options related to unknown options.

Picocli's default behaviour, outside of variadic options, is to reject unknown options.

Existing configuration options for unknown options are:

• CommandLine.setUnmatchedArgumentsAllowed (false by default), if true, accept unmatched input, display a warning on the console, and make values available via CommandLine.getUnmatchedArguments.
• CommandLine.setUnmatchedOptionsArePositionalParams (false by default), if true, treat unknown options as positional parameters
• CommandLine.setStopAtUnmatched (false by default) if true, the unknown option and all following command line arguments are added to the unmatched arguments list, available via CommandLine.getUnmatchedArguments.

Resolution

Some ideas for dealing with this issue:

1. Do nothing. See "Workaround" below. Not great, but it's an option.
2. Add a new parser option to control how to handle unknown options while processing variadic-option parameters. Something like CommandLine.setUnmatchedOptionsAllowedAsVariadicOptionParameters(boolean). When true, the parser behaves like it does now. When false, the above rules apply: if unmatchedArgumentsAllowed is false, the input is rejected, otherwise the value is accepted and can be obtained with CommandLine.getUnmatchedArguments, etc.
3. Change the current behaviour to apply the above rules even when processing variadic-option parameters, and by default reject unknown options. This breaks existing applications but may still be preferable for consistency.

Adding a new parser option is probably the best solution. What I'm less sure about is what should be the default behaviour.

Rejecting by default might break existing applications that rely on the current behaviour. On the other hand, it would be more consistent with behaviour in other contexts. I need to think about this a bit more.

Feedback welcome.

Workaround

A workaround is to do validation in the application. One idea is to have an @Option-annotated setter method instead of an annotated field:

class App {
    List<String> x;
    List<String> y;

    @Spec
    CommandSpec spec;

    @Option(names = {"-x", "--xvalues"}, arity = "0..*")
    void setX(List<String> newValues) {
        validate(newValues);
        x = newValues;
    }

    @Option(names = {"-y", "--yvalues"}, arity = "0..*")
    void setY(List<String> newValues) {
        validate(newValues);
        y = newValues;
    }

    void validate(List<String> newValues) {
        for (String value : newValues) {
            if (value.startsWith("-") || value.startsWith("--")) {
                throw new UnmatchedArgumentException(spec.commandLine(), "ERROR: Unknown option: " + value);
            }
        }
    }
}

[remkop]

CORRECTION: this is not limited to variadic options (options with variable arity like arity = "*").

Any option with a String option parameter can accept a value that "looks like" an option.
For example, the following test passes in current picocli (v3.9.5)...

@Test
public void testUnknownOptionAsOptionValue() {
    class App {
        @Option(names = {"-x", "--xvalue"})
        String x;

        @Option(names = {"-y", "--yvalues"}, arity = "1")
        List<String> y;
    }
    App app = new App();
    CommandLine cmd = new CommandLine(app);
    cmd.parseArgs("-x", "-unknown");
    assertEquals("-unknown", app.x);

    cmd.parseArgs("-y", "-unknown");
    assertEquals(Arrays.asList("-unknown"), app.y);
}

So, perhaps a better name for our new parser option may be unmatchedOptionsAllowedAsOptionParameters or some other name for allowing or disallowing option parameters that resemble options.

[petermr]

Thanks,
I like the Workaround solution as it keeps the validation code close to the definition of the Option.

For interest and reducing confusion, what do other CLIs do? Is leading -- usually a terminator of current Option values?

[remkop]

For unix CLI utilities, the POSIX standard specifies that when -- is specified as a separate argument, all following arguments must be interpreted as positional parameters. Picocli supports this out of the box.

For example:

mycommand -a -b -c -- -d -e

In the above, -d and -e are interpreted as positional parameters.

The -- end-of-options delimiter is not related to the -- prefix for GNU-style long options. It works the same way for long options:

mycommand --long-option1 -- --long-option2

In the above, --long-option2 is interpreted as a positional parameter.

[remkop]

I have started working on this.
The plan is to introduce a parser configuration unmatchedOptionsAllowedAsOptionParameters. The default is true for backwards compatibility. When this is set to false, unknown options are no longer accepted as option values, and instead result in an error message "Unknown option '-unknown'".

[remkop]

Fixed in master.
This will be available in the upcoming picocli 4.4 release.