New IC: Detect changes to class annotations

Both the new and old incremental compilation (IC) analysis rely on Kotlin class metadata to detect a change. However, Kotlin metadata currently doesn't contain info about annotations (KT-57919), so the IC will not be able to detect a change to them. With this commit, we'll fix the new IC such that it can detect a change to class annotations by not relying only on metadata. We currently scope this fix to the new IC (cross-module analysis) first. We'll fix this issue for within-module analysis later. Performance: There seems to be no performance impact from this change. Snapshotting the 400MB ideaIC-2022.1.4/app.jar takes 4.1s before and after this change. Test: Added ClasspathChangesComputerTest.testChangedAnnotations ^KT-58289: Fixed
2023-04-26 14:47:57 +01:00
parent 993925f656
commit 0b09be73c6
22 changed files with 316 additions and 231 deletions
@@ -34,17 +34,13 @@ class ClassFileWithContentsProvider(
    val classFile: ClassFile,
    val contentsProvider: () -> ByteArray
 ) {
-
-    fun loadContents(): ClassFileWithContents {
-        val classContents = contentsProvider.invoke()
-        val classInfo = BasicClassInfo.compute(classContents)
-        return ClassFileWithContents(classFile, classContents, classInfo)
-    }
+    fun loadContents() = ClassFileWithContents(classFile, contentsProvider.invoke())
 }

 /** Information about the location of a .class file ([ClassFile]) and its contents. */
 class ClassFileWithContents(
    @Suppress("unused") val classFile: ClassFile,
-    val contents: ByteArray,
-    val classInfo: BasicClassInfo
-)
+    val contents: ByteArray
+) {
+    val classInfo: BasicClassInfo = BasicClassInfo.compute(contents)
+}
@@ -246,6 +246,22 @@ object ClasspathChangesComputer {
        //     classes, and symbols in removed classes.
        incrementalJvmCache.clearCacheForRemovedClasses(changesCollector)

+        // IncrementalJvmCache currently doesn't use the `KotlinClassInfo.extraInfo.classSnapshotExcludingMembers` info when comparing
+        // classes, so we need to do it here.
+        // TODO(KT-58289): Ensure IncrementalJvmCache uses that info when comparing classes.
+        val currentClassSnapshotsExcludingMembers = currentClassSnapshots
+            .associate { it.classId to it.classMemberLevelSnapshot!!.extraInfo.classSnapshotExcludingMembers }
+            .filter { it.value != null }
+        val previousClassSnapshotsExcludingMembers = previousClassSnapshots
+            .associate { it.classId to it.classMemberLevelSnapshot!!.extraInfo.classSnapshotExcludingMembers }
+            .filter { it.value != null }
+        previousClassSnapshotsExcludingMembers.keys.intersect(currentClassSnapshotsExcludingMembers.keys).forEach {
+            if (previousClassSnapshotsExcludingMembers[it]!! != currentClassSnapshotsExcludingMembers[it]!!) {
+                // `areSubclassesAffected = false` as we don't need to compute impacted symbols at this step
+                changesCollector.collectSignature(fqName = it.asSingleFqName(), areSubclassesAffected = false)
+            }
+        }
+
        // Get the changes and clean up
        val dirtyData = changesCollector.getChangedSymbols(DoNothingICReporter)
        workingDir.deleteRecursively()
@@ -8,7 +8,6 @@ package org.jetbrains.kotlin.incremental.classpathDiff
 import com.intellij.util.containers.Interner
 import com.intellij.util.io.DataExternalizer
 import org.jetbrains.kotlin.build.report.metrics.BuildPerformanceMetric
-import org.jetbrains.kotlin.incremental.ConstantValueExternalizer
 import org.jetbrains.kotlin.incremental.KotlinClassInfo
 import org.jetbrains.kotlin.incremental.storage.*
 import org.jetbrains.kotlin.load.kotlin.header.KotlinClassHeader
@@ -162,8 +161,7 @@ internal object KotlinClassInfoExternalizer : DataExternalizer<KotlinClassInfo>
        ListExternalizer(StringExternalizer).save(output, info.classHeaderData.toList())
        ListExternalizer(StringExternalizer).save(output, info.classHeaderStrings.toList())
        NullableValueExternalizer(StringExternalizer).save(output, info.multifileClassName)
-        MapExternalizer(StringExternalizer, ConstantValueExternalizer).save(output, info.constantsMap)
-        MapExternalizer(InlineFunctionOrAccessorExternalizer, LongExternalizer).save(output, info.inlineFunctionsAndAccessorsMap)
+        ExtraInfoExternalizer.save(output, info.extraInfo)
    }

    override fun read(input: DataInput): KotlinClassInfo {
@@ -174,8 +172,24 @@ internal object KotlinClassInfoExternalizer : DataExternalizer<KotlinClassInfo>
            classHeaderData = ListExternalizer(StringExternalizer).read(input).toTypedArray(),
            classHeaderStrings = ListExternalizer(StringExternalizer).read(input).toTypedArray(),
            multifileClassName = NullableValueExternalizer(StringExternalizer).read(input),
-            constantsMap = MapExternalizer(StringExternalizer, ConstantValueExternalizer).read(input),
-            inlineFunctionsAndAccessorsMap = MapExternalizer(InlineFunctionOrAccessorExternalizer, LongExternalizer).read(input)
+            extraInfo = ExtraInfoExternalizer.read(input)
+        )
+    }
+}
+
+internal object ExtraInfoExternalizer : DataExternalizer<KotlinClassInfo.ExtraInfo> {
+
+    override fun save(output: DataOutput, info: KotlinClassInfo.ExtraInfo) {
+        NullableValueExternalizer(LongExternalizer).save(output, info.classSnapshotExcludingMembers)
+        MapExternalizer(StringExternalizer, LongExternalizer).save(output, info.constantSnapshots)
+        MapExternalizer(InlineFunctionOrAccessorExternalizer, LongExternalizer).save(output, info.inlineFunctionOrAccessorSnapshots)
+    }
+
+    override fun read(input: DataInput): KotlinClassInfo.ExtraInfo {
+        return KotlinClassInfo.ExtraInfo(
+            classSnapshotExcludingMembers = NullableValueExternalizer(LongExternalizer).read(input),
+            constantSnapshots = MapExternalizer(StringExternalizer, LongExternalizer).read(input),
+            inlineFunctionOrAccessorSnapshots = MapExternalizer(InlineFunctionOrAccessorExternalizer, LongExternalizer).read(input)
        )
    }
 }
