Represent Java annotations as interfaces so they can be extended

smarter · smarter · commit 09e39f925b83 · 2022-10-28T17:23:42.000+02:00
Previously we treated Java annotations as if they were classes, like Scala annotations. For example, given @interface Ann { int foo(); } we pretended it was defined as: abstract class Ann(foo: Int) extends java.lang.annotation.Annotation { def foo(): Int } We take advantage of this to type annotation trees as if they were new calls, for example `@Ann(1)` is typed as `new Ann(1)`. Pretending that annotations are classes is fine most of the time and matches what Scala 2.12 did, but it's problematic because the JVM treats annotations as interfaces. In practice this was only an issue with code trying to extend Java annotations, which would either be rejected at compile-time or miscompiled before this commit. This commit switches our representation of annotations to be trait-based instead: trait Ann(foo: Int) extends java.lang.annotation.Annotation { def foo(): Int } Classes are then free to extend annotations using the same pattern as in Scala 2.13: class Foo extends Ann {val annotationType = classOf[Retention]; def foo(): Int = 1} Notice that we still pretend these traits have constructors, this lets us type annotation trees in much the same way as before, and crucially it means that macros that depended on the exact tree shape of annotation trees can continue to work, as demonstrated by the annot-java-tree test extracted from wartremover. To prevent miscompilation issues, we disallow passing arguments to the annotation constructor in `extends` clause. The treatment of default arguments to annotations stays unchanged from 85cd1cf. Fixes #5690. Fixes #12840. Fixes #14199.
diff --git a/compiler/src/dotty/tools/dotc/core/classfile/ClassfileConstants.scala b/compiler/src/dotty/tools/dotc/core/classfile/ClassfileConstants.scala
@@ -353,18 +353,15 @@ object ClassfileConstants {
       if (jflag == 0) base else base | translateFlag(jflag)
 
     private def translateFlags(jflags: Int, baseFlags: FlagSet): FlagSet = {
-      val nflags =
-        if ((jflags & JAVA_ACC_ANNOTATION) == 0) jflags
-        else jflags & ~(JAVA_ACC_ABSTRACT | JAVA_ACC_INTERFACE) // annotations are neither abstract nor interfaces
       var res: FlagSet = baseFlags | JavaDefined
-      res = addFlag(res, nflags & JAVA_ACC_PRIVATE)
-      res = addFlag(res, nflags & JAVA_ACC_PROTECTED)
-      res = addFlag(res, nflags & JAVA_ACC_FINAL)
-      res = addFlag(res, nflags & JAVA_ACC_SYNTHETIC)
-      res = addFlag(res, nflags & JAVA_ACC_STATIC)
-      res = addFlag(res, nflags & JAVA_ACC_ENUM)
-      res = addFlag(res, nflags & JAVA_ACC_ABSTRACT)
-      res = addFlag(res, nflags & JAVA_ACC_INTERFACE)
+      res = addFlag(res, jflags & JAVA_ACC_PRIVATE)
+      res = addFlag(res, jflags & JAVA_ACC_PROTECTED)
+      res = addFlag(res, jflags & JAVA_ACC_FINAL)
+      res = addFlag(res, jflags & JAVA_ACC_SYNTHETIC)
+      res = addFlag(res, jflags & JAVA_ACC_STATIC)
+      res = addFlag(res, jflags & JAVA_ACC_ENUM)
+      res = addFlag(res, jflags & JAVA_ACC_ABSTRACT)
+      res = addFlag(res, jflags & JAVA_ACC_INTERFACE)
       res
     }
 
diff --git a/compiler/src/dotty/tools/dotc/core/classfile/ClassfileParser.scala b/compiler/src/dotty/tools/dotc/core/classfile/ClassfileParser.scala
@@ -165,11 +165,7 @@ class ClassfileParser(
      *  Updates the read pointer of 'in'. */
     def parseParents: List[Type] = {
       val superType =
-        if (isAnnotation) {
-          in.nextChar
-          defn.ObjectType
-        }
-        else if (classRoot.symbol == defn.ComparableClass ||
+        if (classRoot.symbol == defn.ComparableClass ||
                  classRoot.symbol == defn.JavaCloneableClass ||
                  classRoot.symbol == defn.JavaSerializableClass) {
           // Treat these interfaces as universal traits
@@ -844,7 +840,7 @@ class ClassfileParser(
 
   class AnnotConstructorCompleter(classInfo: TempClassInfoType) extends LazyType {
     def complete(denot: SymDenotation)(using Context): Unit = {
-      val attrs = classInfo.decls.toList.filter(sym => sym.isTerm && sym != denot.symbol)
+      val attrs = classInfo.decls.toList.filter(sym => sym.isTerm && sym != denot.symbol && sym.name != nme.CONSTRUCTOR)
       val paramNames = attrs.map(_.name.asTermName)
       val paramTypes = attrs.map(_.info.resultType)
       denot.info = MethodType(paramNames, paramTypes, classRoot.typeRef)
diff --git a/compiler/src/dotty/tools/dotc/parsing/JavaParsers.scala b/compiler/src/dotty/tools/dotc/parsing/JavaParsers.scala
@@ -876,7 +876,7 @@ object JavaParsers {
         List(constructorParams), TypeTree(), EmptyTree).withMods(Modifiers(Flags.JavaDefined))
       val templ = makeTemplate(annotationParents, constr :: body, List(), true)
       val annot = atSpan(start, nameOffset) {
-        TypeDef(name, templ).withMods(mods | Flags.Abstract)
+        TypeDef(name, templ).withMods(mods | Flags.JavaInterface)
       }
       addCompanionObject(statics, annot)
     }
diff --git a/compiler/src/dotty/tools/dotc/typer/Checking.scala b/compiler/src/dotty/tools/dotc/typer/Checking.scala
@@ -1109,6 +1109,8 @@ trait Checking {
   def checkParentCall(call: Tree, caller: ClassSymbol)(using Context): Unit =
     if (!ctx.isAfterTyper) {
       val called = call.tpe.classSymbol
+      if (called.derivesFrom(defn.JavaAnnotationClass))
+        report.error(i"${called.name} must appear without any argument to be a valid class parent because it is a Java annotation", call.srcPos)
       if (caller.is(Trait))
         report.error(i"$caller may not call constructor of $called", call.srcPos)
       else if (called.is(Trait) && !caller.mixins.contains(called))
diff --git a/compiler/src/dotty/tools/dotc/typer/Typer.scala b/compiler/src/dotty/tools/dotc/typer/Typer.scala
@@ -2595,6 +2595,9 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer
    */
   def ensureConstrCall(cls: ClassSymbol, parent: Tree, psym: Symbol)(using Context): Tree =
     if parent.isType && !cls.is(Trait) && !cls.is(JavaDefined) && psym.isClass
+        // Annotations are represented as traits with constructors, but should
+        // never be called as such outside of annotation trees.
+        && !psym.derivesFrom(defn.JavaAnnotationClass)
         && (!psym.is(Trait)
             || psym.primaryConstructor.info.takesParams && !cls.superClass.isSubClass(psym))
     then typed(untpd.New(untpd.TypedSplice(parent), Nil))
diff --git a/tests/neg/i5690.scala b/tests/neg/i5690.scala
diff --git a/tests/neg/java-ann-super-class/Ann.java b/tests/neg/java-ann-super-class/Ann.java
@@ -0,0 +1,3 @@
+public @interface Ann {
+  int value();
+}
diff --git a/tests/neg/java-ann-super-class/Test.scala b/tests/neg/java-ann-super-class/Test.scala
@@ -0,0 +1,9 @@
+class Bar extends Ann(1) { // error
+  def value = 1
+  def annotationType = classOf[Ann]
+}
+
+def test =
+  // Typer errors
+  new Ann // error
+  new Ann(1) {} // error
diff --git a/tests/neg/java-ann-super-class2/Ann.java b/tests/neg/java-ann-super-class2/Ann.java
@@ -0,0 +1,3 @@
+public @interface Ann {
+  int value();
+}
diff --git a/tests/neg/java-ann-super-class2/Test.scala b/tests/neg/java-ann-super-class2/Test.scala
@@ -0,0 +1,3 @@
+def test =
+  // Posttyper errors
+  new Ann(1) // error
diff --git a/tests/neg/java-ann-super-class3/Ann.java b/tests/neg/java-ann-super-class3/Ann.java
@@ -0,0 +1,3 @@
+public @interface Ann {
+  int value();
+}
diff --git a/tests/neg/java-ann-super-class3/Test.scala b/tests/neg/java-ann-super-class3/Test.scala
@@ -0,0 +1,3 @@
+def test =
+  // Refchecks error
+  new Ann {} // error
diff --git a/tests/neg/repeatable/Test_1.scala b/tests/neg/repeatable/Test_1.scala
@@ -6,11 +6,11 @@ import repeatable._
 @FirstLevel_0(Array()) // error
 trait U
 
-@FirstLevel_0(Array(Plain_0(4), Plain_0(5)))
-@FirstLevel_0(Array(Plain_0(6), Plain_0(7)))
+@FirstLevel_0(Array(new Plain_0(4), new Plain_0(5)))
+@FirstLevel_0(Array(new Plain_0(6), new Plain_0(7)))
 @SecondLevel_0(Array()) // error
 trait T
 
 @SecondLevel_0(Array())
 @SecondLevel_0(Array()) // error
-trait S
+trait S
diff --git a/tests/pos/i5690.scala b/tests/pos/i5690.scala
diff --git a/tests/pos/i7467.scala b/tests/pos/i7467.scala
@@ -5,6 +5,6 @@ class DuplicateSymbolError_DirectSuperclass extends DefaultListCellRenderer() {
   override def getListCellRendererComponent(list: JList[_ <: Object], value: Object, index: Int, isSelected: Boolean, cellHasFocus: Boolean): Component = ???
 }
 
-class DuplicateSymbolError_IndirectInterface extends DefaultListCellRenderer() {
-  override def getListCellRendererComponent(list: JList[_], value: Object, index: Int, isSelected: Boolean, cellHasFocus: Boolean): Component = ???
-}
+// class DuplicateSymbolError_IndirectInterface extends DefaultListCellRenderer() {
+//   override def getListCellRendererComponent(list: JList[_], value: Object, index: Int, isSelected: Boolean, cellHasFocus: Boolean): Component = ???
+// }
diff --git a/tests/run-macros/annot-java-tree/AnnoMacro.scala b/tests/run-macros/annot-java-tree/AnnoMacro.scala
@@ -0,0 +1,29 @@
+import scala.quoted.*
+
+inline def checkSuppressWarnings[T]: Unit = ${ checkSuppressWarningsImpl[T] }
+
+def checkSuppressWarningsImpl[T: Type](using Quotes): Expr[Unit] =
+  import quotes.reflect.*
+  val SuppressWarningsSymbol = TypeTree.of[SuppressWarnings].symbol
+  val sym = TypeRepr.of[T].typeSymbol
+  // Imitate what wartremover does, so we can avoid unintentionally breaking it:
+  // https://github.com/wartremover/wartremover/blob/fb18e6eafe9a47823e04960aaf4ec7a9293719ef/core/src/main/scala-3/org/wartremover/WartUniverse.scala#L63-L77
+  val actualArgs = sym
+    .getAnnotation(SuppressWarningsSymbol)
+    .collect {
+      case Apply(
+            Select(_, "<init>"),
+            Apply(Apply(_, Typed(Repeated(values, _), _) :: Nil), Apply(_, _ :: Nil) :: Nil) :: Nil
+          ) =>
+        // "-Yexplicit-nulls"
+        // https://github.com/wartremover/wartremover/issues/660
+        values.collect { case Literal(StringConstant(str)) =>
+          str
+        }
+    }
+    .toList
+    .flatten
+  val expectedArgs = List("a", "b")
+  assert(actualArgs == expectedArgs,
+    s"Expected $expectedArgs arguments for SuppressWarnings annotation of $sym but got $actualArgs")
+  '{}
diff --git a/tests/run-macros/annot-java-tree/S.scala b/tests/run-macros/annot-java-tree/S.scala
@@ -0,0 +1,4 @@
+@SuppressWarnings(Array("a", "b")) class Foo
+
+@main def Test =
+  checkSuppressWarnings[Foo]
diff --git a/tests/run/java-ann-super-class-separate/Ann_1.java b/tests/run/java-ann-super-class-separate/Ann_1.java
@@ -0,0 +1,4 @@
+public @interface Ann_1 {
+  int bar() default 1;
+  int baz() default 2;
+}
diff --git a/tests/run/java-ann-super-class-separate/Test_2.scala b/tests/run/java-ann-super-class-separate/Test_2.scala
@@ -0,0 +1,21 @@
+// scalajs: --skip
+
+class Foo extends Ann_1 {
+  override def bar = 3
+  override def baz = 4
+  def annotationType = classOf[Ann_1]
+}
+
+object Test {
+  def main(args: Array[String]): Unit = {
+    val x = new Foo
+    val y: Ann_1 = x
+    val z: Int @Ann_1(1) = 1
+    val zz: Int @Ann_1() = 1
+    // val x: scala.annotation.Annotation = new Ann {
+    // // val x: java.lang.annotation.Annotation = new Ann {
+    //   def annotationType = classOf[Ann]
+    // }
+    // println(x)
+  }
+}
diff --git a/tests/run/java-ann-super-class/Ann.java b/tests/run/java-ann-super-class/Ann.java
@@ -0,0 +1,4 @@
+public @interface Ann {
+  int bar() default 1;
+  int baz() default 2;
+}
diff --git a/tests/run/java-ann-super-class/Test.scala b/tests/run/java-ann-super-class/Test.scala
@@ -0,0 +1,21 @@
+// scalajs: --skip
+
+class Foo extends Ann {
+  override def bar = 3
+  override def baz = 4
+  def annotationType = classOf[Ann]
+}
+
+object Test {
+  def main(args: Array[String]): Unit = {
+    val x = new Foo
+    val y: Ann = x
+    val z: Int @Ann(1) = 1
+    val zz: Int @Ann() = 1
+    // val x: scala.annotation.Annotation = new Ann {
+    // // val x: java.lang.annotation.Annotation = new Ann {
+    //   def annotationType = classOf[Ann]
+    // }
+    // println(x)
+  }
+}
diff --git a/tests/run/repeatable/Test_1.scala b/tests/run/repeatable/Test_1.scala
@@ -7,16 +7,18 @@ import repeatable._
 @Plain_0(3)
 trait U
 
-@FirstLevel_0(Array(Plain_0(4), Plain_0(5)))
-@FirstLevel_0(Array(Plain_0(6), Plain_0(7)))
+@FirstLevel_0(Array(new Plain_0(4), new Plain_0(5)))
+@FirstLevel_0(Array(new Plain_0(6), new Plain_0(7)))
 trait T
 
-object Test:
-  def main(args: Array[String]) =
+object Test {
+  def main(args: Array[String]) = {
     val annValuesU = classOf[U].getAnnotation(classOf[FirstLevel_0]).value.toList.map(_.value).sorted
     annValuesU.foreach(println)
 
     println()
 
     val annValuesT = classOf[T].getAnnotation(classOf[SecondLevel_0]).value.toList.map(_.value.toList.map(_.value).sorted).sorted
     annValuesT.foreach(println)
+  }
+}
diff --git a/tests/run/t9400.scala b/tests/run/t9400.scala
@@ -0,0 +1,28 @@
+// scalajs: --skip
+
+class Deprecation extends Deprecated {
+  final val annotationType = classOf[Deprecated]
+
+  def forRemoval(): Boolean = false
+  def since(): String = ""
+}
+
+class Suppression extends SuppressWarnings {
+  final val annotationType = classOf[SuppressWarnings]
+
+  def value = Array("unchecked")
+}
+
+class Retention(runtime: Boolean) extends java.lang.annotation.Retention {
+  final val annotationType = classOf[Retention]
+
+  def value =
+    if (runtime) java.lang.annotation.RetentionPolicy.RUNTIME
+    else java.lang.annotation.RetentionPolicy.SOURCE
+}
+
+object Test extends App {
+  new Deprecation
+  new Suppression
+  new Retention(true)
+}

Original file line number	Diff line number	Diff line change
`@@ -876,7 +876,7 @@ object JavaParsers {`
`876`	`876`	`List(constructorParams), TypeTree(), EmptyTree).withMods(Modifiers(Flags.JavaDefined))`
`877`	`877`	`val templ = makeTemplate(annotationParents, constr :: body, List(), true)`
`878`	`878`	`val annot = atSpan(start, nameOffset) {`
`879`		`- TypeDef(name, templ).withMods(mods \| Flags.Abstract)`
	`879`	`+ TypeDef(name, templ).withMods(mods \| Flags.JavaInterface)`
`880`	`880`	`}`
`881`	`881`	`addCompanionObject(statics, annot)`
`882`	`882`	`}`
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+public @interface Ann {`
	`2`	`+ int value();`
	`3`	`+}`
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+def test =`
	`2`	`+ // Posttyper errors`
	`3`	`+ new Ann(1) // error`