Duplicate help output for ArgGroup from a Mixin

[s-falke]

The following example uses a @Mixin in order to import an @ArgGroup:

import picocli.CommandLine;
import picocli.CommandLine.ArgGroup;
import picocli.CommandLine.Command;
import picocli.CommandLine.Mixin;
import picocli.CommandLine.Option;

@Command
class MyMixin
{
    @ArgGroup(exclusive = true, multiplicity = "1")
    Exclusive exclusive;

    static class Exclusive {
        @Option(names = "-a", required = true, description = "Use A.") int a;
        @Option(names = "-b", required = true, description = "Use B.") int b;
        @Option(names = "-c", required = true, description = "Use C.") int c;
    }
}

@Command(mixinStandardHelpOptions = true, name = "exclusivedemo")
public class MutuallyExclusiveOptionsDemo {
    @Mixin
    MyMixin mixin;

    public static void main(String... args) {
        int exitCode = new CommandLine(new MutuallyExclusiveOptionsDemo()).execute(args);
        System.exit(exitCode);
    }
}

The help output generated using picocli 4.7.6 lists the options -a, -b, and -c twice:

Usage: exclusivedemo [-hV] (-a=<a> | -b=<b> | -c=<c>)
  -a=<a>          Use A.
  -a=<a>          Use A.
  -b=<b>          Use B.
  -b=<b>          Use B.
  -c=<c>          Use C.
  -c=<c>          Use C.
  -h, --help      Show this help message and exit.
  -V, --version   Print version information and exit.

In contrast, picocli 4.7.5 lists them only once:

Usage: exclusivedemo [-hV] (-a=<a> | -b=<b> | -c=<c>)
  -a=<a>          Use A.
  -b=<b>          Use B.
  -c=<c>          Use C.
  -h, --help      Show this help message and exit.
  -V, --version   Print version information and exit.

[remkop]

That certainly looks like a bug. Thank you for raising this!

[FreifeldRoyi]

Any updates on this one?

[jrivard]

Bump. This one hit me. For existing projects it's prob not a huge deal but while developing a new project I burned countless hours trying to figure out what I was doing wrong in my complex argument relationships...

Thanks for this awesome library!

[remkop]

Many thanks to @simschla for the PR https://github.com/remkop/picocli/pull/2377 for this issue! I simplified the test a little:

@Test
public void testIssue2309OptionsForArgGroupInMixinsAreNotDuplicatedInHelp() {
    String output = new CommandLine(new Issue2309MutuallyExclusiveOptionsDemo()).getUsageMessage(Help.Ansi.OFF);
    String expected = String.format(
        "Usage: exclusivedemo [-hV] (-a=<a> | -b=<b> | -c=<c>)%n" +
        "  -a=<a>          Use A.%n" +
        "  -b=<b>          Use B.%n" +
        "  -c=<c>          Use C.%n" +
        "  -h, --help      Show this help message and exit.%n" +
        "  -V, --version   Print version information and exit.%n");

    assertEquals(expected, output);
}

@Command
static class Issue2309MyMixin {
    @ArgGroup(exclusive = true, multiplicity = "1") Exclusive exclusive;

    class Exclusive {
        @Option(names = "-a", required = true, description = "Use A.") int a;
        @Option(names = "-b", required = true, description = "Use B.") int b;
        @Option(names = "-c", required = true, description = "Use C.") int c;
    }
}

@Command(mixinStandardHelpOptions = true, name = "exclusivedemo")
class Issue2309MutuallyExclusiveOptionsDemo {
    @CommandLine.Mixin Issue2309MyMixin mixin;
}

I have taken a different approach than in the PR.

With the current implementation of ArgSpec::equals, adding an option may cause OptionSpec hashCode to change. I should revise the implementation of ArgSpec::equals, but meanwhile, using Lists instead of Sets solves this particular issue.

NOTE TO SELF:

Index: src/main/java/picocli/CommandLine.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/main/java/picocli/CommandLine.java b/src/main/java/picocli/CommandLine.java
--- a/src/main/java/picocli/CommandLine.java	
+++ b/src/main/java/picocli/CommandLine.java	(date 1744350913664)
@@ -6948,9 +6948,9 @@
              * @throws InitializationException if the specified group or one of its {@linkplain ArgGroupSpec#parentGroup() ancestors} has already been added
              * @since 4.0 */
             public CommandSpec addArgGroup(ArgGroupSpec group) {
-                return addArgGroup(group, new HashSet<OptionSpec>(), new HashSet<PositionalParamSpec>());
+                return addArgGroup(group, new ArrayList<OptionSpec>(), new ArrayList<PositionalParamSpec>());
             }
-            private CommandSpec addArgGroup(ArgGroupSpec group, Set<OptionSpec> groupOptions, Set<PositionalParamSpec> groupPositionals) {
+            private CommandSpec addArgGroup(ArgGroupSpec group, List<OptionSpec> groupOptions, List<PositionalParamSpec> groupPositionals) {
                 Assert.notNull(group, "group");
                 if (group.parentGroup() != null) {
                     throw new InitializationException("Groups that are part of another group should not be added to a command. Add only the top-level group.");
@@ -6961,7 +6961,7 @@
                 return this;
             }
 
-            private void addGroupArgsToCommand(ArgGroupSpec group, Map<String, ArgGroupSpec> added, Set<OptionSpec> groupOptions, Set<PositionalParamSpec> groupPositionals) {
+            private void addGroupArgsToCommand(ArgGroupSpec group, Map<String, ArgGroupSpec> added, List<OptionSpec> groupOptions, List<PositionalParamSpec> groupPositionals) {
                 Map<String, OptionSpec> options = new HashMap<String, OptionSpec>();
                 for (ArgSpec arg : group.args()) {
                     if (arg.isOption()) {
@@ -7028,12 +7028,12 @@
                 for (Map.Entry<String, CommandLine> entry : mixin.subcommands().entrySet()) {
                     addSubcommand(entry.getKey(), entry.getValue());
                 }
-                Set<OptionSpec> options = new LinkedHashSet<OptionSpec>(mixin.options());
-                Set<PositionalParamSpec> positionals = new LinkedHashSet<PositionalParamSpec>(mixin.positionalParameters());
+                List<OptionSpec> options = new ArrayList<OptionSpec>(mixin.options());
+                List<PositionalParamSpec> positionals = new ArrayList<PositionalParamSpec>(mixin.positionalParameters());
                 for (ArgGroupSpec argGroupSpec : mixin.argGroups()) {
-                    Set<OptionSpec> groupOptions = new HashSet<OptionSpec>();
-                    Set<PositionalParamSpec> groupPositionals = new HashSet<PositionalParamSpec>();
-                    addArgGroup(argGroupSpec, groupOptions, groupPositionals);
+                    List<OptionSpec> groupOptions = new ArrayList<OptionSpec>();
+                    List<PositionalParamSpec> groupPositionals = new ArrayList<PositionalParamSpec>();
+                    addArgGroup(argGroupSpec, groupOptions, groupPositionals); // CAUTION: adding to spec may cause OptionSpec hashCode to change
                     options.removeAll(groupOptions);
                     positionals.removeAll(groupPositionals);
                 }

[simschla]

Nice, thanks for picking it up 😄