KT-45777: Handle problematic classes when taking snapshots

There are certain classes that we are not yet able to take snapshots of, either because the class is faulty, or there is a bug in our code. For these classes, we will use a special snapshot and fall back to the existing approach to compute classpath changes. In the short run, we will update this list of cases as they arise. In the long run, we will: - Fix all cases that are caused by bugs in our code. - Decide what to with the faulty jars in general and remove the list. Test: The following should pass - Step 1: ./gradlew publish - Step 2: ./gradlew publish -Pbootstrap.local=true -Pbootstrap.local.path=/path/to/kotlin/build/repo -Pkotlin.incremental.useClasspathSnapshot=true
2021-09-13 10:29:23 +01:00
parent fb875c484d
commit f400305cc6
9 changed files with 151 additions and 16 deletions
@@ -50,6 +50,7 @@ sealed class ClasspathChanges : Serializable {
    }

    sealed class NotAvailable : ClasspathChanges() {
+        object UnableToCompute : NotAvailable()
        object ForNonIncrementalRun : NotAvailable()
        object ClasspathSnapshotIsDisabled : NotAvailable()
        object ReservedForTestsOnly : NotAvailable()
@@ -36,12 +36,12 @@ import java.io.InputStream
 object JavaClassDescriptorCreator {

    /**
-     * Creates [JavaClassDescriptor]s of the given Java classes.
+     * Creates [JavaClassDescriptor]s of the given Java classes, or returns `null` if it can't be created for some reason.
     *
     * Note that creating a [JavaClassDescriptor] for a nested class will require accessing the outer class (and possibly vice versa).
     * Therefore, outer classes and nested classes must be passed together in one invocation of this method.
     */
-    fun create(classIds: List<ClassId>, classesContents: List<ByteArray>): List<JavaClassDescriptor> {
+    fun create(classIds: List<ClassId>, classesContents: List<ByteArray>): List<JavaClassDescriptor?> {
        val binaryJavaClasses = classIds.mapIndexed { index, classId ->
            createBinaryJavaClass(classId, classesContents[index])
        }
@@ -56,7 +56,6 @@ object JavaClassDescriptorCreator {

        return classIds.map { classId ->
            moduleDescriptor.findClassAcrossModuleDependencies(classId) as? JavaClassDescriptor
-                ?: error("Failed to create JavaClassDescriptor for class '$classId'")
        }
    }
 }
@@ -165,7 +165,7 @@ fun computeJavaClassIds(classNames: List<JavaClassName>): List<ClassId> {
                // "OuterClassWith$Sign$NestedClassWith$Sign", but its ClassId.relativeClassName will be
                // "OuterClassWith$Sign.NestedClassWith$Sign". To disambiguate '$' in the (outer) class name, we need to get the ClassId of
                // the outer class first.
-                val outerClassId = nameToClassName[outerName]?.getClassId() ?: error("Class name not found: $outerName")
+                val outerClassId = nameToClassName[outerName]?.getClassId() ?: error("Can't find outer class '$outerName' of class '$name'")
                val relativeClassName = FqName(outerClassId.relativeClassName.asString() + "." + simpleName)
                // For ClassId, a nested non-local class of a local class is also considered local (see ClassId's kdoc).
                val isLocal = outerClassId.isLocal
@@ -47,6 +47,7 @@ import org.jetbrains.kotlin.incremental.multiproject.EmptyModulesApiHistory
 import org.jetbrains.kotlin.incremental.multiproject.ModulesApiHistory
 import org.jetbrains.kotlin.incremental.util.BufferingMessageCollector
 import org.jetbrains.kotlin.incremental.util.Either
+import org.jetbrains.kotlin.incremental.ClasspathChanges.NotAvailable.UnableToCompute
 import org.jetbrains.kotlin.incremental.ClasspathChanges.NotAvailable.ForJSCompiler
 import org.jetbrains.kotlin.incremental.ClasspathChanges.NotAvailable.ReservedForTestsOnly
 import org.jetbrains.kotlin.incremental.ClasspathChanges.NotAvailable.ForNonIncrementalRun
@@ -218,9 +219,9 @@ class IncrementalJvmCompilerRunner(
            // Note: classpathChanges is deserialized, so they are no longer singleton objects and need to be compared using `is` (not `==`)
            is ClasspathChanges.Available -> ChangesEither.Known(classpathChanges.lookupSymbols, classpathChanges.fqNames)
            is ClasspathChanges.NotAvailable -> when (classpathChanges) {
-                is ClasspathSnapshotIsDisabled, is ReservedForTestsOnly -> {
+                is UnableToCompute, is ClasspathSnapshotIsDisabled, is ReservedForTestsOnly -> {
                    reporter.measure(BuildTime.IC_ANALYZE_CHANGES_IN_DEPENDENCIES) {
-                        val scopes = caches.lookupCache.lookupMap.keys.map { if (it.scope.isBlank()) it.name else it.scope }.distinct()
+                        val scopes = caches.lookupCache.lookupMap.keys.map { it.scope.ifBlank { it.name } }.distinct()
                        getClasspathChanges(
                            args.classpathAsList, changedFiles, lastBuildInfo, modulesApiHistory, reporter, abiSnapshots, withSnapshot,
                            caches.platformCache, scopes
@@ -18,6 +18,11 @@ object ClasspathChangesComputer {
        val currentClassSnapshots = currentClasspathSnapshot.getClassSnapshots()
        val previousClassSnapshots = previousClasspathSnapshot.getClassSnapshots()

+        if (currentClassSnapshots.any { it is ContentHashJavaClassSnapshot }
+            || previousClassSnapshots.any { it is ContentHashJavaClassSnapshot }) {
+            return ClasspathChanges.NotAvailable.UnableToCompute
+        }
+
        val workingDir =
            FileUtil.createTempDirectory(this::class.java.simpleName, "_WorkingDir_${UUID.randomUUID()}", /* deleteOnExit */ true)
        val incrementalJvmCache = IncrementalJvmCache(workingDir, /* targetOutputDir */ null, FileToCanonicalPathConverter)
@@ -39,6 +44,9 @@ object ClasspathChangesComputer {
                is EmptyJavaClassSnapshot -> {
                    // Nothing to process
                }
+                is ContentHashJavaClassSnapshot -> {
+                    error("Unexpected type (it should have been handled earlier): ${previousSnapshot.javaClass.name}")
+                }
            }
        }
        // Call the following method even though there are no removed classes, just in case the method updates the state of
@@ -62,6 +70,9 @@ object ClasspathChangesComputer {
                is EmptyJavaClassSnapshot -> {
                    // Nothing to process
                }
+                is ContentHashJavaClassSnapshot -> {
+                    error("Unexpected type (it should have been handled earlier): ${currentSnapshot.javaClass.name}")
+                }
            }
        }
        incrementalJvmCache.clearCacheForRemovedClasses(changesCollector)
@@ -52,3 +52,10 @@ class RegularJavaClassSnapshot(val serializedJavaClass: SerializedJavaClass) : J
 * changes in a local class will not cause recompilation of other source files.
 */
 object EmptyJavaClassSnapshot : JavaClassSnapshot()
+
+/**
+ * [JavaClassSnapshot] of a Java class where a proper snapshot can't be created for some reason, so we use the hash of the class contents as
+ * the snapshot instead, so that at least it's still correct when used as an input of the `KotlinCompile` task (when the class contents have
+ * changed, this snapshot will also change).
+ */
+class ContentHashJavaClassSnapshot(val contentHash: ByteArray) : JavaClassSnapshot()
@@ -94,19 +94,20 @@ object KotlinClassInfoExternalizer : DataExternalizer<KotlinClassInfo> {
 object JavaClassSnapshotExternalizer : DataExternalizer<JavaClassSnapshot> {

    override fun save(output: DataOutput, snapshot: JavaClassSnapshot) {
-        output.writeBoolean(snapshot is RegularJavaClassSnapshot)
+        output.writeString(snapshot.javaClass.name)
        when (snapshot) {
            is RegularJavaClassSnapshot -> RegularJavaClassSnapshotExternalizer.save(output, snapshot)
            is EmptyJavaClassSnapshot -> EmptyJavaClassSnapshotExternalizer.save(output, snapshot)
+            is ContentHashJavaClassSnapshot -> ContentHashJavaClassSnapshotExternalizer.save(output, snapshot)
        }
    }

    override fun read(input: DataInput): JavaClassSnapshot {
-        val isPlainJavaClassSnapshot = input.readBoolean()
-        return if (isPlainJavaClassSnapshot) {
-            RegularJavaClassSnapshotExternalizer.read(input)
-        } else {
-            EmptyJavaClassSnapshotExternalizer.read(input)
+        return when (val className = input.readString()) {
+            RegularJavaClassSnapshot::class.java.name -> RegularJavaClassSnapshotExternalizer.read(input)
+            EmptyJavaClassSnapshot::class.java.name -> EmptyJavaClassSnapshotExternalizer.read(input)
+            ContentHashJavaClassSnapshot::class.java.name -> ContentHashJavaClassSnapshotExternalizer.read(input)
+            else -> error("Unrecognized class name: $className")
        }
    }
 }
@@ -133,6 +134,17 @@ object EmptyJavaClassSnapshotExternalizer : DataExternalizer<EmptyJavaClassSnaps
    }
 }

+object ContentHashJavaClassSnapshotExternalizer : DataExternalizer<ContentHashJavaClassSnapshot> {
+
+    override fun save(output: DataOutput, snapshot: ContentHashJavaClassSnapshot) {
+        ByteArrayExternalizer.save(output, snapshot.contentHash)
+    }
+
+    override fun read(input: DataInput): ContentHashJavaClassSnapshot {
+        return ContentHashJavaClassSnapshot(contentHash = ByteArrayExternalizer.read(input))
+    }
+}
+
 interface DataSerializer<T> : DataExternalizer<T> {

    fun save(file: File, value: T) {
@@ -6,8 +6,11 @@
 package org.jetbrains.kotlin.gradle.incremental

 import org.jetbrains.kotlin.incremental.*
+import org.jetbrains.kotlin.load.java.descriptors.JavaClassDescriptor
 import org.jetbrains.kotlin.name.ClassId
+import org.jetbrains.kotlin.utils.DFS
 import java.io.File
+import java.security.MessageDigest
 import java.util.zip.ZipInputStream

 /** Computes a [ClasspathEntrySnapshot] of a classpath entry (directory or jar). */
@@ -28,11 +31,38 @@ object ClasspathEntrySnapshotter {
                    ClassFileWithContents(ClassFile(classpathEntry, unixStyleRelativePath), contents)
                }

+        if (isKnownProblematicClasspathEntry(classpathEntry)) {
+            return ClasspathEntrySnapshot(classes.associateTo(LinkedHashMap()) {
+                it.classFile.unixStyleRelativePath to ContentHashJavaClassSnapshot(it.contents.md5())
+            })
+        }
+
        val snapshots = ClassSnapshotter.snapshot(classes)

        val relativePathsToSnapshotsMap = classes.map { it.classFile.unixStyleRelativePath }.zipToMap(snapshots)
        return ClasspathEntrySnapshot(relativePathsToSnapshotsMap)
    }
+
+    /** Returns `true` if it is known that the snapshot of the given classpath entry can't be created for some reason. */
+    private fun isKnownProblematicClasspathEntry(classpathEntry: File): Boolean {
+        if (classpathEntry.name.startsWith("tools-jar-api")) {
+            // [FAULTY JAR] kotlin/dependencies/tools-jar-api/build/libs/tools-jar-api-1.6.255-SNAPSHOT.jar contains class
+            // com/sun/tools/javac/comp/Infer$GraphStrategy$NodeNotFoundException, but doesn't contain its outer class
+            // com/sun/tools/javac/comp/Infer$GraphStrategy.
+            // This happens with a few other similar classes in this jar.
+            // Therefore, this is a faulty jar, and our snapshotting logic cannot process it.
+            return true
+        }
+        if (classpathEntry.name.startsWith("platform-impl")) {
+            // ~/.gradle/kotlin-build-dependencies/repo/kotlin.build/ideaIC/203.8084.24/artifacts/lib/platform-impl.jar contains class
+            // com/intellij/application/options/codeStyle/OptionTableWithPreviewPanel.IntOption. When processing that class,
+            // BinaryJavaAnnotation$Companion.computeTargetType$resolution_common_jvm in Annotations.kt requires the targetType to be
+            // JavaClassifierType, but the actual type is PlainJavaPrimitiveType.
+            // TODO: It's likely that this requirement is incorrect, but let's fix it later.
+            return true
+        }
+        return false
+    }
 }

 /** Creates [ClassSnapshot]s of classes. */
@@ -95,8 +125,27 @@ object ClassSnapshotter {
        val regularClassIds: List<ClassId> = computeJavaClassIds(regularClasses)
        val regularClassesContents: List<ByteArray> = regularClasses.map { classNameToClassFile[it]!!.contents }

-        val snapshots: List<RegularJavaClassSnapshot> = JavaClassDescriptorCreator.create(regularClassIds, regularClassesContents).map {
-            RegularJavaClassSnapshot(it.toSerializedJavaClass())
+        val descriptors: List<JavaClassDescriptor?> = JavaClassDescriptorCreator.create(regularClassIds, regularClassesContents)
+        val snapshots: List<JavaClassSnapshot> = descriptors.mapIndexed { index, descriptor ->
+            val classFileWithContents = classNameToClassFile[regularClasses[index]]!!
+            if (descriptor != null) {
+                try {
+                    RegularJavaClassSnapshot(descriptor.toSerializedJavaClass())
+                } catch (e: Throwable) {
+                    if (isKnownExceptionWhenReadingDescriptor(e)) {
+                        ContentHashJavaClassSnapshot(classFileWithContents.contents.md5())
+                    } else throw e
+                }
+            } else {
+                if (isKnownProblematicClass(classFileWithContents.classFile)) {
+                    ContentHashJavaClassSnapshot(classFileWithContents.contents.md5())
+                } else {
+                    error(
+                        "Failed to create JavaClassDescriptor for class '${classFileWithContents.classFile.unixStyleRelativePath}'" +
+                                " in '${classFileWithContents.classFile.classRoot.path}'"
+                    )
+                }
+            }
        }
        val regularClassSnapshots: LinkedHashMap<JavaClassName, JavaClassSnapshot> = regularClasses.zipToMap(snapshots)

@@ -115,7 +164,9 @@ object ClassSnapshotter {
                true
            } else when (this) {
                is TopLevelClass -> false
-                is NestedNonLocalClass -> nameToClassName[outerName]?.isSpecial() ?: error("Class name not found: $outerName")
+                is NestedNonLocalClass -> {
+                    nameToClassName[outerName]?.isSpecial() ?: error("Can't find outer class '$outerName' of class '$name'")
+                }
                is LocalClass -> true
            }.also {
                specialClasses[this] = it
@@ -124,6 +175,57 @@ object ClassSnapshotter {

        return classNames.filter { it.isSpecial() }
    }
+
+    /** Returns `true` if it is known that the given exception can be thrown when calling [JavaClassDescriptor.toSerializedJavaClass]. */
+    private fun isKnownExceptionWhenReadingDescriptor(throwable: Throwable): Boolean {
+        // When building the Kotlin repo with `./gradlew publish -Pbootstrap.local=true
+        // -Pbootstrap.local.path=/path/to/kotlin/build/repo -Pkotlin.incremental.useClasspathSnapshot=true`, the build can fail with:
+        //   org.gradle.api.internal.artifacts.transform.TransformException: Execution failed for ClasspathEntrySnapshotTransform: ~/.gradle/wrapper/dists/gradle-6.9-bin/2ecsmyp3bolyybemj56vfn4mt/gradle-6.9/lib/kotlin-reflect-1.4.20.jar
+        //   Caused by: java.lang.IncompatibleClassChangeError: Expected static method 'java.lang.Object org.jetbrains.kotlin.utils.DFS.dfsFromNode(java.lang.Object, org.jetbrains.kotlin.utils.DFS$Neighbors, org.jetbrains.kotlin.utils.DFS$Visited, org.jetbrains.kotlin.utils.DFS$NodeHandler)'
+        //     at org.jetbrains.kotlin.builtins.FunctionTypesKt.isTypeOrSubtypeOf(functionTypes.kt:31)
+        //     (... at JavaClassDescriptor.toSerializedJavaClass)
+        // The reason is that:
+        //   - org/jetbrains/kotlin/builtins/FunctionTypesKt.class is located in ~/.gradle/caches/jars-8/66425fb82fd14126e9aa07dcd3100b42/kotlin-compiler-embeddable-1.6.255-20210909.213620-55.jar
+        //   - org/jetbrains/kotlin/utils/DFS.class is located in ~/.gradle/caches/jars-8/c716e2e2d26b16f6f1462e59ba44cf3b/buildSrc.jar
+        // And somehow the two classes are incompatible (probably similar to the NoSuchMethodError documented at JavaClassDescriptorCreatorKt.createBinaryJavaClass).
+        // This happens to a few other jars inside gradle-6.9/lib.
+        // However, outside the Kotlin repo build, we don't have this issue (org/jetbrains/kotlin/builtins/FunctionTypesKt.class and
+        // org/jetbrains/kotlin/utils/DFS.class will be located in the same kotlin-compiler-embeddable.jar).
+        // Therefore, we special-case the Kotlin repo build below.
+        // TODO: See how we can address this issue.
+        return (throwable is IncompatibleClassChangeError &&
+                DFS::class.java.classLoader.getResource(DFS::class.java.name.replace('.', '/') + ".class")
+                    ?.path?.contains("buildSrc.jar") == true
+                )
+    }
+
+    /** Returns `true` if it is known that the snapshot of the given class can't be created for some reason. */
+    private fun isKnownProblematicClass(classFile: ClassFile): Boolean {
+        if (classFile.classRoot.name.startsWith("groovy-all")
+            && classFile.unixStyleRelativePath.endsWith("\$CollectorHelper.class")
+        ) {
+            // [FAULTY JAR] In gradle-6.9/lib/groovy-all-1.3-2.5.12.jar, the bytecode of groovy/cli/OptionField\$CollectorHelper.class
+            // indicates that its outer class is groovy/cli/OptionField, but the bytecode of groovy/cli/OptionField.class does not list any
+            // nested classes.
+            // This happens with a few other CollectorHelper classes in this jar.
+            // Therefore, this is a faulty jar, and our snapshotting logic cannot process it.
+            return true
+        }
+        if (classFile.classRoot.name.startsWith("gradle-api")
+            && classFile.unixStyleRelativePath.startsWith("org/gradle/internal/impldep/META-INF/versions")
+        ) {
+            // [FAULTY JAR] gradle-api-6.9.jar has the following entries:
+            //   - org/gradle/internal/impldep/org/junit/platform/commons/util/ModuleUtils.class
+            //   - org/gradle/internal/impldep/META-INF/versions/9/org/junit/platform/commons/util/ModulesUtils.class
+            //   - org/gradle/internal/impldep/META-INF/versions/9/org/junit/platform/commons/util/ModuleUtils$ModuleReferenceScanner.class
+            // The META-INF directories are located not at the top level (which is not expected), and those directories escaped our filter
+            // which filters out top-level META-INF directories. We then failed to snapshot ModuleUtils$ModuleReferenceScanner.class as
+            // there are 2 versions of ModuleUtils.class, and the one outside the META-INF directory doesn't have any nested classes.
+            // Therefore, this is a faulty jar, and our snapshotting logic cannot process it.
+            return true
+        }
+        return false
+    }
 }

 /** Utility to read the contents of a directory or jar. */
@@ -198,3 +300,5 @@ private fun <K, V> List<K>.zipToMap(other: List<V>): LinkedHashMap<K, V> {
    }
    return map
 }
+
+private fun ByteArray.md5(): ByteArray = MessageDigest.getInstance("MD5").digest(this)
@@ -49,7 +49,7 @@ abstract class ClasspathChangesComputerTest : ClasspathSnapshotTestCommon() {
        return when (this) {
            is KotlinClassSnapshot -> classInfo.classId
            is RegularJavaClassSnapshot -> serializedJavaClass.classId
-            is EmptyJavaClassSnapshot -> null
+            is EmptyJavaClassSnapshot, is ContentHashJavaClassSnapshot -> null
        }
    }