IC: Compute symbols impacted by classpath changes (#5111)

IC: Compute symbols impacted by classpath changes Incremental compilation has 4 key steps: 1. Compile changed/impacted files 2. Detect symbols that have changed after compiling 3. Detect symbols that are impacted by the changed symbols 4. Based on the changed-or-impacted symbols, identify files that need to be recompiled. Go back to step 1. Normally, step 2 and 3 are done together when the changed symbols and impacted symbols are in the same module. However, if the changed symbols and impacted symbols are in different modules (e.g., a `Subclass` in lib1 extends a `Superclass` in lib2), we currently do not compute symbols in the current module that are impacted by changes in another module (step 3 above). This is the case for both the new IC and the old IC. In this commit, we will compute impacted symbols for the new IC. We can fix the old IC later if necessary (they can't be fixed together easily). Test: Added BaseIncrementalCompilationMultiProjectIT.testChangeInterfaceInLib ^KT-56197 Fixed
2023-03-21 12:12:15 +00:00
parent bb09395952
commit 4f3244fb78
9 changed files with 180 additions and 47 deletions
@@ -34,9 +34,6 @@ import org.jetbrains.kotlin.resolve.sam.SAM_LOOKUP_NAME
 import org.jetbrains.kotlin.utils.addToStdlib.flattenTo
 import java.io.File
 import java.nio.file.Files
-import java.util.*
-import kotlin.collections.HashSet
-import kotlin.collections.LinkedHashSet

 const val DELETE_MODULE_FILE_PROPERTY = "kotlin.delete.module.file.after.build"

@@ -143,7 +140,34 @@ data class DirtyData(
    val dirtyClassesFqNamesForceRecompile: Collection<FqName> = emptyList()
 )

-fun ChangesCollector.getDirtyData(
+/**
+ * Returns changed symbols from the changes collected by this [ChangesCollector].
+ *
+ * If impacted symbols are also needed, use [getChangedAndImpactedSymbols].
+ */
+fun ChangesCollector.getChangedSymbols(reporter: ICReporter): DirtyData {
+    // Caches are used to compute impacted symbols. Set `caches = emptyList()` so that we get changed symbols only, not impacted ones.
+    return changes().getChangedAndImpactedSymbols(caches = emptyList(), reporter)
+}
+
+/**
+ * Returns changed and impacted symbols from the changes collected by this [ChangesCollector].
+ *
+ * For example, if `Subclass` extends `Superclass` and `Superclass` has changed, `Subclass` will be impacted.
+ */
+fun ChangesCollector.getChangedAndImpactedSymbols(
+    caches: Iterable<IncrementalCacheCommon>,
+    reporter: ICReporter
+): DirtyData {
+    return changes().getChangedAndImpactedSymbols(caches, reporter)
+}
+
+/**
+ * Returns changed and impacted symbols from this list of changes.
+ *
+ * For example, if `Subclass` extends `Superclass` and `Superclass` has changed, `Subclass` will be impacted.
+ */
+fun List<ChangeInfo>.getChangedAndImpactedSymbols(
    caches: Iterable<IncrementalCacheCommon>,
    reporter: ICReporter
 ): DirtyData {
@@ -152,7 +176,7 @@ fun ChangesCollector.getDirtyData(

    val sealedParents = HashSet<FqName>()

-    for (change in changes()) {
+    for (change in this) {
        reporter.debug { "Process $change" }

        if (change is ChangeInfo.SignatureChanged) {
@@ -522,7 +522,7 @@ abstract class IncrementalCompilerRunner<
                break
            }

-            val (dirtyLookupSymbols, dirtyClassFqNames, forceRecompile) = changesCollector.getDirtyData(
+            val (dirtyLookupSymbols, dirtyClassFqNames, forceRecompile) = changesCollector.getChangedAndImpactedSymbols(
                listOf(caches.platformCache),
                reporter
            )
@@ -598,7 +598,7 @@ abstract class IncrementalCompilerRunner<

        val changesCollector = ChangesCollector()
        removedClasses.forEach { changesCollector.collectSignature(FqName(it), areSubclassesAffected = true) }
-        return changesCollector.getDirtyData(listOf(caches.platformCache), reporter)
+        return changesCollector.getChangedAndImpactedSymbols(listOf(caches.platformCache), reporter)
    }

    open fun runWithNoDirtyKotlinSources(caches: CacheManager): Boolean = false
@@ -250,7 +250,8 @@ class IncrementalJsCompilerRunner(
            val changesCollector = ChangesCollector()
            // todo: split compare and update (or cache comparing)
            caches.platformCache.compare(translatedFiles, changesCollector)
-            val (dirtyLookupSymbols, dirtyClassFqNames) = changesCollector.getDirtyData(listOf(caches.platformCache), reporter)
+            val (dirtyLookupSymbols, dirtyClassFqNames) =
+                changesCollector.getChangedAndImpactedSymbols(listOf(caches.platformCache), reporter)
            // todo unify with main cycle
            newDirtySources.addAll(mapLookupSymbolsToFiles(caches.lookupCache, dirtyLookupSymbols, reporter, excludes = sourcesToCompile))
            newDirtySources.addAll(
@@ -38,22 +38,15 @@ import org.jetbrains.kotlin.cli.jvm.K2JVMCompiler
 import org.jetbrains.kotlin.cli.jvm.compiler.EnvironmentConfigFiles
 import org.jetbrains.kotlin.cli.jvm.compiler.KotlinCoreEnvironment
 import org.jetbrains.kotlin.cli.jvm.config.configureJdkClasspathRoots
-import org.jetbrains.kotlin.config.CompilerConfiguration
-import org.jetbrains.kotlin.config.IncrementalCompilation
-import org.jetbrains.kotlin.config.LanguageVersion
-import org.jetbrains.kotlin.config.Services
-import org.jetbrains.kotlin.config.languageVersionSettings
+import org.jetbrains.kotlin.config.*
 import org.jetbrains.kotlin.incremental.ClasspathChanges.ClasspathSnapshotDisabled
 import org.jetbrains.kotlin.incremental.ClasspathChanges.ClasspathSnapshotEnabled.IncrementalRun.NoChanges
 import org.jetbrains.kotlin.incremental.ClasspathChanges.ClasspathSnapshotEnabled.IncrementalRun.ToBeComputedByIncrementalCompiler
 import org.jetbrains.kotlin.incremental.ClasspathChanges.ClasspathSnapshotEnabled.NotAvailableDueToMissingClasspathSnapshot
 import org.jetbrains.kotlin.incremental.ClasspathChanges.ClasspathSnapshotEnabled.NotAvailableForNonIncrementalRun
 import org.jetbrains.kotlin.incremental.ClasspathChanges.NotAvailableForJSCompiler
-import org.jetbrains.kotlin.incremental.classpathDiff.AccessibleClassSnapshot
+import org.jetbrains.kotlin.incremental.classpathDiff.*
 import org.jetbrains.kotlin.incremental.classpathDiff.ClasspathChangesComputer.computeClasspathChanges
-import org.jetbrains.kotlin.incremental.classpathDiff.ClasspathSnapshotBuildReporter
-import org.jetbrains.kotlin.incremental.classpathDiff.shrinkAndSaveClasspathSnapshot
-import org.jetbrains.kotlin.incremental.classpathDiff.toChangesEither
 import org.jetbrains.kotlin.incremental.components.ExpectActualTracker
 import org.jetbrains.kotlin.incremental.components.LookupTracker
 import org.jetbrains.kotlin.incremental.multiproject.EmptyModulesApiHistory
@@ -165,8 +158,6 @@ open class IncrementalJvmCompilerRunner(
    override fun destinationDir(args: K2JVMCompilerArguments): File =
        args.destinationAsFile

-    private var dirtyClasspathChanges: Collection<FqName> = emptySet()
-
    private val messageCollector = BufferingMessageCollector()
    private val compilerConfiguration: CompilerConfiguration by lazy {
        val filterMessageCollector = FilteringMessageCollector(messageCollector) { !it.isError }
@@ -256,7 +247,7 @@ open class IncrementalJvmCompilerRunner(
        initDirtyFiles(dirtyFiles, changedFiles)

        reporter.debug { "Classpath changes info passed from Gradle task: ${classpathChanges::class.simpleName}" }
-        val classpathChanges = when (classpathChanges) {
+        val changedAndImpactedSymbols = when (classpathChanges) {
            // Note: classpathChanges is deserialized, so they are no longer singleton objects and need to be compared using `is` (not `==`)
            is NoChanges -> ChangesEither.Known(emptySet(), emptySet())
            is ToBeComputedByIncrementalCompiler -> reporter.measure(BuildTime.COMPUTE_CLASSPATH_CHANGES) {
@@ -267,12 +258,15 @@ open class IncrementalJvmCompilerRunner(
                        currentClasspathSnapshot = currentClasspathSnapshotArg
                        shrunkCurrentClasspathAgainstPreviousLookups = shrunkCurrentClasspathAgainstPreviousLookupsArg
                    }
-                computeClasspathChanges(
+                val classpathChanges = computeClasspathChanges(
                    classpathChanges.classpathSnapshotFiles,
                    caches.lookupCache,
                    storeCurrentClasspathSnapshotForReuse,
                    ClasspathSnapshotBuildReporter(reporter)
-                ).toChangesEither()
+                )
+                // `classpathChanges` contains changed and impacted symbols on the classpath.
+                // We also need to compute symbols in the current module that are impacted by `classpathChanges`.
+                classpathChanges.toChangeInfoList().getChangedAndImpactedSymbols(listOf(caches.platformCache), reporter).toChangesEither()
            }
            is NotAvailableDueToMissingClasspathSnapshot -> ChangesEither.Unknown(BuildAttribute.CLASSPATH_SNAPSHOT_NOT_FOUND)
            is NotAvailableForNonIncrementalRun -> ChangesEither.Unknown(BuildAttribute.UNKNOWN_CHANGES_IN_GRADLE_INPUTS)
@@ -291,6 +285,7 @@ open class IncrementalJvmCompilerRunner(
                reporter.debug { "Last Kotlin Build info -- $lastBuildInfo" }
                val scopes = caches.lookupCache.lookupSymbols.map { it.scope.ifBlank { it.name } }.distinct()

+                // FIXME The old IC currently doesn't compute impacted symbols
                getClasspathChanges(
                    args.classpathAsList, changedFiles, lastBuildInfo, modulesApiHistory, reporter, abiSnapshots, withAbiSnapshot,
                    caches.platformCache, scopes
@@ -299,20 +294,16 @@ open class IncrementalJvmCompilerRunner(
            is NotAvailableForJSCompiler -> error("Unexpected type for this code path: ${classpathChanges.javaClass.name}.")
        }

-        @Suppress("UNUSED_VARIABLE") // for sealed when
-        val unused = when (classpathChanges) {
+        when (changedAndImpactedSymbols) {
            is ChangesEither.Unknown -> {
-                reporter.info {
-                    "Could not get classpath's changes: ${classpathChanges.reason}"
-                }
-                return CompilationMode.Rebuild(classpathChanges.reason)
+                reporter.info { "Could not get classpath changes: ${changedAndImpactedSymbols.reason}" }
+                return CompilationMode.Rebuild(changedAndImpactedSymbols.reason)
            }
-            is ChangesEither.Known -> {
-                dirtyFiles.addByDirtySymbols(classpathChanges.lookupSymbols)
-                dirtyClasspathChanges = classpathChanges.fqNames
-                dirtyFiles.addByDirtyClasses(classpathChanges.fqNames)
-            }
-        }
+            is ChangesEither.Known -> Unit
+        }.forceExhaustiveWhen()
+
+        dirtyFiles.addByDirtySymbols(changedAndImpactedSymbols.lookupSymbols)
+        dirtyFiles.addByDirtyClasses(changedAndImpactedSymbols.fqNames)

        reporter.measure(BuildTime.IC_ANALYZE_CHANGES_IN_JAVA_SOURCES) {
            if (!usePreciseJavaTracking) {
@@ -342,6 +333,35 @@ open class IncrementalJvmCompilerRunner(
        return CompilationMode.Incremental(dirtyFiles)
    }

+    private fun ProgramSymbolSet.toChangeInfoList(): List<ChangeInfo> {
+        val changes = mutableListOf<ChangeInfo>()
+        classes.forEach { classId ->
+            // It's important to set `areSubclassesAffected = true` when we don't know
+            changes.add(ChangeInfo.SignatureChanged(classId.asSingleFqName(), areSubclassesAffected = true))
+        }
+        classMembers.forEach { (classId, members) ->
+            changes.add(ChangeInfo.MembersChanged(classId.asSingleFqName(), members))
+        }
+        packageMembers.forEach { (packageFqName, members) ->
+            changes.add(ChangeInfo.MembersChanged(packageFqName, members))
+        }
+        return changes
+    }
+
+    private fun DirtyData.toChangesEither(): ChangesEither.Known {
+        return ChangesEither.Known(
+            lookupSymbols = dirtyLookupSymbols,
+            fqNames = dirtyClassesFqNames + dirtyClassesFqNamesForceRecompile
+        )
+    }
+
+    /**
+     * Helper function to force exhaustive when for statements (see https://youtrack.jetbrains.com/issue/KT-47709).
+     *
+     * If the current IDE/Kotlin compiler already supports exhaustive when for statements, consider removing this function and its usages.
+     */
+    private fun Any.forceExhaustiveWhen() = this
+
    private fun processChangedJava(changedFiles: ChangedFiles.Known, caches: IncrementalJvmCachesManager): BuildAttribute? {
        val javaFiles = (changedFiles.modified + changedFiles.removed).filter(File::isJavaFile)

@@ -247,7 +247,7 @@ object ClasspathChangesComputer {
        incrementalJvmCache.clearCacheForRemovedClasses(changesCollector)

        // Get the changes and clean up
-        val dirtyData = changesCollector.getDirtyData(listOf(incrementalJvmCache), DoNothingICReporter)
+        val dirtyData = changesCollector.getChangedSymbols(DoNothingICReporter)
        workingDir.deleteRecursively()

        // Normalize the changes (convert DirtyData to `ProgramSymbol`s)
@@ -292,6 +292,10 @@ object ClasspathChangesComputer {
            it.removeAll(changedFqNames)
        }

+        if (unmatchedLookupSymbols.isEmpty() && unmatchedFqNames.isEmpty()) {
+            return changedProgramSymbols
+        }
+
        /* When `unmatchedLookupSymbols` or `unmatchedFqNames` is not empty, there are two cases:
         *   1. The unmatched LookupSymbols/FqNames are redundant. This is not ideal but because it does not cause incremental compilation
         *      to be incorrect, we can fix these issues later if they are not easy to fix immediately.
@@ -132,7 +132,8 @@ internal fun collectNewDirtySources(
        visitFirFiles(output)
    }

-    val (dirtyLookupSymbols, dirtyClassFqNames, forceRecompile) = changesCollector.getDirtyData(listOf(caches.platformCache), reporter)
+    val (dirtyLookupSymbols, dirtyClassFqNames, forceRecompile) =
+        changesCollector.getChangedAndImpactedSymbols(listOf(caches.platformCache), reporter)

    val forceToRecompileFiles = mapClassesFqNamesToFiles(listOf(caches.platformCache), forceRecompile, reporter)

@@ -52,9 +52,6 @@ import org.jetbrains.kotlin.utils.KotlinPaths
 import org.jetbrains.kotlin.utils.KotlinPathsFromHomeDir
 import org.jetbrains.kotlin.utils.PathUtil
 import java.io.File
-import java.util.*
-import kotlin.collections.ArrayList
-import kotlin.collections.HashSet
 import kotlin.system.measureTimeMillis

 class KotlinBuilder : ModuleLevelBuilder(BuilderCategory.SOURCE_PROCESSOR) {
@@ -776,7 +773,7 @@ private fun ChangesCollector.getDirtyFiles(
    lookupStorageManager: JpsLookupStorageManager
 ): FilesToRecompile {
    val reporter = JpsICReporter()
-    val (dirtyLookupSymbols, dirtyClassFqNames, forceRecompile) = getDirtyData(caches, reporter)
+    val (dirtyLookupSymbols, dirtyClassFqNames, forceRecompile) = getChangedAndImpactedSymbols(caches, reporter)
    val dirtyFilesFromLookups = lookupStorageManager.withLookupStorage {
        mapLookupSymbolsToFiles(it, dirtyLookupSymbols, reporter)
    }
@@ -6,11 +6,11 @@
 package org.jetbrains.kotlin.gradle

 import org.gradle.api.logging.LogLevel
-import org.gradle.testkit.runner.BuildResult
 import org.gradle.util.GradleVersion
-import org.jetbrains.kotlin.gradle.testbase.*
-import java.nio.file.Path
-import kotlin.io.path.relativeTo
+import org.jetbrains.kotlin.gradle.testbase.BuildOptions
+import org.jetbrains.kotlin.gradle.testbase.KGPBaseTest
+import org.jetbrains.kotlin.gradle.testbase.TestProject
+import org.jetbrains.kotlin.gradle.testbase.project

 abstract class IncrementalCompilationBaseIT : KGPBaseTest() {

@@ -23,6 +23,7 @@ abstract class IncrementalCompilationBaseIT : KGPBaseTest() {
    ): TestProject = project(
        defaultProjectName,
        gradleVersion,
+        buildOptions,
        test = test
    )

@@ -135,6 +135,20 @@ open class IncrementalCompilationJsMultiProjectIT : BaseIncrementalCompilationMu
            }
        }
    }
+
+    @DisplayName("Lib project classes became final")
+    @GradleTest
+    override fun testLibClassBecameFinal(gradleVersion: GradleVersion) {
+        // `impactedClassInAppIsRecompiled = false` for Kotlin/JS (KT-56197 was fixed for Kotlin/JVM only)
+        doTestLibClassBecameFinal(gradleVersion, impactedClassInAppIsRecompiled = false)
+    }
+
+    @DisplayName("KT-56197: Change interface in lib which has subclass in app")
+    @GradleTest
+    override fun testChangeInterfaceInLib(gradleVersion: GradleVersion) {
+        // `impactedClassInAppIsRecompiled = false` for Kotlin/JS (KT-56197 was fixed for Kotlin/JVM only)
+        doTestChangeInterfaceInLib(gradleVersion, impactedClassInAppIsRecompiled = false)
+    }
 }

 class IncrementalCompilationJsMultiProjectWithPreciseBackupIT : IncrementalCompilationJsMultiProjectIT() {
@@ -413,7 +427,6 @@ open class IncrementalCompilationOldICJvmMultiProjectIT : IncrementalCompilation
    override fun testAbiChangeInLib_afterLibClean_withAbiSnapshot(gradleVersion: GradleVersion) {
        defaultProject(
            gradleVersion,
-            buildOptions = defaultBuildOptions.copy(useGradleClasspathSnapshot = true)
        ) {
            build("assemble")

@@ -437,6 +450,20 @@ open class IncrementalCompilationOldICJvmMultiProjectIT : IncrementalCompilation
            }
        }
    }
+
+    @DisplayName("Lib project classes became final")
+    @GradleTest
+    override fun testLibClassBecameFinal(gradleVersion: GradleVersion) {
+        // `impactedClassInAppIsRecompiled = false` for the old IC (KT-56197 was fixed for the new IC only)
+        doTestLibClassBecameFinal(gradleVersion, impactedClassInAppIsRecompiled = false)
+    }
+
+    @DisplayName("KT-56197: Change interface in lib which has subclass in app")
+    @GradleTest
+    override fun testChangeInterfaceInLib(gradleVersion: GradleVersion) {
+        // `impactedClassInAppIsRecompiled = false` for the old IC (KT-56197 was fixed for the new IC only)
+        doTestChangeInterfaceInLib(gradleVersion, impactedClassInAppIsRecompiled = false)
+    }
 }

 class IncrementalCompilationOldICJvmMultiProjectWithPreciseBackupIT : IncrementalCompilationOldICJvmMultiProjectIT() {
@@ -622,7 +649,11 @@ abstract class BaseIncrementalCompilationMultiProjectIT : IncrementalCompilation

    @DisplayName("Lib project classes became final")
    @GradleTest
-    fun testLibClassBecameFinal(gradleVersion: GradleVersion) {
+    open fun testLibClassBecameFinal(gradleVersion: GradleVersion) {
+        doTestLibClassBecameFinal(gradleVersion)
+    }
+
+    protected fun doTestLibClassBecameFinal(gradleVersion: GradleVersion, impactedClassInAppIsRecompiled: Boolean = true) {
        defaultProject(gradleVersion) {
            build("assemble")

@@ -633,7 +664,10 @@ abstract class BaseIncrementalCompilationMultiProjectIT : IncrementalCompilation
            buildAndFail("assemble") {
                val expectedSources = getExpectedKotlinSourcesForDefaultProject(
                    libSources = listOf("bar/B.kt", "bar/barUseAB.kt", "bar/barUseB.kt"),
-                    appSources = listOf("foo/BB.kt", "foo/fooCallUseAB.kt", "foo/fooUseB.kt")
+                    appSources = listOfNotNull(
+                        "foo/BB.kt", "foo/fooUseB.kt", "foo/fooCallUseAB.kt",
+                        "foo/fooUseBB.kt".takeIf { impactedClassInAppIsRecompiled }
+                    )
                )
                assertCompiledKotlinSources(expectedSources, output)
            }
@@ -776,6 +810,57 @@ abstract class BaseIncrementalCompilationMultiProjectIT : IncrementalCompilation
        }
    }

+    @DisplayName("KT-56197: Change interface in lib which has subclass in app")
+    @GradleTest
+    open fun testChangeInterfaceInLib(gradleVersion: GradleVersion) {
+        doTestChangeInterfaceInLib(gradleVersion)
+    }
+
+    protected fun doTestChangeInterfaceInLib(gradleVersion: GradleVersion, impactedClassInAppIsRecompiled: Boolean = true) {
+        defaultProject(gradleVersion) {
+            subProject("lib").kotlinSourcesDir().resolve("bar/InterfaceInLib.kt").writeText(
+                """
+                package bar
+                interface InterfaceInLib {
+                    fun someMethod() {}
+                }
+                """.trimIndent()
+            )
+            subProject("app").kotlinSourcesDir().resolve("foo/SubclassInApp.kt").writeText(
+                """
+                package foo
+                import bar.InterfaceInLib
+                class SubclassInApp : InterfaceInLib
+                """.trimIndent()
+            )
+            subProject("app").kotlinSourcesDir().resolve("foo/ClassUsingSubclassInApp.kt").writeText(
+                """
+                package foo
+                fun main() {
+                    SubclassInApp().someMethod()
+                }
+                """.trimIndent()
+            )
+            build(":app:compileKotlin")
+
+            subProject("lib").kotlinSourcesDir().resolve("bar/InterfaceInLib.kt").modify {
+                it.replace("fun someMethod() {}", "fun someMethod(addedParam: Int = 0) {}")
+            }
+
+            build(":app:compileKotlin") {
+                assertIncrementalCompilation(
+                    expectedCompiledKotlinFiles = getExpectedKotlinSourcesForDefaultProject(
+                        libSources = listOf("bar/InterfaceInLib.kt"),
+                        appSources = listOfNotNull(
+                            "foo/SubclassInApp.kt",
+                            "foo/ClassUsingSubclassInApp.kt".takeIf { impactedClassInAppIsRecompiled }
+                        )
+                    )
+                )
+            }
+        }
+    }
+
    @DisplayName("Test compilation when incremental state is missing")
    @GradleTest
    fun testMissingIncrementalState(gradleVersion: GradleVersion) {