@@ -200,7 +214,7 @@ private object JavaClassSnapshotExternalizer : DataExternalizer<JavaClassSnapsho
    }
 }

-private object JavaClassMemberLevelSnapshotExternalizer : DataExternalizer<JavaClassMemberLevelSnapshot> {
+internal object JavaClassMemberLevelSnapshotExternalizer : DataExternalizer<JavaClassMemberLevelSnapshot> {

    override fun save(output: DataOutput, snapshot: JavaClassMemberLevelSnapshot) {
        JavaElementSnapshotExternalizer.save(output, snapshot.classAbiExcludingMembers)
@@ -9,15 +9,23 @@ import org.jetbrains.kotlin.build.report.metrics.BuildMetricsReporter
 import org.jetbrains.kotlin.build.report.metrics.BuildTime
 import org.jetbrains.kotlin.build.report.metrics.DoNothingBuildMetricsReporter
 import org.jetbrains.kotlin.build.report.metrics.measure
+import org.jetbrains.kotlin.incremental.ClassNodeSnapshotter.snapshotClass
+import org.jetbrains.kotlin.incremental.ClassNodeSnapshotter.snapshotClassExcludingMembers
+import org.jetbrains.kotlin.incremental.ClassNodeSnapshotter.snapshotField
+import org.jetbrains.kotlin.incremental.ClassNodeSnapshotter.snapshotMethod
+import org.jetbrains.kotlin.incremental.ClassNodeSnapshotter.sortClassMembers
 import org.jetbrains.kotlin.incremental.DifferenceCalculatorForPackageFacade.Companion.getNonPrivateMembers
 import org.jetbrains.kotlin.incremental.KotlinClassInfo
 import org.jetbrains.kotlin.incremental.PackagePartProtoData
 import org.jetbrains.kotlin.incremental.classpathDiff.ClassSnapshotGranularity.CLASS_MEMBER_LEVEL
-import org.jetbrains.kotlin.incremental.md5
+import org.jetbrains.kotlin.incremental.hashToLong
 import org.jetbrains.kotlin.incremental.storage.toByteArray
 import org.jetbrains.kotlin.konan.file.use
 import org.jetbrains.kotlin.load.kotlin.header.KotlinClassHeader.Kind.*
 import org.jetbrains.kotlin.resolve.jvm.JvmClassName
+import org.jetbrains.org.objectweb.asm.ClassReader
+import org.jetbrains.org.objectweb.asm.Opcodes
+import org.jetbrains.org.objectweb.asm.tree.ClassNode
 import java.io.Closeable
 import java.io.File
 import java.util.zip.ZipEntry
@@ -94,7 +102,7 @@ object ClassSnapshotter {
                        snapshotKotlinClass(clazz, granularity)
                    }
                    else -> metrics.measure(BuildTime.SNAPSHOT_JAVA_CLASSES) {
-                        JavaClassSnapshotter.snapshot(clazz, granularity)
+                        snapshotJavaClass(clazz, granularity)
                    }
                }
            }
@@ -120,11 +128,7 @@ object ClassSnapshotter {
        }
    }

