Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Options disallow values that match subcommand name #1125

Closed
remkop opened this issue Jul 7, 2020 · 19 comments
Closed

Options disallow values that match subcommand name #1125

remkop opened this issue Jul 7, 2020 · 19 comments
Labels
theme: parser An issue or change related to the parser type: API 🔌 type: enhancement ✨
Milestone

Comments

@remkop
Copy link
Owner

remkop commented Jul 7, 2020

The parser became stricter in picocli 4.4, because of #1055.

The following values became disallowed as option values:

  • option names
  • subcommand names
  • sibling command names (if parent command has subcommandsRepeatable)

(Implementation note: Interpreter::assertNoMissingMandatoryParameter > varargCanConsumeNextValue > isOption)

There are use cases where applications do want to allow such values as option parameters. (See petermr/ami3#48 for some discussion.)

Consider introducing a mechanism that allows this. Some considerations:

  • How can applications allow only some values, but not all option/subcommand names?
  • Some apps may want to allow such value only for some options, but not for all options.
  • Some apps may want to allow such values only if it is the first parameter, (or 2nd, 3rd, etc. ...)
  • Some apps may want to allow such values only if it was "attached" to the option with a = or : separator. (E.g., allow --option=--option but not --option --option)
  • Some apps may need to inspect the stack of remaining args to determine whether to allow such values
  • Some apps may need to inspect the state of other options/positional arguments in the same command to determine whether to allow such values
@alparslanavci
Copy link

We are facing the same issue. We would like to get JVM options from the user by using a -J option. But it fails if the user tries to use a JVM option starting with a char that is defined as an option. For instance;

<cmd> -v -> is a valid command with option -v
<cmd> -J -Xmx1024m -> is again a valid command since there are no options defined starting with -X
<cmd> -J -verbose:class -> fails since -v is a defined option in the command itself
<cmd> -J="-verbose:class" -> again fails even if setTrimQuotes(true)
<cmd> "-J=\"-verbose:class\"" -> this one works

The example in here also doesn't work when you run with java App -x="-y"

@remkop
Copy link
Owner Author

remkop commented Jul 23, 2020

When you say <cmd>, do you mean java -cp xxx MyMainClass [OPTIONS], or do you have a launcher script?

Very often, you can pass Java options to the launcher script by setting an environment variable like JAVA_OPTIONS=-J-Xmx1024m.

Otherwise, you may need to specify the -J option between java and the MyMainClass [OPTIONS].

Also, do you need a space between the -J and the -Xmx1024m?

@alparslanavci
Copy link

We are spawning new Java processes in our command line application, thus we need additional JVM options to be taken from the user.

It is like java MyMainClass -J <additionalJavaOptions>.

Also, do you need a space between the -J and the -Xmx1024m?

Yes, we normally expect to work with space.

The example in here also doesn't work when you run with java App -x="-y"

Would you please verify if the example in the documentation doesn't work? Or am I missing something?

@remkop
Copy link
Owner Author

remkop commented Jul 23, 2020

Ok. Away from my pc now. I’ll need some time for this.

@remkop
Copy link
Owner Author

remkop commented Jul 23, 2020

PS. Does the problem also happen with the previous version of picocli? (4.3.2)

@alparslanavci
Copy link

The example in the documentation works with 4.3.2. However, we still get the same exception with 4.3.2 since we are using a Collection field.

@alparslanavci
Copy link

class App {
    @CommandLine.Option(names = "-x")
    List<String> x;
    @CommandLine.Option(names = "-y") String y;

    public static void main(String... args) {
        App app = new App();
        new CommandLine(app).setTrimQuotes(true).parseArgs(args);
        System.out.printf("x='%s', y='%s'%n", app.x, app.y);
    }
}

This example gets the following output with java App -x="-y":

Exception in thread "main" picocli.CommandLine$MissingParameterException: Expected parameter for option '-x' but found '-y'

@remkop
Copy link
Owner Author

remkop commented Jul 24, 2020

@alparslanavci Is your -J option defined like this?

@Option(names = "-J")
List<String> javaOptions;

One idea is to do custom parameter processing for the -J option.

@Command(name = "myapp")
class MyApp {
    @Option(names = "-J", parameterConsumer = JavaOptionsConsumer.class)
    List<String> javaOptions = new ArrayList<String>();
}

class JavaOptionsConsumer implements IParameterConsumer {
    public void consumeParameters(Stack<String> args, ArgSpec argSpec, CommandSpec commandSpec) {
        if (args.isEmpty()) {
            throw new ParameterException(commandSpec.commandLine(), "Error: option '-J' requires a parameter");
        }
        String arg = args.pop();
        List<String> list = argSpec.getValue();
        list.add(arg);
    }
}

@alparslanavci
Copy link

@remkop thanks for the suggestion! Yes, we can use this one as a workaround until the issue is fixed with the new version.

Just want to confirm, we need to parse the args manually if it is provided as a list of parameters such as -verbose:class,verbose:gc, right? I guess the split property of @Option annotation doesn't work when we use this method?

@remkop
Copy link
Owner Author

remkop commented Jul 29, 2020

Yes that's correct; with custom parameter handling the trade off is that you gain complete freedom but you give up on functionality: all the work of unquoting and splitting parameter values becomes the responsibility of the application.

@bherw
Copy link
Contributor

bherw commented Nov 4, 2021

An option to disable the error, at least for the case where option values == option names would be greatly appreciated.

We're evaluating the use of picocli to convert a large old bespoke CLI app which uses option names without any sort of prefix. Sometimes the same value appears as both a value to one option and as a "flag" on its own. All enums accept case-insensitive values. Changing the interface is not an option.

For this example, the command java Main type simple simple and java Main simple type simple would both be accepted, with type=SIMPLE, and doSimpleVerification=true:

enum ItemType {
    SIMPLE,
    COMPLEX;
}

class Main {
    @Option(names="type") ItemType type;
    @Option(names="simple") boolean doSimpleVerification;
}

Right now we're working around this using custom parameter consumers for the affected commands, but this is not ideal.

@remkop
Copy link
Owner Author

remkop commented Nov 6, 2021

@bherw I understand your situation but it may not be easy to change the parser behavior, and there is a workaround.

My time to work on picocli has become extremely limited, so there are other items that I will probably work on first.

If this is important to you, you can consider contributing a pull request. Something that provides a parser configuration option to allow option values matching option names and subcommands. (Maybe separate flags for each of these.) A great pull request would include tests and documentation, to minimize the work I would need to do to get this feature in.

@rgoldberg
Copy link
Contributor

rgoldberg commented Jan 2, 2022

Disallowing any values for options does not make sense to me. Why can't a whitespace-separated option-value pair just use the next token as its value, and a conjoined option-value pair (using a separator like = or :) just use the text after the separator?

Is this behavior intentional, or is it just a consequence of not being able to get the parser to work correctly?

@remkop
Copy link
Owner Author

remkop commented Jan 3, 2022

Interesting point.
Currently, the parser does not distinguish between input like -x 123 and -x=123.
However, it is fair to say that with the = separator, the intention of the end user is clearly to use 123 as a parameter for -x, while with the space separator, this is less clear.

So perhaps we can improve the parser. (Hopefully while staying as backwards compatible as possible...) 😅

For this example:

class App {
    @Option(names = "-x") String x;
    @Option(names = "-y") String y;
}

Currently picocli (correctly, in my opinion) shows an error for input like -x -y=123 , saying Expected parameter for option '-x' but found '-y=123'.

However, input like -x=-y=123 currently gives the same error, and if we modify the parser such that -option= unconditionally assigns the token attached to the right of the = separator as the option parameter, we can avoid this error and allow this input.

I am still thinking about this; there may be edge cases that I haven't considered yet, and also there may be users who do not want this behaviour, so we need a parser configuration option to switch this off again.

remkop added a commit that referenced this issue Jan 3, 2022
@rgoldberg
Copy link
Contributor

rgoldberg commented Jan 4, 2022

Each option should have a count of expected values. count = 0 is a true/false flag option, like -version for java. count = 1 is like --module-path <path> or --module-path=<path> for java. I don't know if any options require more than 1 value, but they would be like --longoption <val1> <val2> or --longoption=<val1>,<val2>, where = is a configurable name/value separator and , is a configurable value/value separator.

For options using a whitespace option-name/value separator, the parser would just grab the subsequent command-line tokens as the values for the option. If there aren't enough values, it would output an error.

@remkop
Copy link
Owner Author

remkop commented Jan 4, 2022

Such a count exists, it is the arity of an option or positional parameter.

@rgoldberg
Copy link
Contributor

rgoldberg commented Jan 5, 2022

If arity is an exact count, the parser should capture arity commmand-line tokens regardless of their value.

If arity is a range, rather than an exact count, the parser should capture the minimum number of tokens regardless of values, then capture all subsequent tokens up to the maximum count, stopping early at a configurable terminator value. There needs to be some way to specify the same characters as the terminator as a value instead of as the terminator.

That should take care of allowing values to have any value.

Every command-line program I've ever used allows all values for option values. I've never seen any program disallow values that begin with -, or that are the same as an option or sub-command name, etc. So the above will make PicoCLI consistent with common usage patterns.

@remkop remkop added the theme: parser An issue or change related to the parser label Feb 12, 2022
@remkop
Copy link
Owner Author

remkop commented Feb 18, 2022

To clarify, this is the problem space as I see it:

match a subcommand match an option resemble an option
Should options consume args that... 1 2 3
Should positional params consume args that... 4 5 6
  1. Not allowed in picocli 4.6.3 (disallowed from 4.4.0)
  2. Not allowed in picocli 4.6.3 (disallowed from 4.4.0)
  3. Allowed by default, can be disallowed on a per-command basis with unmatchedOptionsAllowedAsOptionParameters=false
  4. Not allowed in picocli 4.6.3 - should this ever be allowed?
  5. Not allowed in picocli 4.6.3 - should this ever be allowed?
  6. Not allowed by default, can be allowed on a per-command basis with unmatchedOptionsArePositionalParams=true

I think we only need to consider cases 1 and 2. (If anyone thinks differently, please comment!)
One idea is to introduce parser configuration options (similar to case 3 and case 6 in the table above), something like this:

  • subcommandsAllowedAsOptionParameters - default is false
  • optionsAllowedAsOptionParameters - default is false

This would allow all options in a command to consume arguments that match subcommands or options, respectively.

Any other requirement (like, allow only some options in the command to consume subcommands/options) can be taken care of with custom parameter processing.

@remkop
Copy link
Owner Author

remkop commented Feb 19, 2022

@alparslanavci @bherw @rgoldberg
This has now been implemented and will be included in the 4.7.0 release.

From the release notes:

Enable Consuming Option Names or Subcommands

By default, options that take a parameter do not consume values that match a subcommand name or an option name.

This release introduces two parser configuration options to change this behaviour:

  • CommandLine::setAllowOptionsAsOptionParameters allows options to consume option names
  • CommandLine::setAllowSubcommandsAsOptionParameters allows options to consume subcommand names

When set to true, all options in the command (options that take a parameter) can consume values that match option names or subcommand names.

This means that any option will consume the maximum number of arguments possible for its arity.

USE WITH CAUTION!

If an option is defined as arity = "*", this option will consume all remaining command line arguments following this option (until the End-of-options delimiter) as parameters of this option.


Feedback welcome!

@remkop remkop closed this as completed Feb 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme: parser An issue or change related to the parser type: API 🔌 type: enhancement ✨
Projects
None yet
Development

No branches or pull requests

4 participants