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

Undefined short options are misinterpreted as positional parameter #149

Closed
giaco777 opened this issue Jul 17, 2017 · 5 comments
Closed

Undefined short options are misinterpreted as positional parameter #149

giaco777 opened this issue Jul 17, 2017 · 5 comments
Milestone

Comments

@giaco777
Copy link

I use picocli and entered an undefined short option like e.g. '-x' on the command line as the first argument
in the list like so:

-x -v -g abc def

This short option is not defined in the @Options/@parameter definition. When running, it confused the interpreter such, that none of the following command line arguments is parsed correctly anymore. No UnmatchedArgumentException is thrown!

I guess, I found the reason. In line 1330 of CommandLine.java there is a check of the length of the argument:

else if (arg.length() > 2 && arg.startsWith("-")) {

I my case the arg length is 2 and therefore it is not recognized as a short option. As a consequence the positional parameter processing is entered.

I changed the line such:

else if (arg.length() >= 2 && arg.startsWith("-")) {

and now it seems to work properly

@remkop
Copy link
Owner

remkop commented Jul 18, 2017

Thanks for reporting this!

The current behavior is by design, but I am open to improvements.

My thinking was that when an argument is encountered which cannot be recognized as an option, to interpret that argument as a positional parameter. Currently picocli follows POSIX Guideline 9 that says all options should come before positional parameters. This means that once -x is interpreted as a positional parameter, all remaining arguments are also interpreted as positional parameters.

First improvement:
I am planning to support mixing options with positional parameters. After this change, only -x will be interpreted as a positional parameter, -v and -g (when defined with @Option) will be recognized as options, and "abc" and "def" would be recognized as additional positional parameters.

Further improvements:
If I understand correctly you want picocli to throw an UnmatchedArgumentException if an argument starting with a hyphen (-) cannot be mapped to any of the @Option fields.

I don't want to hard-code this. Currently picocli accepts any value as a positional parameter, which I think is a good thing. (It does perform type conversion, but after that there is no validation.) An additional thing to bear in mind is that options can have any prefix, they don't need to start with a hypen. Picocli also supports options like /H /F etc., or even options without any prefix character.

A generic way to deal with the above requirements would be to allow some customizable validation on positional parameters. Still thinking about how to implement this...

Should this validation really be done in picocli though? The application could also implement this easily with something like: if (command.positionalParams[0].startsWith("-")) { throw new UnmatchedArgumentException(command.positionalParams[0]); }.
Picocli would need to offer a significanty shorter alternative or there would not be much value...

@giaco777
Copy link
Author

giaco777 commented Jul 19, 2017 via email

@remkop
Copy link
Owner

remkop commented Aug 13, 2017

To follow up on this, I believe #158 will improve the situation.

#158 will add internal logging. As part of that work, I'm thinking to emit a WARN-level message (printed to System.err by default) when a command line argument starting with "-" cannot be assigned to any @Option and is assigned to a @Parameters field.

@remkop remkop added this to the 1.0.0 milestone Aug 18, 2017
@remkop
Copy link
Owner

remkop commented Aug 23, 2017

Reading your comment again, I agree with what you are saying:

Yes, this was my understanding of this exception. Why should a single character option like -x be interpreted as a positional parameter, whereas a multi-character option like -xyz not? Remember the query for the length of the option: 'else if (arg.length() > 2 && arg.startsWith("-")) {processClusteredShortOptions(required, initialized, arg, args);}'. To me this seems inconsistent.

You are correct, unknown single-char options are treated as positional params, while unknown multi-char options result in an UnmatchedArgumentException. This is inconsistent.

For background, processClusteredShortOptions is trying to parse a cluster of one-character options, for example like jar -xvf file.jar. My intention was to only throw an UnmatchedArgException when such a cluster was partially, but not fully, processed, like for example jar -xvHHHH file (where -x and -v are valid options but -H is an unknown option). A multi-character option that could not be parsed as an option at all (not even partially) should have been treated as a positional parameter. This is a bug.

I originally thought that it would be sufficient to emit a WARN-level message "Unknown option -x", but still process that argument and the remainder as positional parameters, but I can see how this may be unsatisfactory.

I am now considering introducing a "strict mode" where arguments that "look like" an option but don't match any of the defined options would cause an UnmatchedArgumentException. In your example, -x looks like an option because it starts with a '-'.

What I haven't figured out yet is how to do this while at the same time allowing any option prefix, because picocli also allows options starting with '/' or any character (there are no restrictions on the option prefix).

@remkop
Copy link
Owner

remkop commented Aug 25, 2017

Thinking about this more, I now think the strict mode should be the default. Arguments that "resemble" an option but cannot be matched with the defined options should cause an UnmatchedArgumentException. Arguments that do not resemble an option should be added to the positional arguments.

"Resemblance" can be defined as shared prefix: if an argument has one or more prefix characters in common with 9 out of 10 defined options, we decide it resembles an option and it is treated as an unmatched argument.

We also consider the length of the shared prefix: so if known options are /a, /b, /c and --ccc, then argument --cc is still considered to resemble an option, since it has 4 matching prefix chars out of 4 option names.

remkop added a commit that referenced this issue Aug 25, 2017
…ble options but are not, instead of treating like them positional parameters
@remkop remkop closed this as completed in e1817d2 Aug 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants