SI-9393 fix modifiers of ClassBTypes for Java annotations

The Scala classfile and java source parsers make Java annotation
classes (which are actually interfaces at the classfile level) look
like Scala annotation classes:
  - the INTERFACE / ABSTRACT flags are not added
  - scala.annotation.Annotation is added as superclass
  - scala.annotation.ClassfileAnnotation is added as interface

This makes type-checking @annot uniform, whether it is defined in Java
or Scala.

This is a hack that leads to various bugs (SI-9393, SI-9400). Instead
the type-checking should be special-cased for Java annotations.

This commit fixes SI-9393 and a part of SI-9400, but it's still easy
to generate invalid classfiles. Restores the assertions that were
disabled in scala#4621. I'd like to leave these assertions in: they
are valuable and helped uncovering the issue being fixed here.

A new flag JAVA_ANNOTATION is introduced for Java annotation
ClassSymbols, similar to the existing ENUM flag. When building
ClassBTypes for Java annotations, the flags, superclass and interfaces
are recovered to represent the situation in the classfile.

Cleans up and documents the flags space in the area of "late" and
"anti" flags.

The test for SI-9393 is extended to test both the classfile and the
java source parser.

Author: lrytz

src/compiler/scala/tools/nsc/backend/jvm/BCodeAsmCommon.scala

