handle annotations in classfile parser

fixes sbt#630
SethTisue · May 9, 2022 · 68b4ee8 · 68b4ee8
1 parent c5187ca
commit 68b4ee8
Show file tree

Hide file tree

Showing 15 changed files with 231 additions and 7 deletions.
diff --git a/internal/zinc-apiinfo/src/main/scala/xsbt/api/APIUtil.scala b/internal/zinc-apiinfo/src/main/scala/xsbt/api/APIUtil.scala
@@ -35,6 +35,10 @@ object APIUtil {
     check.hasMacro
   }
 
+  def isAnnotationDeclaration(c: ClassLike): Boolean =
+    c.structure.parents.flatMap(Discovery.simpleName)
+      .contains("java.lang.annotation.Annotation")
+
   private[this] class HasMacro extends Visit {
     var hasMacro = false
 

diff --git a/internal/zinc-classfile/src/main/scala/sbt/internal/inc/classfile/ClassFile.scala b/internal/zinc-classfile/src/main/scala/sbt/internal/inc/classfile/ClassFile.scala
@@ -95,6 +95,8 @@ private[sbt] final case class AttributeInfo(name: Option[String], value: Array[B
   def isNamed(s: String) = name.exists(s == _)
   def isSignature = isNamed("Signature")
   def isSourceFile = isNamed("SourceFile")
+  def isRuntimeVisibleAnnotations = isNamed("RuntimeVisibleAnnotations")
+  def isRuntimeInvisibleAnnotations = isNamed("RuntimeInvisibleAnnotations")
 }
 private[sbt] object Constants {
   final val ACC_STATIC = 0x0008

diff --git a/internal/zinc-classfile/src/main/scala/sbt/internal/inc/classfile/Parser.scala b/internal/zinc-classfile/src/main/scala/sbt/internal/inc/classfile/Parser.scala
@@ -29,6 +29,9 @@ import sbt.util.Logger
 // (BSD license at time of initial port; MIT license as of 2022)
 //
 // Note that unlike the rest of sbt, some things might be null.
+//
+// For debugging this, it's useful to uncomment this in JavaCompilerForUnitTesting:
+// logger.setLevel(sbt.util.Level.Debug)
 
 import Constants._
 
@@ -66,6 +69,7 @@ private[sbt] object Parser {
       val accessFlags: Int = in.readUnsignedShort()
 
       val className = getClassConstantName(in.readUnsignedShort())
+      log.debug(s"[zinc] classfile.Parser parsing $className")
       val superClassName = getClassConstantName(in.readUnsignedShort())
       val interfaceNames =
         array(in.readUnsignedShort())(getClassConstantName(in.readUnsignedShort()))
@@ -110,7 +114,8 @@ private[sbt] object Parser {
         AttributeInfo(name, value)
       }
 
-      def types = (classConstantReferences ++ fieldTypes ++ methodTypes).toSet
+      def types =
+        (classConstantReferences ++ fieldTypes ++ methodTypes ++ annotationsReferences).toSet
 
       private def getTypes(fieldsOrMethods: Array[FieldOrMethodInfo]) =
         fieldsOrMethods.flatMap { fieldOrMethod =>
@@ -120,6 +125,52 @@ private[sbt] object Parser {
       private def fieldTypes = getTypes(fields)
       private def methodTypes = getTypes(methods)
 
+      private def annotationsReferences: List[String] = {
+        // TODO do I actually need to look inside fields and methods here?
+        // I guess I must because the jdepend code does.
+        // but I should back it up with a test
+        val allAttributes =
+          attributes ++
+            fields.flatMap(_.attributes) ++
+            methods.flatMap(_.attributes)
+        allAttributes
+          .filter(x => x.isRuntimeVisibleAnnotations || x.isRuntimeInvisibleAnnotations)
+          .flatMap { attr =>
+            val in = new DataInputStream(new java.io.ByteArrayInputStream(attr.value))
+            val numAnnotations = in.readUnsignedShort()
+            val result = collection.mutable.ListBuffer[String]()
+            def parseElementValue(): Unit = {  // JVMS 4.7.16.1
+              val c = in.readUnsignedByte().toChar
+              c match {
+                case 'e' =>
+                  result += slashesToDots(toUTF8(in.readUnsignedShort()))
+                  val _ = in.readUnsignedShort()
+                case 'c' =>
+                  result += slashesToDots(toUTF8(in.readUnsignedShort()))
+                case '@' =>
+                  parseAnnotation()
+                case '[' =>
+                  for (_ <- 0 until in.readUnsignedShort())
+                    parseElementValue()
+                case cc =>
+                  throw new IllegalStateException(cc.toString)
+              }
+            }
+            def parseAnnotation(): Unit = {  // JVMS 4.7.16
+              result += slashesToDots(toUTF8(in.readUnsignedShort()))
+              for (_ <- 0 until in.readUnsignedShort()) {
+                in.readUnsignedShort() // skip element name index
+                parseElementValue()
+              }
+            }
+            for (_ <- 0 until numAnnotations)
+              parseAnnotation()
+            // the type names in the constant pool have the form e.g. `Ljava/lang/Object;`;
+            // we've already used `slashesToDots`, but we still need to drop the `L` and the semicolon
+            result.map(name => name.slice(1, name.length - 1))
+          }.toList
+      }
+
       private def classConstantReferences =
         constants.flatMap { constant =>
           constant.tag match {
@@ -200,11 +251,15 @@ private[sbt] object Parser {
   }
 
   private def toInt(v: Byte) = if (v < 0) v + 256 else v.toInt
-  def entryIndex(a: AttributeInfo) = {
-    require(a.value.length == 2, s"Expected two bytes for unsigned value; got: ${a.value.length}")
-    val Array(v0, v1) = a.value
-    toInt(v0) * 256 + toInt(v1)
-  }
+  private def u2(highByte: Byte, lowByte: Byte): Int =
+    toInt(highByte) * 256 + toInt(lowByte)
+  def entryIndex(a: AttributeInfo) =
+    a.value match {
+      case Array(v0, v1) =>
+        u2(v0, v1)
+      case _ =>
+        sys.error(s"Expected two bytes for unsigned value; got: ${a.value.length}")
+    }
 
   private def slashesToDots(s: String) = s.replace('/', '.')
 

diff --git a/internal/zinc-classfile/src/test/scala/sbt/internal/inc/classfile/AnalyzeSpecification.scala b/internal/zinc-classfile/src/test/scala/sbt/internal/inc/classfile/AnalyzeSpecification.scala
@@ -46,4 +46,104 @@ class AnalyzeSpecification extends UnitSpec {
     assert(deps.memberRef("D") === Set.empty)
   }
 
+  "Analyze" should "process runtime-visible annotations" in {
+    val srcTest =
+      """|import java.lang.annotation.Retention;
+         |import java.lang.annotation.RetentionPolicy;
+         |@Retention(RetentionPolicy.RUNTIME)
+         |public @interface Test { }
+         |""".stripMargin
+    val srcFoo =
+      """|@Test
+         |public class Foo {
+         |  public static void main(String[] args){
+         |    System.out.println(Foo.class.getAnnotations().length);
+         |  }
+         |}
+         |""".stripMargin
+    val deps = JavaCompilerForUnitTesting.extractDependenciesFromSrcs(
+      "Test.java" -> srcTest,
+      "Foo.java" -> srcFoo,
+    )
+    assert(deps.memberRef("Foo").contains("Test"))
+  }
+
+  "Analyze" should "process annotation with array argument" in {
+    val srcTest =
+      """|import java.lang.annotation.Retention;
+         |import java.lang.annotation.RetentionPolicy;
+         |import java.lang.annotation.ElementType;
+         |@Retention(RetentionPolicy.RUNTIME)
+         |public @interface Test {
+         |  ElementType[] value();
+         |}
+         |""".stripMargin
+    val srcFoo =
+      """|import java.lang.annotation.ElementType;
+         |@Test(ElementType.TYPE)
+         |public class Foo {
+         |  public static void main(String[] args){
+         |    System.out.println(Foo.class.getAnnotations().length);
+         |  }
+         |}
+         |""".stripMargin
+    val deps = JavaCompilerForUnitTesting.extractDependenciesFromSrcs(
+      "Test.java" -> srcTest,
+      "Foo.java" -> srcFoo,
+    )
+    assert(deps.memberRef("Foo").contains("Test"))
+  }
+
+  "Analyze" should "detect annotation in array argument to annotation" in {
+    val srcTest1 =
+      """|import java.lang.annotation.Retention;
+         |import java.lang.annotation.RetentionPolicy;
+         |@Retention(RetentionPolicy.RUNTIME)
+         |public @interface Test1 { }
+         |""".stripMargin
+    val srcTest2 =
+      """|import java.lang.annotation.Retention;
+         |import java.lang.annotation.RetentionPolicy;
+         |@Retention(RetentionPolicy.RUNTIME)
+         |public @interface Test2 {
+         |  Test1[] value();
+         |}
+         |""".stripMargin
+    val srcFoo =
+      """|import java.lang.annotation.ElementType;
+         |@Test2(@Test1)
+         |public class Foo {
+         |  public static void main(String[] args){
+         |    System.out.println(Foo.class.getAnnotations().length);
+         |  }
+         |}
+         |""".stripMargin
+    val deps = JavaCompilerForUnitTesting.extractDependenciesFromSrcs(
+      "Test1.java" -> srcTest1,
+      "Test2.java" -> srcTest2,
+      "Foo.java" -> srcFoo,
+    )
+    assert(deps.memberRef("Foo").contains("Test1"))
+    assert(deps.memberRef("Foo").contains("Test2"))
+  }
+
+  "Analyze" should "process runtime-invisible annotations" in {
+    val srcTest =
+      """|public @interface Test { }
+         |""".stripMargin
+    val srcFoo =
+      """|@Test
+         |public class Foo {
+         |  public static void main(String[] args){
+         |    System.out.println(Foo.class.getAnnotations().length);
+         |  }
+         |}
+         |""".stripMargin
+    val deps = JavaCompilerForUnitTesting.extractDependenciesFromSrcs(
+      "Test.java" -> srcTest,
+      "Foo.java" -> srcFoo,
+    )
+    assert(deps.memberRef("Foo").contains("Test"))
+  }
+
 }
diff --git a/internal/zinc-core/src/main/scala/sbt/internal/inc/Incremental.scala b/internal/zinc-core/src/main/scala/sbt/internal/inc/Incremental.scala
@@ -652,6 +652,8 @@ private final class AnalysisCallback(
   private[this] val binaryClassName = new TrieMap[VirtualFile, String]
   // source files containing a macro def.
   private[this] val macroClasses = ConcurrentHashMap.newKeySet[String]()
+  // source files containing a Java annotation declaration
+  private[this] val annotationClasses = ConcurrentHashMap.newKeySet[String]()
 
   // Results of invalidation calculations (including whether to continue cycles) - the analysis at this point is
   // not useful and so isn't included.
@@ -839,6 +841,9 @@ private final class AnalysisCallback(
     val className = classApi.name
     if (APIUtil.isScalaSourceName(sourceFile.id) && APIUtil.hasMacro(classApi))
       macroClasses.add(className)
+    // sbt/zinc#630
+    if (!APIUtil.isScalaSourceName(sourceFile.id) && APIUtil.isAnnotationDeclaration(classApi))
+      annotationClasses.add(className)
     val shouldMinimize = !Incremental.apiDebug(options)
     val savedClassApi = if (shouldMinimize) APIUtil.minimize(classApi) else classApi
     val apiHash: HashAPI.Hash = HashAPI(classApi)
@@ -951,7 +956,8 @@ private final class AnalysisCallback(
   }
 
   private def analyzeClass(name: String): AnalyzedClass = {
-    val hasMacro: Boolean = macroClasses.contains(name)
+    // TODO temporary -- it's a kludge to overload `hasMacro` to also cover annotation declarations
+    val hasMacro: Boolean = macroClasses.contains(name) || annotationClasses.contains(name)
     val (companions, apiHash, extraHash) = companionsWithHash(name)
     val nameHashes = nameHashesForCompanions(name)
     val safeCompanions = SafeLazyProxy(companions)

diff --git a/zinc/src/sbt-test/source-dependencies/annotations-in-java-sources-a/Foo.java b/zinc/src/sbt-test/source-dependencies/annotations-in-java-sources-a/Foo.java
@@ -0,0 +1,3 @@
+package test;
+@Test
+public class Foo { }
diff --git a/zinc/src/sbt-test/source-dependencies/annotations-in-java-sources-a/Test.java b/zinc/src/sbt-test/source-dependencies/annotations-in-java-sources-a/Test.java
@@ -0,0 +1,3 @@
+package test;
+public @interface Test {
+}
diff --git a/zinc/src/sbt-test/source-dependencies/annotations-in-java-sources-a/changes/Test1.java b/zinc/src/sbt-test/source-dependencies/annotations-in-java-sources-a/changes/Test1.java
@@ -0,0 +1,4 @@
+package test;
+public @interface Test {
+  int x() default 0;
+}
diff --git a/zinc/src/sbt-test/source-dependencies/annotations-in-java-sources-a/incOptions.properties b/zinc/src/sbt-test/source-dependencies/annotations-in-java-sources-a/incOptions.properties
@@ -0,0 +1,2 @@
+# turn off pipelining because compileAllJava over-compiles
+pipelining = false
diff --git a/zinc/src/sbt-test/source-dependencies/annotations-in-java-sources-a/test b/zinc/src/sbt-test/source-dependencies/annotations-in-java-sources-a/test
@@ -0,0 +1,11 @@
+# Verifies that changes to annotations in upstream Java sources causes
+# recompilation downstream (sbt/zinc#630)
+
+> compile
+> checkDependencies test.Foo: test.Test
+> checkRecompilations 0 test.Test test.Foo
+
+$ copy-file changes/Test1.java Test.java
+> compile
+> checkRecompilations 1 test.Test
+> checkRecompilations 2 test.Foo
diff --git a/zinc/src/sbt-test/source-dependencies/annotations-in-java-sources-b/Foo.java b/zinc/src/sbt-test/source-dependencies/annotations-in-java-sources-b/Foo.java
@@ -0,0 +1,7 @@
+package test;
+@Test
+public class Foo {
+  public static void main(String[] args){
+    System.out.println(Foo.class.getAnnotations().length);
+  }
+}
diff --git a/zinc/src/sbt-test/source-dependencies/annotations-in-java-sources-b/Test.java b/zinc/src/sbt-test/source-dependencies/annotations-in-java-sources-b/Test.java
@@ -0,0 +1,7 @@
+package test;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+
+//@Retention(RetentionPolicy.RUNTIME)
+public @interface Test {
+}
diff --git a/zinc/src/sbt-test/source-dependencies/annotations-in-java-sources-b/changes/Test1.java b/zinc/src/sbt-test/source-dependencies/annotations-in-java-sources-b/changes/Test1.java
@@ -0,0 +1,7 @@
+package test;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+
+@Retention(RetentionPolicy.RUNTIME)
+public @interface Test {
+}
diff --git a/zinc/src/sbt-test/source-dependencies/annotations-in-java-sources-b/incOptions.properties b/zinc/src/sbt-test/source-dependencies/annotations-in-java-sources-b/incOptions.properties
@@ -0,0 +1,2 @@
+# turn off pipelining because compileAllJava over-compiles
+pipelining = false
diff --git a/zinc/src/sbt-test/source-dependencies/annotations-in-java-sources-b/test b/zinc/src/sbt-test/source-dependencies/annotations-in-java-sources-b/test
@@ -0,0 +1,11 @@
+# Verifies that changes to annotations in upstream Java sources causes
+# recompilation downstream (sbt/zinc#630)
+
+> compile
+> checkDependencies test.Foo: test.Test
+> checkRecompilations 0 test.Test test.Foo
+
+$ copy-file changes/Test1.java Test.java
+> compile
+> checkRecompilations 1 test.Test
+> checkRecompilations 2 test.Foo