GROOVY-10811: final or abstract is implicit for an enum definition

src/main/java/org/apache/groovy/parser/antlr4/AstBuilder.java

@@ -3360,8 +3360,6 @@ public InnerClassNode visitAnonymousInnerClassDeclaration(final AnonymousInnerCl
         InnerClassNode anonymousInnerClass;
         if (ctx.t == 1) {
             anonymousInnerClass = new EnumConstantClassNode(outerClass, innerClassName, superClass.getPlainNodeReference());
-            // and remove the final modifier from superClass to allow the sub class
-            superClass.setModifiers(superClass.getModifiers() & ~Opcodes.ACC_FINAL);
         } else {
             anonymousInnerClass = new InnerClassNode(outerClass, innerClassName, 0, superClass);
         }

src/main/java/org/codehaus/groovy/antlr/EnumHelper.java

@@ -29,38 +29,30 @@
 import org.objectweb.asm.Opcodes;
 
 public class EnumHelper {
-    private static final int FS = Opcodes.ACC_FINAL | Opcodes.ACC_STATIC;
-    private static final int PUBLIC_FS = Opcodes.ACC_PUBLIC | FS; 
 
-    public static ClassNode makeEnumNode(String name, int modifiers, ClassNode[] interfaces, ClassNode outerClass) {
-        modifiers = modifiers | Opcodes.ACC_FINAL | Opcodes.ACC_ENUM;
+    public static ClassNode makeEnumNode(final String name, final int modifiers, final ClassNode[] interfaces, final ClassNode outerClass) {
         ClassNode enumClass;
-        if (outerClass==null) {
-            enumClass = new ClassNode(name,modifiers,null,interfaces,MixinNode.EMPTY_ARRAY);
+        if (outerClass == null) {
+            enumClass = new ClassNode(name, modifiers | Opcodes.ACC_ENUM, null, interfaces, MixinNode.EMPTY_ARRAY);
         } else {
-            name = outerClass.getName() + "$" + name;
-            modifiers |= Opcodes.ACC_STATIC;
-            enumClass = new InnerClassNode(outerClass,name,modifiers,null,interfaces,MixinNode.EMPTY_ARRAY);
+            enumClass = new InnerClassNode(outerClass, outerClass.getName() + "$" + name, modifiers | Opcodes.ACC_ENUM, null, interfaces, MixinNode.EMPTY_ARRAY);
         }
 
-        // set super class and generics info
-        // "enum X" -> class X extends Enum<X>
-        GenericsType gt = new GenericsType(enumClass);
-        ClassNode superClass = ClassHelper.makeWithoutCaching("java.lang.Enum");
-        superClass.setGenericsTypes(new GenericsType[]{gt});
+        // enum E extends java.lang.Enum<E>
+        ClassNode superClass = ClassHelper.Enum_Type.getPlainNodeReference();
+        superClass.setGenericsTypes(new GenericsType[]{new GenericsType(enumClass)});
         enumClass.setSuperClass(superClass);
-        superClass.setRedirect(ClassHelper.Enum_Type);
 
         return enumClass;
     }
 
-    public static FieldNode addEnumConstant(ClassNode enumClass, String name, Expression init) {
-        int modifiers = PUBLIC_FS | Opcodes.ACC_ENUM;
+    public static FieldNode addEnumConstant(final ClassNode enumClass, final String name, Expression init) {
         if (init != null && !(init instanceof ListExpression)) {
             ListExpression list = new ListExpression();
             list.addExpression(init);
             init = list;
         }
+        final int modifiers = Opcodes.ACC_ENUM | Opcodes.ACC_FINAL | Opcodes.ACC_STATIC | Opcodes.ACC_PUBLIC;
         FieldNode fn = new FieldNode(name, modifiers, enumClass.getPlainNodeReference(), enumClass, init);
         enumClass.addField(fn);
         return fn;

src/main/java/org/codehaus/groovy/classgen/ClassCompletionVerifier.java

@@ -59,6 +59,7 @@
 import static java.lang.reflect.Modifier.isFinal;
 import static java.lang.reflect.Modifier.isNative;
 import static java.lang.reflect.Modifier.isPrivate;
+import static java.lang.reflect.Modifier.isProtected;
 import static java.lang.reflect.Modifier.isStatic;
 import static java.lang.reflect.Modifier.isStrict;
 import static java.lang.reflect.Modifier.isSynchronized;
@@ -257,6 +258,7 @@ private void checkClassForIncorrectModifiers(final ClassNode node) {
         List<String> modifiers = new ArrayList<>();
 
         if (!(node instanceof InnerClassNode)) {
+            if (isProtected(node.getModifiers())) modifiers.add("protected");
             if (isPrivate(node.getModifiers())) modifiers.add("private");
             if (isStatic(node.getModifiers())) modifiers.add("static");
         }

src/main/java/org/codehaus/groovy/classgen/EnumVisitor.java

@@ -73,6 +73,7 @@
 import static org.codehaus.groovy.ast.tools.GeneralUtils.returnS;
 import static org.codehaus.groovy.ast.tools.GeneralUtils.spreadX;
 import static org.codehaus.groovy.ast.tools.GeneralUtils.varX;
+import static org.codehaus.groovy.runtime.StreamGroovyMethods.stream;
 import static org.objectweb.asm.Opcodes.ACC_ABSTRACT;
 import static org.objectweb.asm.Opcodes.ACC_FINAL;
 import static org.objectweb.asm.Opcodes.ACC_PRIVATE;
@@ -119,23 +120,25 @@ private void completeEnum(final ClassNode enumClass) {
                 }
             }
 
+            // GROOVY-10811: final and abstract are implicit modifiers
+            boolean isInnerClass = (enumClass.getOuterClass() != null);
+            if ((enumClass.getModifiers() & (ACC_ABSTRACT | ACC_FINAL)) != 0) {
+                String name = enumClass.getNameWithoutPackage(); // TODO: substring after the $
+                String permitted = (!isInnerClass ? "public is" : "public, private, protected & static are");
+                addError(enumClass, "Illegal modifier for the enum " + name + "; only " + permitted + " permitted.");
+            }
+
             addMethods(enumClass, values, minValue, maxValue);
-            checkForAbstractMethods(enumClass);
+
+            // for now, inner enum is always static
+            if (isInnerClass) enumClass.setModifiers(enumClass.getModifiers() | ACC_STATIC);
+            if (isAnyAbstract(enumClass)) enumClass.setModifiers(enumClass.getModifiers() | ACC_ABSTRACT);
+            else if (isNotExtended(enumClass)) enumClass.setModifiers(enumClass.getModifiers() | ACC_FINAL);
         }
 
         addInit(enumClass, minValue, maxValue, values, isAIC);
     }
 
-    private static void checkForAbstractMethods(final ClassNode enumClass) {
-        for (MethodNode method : enumClass.getMethods()) {
-            if (method.isAbstract()) {
-                // make the class abstract also; see Effective Java p.152
-                enumClass.setModifiers(enumClass.getModifiers() | ACC_ABSTRACT);
-                break;
-            }
-        }
-    }
-
     private static void addMethods(final ClassNode enumClass, final FieldNode values, FieldNode minValue, FieldNode maxValue) {
 
         boolean hasNext = false;
@@ -328,4 +331,12 @@ static boolean isAnonymousInnerClass(final ClassNode enumClass) {
         return enumClass instanceof EnumConstantClassNode
             && ((EnumConstantClassNode) enumClass).getVariableScope() == null;
     }
+
+    private static boolean isAnyAbstract(final ClassNode enumClass) {
+        return enumClass.getMethods().stream().anyMatch(MethodNode::isAbstract);
+    }
+
+    private static boolean isNotExtended(final ClassNode enumClass) {
+        return stream(enumClass.getInnerClasses()).noneMatch(it -> it instanceof EnumConstantClassNode);
+    }
 }

src/test/gls/enums/EnumTest.groovy

@@ -826,6 +826,39 @@ class EnumTest extends CompilableTestSupport {
             assert Outer.props == [bar: 10, baz: 20, foo: 30]
         '''
     }
+
+    // GROOVY-10811
+    void testIllegalModifiers() {
+        for (mod in ['','public','@groovy.transform.PackageScope']) {
+            shouldCompile """
+                $mod enum E {
+                }
+            """
+        }
+        for (mod in ['abstract','final','static','private','protected']) {
+            def err = shouldNotCompile """
+                $mod enum E {
+                }
+            """
+        }
+
+        for (mod in ['','public','private','protected','@groovy.transform.PackageScope','static']) {
+            shouldCompile """
+                class C {
+                    $mod enum E {
+                    }
+                }
+            """
+        }
+        for (mod in ['abstract','final']) {
+            shouldNotCompile """
+                class C {
+                    $mod enum E {
+                    }
+                }
+            """
+        }
+    }
 }
 
 enum UsCoin {

src/test/groovy/bugs/GroovyInnerEnumBug.groovy

@@ -21,29 +21,30 @@ package groovy.bugs
 import groovy.test.GroovyTestCase
 
 class GroovyInnerEnumBug extends GroovyTestCase {
-    static public enum MyEnum { 
-        a, b, c
-        public static MyEnum[] myenums = [a, b, c];
-    }
 
     // GROOVY-3979
-    void testEnumInsideAClass3979() {
-        assertScript """
+    void testInnerEnum1() {
+        assertScript '''
             class EnumTest2 {
                 enum Direction3979 { North, East, South, West }
                 static void main(args) {
-                    for (d in Direction3979) { 
+                    for (d in Direction3979) {
                         assert d instanceof Direction3979
                     }
                 }
             }
-        """
+        '''
     }
 
     // GROOVY-3994
-    void testEnumInsideAClass3994() {
+    void testInnerEnum2() {
         assert MyEnum.a.name() == 'a'
         assertTrue Enum.isAssignableFrom(MyEnum.class)
-        assert EnumSet.allOf(MyEnum.class) instanceof EnumSet 
+        assert EnumSet.allOf(MyEnum.class) instanceof EnumSet
+    }
+
+    public static enum MyEnum {
+        a, b, c;
+        public static MyEnum[] myenums = [a, b, c];
     }
 }

subprojects/groovy-astbuilder/src/test/groovy/org/codehaus/groovy/control/customizers/ASTTransformationCustomizerTest.groovy

@@ -108,12 +108,12 @@ class ASTTransformationCustomizerTest extends GroovyTestCase {
 @Retention(RetentionPolicy.SOURCE)
 @Target([ElementType.TYPE])
 @GroovyASTTransformationClass("org.codehaus.groovy.control.customizers.ContractAnnotation")
-protected @interface Contract {
+@interface Contract {
     Class value();
 }
 
 @GroovyASTTransformation(phase=CompilePhase.CONVERSION)
-protected class ContractAnnotation implements ASTTransformation, Opcodes {
+class ContractAnnotation implements ASTTransformation, Opcodes {
     void visit(ASTNode[] nodes, SourceUnit source) {
         def node = nodes[0]
         def member = node.getMember("value")
@@ -123,6 +123,6 @@ protected class ContractAnnotation implements ASTTransformation, Opcodes {
 
 @Retention(RetentionPolicy.SOURCE)
 @Target([ElementType.TYPE])
-protected @interface Contract2 {
+@interface Contract2 {
     Class value();
 }