Merge pull request #4566 from lrytz/t9359

SI-9359 Fix InnerClass entry flags for nested Java enums

15 files changed, 175 insertions, 66 deletions

diff --git a/src/compiler/scala/tools/nsc/backend/jvm/BCodeAsmCommon.scala b/src/compiler/scala/tools/nsc/backend/jvm/BCodeAsmCommon.scala
index eadc404bee..dec5adc9aa 100644
--- a/src/compiler/scala/tools/nsc/backend/jvm/BCodeAsmCommon.scala
+++ b/src/compiler/scala/tools/nsc/backend/jvm/BCodeAsmCommon.scala
@@ -9,6 +9,7 @@ package backend.jvm
 import scala.tools.nsc.Global
 import scala.tools.nsc.backend.jvm.BTypes.{InternalName, MethodInlineInfo, InlineInfo}
 import BackendReporting.ClassSymbolInfoFailureSI9111
+import scala.tools.asm
 
 /**
  * This trait contains code shared between GenBCode and GenASM that depends on types defined in
@@ -229,6 +230,44 @@ final class BCodeAsmCommon[G <: Global](val global: G) {
   }
 
   /**
+   * Reconstruct the classfile flags from a Java defined class symbol.
+   *
+   * The implementation of this method is slightly different that `javaFlags` in BTypesFromSymbols.
+   * The javaFlags method is primarily used to map Scala symbol flags to sensible classfile flags
+   * that are used in the generated classfiles. For example, all classes emitted by the Scala
+   * compiler have ACC_PUBLIC.
+   *
+   * When building a [[ClassBType]] from a Java class symbol, the flags in the type's `info` have
+   * to correspond exactly to the flags in the classfile. For example, if the class is package
+   * protected (i.e., it doesn't have the ACC_PUBLIC flag), this needs to be reflected in the
+   * ClassBType. For example, the inliner needs the correct flags for access checks.
+   *
+   * Class flags are listed here:
+   *   https://docs.oracle.com/javase/specs/jvms/se7/html/jvms-4.html#jvms-4.1-200-E.1
+   */
+  def javaClassfileFlags(classSym: Symbol): Int = {
+    assert(classSym.isJava, s"Expected Java class symbol, got ${classSym.fullName}")
+    import asm.Opcodes._
+    def enumFlags = ACC_ENUM | {
+      // Java enums have the `ACC_ABSTRACT` flag if they have a deferred method.
+      // We cannot trust `hasAbstractFlag`: the ClassfileParser adds `ABSTRACT` and `SEALED` to all
+      // Java enums for exhaustiveness checking.
+      val hasAbstractMethod = classSym.info.decls.exists(s => s.isMethod && s.isDeferred)
+      if (hasAbstractMethod) ACC_ABSTRACT else 0
+    }
+    GenBCode.mkFlags(
+      if (classSym.isPublic)                                 ACC_PUBLIC    else 0,
+      if (classSym.isFinal)                                  ACC_FINAL     else 0,
+      // see the link above. javac does the same: ACC_SUPER for all classes, but not interfaces.
+      if (classSym.isInterface)                              ACC_INTERFACE else ACC_SUPER,
+      // for Java enums, we cannot trust `hasAbstractFlag` (see comment in enumFlags)
+      if (!classSym.hasEnumFlag && classSym.hasAbstractFlag) ACC_ABSTRACT  else 0,
+      if (classSym.isArtifact)                               ACC_SYNTHETIC else 0,
+      if (classSym.hasEnumFlag)                              enumFlags     else 0
+    )
+  }
+
+  /**
    * The member classes of a class symbol. Note that the result of this method depends on the
    * current phase, for example, after lambdalift, all local classes become member of the enclosing
    * class.
@@ -399,3 +438,16 @@ final class BCodeAsmCommon[G <: Global](val global: G) {
     InlineInfo(traitSelfType, isEffectivelyFinal, methodInlineInfos, warning)
   }
 }
+
+object BCodeAsmCommon {
+  /**
+   * Valid flags for InnerClass attribute entry.
+   * See http://docs.oracle.com/javase/specs/jvms/se8/html/jvms-4.html#jvms-4.7.6
+   */
+  val INNER_CLASSES_FLAGS = {
+    asm.Opcodes.ACC_PUBLIC   | asm.Opcodes.ACC_PRIVATE   | asm.Opcodes.ACC_PROTECTED  |
+      asm.Opcodes.ACC_STATIC   | asm.Opcodes.ACC_FINAL     | asm.Opcodes.ACC_INTERFACE  |
+      asm.Opcodes.ACC_ABSTRACT | asm.Opcodes.ACC_SYNTHETIC | asm.Opcodes.ACC_ANNOTATION |
+      asm.Opcodes.ACC_ENUM
+  }
+}
diff --git a/src/compiler/scala/tools/nsc/backend/jvm/BTypes.scala b/src/compiler/scala/tools/nsc/backend/jvm/BTypes.scala
index e61190bf3a..176292669c 100644
--- a/src/compiler/scala/tools/nsc/backend/jvm/BTypes.scala
+++ b/src/compiler/scala/tools/nsc/backend/jvm/BTypes.scala
@@ -898,7 +898,7 @@ abstract class BTypes {
             // the static flag in the InnerClass table has a special meaning, see InnerClass comment
             i.flags & ~Opcodes.ACC_STATIC,
             if (isStaticNestedClass) Opcodes.ACC_STATIC else 0
-          ) & ClassBType.INNER_CLASSES_FLAGS
+          ) & BCodeAsmCommon.INNER_CLASSES_FLAGS
         )
     })
 
