negatable=true option in an ArgGroup tries to add negated option twice

[DevSnobo]

The following example uses a Mixin to add an ArgGroup which contains two boolean flags.

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

@Command(name = "Demo CLI Tool", description = "Short demo to showcase problem negatable option",
    mixinStandardHelpOptions = true)
public class Application implements Runnable {
    @Mixin
    private WrapperClass wrapper;

    @Override
    public void run() {
        System.out.println(wrapper.getCustom().flag1);
        System.out.println(wrapper.getCustom().flag2);
    }

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

@Command //I've seen this annotation on the Mixin in issue #2309, but doesn't change the error
class WrapperClass {
    @ArgGroup(exclusive = false)
    MyArgGroup custom = new MyArgGroup();

    public MyArgGroup getCustom() {
        return custom;
    }

    static class MyArgGroup {
        @Option(names = "--flag1", description = "first cool flag", negatable = false, defaultValue = "true") boolean flag1;
        @Option(names = "--flag2", description = "second cool flag", negatable = true, defaultValue = "true") boolean flag2;
    }
}

Setting "negatable = true" on them breaks compilation with the following:

DuplicateOptionAnnotationsException

(default-compile) on project net.snobo.democli: Compilation failure
FATAL ERROR: picocli.CommandLine$DuplicateOptionAnnotationsException: Option name '--no-flag2' is used by both field boolean net.snobo.demo.WrapperClass.MyArgGroup.flag2 and field boolean net.snobo.demo.WrapperClass.MyArgGroup.flag2
        at picocli.CommandLine$DuplicateOptionAnnotationsException.create(CommandLine.java:18698)
        at picocli.CommandLine$DuplicateOptionAnnotationsException.access$2900(CommandLine.java:18692)
        at picocli.CommandLine$Model$CommandSpec.addOptionNegative(CommandLine.java:6828)
        at picocli.CommandLine$Model$CommandSpec.addOption(CommandLine.java:6797)
        at picocli.CommandLine$Model$CommandSpec.addMixin(CommandLine.java:7036)
        at picocli.CommandLine$Model$CommandSpec.addMixin(CommandLine.java:7007)
        at picocli.codegen.annotation.processing.AbstractCommandSpecProcessor$Context.connectModel(AbstractCommandSpecProcessor.java:938)
        at picocli.codegen.annotation.processing.AbstractCommandSpecProcessor$Context.access$000(AbstractCommandSpecProcessor.java:829)
        at picocli.codegen.annotation.processing.AbstractCommandSpecProcessor.tryProcess(AbstractCommandSpecProcessor.java:209)
        at picocli.codegen.annotation.processing.AbstractCommandSpecProcessor.process(AbstractCommandSpecProcessor.java:168)
        at jdk.compiler/com.sun.tools.javac.processing.JavacProcessingEnvironment.callProcessor(JavacProcessingEnvironment.java:1021)
        at jdk.compiler/com.sun.tools.javac.processing.JavacProcessingEnvironment.discoverAndRunProcs(JavacProcessingEnvironment.java:937)
        at jdk.compiler/com.sun.tools.javac.processing.JavacProcessingEnvironment$Round.run(JavacProcessingEnvironment.java:1265)
        at jdk.compiler/com.sun.tools.javac.processing.JavacProcessingEnvironment.doProcessing(JavacProcessingEnvironment.java:1380)
        at jdk.compiler/com.sun.tools.javac.main.JavaCompiler.processAnnotations(JavaCompiler.java:1272)
        at jdk.compiler/com.sun.tools.javac.main.JavaCompiler.compile(JavaCompiler.java:946)
        at jdk.compiler/com.sun.tools.javac.api.JavacTaskImpl.lambda$doCall$0(JavacTaskImpl.java:104)
        at jdk.compiler/com.sun.tools.javac.api.JavacTaskImpl.invocationHelper(JavacTaskImpl.java:152)
        at jdk.compiler/com.sun.tools.javac.api.JavacTaskImpl.doCall(JavacTaskImpl.java:100)
        at jdk.compiler/com.sun.tools.javac.api.JavacTaskImpl.call(JavacTaskImpl.java:94)
        at org.codehaus.plexus.compiler.javac.JavaxToolsCompiler.compileInProcess(JavaxToolsCompiler.java:126)
        at org.codehaus.plexus.compiler.javac.JavacCompiler.performCompile(JavacCompiler.java:174)
        at org.apache.maven.plugin.compiler.AbstractCompilerMojo.execute(AbstractCompilerMojo.java:1134)
        at org.apache.maven.plugin.compiler.CompilerMojo.execute(CompilerMojo.java:187)
        at org.apache.maven.plugin.DefaultBuildPluginManager.executeMojo(DefaultBuildPluginManager.java:137)
        at org.apache.maven.lifecycle.internal.MojoExecutor.doExecute2(MojoExecutor.java:370)
        at org.apache.maven.lifecycle.internal.MojoExecutor.doExecute(MojoExecutor.java:351)
        at org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:215)
        at org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:171)
        at org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:163)
        at org.apache.maven.lifecycle.internal.LifecycleModuleBuilder.buildProject(LifecycleModuleBuilder.java:117)
        at org.apache.maven.lifecycle.internal.LifecycleModuleBuilder.buildProject(LifecycleModuleBuilder.java:81)
        at org.apache.maven.lifecycle.internal.builder.singlethreaded.SingleThreadedBuilder.build(SingleThreadedBuilder.java:56)
        at org.apache.maven.lifecycle.internal.LifecycleStarter.execute(LifecycleStarter.java:128)
        at org.apache.maven.DefaultMaven.doExecute(DefaultMaven.java:298)
        at org.apache.maven.DefaultMaven.doExecute(DefaultMaven.java:192)
        at org.apache.maven.DefaultMaven.execute(DefaultMaven.java:105)
        at org.apache.maven.cli.MavenCli.execute(MavenCli.java:960)
        at org.apache.maven.cli.MavenCli.doMain(MavenCli.java:293)
        at org.apache.maven.cli.MavenCli.main(MavenCli.java:196)
        at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
        at java.base/java.lang.reflect.Method.invoke(Method.java:580)
        at org.codehaus.plexus.classworlds.launcher.Launcher.launchEnhanced(Launcher.java:282)
        at org.codehaus.plexus.classworlds.launcher.Launcher.launch(Launcher.java:225)
        at org.codehaus.plexus.classworlds.launcher.Launcher.mainWithExitCode(Launcher.java:406)
        at org.codehaus.plexus.classworlds.launcher.Launcher.main(Launcher.java:347)