-    /**
-     * Computes a [KotlinClassSnapshot] of the given Kotlin class.
-     *
-     * (The caller must ensure that the given class is a Kotlin class.)
-     */
+    /** Computes a [KotlinClassSnapshot] of the given Kotlin class. */
    private fun snapshotKotlinClass(classFile: ClassFileWithContents, granularity: ClassSnapshotGranularity): KotlinClassSnapshot {
        val kotlinClassInfo =
            KotlinClassInfo.createFrom(classFile.classInfo.classId, classFile.classInfo.kotlinClassHeader!!, classFile.contents)
@@ -145,13 +149,60 @@ object ClassSnapshotter {
            )
            MULTIFILE_CLASS -> MultifileClassKotlinClassSnapshot(
                classId, classAbiHash, classMemberLevelSnapshot,
-                constantNames = kotlinClassInfo.constantsMap.keys
+                constantNames = kotlinClassInfo.extraInfo.constantSnapshots.keys
            )
            SYNTHETIC_CLASS -> error("Unexpected class $classId with class kind ${SYNTHETIC_CLASS.name} (synthetic classes should have been removed earlier)")
            UNKNOWN -> error("Can't handle class $classId with class kind ${UNKNOWN.name}")
        }
    }

+    /** Computes a [JavaClassSnapshot] of the given Java class. */
+    private fun snapshotJavaClass(classFile: ClassFileWithContents, granularity: ClassSnapshotGranularity): JavaClassSnapshot {
+        // For incremental compilation, we only care about the ABI info of a class. There are 2 approaches:
+        //   1. Collect ABI info directly
+        //   2. Remove non-ABI info from the full class
+        // Note that for incremental compilation to be correct, all ABI info must be collected exhaustively (now and in the future when
+        // there are updates to Java/ASM), whereas it is acceptable if non-ABI info is not removed completely.
+        // In the following, we will use the second approach as it is safer and easier.
+
+        val classNode = ClassNode()
+        val classReader = ClassReader(classFile.contents)
+
+        // Note the `parsingOptions` passed to `classReader`:
+        //   - Pass SKIP_CODE as method bodies are not important
+        //   - Do not pass SKIP_DEBUG as debug info (e.g., method parameter names) may be important
+        classReader.accept(classNode, ClassReader.SKIP_CODE)
+        sortClassMembers(classNode)
+
+        // Remove private fields and methods
+        fun Int.isPrivate() = (this and Opcodes.ACC_PRIVATE) != 0
+        classNode.fields.removeIf { it.access.isPrivate() }
+        classNode.methods.removeIf { it.access.isPrivate() }
+
+        // Snapshot the class
+        val classMemberLevelSnapshot = if (granularity == CLASS_MEMBER_LEVEL) {
+            JavaClassMemberLevelSnapshot(
+                classAbiExcludingMembers = JavaElementSnapshot(classNode.name, snapshotClassExcludingMembers(classNode)),
+                fieldsAbi = classNode.fields.map { JavaElementSnapshot(it.name, snapshotField(it)) },
+                methodsAbi = classNode.methods.map { JavaElementSnapshot(it.name, snapshotMethod(it, classNode.version)) }
+            )
+        } else {
+            null
+        }
+        val classAbiHash = if (granularity == CLASS_MEMBER_LEVEL) {
+            JavaClassMemberLevelSnapshotExternalizer.toByteArray(classMemberLevelSnapshot!!).hashToLong()
+        } else {
+            snapshotClass(classNode)
+        }
+
+        return JavaClassSnapshot(
+            classId = classFile.classInfo.classId,
+            classAbiHash = classAbiHash,
+            classMemberLevelSnapshot = classMemberLevelSnapshot,
+            supertypes = classFile.classInfo.supertypes
+        )
+    }
+
 }

 private sealed interface DirectoryOrJarReader : Closeable {
@@ -227,9 +278,3 @@ private class JarReader(jar: File) : DirectoryOrJarReader {
        zipFile.close()
    }
 }
