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

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

Closed
hq6 opened this issue Apr 7, 2024 · 10 comments
Labels
theme: parser An issue or change related to the parser type: bug 🐛
Milestone

Comments

@hq6
Copy link

hq6 commented Apr 7, 2024

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
Copy link
Owner

remkop commented Apr 7, 2024

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
Copy link
Author

hq6 commented Apr 7, 2024

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
Copy link
Owner

remkop commented Apr 7, 2024

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

@hq6
Copy link
Author

hq6 commented Apr 7, 2024

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
Copy link
Owner

remkop commented Apr 7, 2024

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
Copy link
Owner

remkop commented Apr 8, 2024

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
Copy link
Author

hq6 commented Apr 8, 2024

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
Copy link
Owner

remkop commented Apr 8, 2024

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 remkop added this to the 4.7.6 milestone Apr 9, 2024
@remkop remkop added type: bug 🐛 theme: parser An issue or change related to the parser labels Apr 9, 2024
remkop added a commit that referenced this issue Apr 9, 2024
@remkop
Copy link
Owner

remkop commented Apr 9, 2024

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

@remkop remkop closed this as completed Apr 9, 2024
@hq6
Copy link
Author

hq6 commented Apr 11, 2024

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?

hq6 added a commit to square/perf that referenced this issue May 3, 2024
This commit adds a source snapshot of picocli which fixes the following
issue, which is required for Perf's use case.

remkop/picocli#2232

After the next release of Picocli (time TBD), we can remove this source
snapshot and include picocli using gradle.
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: bug 🐛
Projects
None yet
Development

No branches or pull requests

2 participants