@@ -256,6 +256,9 @@ final class BCodeAsmCommon[G <: Global](val global: G) {
       if (hasAbstractMethod) ACC_ABSTRACT else 0
     }
     GenBCode.mkFlags(
+      // SI-9393: the classfile / java source parser make java annotation symbols look like classes.
+      // here we recover the actual classfile flags.
+      if (classSym.hasJavaAnnotationFlag)                    ACC_ANNOTATION | ACC_INTERFACE | ACC_ABSTRACT else 0,
       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.
@@ -310,10 +313,10 @@ final class BCodeAsmCommon[G <: Global](val global: G) {
   }
 
   private def retentionPolicyOf(annot: AnnotationInfo): Symbol =
-    annot.atp.typeSymbol.getAnnotation(AnnotationRetentionAttr).map(_.assocs).map(assoc =>
+    annot.atp.typeSymbol.getAnnotation(AnnotationRetentionAttr).map(_.assocs).flatMap(assoc =>
       assoc.collectFirst {
         case (`nme`.value, LiteralAnnotArg(Constant(value: Symbol))) => value
-      }).flatten.getOrElse(AnnotationRetentionPolicyClassValue)
+      }).getOrElse(AnnotationRetentionPolicyClassValue)
 
   def implementedInterfaces(classSym: Symbol): List[Symbol] = {
     // Additional interface parents based on annotations and other cues
@@ -322,9 +325,18 @@ final class BCodeAsmCommon[G <: Global](val global: G) {
       case _          => None
     }
 
-    def isInterfaceOrTrait(sym: Symbol) = sym.isInterface || sym.isTrait
+    // SI-9393: java annotations are interfaces, but the classfile / java source parsers make them look like classes.
+    def isInterfaceOrTrait(sym: Symbol) = sym.isInterface || sym.isTrait || sym.hasJavaAnnotationFlag
 
-    val allParents = classSym.info.parents ++ classSym.annotations.flatMap(newParentForAnnotation)
+    val classParents = {
+      val parents = classSym.info.parents
+      // SI-9393: the classfile / java source parsers add Annotation and ClassfileAnnotation to the
+      // parents of a java annotations. undo this for the backend (where we need classfile-level information).
+      if (classSym.hasJavaAnnotationFlag) parents.filterNot(c => c.typeSymbol == ClassfileAnnotationClass || c.typeSymbol == AnnotationClass)
+      else parents
+    }
+
+    val allParents = classParents ++ classSym.annotations.flatMap(newParentForAnnotation)
 
     // We keep the superClass when computing minimizeParents to eliminate more interfaces.
     // Example: T can be eliminated from D

src/compiler/scala/tools/nsc/backend/jvm/BCodeSkelBuilder.scala

@@ -153,9 +153,9 @@ abstract class BCodeSkelBuilder extends BCodeHelpers {
      */
     private def initJClass(jclass: asm.ClassVisitor) {
 
-      val ps = claszSymbol.info.parents
-      val superClass: String = if (ps.isEmpty) ObjectReference.internalName else internalName(ps.head.typeSymbol)
-      val interfaceNames = classBTypeFromSymbol(claszSymbol).info.get.interfaces map {
+      val bType = classBTypeFromSymbol(claszSymbol)
+      val superClass = bType.info.get.superClass.getOrElse(ObjectReference).internalName
+      val interfaceNames = bType.info.get.interfaces map {
         case classBType =>
           if (classBType.isNestedClass.get) { innerClassBufferASM += classBType }
           classBType.internalName

src/compiler/scala/tools/nsc/backend/jvm/BTypes.scala

@@ -841,17 +841,16 @@ abstract class BTypes {
 
       assert(!ClassBType.isInternalPhantomType(internalName), s"Cannot create ClassBType for phantom type $this")
 
-      // TODO bring these back in a way that doesn't trip pos/t9393
-      // assert(
-      //   if (info.get.superClass.isEmpty) { isJLO(this) || (isCompilingPrimitive && ClassBType.hasNoSuper(internalName)) }
-      //   else if (isInterface.get) isJLO(info.get.superClass.get)
-      //   else !isJLO(this) && ifInit(info.get.superClass.get)(!_.isInterface.get),
-      //   s"Invalid superClass in $this: ${info.get.superClass}"
-      // )
-      // assert(
-      //   info.get.interfaces.forall(c => ifInit(c)(_.isInterface.get)),
-      //   s"Invalid interfaces in $this: ${info.get.interfaces}"
-      // )
+      assert(
+        if (info.get.superClass.isEmpty) { isJLO(this) || (isCompilingPrimitive && ClassBType.hasNoSuper(internalName)) }
+        else if (isInterface.get) isJLO(info.get.superClass.get)
+        else !isJLO(this) && ifInit(info.get.superClass.get)(!_.isInterface.get),
+        s"Invalid superClass in $this: ${info.get.superClass}"
+      )
+      assert(
+        info.get.interfaces.forall(c => ifInit(c)(_.isInterface.get)),
+        s"Invalid interfaces in $this: ${info.get.interfaces}"
+      )
 
       assert(info.get.nestedClasses.forall(c => ifInit(c)(_.isNestedClass.get)), info.get.nestedClasses)
     }

src/compiler/scala/tools/nsc/backend/jvm/BTypesFromSymbols.scala

@@ -216,7 +216,18 @@ class BTypesFromSymbols[G <: Global](val global: G) extends BTypes {
   }
 
   private def setClassInfo(classSym: Symbol, classBType: ClassBType): ClassBType = {
-    val superClassSym = if (classSym.isImplClass) ObjectClass else classSym.superClass
+    // Check for isImplClass: trait implementation classes have NoSymbol as superClass
+    // Check for hasAnnotationFlag for SI-9393: the classfile / java source parsers add
+    // scala.annotation.Annotation as superclass to java annotations. In reality, java
+    // annotation classfiles have superclass Object (like any interface classfile).
+    val superClassSym = if (classSym.isImplClass || classSym.hasJavaAnnotationFlag) ObjectClass else {
+      val sc = classSym.superClass
+      // SI-9393: Java annotation classes don't have the ABSTRACT/INTERFACE flag, so they appear
+      // (wrongly) as superclasses. Fix this for BTypes: the java annotation will appear as interface
+      // (handled by method implementedInterfaces), the superclass is set to Object.
+      if (sc.hasJavaAnnotationFlag) ObjectClass
+      else sc
+    }
     assert(
       if (classSym == ObjectClass)
         superClassSym == NoSymbol

src/compiler/scala/tools/nsc/javac/JavaParsers.scala

@@ -751,7 +751,7 @@ trait JavaParsers extends ast.parser.ParsersCommon with JavaScanners {
       val (statics, body) = typeBody(AT, name)
       val templ = makeTemplate(annotationParents, body)
       addCompanionObject(statics, atPos(pos) {
-        ClassDef(mods, name, List(), templ)
+        ClassDef(mods | Flags.JAVA_ANNOTATION, name, List(), templ)
       })
     }
 

src/reflect/scala/reflect/internal/ClassfileConstants.scala

@@ -345,6 +345,7 @@ object ClassfileConstants {
       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 JAVA_ACC_ANNOTATION => JAVA_ANNOTATION
       case _                   => 0L
     }
     private def translateFlags(jflags: Int, baseFlags: Long, isClass: Boolean): Long = {
@@ -360,6 +361,7 @@ object ClassfileConstants {
       res |= translateFlag0(jflags & JAVA_ACC_ABSTRACT)
       res |= translateFlag0(jflags & JAVA_ACC_INTERFACE)
       res |= translateFlag0(jflags & JAVA_ACC_ENUM)
+      res |= translateFlag0(jflags & JAVA_ACC_ANNOTATION)
       res
     }
 

src/reflect/scala/reflect/internal/Flags.scala

@@ -64,7 +64,7 @@ import scala.collection.{ mutable, immutable }
 // 46:        ARTIFACT
 // 47: DEFAULTMETHOD/M
 // 48:            ENUM
-// 49:
+// 49:      JAVA_ANNOTATION
 // 50:
 // 51:    lateDEFERRED
 // 52:       lateFINAL
@@ -120,7 +120,8 @@ class ModifierFlags {
   final val ARTIFACT      = 1L << 46      // symbol should be ignored when typechecking; will be marked ACC_SYNTHETIC in bytecode
                                           // to see which symbols are marked as ARTIFACT, see scaladocs for FlagValues.ARTIFACT
   final val DEFAULTMETHOD = 1L << 47      // symbol is a java default method
-  final val ENUM          = 1L << 48      // symbol is an enum
+  final val ENUM          = 1L << 48      // symbol is a java enum
+  final val JAVA_ANNOTATION    = 1L << 49      // symbol is a java annotation
 
   // Overridden.
   def flagToString(flag: Long): String = ""
@@ -172,12 +173,28 @@ class Flags extends ModifierFlags {
   final val SYNCHRONIZED  = 1L << 45      // symbol is a method which should be marked ACC_SYNCHRONIZED
 
   // ------- shift definitions -------------------------------------------------------
+  //
+  // Flags from 1L to (1L << 50) are normal flags.
+  //
+  // The flags DEFERRED (1L << 4) to MODULE (1L << 8) have a `late` counterpart. Late flags change
+  // their counterpart from 0 to 1 after a specific phase (see below). The first late flag
+  // (lateDEFERRED) is at (1L << 51), i.e., late flags are shifted by 47. The last one is (1L << 55).
+  //
+  // The flags PROTECTED (1L) to PRIVATE (1L << 2) have a `not` counterpart. Negated flags change
+  // their counterpart from 1 to 0 after a specific phase (see below). They are shifted by 56, i.e.,
+  // the first negated flag (notPROTECTED) is at (1L << 56), the last at (1L << 58).
+  //
+  // Late and negative flags are only enabled after certain phases, implemented by the phaseNewFlags
+  // method of the SubComponent, so they implement a bit of a flag history.
+  //
+  // The flags (1L << 59) to (1L << 63) are currently unused. If added to the InitialFlags mask,
+  // they could be used as normal flags.
 
-  final val InitialFlags  = 0x0001FFFFFFFFFFFFL // flags that are enabled from phase 1.
-  final val LateFlags     = 0x00FE000000000000L // flags that override flags in 0x1FC.
-  final val AntiFlags     = 0x7F00000000000000L // flags that cancel flags in 0x07F
-  final val LateShift     = 47L
-  final val AntiShift     = 56L
+  final val InitialFlags  = 0x0007FFFFFFFFFFFFL // normal flags, enabled from the first phase: 1L to (1L << 50)
+  final val LateFlags     = 0x00F8000000000000L // flags that override flags in (1L << 4) to (1L << 8): DEFERRED, FINAL, INTERFACE, METHOD, MODULE
+  final val AntiFlags     = 0x0700000000000000L // flags that cancel flags in 1L to (1L << 2): PROTECTED, OVERRIDE, PRIVATE
+  final val LateShift     = 47
+  final val AntiShift     = 56
 
   // Flags which sketchily share the same slot
   // 16:   BYNAMEPARAM/M      CAPTURED COVARIANT/M
@@ -436,7 +453,7 @@ class Flags extends ModifierFlags {
     case            ARTIFACT => "<artifact>"                          // (1L << 46)
     case       DEFAULTMETHOD => "<defaultmethod>"                     // (1L << 47)
     case                ENUM => "<enum>"                              // (1L << 48)
-    case    0x2000000000000L => ""                                    // (1L << 49)
+    case     JAVA_ANNOTATION => "<annotation>"                        // (1L << 49)
     case    0x4000000000000L => ""                                    // (1L << 50)
     case      `lateDEFERRED` => "<latedeferred>"                      // (1L << 51)
     case         `lateFINAL` => "<latefinal>"                         // (1L << 52)

src/reflect/scala/reflect/internal/HasFlags.scala

@@ -83,6 +83,7 @@ trait HasFlags {
   def hasAccessorFlag    = hasFlag(ACCESSOR)
   def hasDefault         = hasFlag(DEFAULTPARAM) && hasFlag(METHOD | PARAM) // Second condition disambiguates with TRAIT
   def hasEnumFlag        = hasFlag(ENUM)
+  def hasJavaAnnotationFlag = hasFlag(JAVA_ANNOTATION)
   @deprecated("Use isLocalToThis instead", "2.11.0")
   def hasLocalFlag       = hasFlag(LOCAL)
   def isLocalToThis      = hasFlag(LOCAL)

src/reflect/scala/reflect/internal/Symbols.scala

@@ -732,7 +732,7 @@ trait Symbols extends api.Symbols { self: SymbolTable =>
     final def flags: Long = {
       if (Statistics.hotEnabled) Statistics.incCounter(flagsCount)
       val fs = _rawflags & phase.flagMask
-      (fs | ((fs & LateFlags) >>> LateShift)) & ~(fs >>> AntiShift)
+      (fs | ((fs & LateFlags) >>> LateShift)) & ~((fs & AntiFlags) >>> AntiShift)
     }
     def flags_=(fs: Long) = _rawflags = fs
     def rawflags_=(x: Long) { _rawflags = x }

test/files/jvm/innerClassAttribute/Test.scala

@@ -149,9 +149,7 @@ object Test extends BytecodeTest {
 
   def testA11() = {
     val List(ann) = innerClassNodes("A11")
-    // in the java class file, the INNERCLASS attribute has more flags (public | static | abstract | interface | annotation)
-    // the scala compiler has its own interpretation of java annotations ant their flags.. it only emits publicStatic.
-    assertMember(ann, "JavaAnnot_1", "Ann", flags = publicStatic)
+    assertMember(ann, "JavaAnnot_1", "Ann", flags = publicAbstractInterface | Flags.ACC_STATIC | Flags.ACC_ANNOTATION)
   }
 
   def testA13() = {

test/files/pos/t9393/Named.java

@@ -3,11 +3,11 @@
  */
 package bug;
 
-import bug.Named;
+import bug.Named_1;
 import java.io.Serializable;
 import java.lang.annotation.Annotation;
 
-public class NamedImpl implements Named {
+public class NamedImpl_1 implements Named_1 {
 
     public Class<? extends Annotation> annotationType() {
         return null;

test/files/pos/t9393/NamedImpl.java renamed to test/files/pos/t9393/NamedImpl_1.java

@@ -0,0 +1,15 @@
+/*
+ * Copyright (C) 2009-2015 Typesafe Inc. <http://www.typesafe.com>
+ */
+package bug;
+
+import bug.Named_2;
+import java.io.Serializable;
+import java.lang.annotation.Annotation;
+
+public class NamedImpl_2 implements Named_2 {
+
+    public Class<? extends Annotation> annotationType() {
+        return null;
+    }
+}

test/files/pos/t9393/NamedImpl_2.java

@@ -0,0 +1,3 @@
+package bug;
+
+public @interface Named_1 {}

test/files/pos/t9393/Named_1.java

@@ -0,0 +1,3 @@
+package bug;
+
+public @interface Named_2 {}

test/files/pos/t9393/Named_2.java

@@ -0,0 +1,4 @@
+class C {
+  new bug.NamedImpl_1 // separate compilation, testing the classfile parser
+  new bug.NamedImpl_2 // mixed compilation, testing the java source parser
+}

test/files/pos/t9393/test.scala

@@ -0,0 +1,89 @@
+package scala.tools.nsc
+package symtab
+
+import org.junit.Assert._
+import scala.tools.testing.AssertUtil._
+import org.junit.Test
+import org.junit.runner.RunWith
+import org.junit.runners.JUnit4
+
+@RunWith(classOf[JUnit4])
+class FlagsTest {
+  object symbolTable extends SymbolTableForUnitTesting
+  import symbolTable._
+  import Flags._
+
+  def sym = NoSymbol.newTermSymbol(nme.EMPTY)
+
+  def withFlagMask[A](mask: Long)(body: => A): A = enteringPhase(new Phase(NoPhase) {
+    override def flagMask = mask
+    def name = ""
+    def run() = ()
+  })(body)
+
+  def testTimedFlag(flag: Long, test: Symbol => Boolean, enabling: Boolean) = {
+    assertEquals(withFlagMask(InitialFlags)(test(sym.setFlag(flag))), !enabling)
+    assertEquals(withFlagMask(InitialFlags | flag)(test(sym.setFlag(flag))), enabling)
+  }
+
+  def testLate(flag: Long, test: Symbol => Boolean) = testTimedFlag(flag, test, enabling = true)
+  def testNot(flag: Long, test: Symbol => Boolean) = testTimedFlag(flag, test, enabling = false)
+
+  @Test
+  def testTimedFlags(): Unit = {
+    testLate(lateDEFERRED, _.isDeferred)
+    testLate(lateFINAL, _.isFinal)
+    testLate(lateINTERFACE, _.isInterface)
+    testLate(lateMETHOD, _.isMethod)
+    testLate(lateMODULE, _.isModule)
+    testNot(PROTECTED | notPROTECTED, _.isProtected)
+    testNot(OVERRIDE | notOVERRIDE, _.isOverride)
+    testNot(PRIVATE | notPRIVATE, _.isPrivate)
+
+    assertFalse(withFlagMask(AllFlags)(sym.setFlag(PRIVATE | notPRIVATE).isPrivate))
+
+    assertEquals(withFlagMask(InitialFlags)(sym.setFlag(PRIVATE | notPRIVATE).flags & PRIVATE), PRIVATE)
+    assertEquals(withFlagMask(AllFlags)(sym.setFlag(PRIVATE | notPRIVATE).flags & PRIVATE), 0)
+  }
+
+  @Test
+  def normalLateOverlap(): Unit = {
+    // late flags are shifted by LateShift == 47.
+    // however, the first late flag is lateDEFERRED, which is DEFERRED << 47 == (1 << 4) << 47 == 1 << 51
+    // the flags from 1 << 47 to 1 << 50 are not late flags. this is ensured by the LateFlags mask.
+
+    for (i <- 0 to 3) {
+      val f = 1L << i
+      assertEquals(withFlagMask(AllFlags)(sym.setFlag(f << LateShift).flags & f), 0) // not treated as late flag
+    }
+    for (i <- 4 to 8) {
+      val f = 1L << i
+      assertEquals(withFlagMask(AllFlags)(sym.setFlag(f << LateShift).flags & f), f) // treated as late flag
+    }
+  }
+
+  @Test
+  def normalAnti(): Unit = {
+    for (i <- 0 to 2) {
+      val f = 1L << i
+      assertEquals(withFlagMask(AllFlags)(sym.setFlag(f | (f << AntiShift)).flags & f), 0) // negated flags
+    }
+    for (i <- 3 to 7) {
+      val f = 1L << i
+      assertEquals(withFlagMask(AllFlags)(sym.setFlag(f | (f << AntiShift)).flags & f), f) // not negated
+    }
+  }
+
+  @Test
+  def lateAntiCrossCheck(): Unit = {
+    val allButNegatable = AllFlags & ~(PROTECTED | OVERRIDE | PRIVATE)
+    val lateable        = 0L | DEFERRED | FINAL | INTERFACE | METHOD | MODULE
+    val lateFlags       = lateable << LateShift
+    val allButLateable  = AllFlags & ~lateable
+
+    assertEquals(withFlagMask(AllFlags)(sym.setFlag(AllFlags).flags), allButNegatable)
+    assertEquals(withFlagMask(AllFlags)(sym.setFlag(allButLateable).flags), allButNegatable)
+
+    assertEquals(withFlagMask(AllFlags)(sym.setFlag(lateFlags).flags), lateFlags | lateable)
+  }
+}