NullPointerException for empty list

[garretwilson]

I have a CLI that takes a path as a parameter, e.g. foo do C:\temp. I just decided to allow multiple files/directories, so I changed the parameter to:

@Parameters(paramLabel = "<data>", description = "The file or base directory of the data. Multiple data sources are allowed.")
    @Nonnull List<Path> argDataPaths

I have a loop in the code that looks like this:

for(final Path dataPath : argDataPaths)

I tested it with no paths just to make sure I had things configured correctly.

foo do

But I got the following, along with a stack trace pointing the line of code above!

java.lang.NullPointerException: Cannot invoke "java.util.List.iterator()" because "argDataPaths" is null

OK, so I goofed and forgot to put an arity="1..*". But still, there should not have been a NullPointereException, because argDataPaths should never under any circumstances have been null. If the single parameter is a List<> and no CLI arguments are actually supplied, I expect to get an empty List<>! (I'm sure you're well aware of the huge distinction in semantics between "I gave you a list with nothing in it" and "I did not give you a list".)

After I put in my arity this won't affect me (for now), but it will affect me if I do allow arity 0..*. But just as great I cringe to think such an ugly bug could soil this awesome library. 😄

[remkop]

When the user does not specify an option or positional parameter, picocli will assign the default value.
If no default value is defined, picocli will assign the initial value (the value that the field had when its enclosing object was passed to the CommandLine constructor).

In your example, the argDataPaths field is declared with an initial value of null, so that is the value that picocli will assign to the field when the end user does not specify the option or positional parameter associated with that field.

This is by design.

To make sure that the argDataPaths field is not null when the end user does not specify the associated parameter, you can define the parameter like this:

@Parameters//...
List<Path> argDataPaths = new ArrayList<>(); // empty list as initial value

[garretwilson]

In your example, the argDataPaths field is declared with an initial value of null, …

I'm not seeing any initial value of null I declared. This argDataPaths parameter is a parameter of the do() method. Java does not allow me to assign any "initial value" to a method parameter.

If I pass two paths, I expect a List<Path> with two paths. If I pass one path, I expect a List<Path> containing a single path. If I pass no paths, I expect a List<Path> containing no paths. picocli should have consistent behavior whether I pass three paths or zero paths.

picocli should know that a collection such as List<> should have a default value other than null. Maybe this requires special-case code inside picocli. But that's the lesser of two evils. If picocli just blindly assigns null instead of an empty list, then my application requires special-case code:

if(argDataPaths!=null) {
  for(final Path dataPath : argDataPaths)

Since the whole purpose of picocli is to make the developer's life easier, don't you agree that it would be preferable to have special-case code inside picocli instead of the application requiring special-case code to work around picocli's deficiencies?

[remkop]

I see, yes, for @Command-annotated methods you cannot assign an initial value, and assigning a defaultValue would put that value into the list, which is also not what you want.

With the current version of picocli, the trade-off then becomes either checking for null values like in your previous comment, or converting the method to a @Command-annotated class instead. Neither is ideal.

This will be solved when picocli adds support for multi-value default values:
#879 (comment)

That would allow you to define that positional parameter like this:

@Parameters(defaultValues = {}) // empty collection as default values
List<Path> argDataPaths

[garretwilson]

I think the problem in the reasoning in what you're saying is that you're considering a collection of things to be the value the user is providing. But it's not. When a collection is indicated, it is a collection of values, implicit.

When I say foo do a b c, am I providing a list? No, I am providing three values, a, b, and c. We simply use List<> as a convenience to indicate that multiple values are allowed. If I were actually providing a list, I would say something like foo do [a, b, c]. In other words, the List<> is simply the means to return to the program the three values the user entered on the command line. It is impossible for the user to provide list = null, because the user is not providing a list. The list is the transport, between the CLI and the program.

Think about Java main(String[] args). Would Java ever pass args = null? "Well the user didn't provide an argument, so we think the array was null". No, that's absurd. The user isn't providing an array. The array is a transport to provide the arguments (which may be none, which means the array will be empty) to the Java program.

That would allow you to define that positional parameter like this:
@Parameters(defaultValues = {}) // empty collection as default values
List<Path> argDataPaths

This raises two questions. If I don't indicate defaultValues, then it itself must have a default value, right? Will the default value of defaultValue be {}? (Please don't tell me you're going to make it null.)

And here's the other thing: if the default values are no values at all, then it seems logical that there should be a way for the user to manually provide the same value as the default, even if no default is provided. So can the user do this?

foo do []

I don't think so. So you're providing a way to specify a default value that it is impossible for the user to provide. (That's like saying that the default value is "5", but there is no way for the user to explicitly indicate "5".) That's certainly not unheard of, but it's another design smell to indicate there's something inconsistent going on with the design.

null is bad in general, and mixing and matching it with "empty collection" semantics is making the framework intrude into the application logic, making the developer to create workarounds just for using the framework. I mean, if I threw away picocli and just parsed args, I wouldn't have this particular problem! I would have other problems, sure, and 90% of picocli is amazing. I'm just pointing out that here it is introducing an inconsistency that requires a workaround that even main(String[] args) doesn't require.

[garretwilson]

If I seem particularly adamant about this, it's because I've seen some students in my class not get this, and have an API that says List<Thing> getStuff(), and if it returns no things then they return null instead of returning an empty collection.

And I'm sure you've heard (if you've been through the SRE books) about the famous Google bug where they did just the opposite: they used a filter for decommissioning machines to essentially say "these are the machines to decommission". And empty list should mean "don't decommission any machines", but there was a bug that supposed it meant "decommission all machines". It's not exactly the same, but it stems from a similar twisting and mixing of the semantics of "an empty collection" and "no collection at all", just from the opposite direction.

[remkop]

That would allow you to define that positional parameter like this:
@Parameters(defaultValues = {}) // empty collection as default values
List<Path> argDataPaths

This raises two questions. If I don't indicate defaultValues, then it itself must have a default value, right? Will the default value of defaultValue be {}? (Please don't tell me you're going to make it null.)

For backwards compatibility, the behaviour will be unchanged from its current behaviour without defaultValues: that is, picocli will assign the initial value, like it does now. Method parameters cannot have an initial value, so in your use case you will get null. Once the defaultValues = {} mechanism is available you have a way to ensure you no longer get null values so you will be able to remove the if(argDataPaths!=null) logic.

And here's the other thing: if the default values are no values at all, then it seems logical that there should be a way for the user to manually provide the same value as the default, even if no default is provided. So can the user do this?

foo do []

I don't think so. So you're providing a way to specify a default value that it is impossible for the user to provide. (That's like saying that the default value is "5", but there is no way for the user to explicitly indicate "5".) That's certainly not unheard of, but it's another design smell to indicate there's something inconsistent going on with the design.

I am not sure that I followed all that, but the default value is determined by the application. When #879 is implemented, applications can choose between a null default value, an empty collection, or anything else. Applications may deliberately choose a default value that cannot be specified by the user, to detect whether the end user specified a value on the command line or not.

null is bad in general, and mixing and matching it with "empty collection" semantics is making the framework intrude into the application logic, making the developer to create workarounds just for using the framework. I mean, if I threw away picocli and just parsed args, I wouldn't have this particular problem! I would have other problems, sure, and 90% of picocli is amazing. I'm just pointing out that here it is introducing an inconsistency that requires a workaround that even main(String[] args) doesn't require.

From my point of view, I am trying to evolve the library and add useful features without breaking anybody's code.
Backwards compatibility outweighs esthetics.

[garretwilson]

This issue is coming up again; it's a typical scenario. It's an inconvenience to have to add boilerplate to every method that says:

List<String> theRealListAndNotNull = theListPicocliGaveMe !=null ? theListPicocliGaveMe : List.of();

[garretwilson]

To get around this so that I don't have to work with null in my code, I changed that to a Optional<List<String>>, but now I can't use split = ",", because I guess picocli doesn't recognize that Optional<List<String>> is simply a modern alternative to a nullable List<String>. This probably means the repeated options implementation should be updated to recognize Optional.