-
-internal fun ByteArray.hashToLong(): Long {
-    // Note: The returned type `Long` is 64-bit, but we currently don't have a good 64-bit hash function.
-    // The method below uses `md5` which is 128-bit and converts it to `Long`.
-    return md5()
-}
@@ -1,95 +0,0 @@
-/*
- * Copyright 2010-2021 JetBrains s.r.o. and Kotlin Programming Language contributors.
- * Use of this source code is governed by the Apache 2.0 license that can be found in the license/LICENSE.txt file.
- */
-
-package org.jetbrains.kotlin.incremental.classpathDiff
-
-import org.jetbrains.kotlin.incremental.classpathDiff.ClassSnapshotGranularity.CLASS_MEMBER_LEVEL
-import org.jetbrains.org.objectweb.asm.ClassReader
-import org.jetbrains.org.objectweb.asm.ClassWriter
-import org.jetbrains.org.objectweb.asm.Opcodes
-import org.jetbrains.org.objectweb.asm.tree.ClassNode
-import org.jetbrains.org.objectweb.asm.tree.FieldNode
-import org.jetbrains.org.objectweb.asm.tree.MethodNode
-
-/** Computes a [JavaClassSnapshot] of a Java class. */
-object JavaClassSnapshotter {
-
-    fun snapshot(classFile: ClassFileWithContents, granularity: ClassSnapshotGranularity): JavaClassSnapshot {
-        // We will extract ABI information from the given class and store it into the `abiClass` variable.
-        // It is acceptable to collect more info than required, but it is incorrect to collect less info than required.
-        // There are 2 approaches:
-        //   1. Collect ABI info directly. The collected info must be exhaustive (now and in the future when there are updates to Java/ASM).
-        //   2. Collect all info and remove non-ABI info. The removed info should be exhaustive, but even if it's not, it is still
-        //      acceptable.
-        // In the following, we will use the second approach as it is safer.
-        val abiClass = ClassNode()
-
-        // First, collect all info.
-        // Note the parsing options passed to ClassReader:
-        //   - SKIP_CODE is set as method bodies will not be part of the ABI of the class.
-        //   - SKIP_DEBUG seems possible but is *not* set just to be safe, so that we don't skip any info that might be important.
-        //   - SKIP_FRAMES or EXPAND_FRAMES is not needed (and not relevant when SKIP_CODE is set).
-        val classReader = ClassReader(classFile.contents)
-        classReader.accept(abiClass, ClassReader.SKIP_CODE)
-
-        // Then, remove non-ABI info, which includes:
-        //   - Method bodies: Should have already been removed (see SKIP_CODE above)
-        //   - Private fields and methods
-        fun Int.isPrivate() = (this and Opcodes.ACC_PRIVATE) != 0
-        abiClass.fields.removeIf { it.access.isPrivate() }
-        abiClass.methods.removeIf { it.access.isPrivate() }
-
-        // Normalize the class: Sort fields and methods as their order is not important (we still use List instead of Set as we want the
-        // serialized snapshot to be deterministic).
-        abiClass.fields.sortWith(compareBy({ it.name }, { it.desc }))
-        abiClass.methods.sortWith(compareBy({ it.name }, { it.desc }))
-
-        // Snapshot the class
-        val classAbiHash = snapshotClass(abiClass)
-        val classMemberLevelSnapshot = if (granularity == CLASS_MEMBER_LEVEL) {
-            val fieldsAbi = abiClass.fields.map { JavaElementSnapshot(it.name, snapshotField(it)) }
-            val methodsAbi = abiClass.methods.map { JavaElementSnapshot(it.name, snapshotMethod(it)) }
-            val classAbiExcludingMembers = abiClass.let {
-                it.fields.clear()
-                it.methods.clear()
-                JavaElementSnapshot(it.name, snapshotClass(it))
-            }
-            JavaClassMemberLevelSnapshot(classAbiExcludingMembers, fieldsAbi, methodsAbi)
-        } else null
-
-        return JavaClassSnapshot(
-            classId = classFile.classInfo.classId,
-            classAbiHash = classAbiHash,
-            classMemberLevelSnapshot = classMemberLevelSnapshot,
-            supertypes = classFile.classInfo.supertypes
-        )
-    }
-
-    private fun snapshotClass(classNode: ClassNode): Long {
-        val classWriter = ClassWriter(0)
-        classNode.accept(classWriter)
-        return classWriter.toByteArray().hashToLong()
-    }
-
-    private fun snapshotField(fieldNode: FieldNode): Long {
-        val classNode = emptyClass()
-        classNode.fields.add(fieldNode)
-        return snapshotClass(classNode)
-    }
-
-    private fun snapshotMethod(methodNode: MethodNode): Long {
-        val classNode = emptyClass()
-        classNode.methods.add(methodNode)
-        return snapshotClass(classNode)
-    }
-
-    private fun emptyClass() = ClassNode().also {
-        // We need to provide some minimal info to the class:
-        //   - Name is required.
-        //   - Class version is required if method bodies are considered, but we have removed method bodies in this class, so it's optional.
-        //   - Other info is optional.
-        it.name = "EmptyClass"
-    }
-}
@@ -259,6 +259,9 @@ class KotlinOnlyClasspathChangesComputerTest : ClasspathChangesComputerTest() {
    /** Regression test for KT-55021. */
    @Test
    fun testRenameFileFacade() {
+        // Check that classpath changes computation doesn't fail.
+        // Ideally, the returned changes should be empty (renaming a file facade alone shouldn't cause any `LookupSymbol`s to change), but
+        // it is currently not the case. However, this is just a small efficiency, not a serious bug.
        val changes = computeClasspathChanges(File(testDataDir, "KotlinOnly/testRenameFileFacade/src"), tmpDir)
        Changes(
            lookupSymbols = setOf(
@@ -269,6 +272,20 @@ class KotlinOnlyClasspathChangesComputerTest : ClasspathChangesComputerTest() {
        ).assertEquals(changes)
    }

+    /** Regression test for KT-58289.*/
+    @Test
+    fun testChangedAnnotations() {
+        val changes = computeClasspathChanges(File(testDataDir, "KotlinOnly/testChangedAnnotations/src"), tmpDir)
+        Changes(
+            lookupSymbols = setOf(
+                LookupSymbol(name = "SomeClassWithChangedAnnotation", scope = "com.example"),
+            ),
+            fqNames = setOf(
+                "com.example.SomeClassWithChangedAnnotation",
+            )
+        ).assertEquals(changes)
+    }
+
    /** Tests [SupertypesInheritorsImpact]. */
    @Test
    override fun testImpactComputation_SupertypesInheritors() {
@@ -0,0 +1,20 @@
+package com.example
+
+@AnnotationTwo // Changed from @AnnotationOne
+class SomeClassWithChangedAnnotation {
+    val unchangeProperty = 0
+    fun unchangedFunction() {}
+}
+
+class SomeClass {
+
+    @AnnotationTwo // Changed from @AnnotationOne
+    val propertyWithChangedAnnotation = 0
+
+    @AnnotationTwo // Changed from @AnnotationOne
+    fun functionWithChangedAnnotation() {
+    }
+}
+
+annotation class AnnotationOne
+annotation class AnnotationTwo
@@ -0,0 +1,20 @@
+package com.example
+
+@AnnotationOne // Will change to @AnnotationTwo
+class SomeClassWithChangedAnnotation {
+    val unchangeProperty = 0
+    fun unchangedFunction() {}
+}
+
+class SomeClass {
+
+    @AnnotationOne // Will change to @AnnotationTwo
+    val propertyWithChangedAnnotation = 0
+
+    @AnnotationOne // Will change to @AnnotationTwo
+    fun functionWithChangedAnnotation() {
+    }
+}
+
+annotation class AnnotationOne
+annotation class AnnotationTwo
@@ -12,7 +12,7 @@
    },
    "local": false
  },
-  "classAbiHash": -6515999856905133685,
+  "classAbiHash": -6381547343043280587,
  "classMemberLevelSnapshot": {
    "classAbiExcludingMembers": {
      "name": "com/example/SimpleClass",
@@ -27,11 +27,11 @@
    "methodsAbi": [
      {
        "name": "\u003cinit\u003e",
-        "abiHash": 2413123887428571534
+        "abiHash": 9200648777950343158
      },
      {
        "name": "publicMethod",
-        "abiHash": 7574889098198027162
+        "abiHash": -2847179652577921235
      }
    ]
  },
@@ -12,7 +12,7 @@
    },
    "local": false
  },
-  "classAbiHash": -2605231127310346403,
+  "classAbiHash": 8283317449409255124,
  "supertypes": [
    {
      "internalName": "java/lang/Object"
@@ -12,7 +12,7 @@
    },
    "local": false
  },
-  "classAbiHash": -2605231127310346403,
+  "classAbiHash": 8283317449409255124,
  "classMemberLevelSnapshot": {
    "classId": {
      "packageFqName": {
@@ -43,8 +43,11 @@
      "privateFunction",
      "publicFunction"
    ],
-    "constantsMap": {},
-    "inlineFunctionsAndAccessorsMap": {},
+    "extraInfo": {
+      "classSnapshotExcludingMembers": -3712984790086353322,
+      "constantSnapshots": {},
+      "inlineFunctionOrAccessorSnapshots": {}
+    },
    "className$delegate": {
      "initializer": {},
      "_value": {}