@@ -987,17 +987,6 @@ abstract class BTypes {
   }
 
   object ClassBType {
-    /**
-     * Valid flags for InnerClass attribute entry.
-     * See http://docs.oracle.com/javase/specs/jvms/se8/html/jvms-4.html#jvms-4.7.6
-     */
-    private val INNER_CLASSES_FLAGS = {
-      asm.Opcodes.ACC_PUBLIC   | asm.Opcodes.ACC_PRIVATE   | asm.Opcodes.ACC_PROTECTED  |
-      asm.Opcodes.ACC_STATIC   | asm.Opcodes.ACC_FINAL     | asm.Opcodes.ACC_INTERFACE  |
-      asm.Opcodes.ACC_ABSTRACT | asm.Opcodes.ACC_SYNTHETIC | asm.Opcodes.ACC_ANNOTATION |
-      asm.Opcodes.ACC_ENUM
-    }
-
     // Primitive classes have no super class. A ClassBType for those is only created when
     // they are actually being compiled (e.g., when compiling scala/Boolean.scala).
     private val hasNoSuper = Set(
diff --git a/src/compiler/scala/tools/nsc/backend/jvm/BTypesFromSymbols.scala b/src/compiler/scala/tools/nsc/backend/jvm/BTypesFromSymbols.scala
index 356af36455..d68c916f09 100644
--- a/src/compiler/scala/tools/nsc/backend/jvm/BTypesFromSymbols.scala
+++ b/src/compiler/scala/tools/nsc/backend/jvm/BTypesFromSymbols.scala
@@ -213,35 +213,6 @@ class BTypesFromSymbols[G <: Global](val global: G) extends BTypes {
     assert(!primitiveTypeMap.contains(sym) || isCompilingPrimitive, sym)
   }
 
-  /**
-   * Reconstruct the classfile flags from a Java defined class symbol.
-   *
-   * The implementation of this method is slightly different that [[javaFlags]]. The javaFlags
-   * method is primarily used to map Scala symbol flags to sensible classfile flags that are used
-   * in the generated classfiles. For example, all classes emitted by the Scala compiler have
-   * ACC_PUBLIC.
-   *
-   * When building a [[ClassBType]] from a Java class symbol, the flags in the type's `info` have
-   * to correspond exactly to the flags in the classfile. For example, if the class is package
-   * protected (i.e., it doesn't have the ACC_PUBLIC flag), this needs to be reflected in the
-   * ClassBType. For example, the inliner needs the correct flags for access checks.
-   *
-   * Class flags are listed here:
-   *   https://docs.oracle.com/javase/specs/jvms/se7/html/jvms-4.html#jvms-4.1-200-E.1
-   */
-  private def javaClassfileFlags(classSym: Symbol): Int = {
-    assert(classSym.isJava, s"Expected Java class symbol, got ${classSym.fullName}")
-    import asm.Opcodes._
-    GenBCode.mkFlags(
-      if (classSym.isPublic)        ACC_PUBLIC    else 0,
-      if (classSym.isFinal)         ACC_FINAL     else 0,
-      if (classSym.isInterface)     ACC_INTERFACE else ACC_SUPER, // see the link above. javac does the same: ACC_SUPER for all classes, but not interfaces.
-      if (classSym.hasAbstractFlag) ACC_ABSTRACT  else 0,
-      if (classSym.isArtifact)      ACC_SYNTHETIC else 0,
-      if (classSym.hasEnumFlag)     ACC_ENUM      else 0
-    )
-  }
-
   private def setClassInfo(classSym: Symbol, classBType: ClassBType): ClassBType = {
     val superClassSym = if (classSym.isImplClass) ObjectClass else classSym.superClass
     assert(
diff --git a/src/compiler/scala/tools/nsc/backend/jvm/GenASM.scala b/src/compiler/scala/tools/nsc/backend/jvm/GenASM.scala
index 76af40b330..71686fd9d7 100644
--- a/src/compiler/scala/tools/nsc/backend/jvm/GenASM.scala
+++ b/src/compiler/scala/tools/nsc/backend/jvm/GenASM.scala
@@ -479,10 +479,6 @@ abstract class GenASM extends SubComponent with BytecodeWriters { self =>
     val CLASS_CONSTRUCTOR_NAME    = "<clinit>"
     val INSTANCE_CONSTRUCTOR_NAME = "<init>"
 
-    val INNER_CLASSES_FLAGS =
-      (asm.Opcodes.ACC_PUBLIC    | asm.Opcodes.ACC_PRIVATE | asm.Opcodes.ACC_PROTECTED |
-       asm.Opcodes.ACC_STATIC    | asm.Opcodes.ACC_INTERFACE | asm.Opcodes.ACC_ABSTRACT | asm.Opcodes.ACC_FINAL)
-
     // -----------------------------------------------------------------------------------------
     // factory methods
     // -----------------------------------------------------------------------------------------
@@ -756,9 +752,9 @@ abstract class GenASM extends SubComponent with BytecodeWriters { self =>
           val flagsWithFinal: Int = mkFlags(
             // See comment in BTypes, when is a class marked static in the InnerClass table.
             if (isOriginallyStaticOwner(innerSym.originalOwner)) asm.Opcodes.ACC_STATIC else 0,
-            javaFlags(innerSym),
+            (if (innerSym.isJava) javaClassfileFlags(innerSym) else javaFlags(innerSym)) & ~asm.Opcodes.ACC_STATIC,
             if(isDeprecated(innerSym)) asm.Opcodes.ACC_DEPRECATED else 0 // ASM pseudo-access flag
-          ) & (INNER_CLASSES_FLAGS | asm.Opcodes.ACC_DEPRECATED)
+          ) & (BCodeAsmCommon.INNER_CLASSES_FLAGS | asm.Opcodes.ACC_DEPRECATED)
           val flags = if (innerSym.isModuleClass) flagsWithFinal & ~asm.Opcodes.ACC_FINAL else flagsWithFinal // For SI-5676, object overriding.
           val jname = javaName(innerSym)  // never null
           val oname = outerName(innerSym) // null when method-enclosed
diff --git a/src/compiler/scala/tools/nsc/javac/JavaParsers.scala b/src/compiler/scala/tools/nsc/javac/JavaParsers.scala
index d34c14be0f..9708cba281 100644
--- a/src/compiler/scala/tools/nsc/javac/JavaParsers.scala
+++ b/src/compiler/scala/tools/nsc/javac/JavaParsers.scala
@@ -761,9 +761,13 @@ trait JavaParsers extends ast.parser.ParsersCommon with JavaScanners {
       val interfaces = interfacesOpt()
       accept(LBRACE)
       val buf = new ListBuffer[Tree]
+      var enumIsFinal = true
       def parseEnumConsts() {
         if (in.token != RBRACE && in.token != SEMI && in.token != EOF) {
-          buf += enumConst(enumType)
+          val (const, hasClassBody) = enumConst(enumType)
+          buf += const
+          // if any of the enum constants has a class body, the enum class is not final (JLS 8.9.)
+          enumIsFinal &&= !hasClassBody
           if (in.token == COMMA) {
             in.nextToken()
             parseEnumConsts()
@@ -793,15 +797,25 @@ trait JavaParsers extends ast.parser.ParsersCommon with JavaScanners {
       accept(RBRACE)
       val superclazz =
         AppliedTypeTree(javaLangDot(tpnme.Enum), List(enumType))
+      val finalFlag = if (enumIsFinal) Flags.FINAL else 0l
+      val abstractFlag = {
+        // javac adds `ACC_ABSTRACT` to enum classes with deferred members
+        val hasAbstractMember = body exists {
+          case d: DefDef => d.mods.isDeferred
+          case _         => false
+        }
+        if (hasAbstractMember) Flags.ABSTRACT else 0l
+      }
       addCompanionObject(consts ::: statics ::: predefs, atPos(pos) {
-        ClassDef(mods | Flags.ENUM, name, List(),
+        ClassDef(mods | Flags.ENUM | finalFlag | abstractFlag, name, List(),
                  makeTemplate(superclazz :: interfaces, body))
       })
     }
 
-    def enumConst(enumType: Tree) = {
+    def enumConst(enumType: Tree): (ValDef, Boolean) = {
       annotations()
-      atPos(in.currentPos) {
+      var hasClassBody = false
+      val res = atPos(in.currentPos) {
         val name = ident()
         if (in.token == LPAREN) {
           // skip arguments
@@ -809,12 +823,14 @@ trait JavaParsers extends ast.parser.ParsersCommon with JavaScanners {
           accept(RPAREN)
         }
         if (in.token == LBRACE) {
+          hasClassBody = true
           // skip classbody
           skipAhead()
           accept(RBRACE)
         }
         ValDef(Modifiers(Flags.ENUM | Flags.STABLE | Flags.JAVA | Flags.STATIC), name.toTermName, enumType, blankExpr)
       }
+      (res, hasClassBody)
     }
 
     def typeDecl(mods: Modifiers): List[Tree] = in.token match {
diff --git a/src/compiler/scala/tools/nsc/symtab/classfile/ClassfileParser.scala b/src/compiler/scala/tools/nsc/symtab/classfile/ClassfileParser.scala
index 518a402230..660028eab8 100644
--- a/src/compiler/scala/tools/nsc/symtab/classfile/ClassfileParser.scala
+++ b/src/compiler/scala/tools/nsc/symtab/classfile/ClassfileParser.scala
@@ -539,6 +539,8 @@ abstract class ClassfileParser {
             devWarning(s"no linked class for java enum $sym in ${sym.owner}. A referencing class file might be missing an InnerClasses entry.")
           case linked =>
             if (!linked.isSealed)
+              // Marking the enum class SEALED | ABSTRACT enables exhaustiveness checking.
+              // This is a bit of a hack and requires excluding the ABSTRACT flag in the backend, see method javaClassfileFlags.
               linked setFlag (SEALED | ABSTRACT)
             linked addChild sym
         }
diff --git a/src/reflect/scala/reflect/internal/ClassfileConstants.scala b/src/reflect/scala/reflect/internal/ClassfileConstants.scala
index e0a6757d34..53241fb15b 100644
--- a/src/reflect/scala/reflect/internal/ClassfileConstants.scala
+++ b/src/reflect/scala/reflect/internal/ClassfileConstants.scala
@@ -344,10 +344,12 @@ object ClassfileConstants {
       case JAVA_ACC_STATIC     => STATIC
       case JAVA_ACC_ABSTRACT   => if (isAnnotation) 0L else if (isClass) ABSTRACT else DEFERRED
       case JAVA_ACC_INTERFACE  => if (isAnnotation) 0L else TRAIT | INTERFACE | ABSTRACT
+      case JAVA_ACC_ENUM       => ENUM
       case _                   => 0L
     }
-    private def translateFlags(jflags: Int, baseFlags: Long, isAnnotation: Boolean, isClass: Boolean): Long = {
-      def translateFlag0(jflags: Int): Long = translateFlag(jflags, isAnnotation, isClass)
+    private def translateFlags(jflags: Int, baseFlags: Long, isClass: Boolean): Long = {
+      val isAnnot = isAnnotation(jflags)
+      def translateFlag0(jflags: Int): Long = translateFlag(jflags, isAnnot, isClass)
       var res: Long = JAVA | baseFlags
       /* fast, elegant, maintainable, pick any two... */
       res |= translateFlag0(jflags & JAVA_ACC_PRIVATE)
@@ -357,17 +359,18 @@ object ClassfileConstants {
       res |= translateFlag0(jflags & JAVA_ACC_STATIC)
       res |= translateFlag0(jflags & JAVA_ACC_ABSTRACT)
       res |= translateFlag0(jflags & JAVA_ACC_INTERFACE)
+      res |= translateFlag0(jflags & JAVA_ACC_ENUM)
       res
     }
 
     def classFlags(jflags: Int): Long = {
-      translateFlags(jflags, 0, isAnnotation(jflags), isClass = true)
+      translateFlags(jflags, 0, isClass = true)
     }
     def fieldFlags(jflags: Int): Long = {
-      translateFlags(jflags, if ((jflags & JAVA_ACC_FINAL) == 0) MUTABLE else 0 , isAnnotation(jflags), isClass = false)
+      translateFlags(jflags, if ((jflags & JAVA_ACC_FINAL) == 0) MUTABLE else 0 , isClass = false)
     }
     def methodFlags(jflags: Int): Long = {
-      translateFlags(jflags, if ((jflags & JAVA_ACC_BRIDGE) != 0) BRIDGE | ARTIFACT else 0, isAnnotation(jflags), isClass = false)
+      translateFlags(jflags, if ((jflags & JAVA_ACC_BRIDGE) != 0) BRIDGE | ARTIFACT else 0, isClass = false)
     }
   }
   object FlagTranslation extends FlagTranslation { }
diff --git a/test/files/run/t7582.check b/test/files/run/t7582.check
index 2a11210000..0cfbf08886 100644
--- a/test/files/run/t7582.check
+++ b/test/files/run/t7582.check
@@ -1,6 +1 @@
-#partest !-Ybackend:GenBCode
-warning: there was one inliner warning; re-run with -Yinline-warnings for details
-#partest -Ybackend:GenBCode
-warning: there was one inliner warning; re-run with -Yopt-warnings for details
-#partest
 2
diff --git a/test/files/run/t7582/InlineHolder.scala b/test/files/run/t7582/InlineHolder.scala
index a18b9effaa..3cbf233ce1 100644
--- a/test/files/run/t7582/InlineHolder.scala
+++ b/test/files/run/t7582/InlineHolder.scala
@@ -1,3 +1,6 @@
+/*
+ * filter: inliner warning; re-run with
+ */
 package p1 {
   object InlineHolder {
     @inline def inlinable = p1.PackageProtectedJava.protectedMethod() + 1
diff --git a/test/files/run/t7582b.check b/test/files/run/t7582b.check
index 2a11210000..0cfbf08886 100644
--- a/test/files/run/t7582b.check
+++ b/test/files/run/t7582b.check
@@ -1,6 +1 @@
-#partest !-Ybackend:GenBCode
-warning: there was one inliner warning; re-run with -Yinline-warnings for details
-#partest -Ybackend:GenBCode
-warning: there was one inliner warning; re-run with -Yopt-warnings for details
-#partest
 2
diff --git a/test/files/run/t7582b/InlineHolder.scala b/test/files/run/t7582b/InlineHolder.scala
index a18b9effaa..3cbf233ce1 100644
--- a/test/files/run/t7582b/InlineHolder.scala
+++ b/test/files/run/t7582b/InlineHolder.scala
@@ -1,3 +1,6 @@
+/*
+ * filter: inliner warning; re-run with
+ */
 package p1 {
   object InlineHolder {
     @inline def inlinable = p1.PackageProtectedJava.protectedMethod() + 1
diff --git a/test/files/run/t9359.check b/test/files/run/t9359.check
new file mode 100644
index 0000000000..8dcfe4f60a
--- /dev/null
+++ b/test/files/run/t9359.check
@@ -0,0 +1,18 @@
+  // access flags 0x4009
+  public static enum INNERCLASS A_1$A1N A_1 A1N
+
+  // access flags 0x4409
+  public static abstract enum INNERCLASS A_1$A1N_ABSTRACT A_1 A1N_ABSTRACT
+
+  // access flags 0x4019
+  public final static enum INNERCLASS A_1$A1N_FINAL A_1 A1N_FINAL
+
+  // access flags 0x4009
+  public static enum INNERCLASS B_2$A1N B_2 A1N
+
+  // access flags 0x4409
+  public static abstract enum INNERCLASS B_2$A1N_ABSTRACT B_2 A1N_ABSTRACT
+
+  // access flags 0x4019
+  public final static enum INNERCLASS B_2$A1N_FINAL B_2 A1N_FINAL
+
diff --git a/test/files/run/t9359/A_1.java b/test/files/run/t9359/A_1.java
new file mode 100644
index 0000000000..3ac82ed55f
--- /dev/null
+++ b/test/files/run/t9359/A_1.java
@@ -0,0 +1,19 @@
+public class A_1 {
+  // nested final
+  public static enum A1N_FINAL {
+    A1N_FINAL_VAL
+  }
+
+  // nested, non-final
+  public enum A1N {
+    A1N_VAL { } // value has a body, so a class extending A1N is generated
+  }
+
+  // nested, non-final, abstract
+  public enum A1N_ABSTRACT {
+    A1N_ABSTRACT_VAL {
+      void foo() { return; }
+    };
+    abstract void foo(); // abstract member makes the enum class abstract
+  }
+}
diff --git a/test/files/run/t9359/B_2.java b/test/files/run/t9359/B_2.java
new file mode 100644
index 0000000000..d824facda9
--- /dev/null
+++ b/test/files/run/t9359/B_2.java
@@ -0,0 +1,19 @@
+public class B_2 {
+  // nested final
+  public enum A1N_FINAL {
+    A1N_FINAL_VAL
+  }
+
+  // nested, non-final
+  public enum A1N {
+    A1N_VAL { } // value has a body, so a class extending A1N is generated
+  }
+
+  // nested, non-final, abstract
+  public enum A1N_ABSTRACT {
+    A1N_ABSTRACT_VAL {
+      void foo() { return; }
+    };
+    abstract void foo(); // abstract member makes the enum class abstract
+  }
+}
diff --git a/test/files/run/t9359/Test_2.scala b/test/files/run/t9359/Test_2.scala
new file mode 100644
index 0000000000..869c51b619
--- /dev/null
+++ b/test/files/run/t9359/Test_2.scala
@@ -0,0 +1,28 @@
+import scala.tools.partest.BytecodeTest
+import scala.tools.asm
+import asm.tree.{ClassNode, InnerClassNode}
+import asm.{Opcodes => Flags}
+import scala.collection.JavaConverters._
+
+class C {
+  def f1: A_1.A1N_FINAL    = A_1.A1N_FINAL.A1N_FINAL_VAL
+  def f2: A_1.A1N          = A_1.A1N.A1N_VAL
+  def f3: A_1.A1N_ABSTRACT = A_1.A1N_ABSTRACT.A1N_ABSTRACT_VAL
+
+  def f4: B_2.A1N_FINAL    = B_2.A1N_FINAL.A1N_FINAL_VAL
+  def f5: B_2.A1N          = B_2.A1N.A1N_VAL
+  def f6: B_2.A1N_ABSTRACT = B_2.A1N_ABSTRACT.A1N_ABSTRACT_VAL
+}
+
+object Test extends BytecodeTest {
+  def tost(n: InnerClassNode) = {
+    val t = new asm.util.Textifier
+    t.visitInnerClass(n.name, n.outerName, n.innerName, n.access)
+    t.getText.get(0);
+  }
+  def show(): Unit = {
+    for (n <- loadClassNode("C").innerClasses.asScala.toList.sortBy(_.name)) {
+      println(tost(n))
+    }
+  }
+}