I've initially tried to use this in a subcommand to group lots of arguments in my application, but condensed it down to this minimal example.

JDK: Azul Zulu 21.0.4
Maven: 3.8.7
OS: Windows

If I can provide any more useful info, please let me know!

[remkop]

There are some issues in the annotation processor, but the cause of the problem has to do with the hashCode of OptionSpec changing when it is added to a CommandSpec. This causes internal bookkeeping to fail, and options are incorrectly added twice.

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 1730625846117)
@@ -6944,9 +6944,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.");
@@ -6957,7 +6957,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()) {
@@ -7024,12 +7024,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);
                 }

and

Index: picocli-codegen/src/main/java/picocli/codegen/annotation/processing/AbstractCommandSpecProcessor.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/picocli-codegen/src/main/java/picocli/codegen/annotation/processing/AbstractCommandSpecProcessor.java b/picocli-codegen/src/main/java/picocli/codegen/annotation/processing/AbstractCommandSpecProcessor.java
--- a/picocli-codegen/src/main/java/picocli/codegen/annotation/processing/AbstractCommandSpecProcessor.java	
+++ b/picocli-codegen/src/main/java/picocli/codegen/annotation/processing/AbstractCommandSpecProcessor.java	(date 1730624284461)
@@ -516,15 +516,34 @@
             error(element, "Only methods or variables can be annotated with @ArgGroup, not %s", element);
         } else {
             builder.updateArgGroupAttributes(element.getAnnotation(ArgGroup.class));
-            context.argGroupElements.put(element, builder);
+            context.argGroupElementsByVar.put(element, builder);
+
+            DeclaredType declaredType = getDeclaredTypeForArgGroupVarOrMethod(element, this);
+            if (declaredType != null) {
+                context.argGroupElementsByType.put(declaredType, builder);
+                processEnclosedElements(context, roundEnv, declaredType.asElement().getEnclosedElements());
+            }
+        }
+    }
+
+    private static DeclaredType getDeclaredTypeForArgGroupVarOrMethod(Element element, AbstractCommandSpecProcessor proc) {
+        TypeMirror elementType = element.asType();
+
+        if (elementType.getKind() == TypeKind.EXECUTABLE) {
+            // for @ArgGroup-annotated methods, use the method return type
+            elementType = ((ExecutableType) elementType).getReturnType();
+        }
+        if (elementType.getKind() != TypeKind.DECLARED && elementType.getKind() != TypeKind.ARRAY) {
+            proc.error(element,
+                "The type of an @ArgGroup-annotated element '%s' must be a declared class, " +
+                    "a collection or an array, but was %s", element.getSimpleName(), elementType);
+            return null;
+        }
 
-            DeclaredType declaredType = (elementType.getKind() == TypeKind.ARRAY)
-                    ? (DeclaredType) ((ArrayType) elementType).getComponentType()
-                    : (DeclaredType) elementType;
-
-            TypeElement typeElement = (TypeElement) declaredType.asElement();
-            processEnclosedElements(context, roundEnv, typeElement.getEnclosedElements());
-        }
+        DeclaredType declaredType = (elementType.getKind() == TypeKind.ARRAY)
+            ? (DeclaredType) ((ArrayType) elementType).getComponentType()
+            : (DeclaredType) elementType;
+        return declaredType;
     }
 
     private void buildOptions(RoundEnvironment roundEnv, Context context) {
@@ -613,7 +632,12 @@
                 TypedMember typedMember = new TypedMember(variable, -1);
                 ArgGroupSpec.Builder builder = ArgGroupSpec.builder(typedMember);
                 builder.updateArgGroupAttributes(variable.getAnnotation(ArgGroup.class));
-                context.argGroupElements.put(variable, builder);
+                context.argGroupElementsByVar.put(variable, builder);
+
+                DeclaredType declaredType = getDeclaredTypeForArgGroupVarOrMethod(variable, this);
+                if (declaredType != null) {
+                    context.argGroupElementsByType.put(declaredType, builder);
+                }
 
             } else if (!isMixin) { // params without any annotation are also positional
                 position++;
@@ -832,7 +856,8 @@
         Map<TypeMirror, List<CommandSpec>> commandTypes = new LinkedHashMap<TypeMirror, List<CommandSpec>>();
         Map<Element, OptionSpec.Builder> options = new LinkedHashMap<Element, OptionSpec.Builder>();
         Map<Element, PositionalParamSpec.Builder> parameters = new LinkedHashMap<Element, PositionalParamSpec.Builder>();
-        Map<Element, ArgGroupSpec.Builder> argGroupElements = new LinkedHashMap<Element, ArgGroupSpec.Builder>();
+        Map<Element, ArgGroupSpec.Builder> argGroupElementsByVar = new LinkedHashMap<Element, ArgGroupSpec.Builder>();
+        Map<DeclaredType, ArgGroupSpec.Builder> argGroupElementsByType = new LinkedHashMap<DeclaredType, ArgGroupSpec.Builder>();
         Map<CommandSpec, Set<MixinInfo>> mixinInfoMap = new IdentityHashMap<CommandSpec, Set<MixinInfo>>();
         Map<Element, IAnnotatedElement> parentCommandElements = new LinkedHashMap<Element, IAnnotatedElement>();
         Map<Element, IAnnotatedElement> specElements = new LinkedHashMap<Element, IAnnotatedElement>();
@@ -854,7 +879,8 @@
             }
 
             for (Map.Entry<Element, OptionSpec.Builder> option : options.entrySet()) {
-                ArgGroupSpec.Builder group = argGroupElements.get(option.getKey().getEnclosingElement());
+                TypeMirror typeMirror = option.getKey().getEnclosingElement().asType();
+                ArgGroupSpec.Builder group = argGroupElementsByType.get(typeMirror);
                 if (group != null) {
                     logger.fine("Building OptionSpec for " + option + " in arg group " + group);
                     group.addArg(option.getValue().build());
@@ -865,7 +891,8 @@
                 }
             }
             for (Map.Entry<Element, PositionalParamSpec.Builder> parameter : parameters.entrySet()) {
-                ArgGroupSpec.Builder group = argGroupElements.get(parameter.getKey().getEnclosingElement());
+                TypeMirror typeMirror = parameter.getKey().getEnclosingElement().asType();
+                ArgGroupSpec.Builder group = argGroupElementsByType.get(typeMirror);
                 if (group != null) {
                     logger.fine("Building PositionalParamSpec for " + parameter + " in arg group " + group);
                     group.addArg(parameter.getValue().build());
@@ -952,25 +979,20 @@
             Map<Element, TypeElement> argGroupElementsToType = new LinkedHashMap<Element, TypeElement>();
             Map<TypeElement, TypeElement> groupTypeToParentGroupType = new LinkedHashMap<TypeElement, TypeElement>();
             Map<TypeElement, ArgGroupSpec.Builder> argGroupsByType = new LinkedHashMap<TypeElement, ArgGroupSpec.Builder>();
-            for (Map.Entry<Element, ArgGroupSpec.Builder> entry : argGroupElements.entrySet()) {
+            for (Map.Entry<Element, ArgGroupSpec.Builder> entry : argGroupElementsByVar.entrySet()) {
                 Element argGroupElement = entry.getKey(); // field, method or parameter
                 ArgGroupSpec.Builder builder = entry.getValue();
                 //logger.severe(argGroupElement.toString());
                 //proc.processingEnv.getMessager().printMessage(Diagnostic.Kind.NOTE, "ArgGroup", argGroupElement);
 
-                Types typeUtils = proc.processingEnv.getTypeUtils();
-
                 // get the type or return type of the @ArgGroup-annotated field, method or parameter
-                TypeMirror typeMirror = argGroupElement.asType();
-                if (typeMirror.getKind() == TypeKind.EXECUTABLE) {
-                    // for @ArgGroup-annotated methods, use the method return type
-                    typeMirror = ((ExecutableType) typeMirror).getReturnType();
-                }
-                if (typeMirror.getKind() != TypeKind.DECLARED && typeMirror.getKind() != TypeKind.ARRAY) {
-                    proc.error(entry.getKey(), "The type of an @ArgGroup-annotated element '%s' must be a declared class, a collection or an array, but was %s", argGroupElement.getSimpleName(), typeMirror);
+                DeclaredType declaredType = getDeclaredTypeForArgGroupVarOrMethod(argGroupElement, proc);
+                if (declaredType == null) {
                     return true;
                 }
-                CompileTimeTypeInfo typeInfo = new CompileTimeTypeInfo(typeMirror);
+
+                Types typeUtils = proc.processingEnv.getTypeUtils();
+                CompileTimeTypeInfo typeInfo = new CompileTimeTypeInfo(declaredType);
                 TypeElement typeElement = (TypeElement) typeUtils.asElement(typeInfo.auxTypeMirrors.get(0));
 
                 CommandSpec argsHolder = commands.get(typeElement);
@@ -994,11 +1016,11 @@
                 }
             }
 
-            Element[] lookup = new Element[argGroupElements.size()];
-            Graph graph = new Graph(argGroupElements.size());
+            Element[] lookup = new Element[argGroupElementsByVar.size()];
+            Graph graph = new Graph(argGroupElementsByVar.size());
             int i = 0;
             Map<TypeElement, Integer> typeToIndex = new LinkedHashMap<TypeElement, Integer>();
-            for (Map.Entry<Element, ArgGroupSpec.Builder> entry : argGroupElements.entrySet()) {
+            for (Map.Entry<Element, ArgGroupSpec.Builder> entry : argGroupElementsByVar.entrySet()) {
                 Element argGroupElement = entry.getKey(); // field, method or parameter
                 lookup[i] = argGroupElement;
                 typeToIndex.put(argGroupElementsToType.get(argGroupElement), i++);
@@ -1011,11 +1033,11 @@
                 }
             }
             Stack<Integer> sortedGroups = graph.topologicalSort();
-            logger.fine(argGroupElements.toString());
+            logger.fine("@ArgGroup-annotated variables/methods: " + argGroupElementsByVar.toString());
             while (!sortedGroups.isEmpty()) {
                 Element argGroupElement = lookup[sortedGroups.pop()];
-                ArgGroupSpec.Builder argGroupBuilder = argGroupElements.get(argGroupElement);
-                logger.log(Level.FINE, "args=%s, typeInfo=%s", new Object[]{argGroupBuilder.args(), argGroupBuilder.typeInfo()});
+                ArgGroupSpec.Builder argGroupBuilder = argGroupElementsByVar.get(argGroupElement);
+                logger.fine(String.format("ArgGroup args=%s, typeInfo=%s", argGroupBuilder.args(), argGroupBuilder.typeInfo()));
                 ArgGroupSpec group = argGroupBuilder.build();
 
                 CommandSpec commandSpec = getOrCreateCommandSpecForArg(argGroupElement, commands);
@@ -1027,7 +1049,7 @@
                 ArgGroupSpec.Builder parentGroup = argGroupsByType.get(parentGroupElement);
                 if (parentGroup != null) {
                     // there may be multiple commands/subcommands with this parent arg group
-                    for (Map.Entry<Element, ArgGroupSpec.Builder> entry : argGroupElements.entrySet()) {
+                    for (Map.Entry<Element, ArgGroupSpec.Builder> entry : argGroupElementsByVar.entrySet()) {
                         TypeMirror entryTypeMirror = entry.getKey().asType();
                         if (entryTypeMirror.getKind() == TypeKind.DECLARED || entryTypeMirror.getKind() == TypeKind.ARRAY) {
                             CompileTimeTypeInfo typeInfo = new CompileTimeTypeInfo(entryTypeMirror);

//picocli-codegen-tests-java9plus\src\test\java\picocli\annotation\processing\tests\Issue2344Test.java
package picocli.annotation.processing.tests;

import com.google.testing.compile.Compilation;
import com.google.testing.compile.JavaFileObjects;
import org.junit.Test;

import javax.annotation.processing.Processor;

import static com.google.testing.compile.CompilationSubject.assertThat;
import static com.google.testing.compile.Compiler.javac;

public class Issue2344Test
{
    //@Ignore("https://github.com/remkop/picocli/issues/2344")
    @Test
    public void testIssue2344() {
        Processor processor = new AnnotatedCommandSourceGeneratorProcessor();
        Compilation compilation =
                javac()
                        .withProcessors(processor)
                        .compile(JavaFileObjects.forResource(
                                "picocli/issue2344/Application.java"));

        assertThat(compilation).succeeded();
    }
}

//picocli-codegen-tests-java9plus\src\test\resources\picocli\issue2344\Application.java
package picocli.issue2344;

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

@Command(name = "Demo CLI Tool",
        description = "Short demo to showcase problem negatable option",
        mixinStandardHelpOptions = true)
public class Application implements Runnable {
    @Mixin
    private WrapperClass wrapper;

    @Override
    public void run() {
        System.out.println(wrapper.getCustom().flag1);
        System.out.println(wrapper.getCustom().flag2);
    }

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

@Command(name = "mixinwrapper") //I've seen this annotation on the Mixin in issue #2309, but doesn't change the error
class WrapperClass {
    @ArgGroup(exclusive = false)
    MyArgGroup custom = new MyArgGroup();

    public MyArgGroup getCustom() {
        return custom;
    }

    static class MyArgGroup {
        @Option(names = "--flag1", description = "first cool flag",
                negatable = false, defaultValue = "true")
        boolean flag1;

        @Option(names = "--flag2", description = "second cool flag",
                negatable = true, defaultValue = "true")
        boolean flag2;
    }
}