remkop / picocli

Picocli is a modern framework for building powerful, user-friendly, GraalVM-enabled command line apps with ease. It supports colors, autocompletion, subcommands, and more. In 1 source file so apps can include as source & avoid adding a dependency. Written in Java, usable from Groovy, Kotlin, Scala, etc.
https://picocli.info
Apache License 2.0
4.8k stars 414 forks source link

Picocli overwriting a value set in a constructor with no default value set and no option given on cli #2232

Closed hq6 closed 3 months ago

hq6 commented 3 months ago

Consider the following MWE:

import picocli.CommandLine;
import static picocli.CommandLine.*;
import java.util.*;
import java.io.*;

public class MWE {
  static class Tar {
    @Option(names = { "-f", "--file" }, paramLabel = "ARCHIVE", description = "the archive file")
    Optional<File> archive;

    public Tar() {
      archive = Optional.of(new File("helloworld"));
    }
  }

  public static void main(String[] args) throws Exception{
    Tar tar = new Tar();
    System.out.println(tar.archive);
    new CommandLine(tar).parseArgs(args);
    System.out.println(tar.archive);
  }
}

The picocli documentation claims that defaultValue has a default value of "__no_default_value__", which I am interpreting to imply that it will not change the value of any field if there is no flag given on the command line.

Thus, when I invoke this program with no arguments, I expect the following output:

Optional[helloworld]
Optional[helloworld]

However, the actual output I get is:

Optional[helloworld]
Optional.empty

It appears that the parseArguments call has set archive to Optional.empty despite the lack of arguments given on the command line and the lack of explicit setting for the default value of this option.

Two questions about this:

  1. Is this a bug or is this expected behavior?
  2. If it is expected behavior, is there a way to explicitly prevent this behavior and not touch values after construction when neither arguments nor default values are given?
remkop commented 3 months ago

Can you please run that again with picocli tracing switched on, and paste the output here? Thank you!

Either with system property -Dpicocli.trace=DEBUG or via the programmatic API :

// at the beginning of your main method 
CommandLine.tracer().setLevel(CommandLine.TraceLevel.DEBUG);
hq6 commented 3 months ago

Please see output below:

java -Dpicocli.trace=DEBUG MWE.java
Optional[helloworld]
[picocli DEBUG] Creating CommandSpec for MWE$Tar@6b53e23f with factory picocli.CommandLine$DefaultFactory
[picocli INFO] Picocli version: 4.7.5, JVM: 17.0.10 (Private Build OpenJDK 64-Bit Server VM 17.0.10+7-Ubuntu-120.04.1), OS: Linux 5.4.0-174-generic amd64
[picocli INFO] Parsing 0 command line args []
[picocli DEBUG] Parser configuration: optionsCaseInsensitive=false, subcommandsCaseInsensitive=false, abbreviatedOptionsAllowed=false, abbreviatedSubcommandsAllowed=false, allowOptionsAsOptionParameters=false, allowSubcommandsAsOptionParameters=false, aritySatisfiedByAttachedOptionParam=false, atFileCommentChar=#, caseInsensitiveEnumValuesAllowed=false, collectErrors=false, endOfOptionsDelimiter=--, expandAtFiles=true, limitSplit=false, overwrittenOptionsAllowed=false, posixClusteredShortOptionsAllowed=true, separator=null, splitQuotedStrings=false, stopAtPositional=false, stopAtUnmatched=false, toggleBooleanFlags=false, trimQuotes=false, unmatchedArgumentsAllowed=false, unmatchedOptionsAllowedAsOptionParameters=true, unmatchedOptionsArePositionalParams=false, useSimplifiedAtFiles=false
[picocli DEBUG] (ANSI is enabled by default: systemproperty[picocli.ansi]=null, isatty=true, TERM=screen, OSTYPE=null, isWindows=false, JansiConsoleInstalled=false, ANSICON=null, ConEmuANSI=null, NO_COLOR=null, CLICOLOR=null, CLICOLOR_FORCE=null)
[picocli DEBUG] Initializing command 'null' (user object: MWE$Tar@6b53e23f): 1 options, 0 positional parameters, 0 required, 0 groups, 0 subcommands.
[picocli DEBUG] Set initial value for field java.util.Optional<java.io.File> MWE$Tar.archive of type class java.util.Optional to Optional[helloworld].
[picocli DEBUG] Applying default values for command '<main class>'
[picocli DEBUG] Applying Optional.empty() to field java.util.Optional<java.io.File> MWE$Tar.archive on Tar@6b53e23f
Optional.empty
remkop commented 3 months ago

Thank you; I'll have to look with the debugger to see what's happening.

hq6 commented 3 months ago

I took a quick look at the code and found the following lines, which seem to always default Optional to empty, even if the defaultValue is null or ArgSpec.NULL_VALUE.equals(defaultValue).

                if (arg.typeInfo().isOptional()) {
                    if (tracer.isDebug()) {
                        tracer.debug("Applying Optional.empty() to %s on %s", arg, arg.scopeString());}
                    arg.setValue(getOptionalEmpty());
                    arg.valueIsDefaultValue = true;
                } else if (ArgSpec.UNSPECIFIED.equals(arg.originalDefaultValue)) {

If I'm interpreting this code correctly, this behavior looks intentional.

Is it necessary for some reason for Optional values to not respect the lack of default? If not, would you be open to changing this behavior?

remkop commented 3 months ago

For some reason, the defaultvalue comes up as null, which is why picocli ends up taking that code path. There seems to be a bug. Still investigating.

remkop commented 3 months ago

In the debugger, I see that the defaultvalue obtained from arg.defaultValue() for the archive field is null. I thought earlier that this might be a bug, but this is working correctly: picocli distinguishes between initial value and default value. Initial value is set before parsing starts, and the default value is only applied to options that did not have a value specified on the commandline.

For args that are not Optional<T>, the code is intended to leave the initial value in place. For args that are Optional<T>, the code sets the value to Optional.empty().

I was probably assuming that many applications would not specify an initial value, and was thinking that Optional.empty() would be more desirable than null (otherwise, what is the point of using Optional<T>....?

FYI, looking at the user manual, section 4.6 Optional does mention this behaviour:

If the option or positional parameter was not specified on the command line, picocli assigns the value Optional.empty() instead of null.

The current behaviour is following the documentation, but it is a bit inconsistent to have the initial value not work as the default value for Optional<T> args...

To be honest, I am not sure how to proceed.

hq6 commented 3 months ago

I think your assumption that Optional.empty is more desirable than null is reasonable, but the current code sets the value to Optional.empty() even when the initial value is not null.

Is it possible to set it Optional.empty() iff a default value is not specified and the initial value is null?

If you did that, then the common case of Optional defaulting to Optional.empty() when not given an initial value would still work as expected, but if it is given an initial value that is not null, then that value will be respected.

Then, you could update the docs to say the following

If the option or positional parameter was not specified on the command line, and was not given a non-null initial value, picocli assigns the value Optional.empty() instead of null.


Another idea is to only set it to Optional.empty() if the null value was not explicitly set (and was a Java reference default of null) but I'm not sure if Java allows you to detect whether a value has ever been set at runtime.

remkop commented 3 months ago

Is it possible to set it Optional.empty() iff a default value is not specified and the initial value is null?

Yes. Makes sense. Looking into it.

remkop commented 3 months ago

Fixed now in the main branch. Thank you for raising this!

hq6 commented 3 months ago

Thank you for the investigation and the quick fix!

Will this be in the next release? Also, do you have a planned time for the next release to the Maven repository?