Open marinier opened 2 years ago
One idea is to construct and return a new CommandLine
instance from getCommand
instead of caching the CommandLine
instance.
Another idea is to define the array option such that it has a default, but you'd have to try to see if that helps:
//QMemoryCommand.QMemoryC
@Option(names={"-s", "--set"}, arity="2", description="Path and item to set in qmemory (2 parameters required)")
String[] setPathAndValue = new String[];
Sorry, my time is extremely limited at the moment... If you find the cause, and can provide a PR with a test that demonstrates the issue and a fix, I can review and merge it.
Unfortunately, constructing a new CommandLine
won't work for me because of performance (see our discussion from several years ago here: https://stackoverflow.com/questions/59415247/how-to-improve-command-performance-at-runtime).
I tried setting a default value for all the fields, but this did not fix the problem.
I haven't yet figured out the actual problem, but I was able to update the test to further narrow the possibilities -- it is only necessary to do the second autocomplete to cause the problem. In stepping through picocli.AutoComplete.complete()
it appears that during the process of clearing the fields, the initial value for the setPathAndValue
field gets set to the previous call's arguments. The callstack at that point looks like this:
CommandLine$Model$OptionSpec(CommandLine$Model$ArgSpec).initialValue() line: 8878
CommandLine$Model$OptionSpec(CommandLine$Model$ArgSpec).applyInitialValue(CommandLine$Tracer) line: 8668
CommandLine$Interpreter.clear(CommandLine$Model$ArgSpec) line: 13129
CommandLine$Interpreter.clear() line: 13116
CommandLine$Interpreter.parse(List<CommandLine>, Stack<String>, String[], List<Object>, Collection<ArgSpec>, Set<ArgSpec>) line: 13156
CommandLine$Interpreter.parse(List<CommandLine>, Stack<String>, String[], List<Object>, Collection<ArgSpec>) line: 13146
CommandLine$Interpreter.parse(String...) line: 13047
CommandLine.parseArgs(String...) line: 1478
AutoComplete.complete(CommandSpec, String[], int, int, int, List<CharSequence>) line: 849
The line annotatedElement.getter().get()
is returning the value from the previous call to this command instead of the actual initial value. I'm not sure where annotatedElement
is coming from or how its value got changed.
Unfortunately I'm also very busy, so this is as far as I can get for this weekend. Perhaps this gives you an idea I can look into next weekend?
I updated to picocli 4.7.0. The issue did not magically go away, but it changed a little (now the problem occurs when
I understand part of the issue a bit better. Internally, I have my classes that are annotated with @Command
, and these are cached (there is only ever one instance of each command, and they get reused).
The problem appears to be that the CommandLine$Model$OptionSpec
objects used by AutoComplete
for each option are different than the one that is used when executing the commands, but both wrap the same underlying @Command
object for a given command. AutoComplete
correctly caches the initialValue
for an option (in CommandLine$Model$OptionSpec.initialValue()
), and then the underlying command object is modified based on the options that were passed in. But this CommandLine$Model$OptionSpec
is lost and we end up with a new one when the execution happens, so the initial value gets cached again in the new instance of CommandLine$Model$OptionSpec
, which does not have a cached value yet. So during initialValue()
it gets the current value of the field, seemingly assuming that the underlying command object is "fresh" -- but it's the same object that was modified during autocompletion, so the value it gets is the values from the options that were passed in as part of autocomplete, and that gets cached as the initial value. And that CommandLine$Model$OptionSpec
is reused for all future executions of this command, and thus everything has the wrong initial value for the fields that were modified during the autocomplete.
I was able to seemingly fix this on my end by doing two things:
CommandLine
object so it gets reused, but for autocompletion I was creating a new CommandLine
object each time. Now I use the cached one instead (that is, I get the CommandSpec
from the cached CommandLine
, which is what AutoComplete.complete
needs -- it creates a new CommandLine
from that internally).CommandSpec
each time via:
// we need to get a "fresh" command spec each time to avoid accidentally reusing one that may already be in use
CommandSpec commandSpec = CommandSpec.forAnnotatedObject(commandLine.getCommandSpec().userObject());
I removed that, and combined with reusing the CommandLine
object, it appears to work now. The comment above implies that the person who wrote that thought it was needed for what sounds like a threading issue, but since that was originally written I fixed a bunch of threading issues and I'm pretty sure this is no longer needed.
So I was able to make it work in my application. It's still not clear to me if this means there's no problem in AutoComplete
or if there is in fact a problem and I'm just working around it. At the very least, it was possible to get things into a bad state without it being obvious why.
Thanks for looking into this! I’ll try to take a closer look when I’m back from traveling.
On Nov 5, 2022, at 16:45, marinier @.***> wrote:
I updated to picocli 4.7.0. The issue did not magically go away, but it changed a little (now the problem occurs when I understand part of the issue a bit better. Internally, I have my classes that are annotated with @Command, and these are cached (there is only ever one instance of each command, and they get reused).
The problem appears to be that the CommandLine$Model$OptionSpec objects used by AutoComplete for each option are different than the one that is used when executing the commands, but both wrap the same underlying @Command object for a given command. AutoComplete correctly caches the initialValue for an option (in CommandLine$Model$OptionSpec.initialValue()), and then the underlying command object is modified based on the options that were passed in. But this CommandLine$Model$OptionSpec is lost and we end up with a new one when the execution happens, so the initial value gets cached again in the new instance of CommandLine$Model$OptionSpec, which does not have a cached value yet. So during initialValue() it gets the current value of the field, seemingly assuming that the underlying command object is "fresh" -- but it's the same object that was modified during autocompletion, so the value it gets is the values from the options that were passed in as part of autocomplete, and that gets cached as the initial value. And that CommandLine$Model$OptionSpec is reused for all future executions of this command, and thus everything has the wrong initial value for the fields that were modified during the autocomplete.
I was able to seemingly fix this on my end by doing two things:
For command executions I'm already caching the CommandLine object so it gets reused, but for autocompletion I was creating a new CommandLine object each time. Now I use the cached one instead (that is, I get the CommandSpec from the cached CommandLine, which is what AutoComplete.complete needs -- it creates a new CommandLine from that internally). As part of autocompletion I was creating a fresh CommandSpec each time via: // we need to get a "fresh" command spec each time to avoid accidentally reusing one that may already be in use CommandSpec commandSpec = CommandSpec.forAnnotatedObject(commandLine.getCommandSpec().userObject()); I removed that, and combined with reusing the CommandLine object, it appears to work now. The comment above implies that the person who wrote that thought it was needed for what sounds like a threading issue, but since that was originally written I fixed a bunch of threading issues and I'm pretty sure this is no longer needed.
So I was able to make it work in my application. It's still not clear to me if this means there's no problem in AutoComplete or if there is in fact a problem and I'm just working around it. At the very least, it was possible to get things into a bad state without it being obvious why.
— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you commented.
My application, jsoar, includes a REPL, and all the commands are implemented using picocli (I love picocli!). As part of this, we use picocli to recommend command completions as users type. It appears that sometimes a command's fields are not cleared between executions, so that if the same command is executed twice with different arguments, the wrong thing happens (e.g., it may execute as if the command were given the first set of arguments instead of the second). This appears to have something to do with autocompletion, because if I remove the call to
picocli.AutoComplete.complete
then the commands execte as expected (although then the completion doesn't work of course).I have created a junit test in my project that reproduces the issue:
https://github.com/soartech/jsoar/blob/picocli-field-not-reset/jsoar-core/src/test/java/org/jsoar/kernel/commands/QMemoryCommandTest.java
Note this is on the
picocli-field-not-reset
branch of the repo. There are comments in the test explaining what is expected to happen.I ran the test with
-Dpicocli.trace=DEBUG
. I believe the relevant output is below (does not include all of the initial command registration, which is a LOT of output as there are many commands).