From 2b65d673bb3cec19909c046448645304d66a0376 Mon Sep 17 00:00:00 2001 From: Hung Nguyen Date: Wed, 17 Nov 2021 20:28:25 +0000 Subject: [PATCH] KT-45777: Also collect added classes/class members as classpath changes as they can impact recompilation (examples shown in the code comments). Test: Updated ClasspathChangesComputerTest --- .../classpathDiff/JavaClassChangesComputer.kt | 26 +++++++++++++------ .../ClasspathChangesComputerTest.kt | 25 ++++++------------ 2 files changed, 26 insertions(+), 25 deletions(-) diff --git a/compiler/incremental-compilation-impl/src/org/jetbrains/kotlin/incremental/classpathDiff/JavaClassChangesComputer.kt b/compiler/incremental-compilation-impl/src/org/jetbrains/kotlin/incremental/classpathDiff/JavaClassChangesComputer.kt index fe54307749c..0af4f04c385 100644 --- a/compiler/incremental-compilation-impl/src/org/jetbrains/kotlin/incremental/classpathDiff/JavaClassChangesComputer.kt +++ b/compiler/incremental-compilation-impl/src/org/jetbrains/kotlin/incremental/classpathDiff/JavaClassChangesComputer.kt @@ -24,11 +24,18 @@ object JavaClassChangesComputer { val currentClasses: Map = currentJavaClassSnapshots.associateBy { it.classId } val previousClasses: Map = previousJavaClassSnapshots.associateBy { it.classId } - // No need to collect added classes as they don't impact recompilation + // Note: Added classes can also impact recompilation. + // For example, suppose a source file uses `SomeClass` through `*` imports: + // import foo.* // foo.SomeClass is added in the second build + // import bar.* // bar.SomeClass is present in the first build and unchanged in the second build + // In the second build, the source file needs to be recompiled as `SomeClass` is ambiguous now (in this example, the recompilation + // will fail, but recompilation needs to happen). + val addedClasses = currentClasses.keys - previousClasses.keys val removedClasses = previousClasses.keys - currentClasses.keys - val unchangedOrModifiedClasses = previousClasses.keys - removedClasses + val unchangedOrModifiedClasses = currentClasses.keys - addedClasses return ChangeSet.Collector().run { + addChangedClasses(addedClasses) addChangedClasses(removedClasses) unchangedOrModifiedClasses.forEach { collectClassChanges(currentClasses[it]!!, previousClasses[it]!!, this) @@ -63,18 +70,21 @@ object JavaClassChangesComputer { previousMemberSnapshots: List, changes: ChangeSet.Collector ) { - val currentMemberHashes: Set = currentMemberSnapshots.map { it.abiHash }.toSet() + val currentMemberHashes: Map = currentMemberSnapshots.associateBy { it.abiHash } val previousMemberHashes: Map = previousMemberSnapshots.associateBy { it.abiHash } - val addedMembers = currentMemberHashes - previousMemberHashes.keys - val removedMembers = previousMemberHashes.keys - currentMemberHashes + val addedMembers = currentMemberHashes.keys - previousMemberHashes.keys + val removedMembers = previousMemberHashes.keys - currentMemberHashes.keys // Note: - // - No need to collect added members as they don't impact recompilation. - // - Modified members have a current version and a previous version. The current version will appear in addedMembers (which will - // not be collected), and the previous version will appear in removedMembers (which will be collected). + // - Added members can also impact recompilation. For example, suppose a source file calls `foo(1)` where `foo` is defined as: + // fun foo(x: Int) { } // Added in the second build + // fun foo(x: Any) { } // Present in the first build and unchanged in the second build + // In the second build, the source file needs to be recompiled as `foo(1)` will now resolve to `foo(Int)` instead of `foo(Any)`. + // - Modified members will appear in both addedMembers and removedMembers. // - Multiple members may have the same name (but never the same signature (name + desc) or ABI hash). It's okay to report the // same name multiple times. + changes.addChangedClassMembers(classId, addedMembers.map { currentMemberHashes[it]!!.name }) changes.addChangedClassMembers(classId, removedMembers.map { previousMemberHashes[it]!!.name }) // TODO: Check whether the condition to add SAM_LOOKUP_NAME below is too broad, and correct it if necessary. diff --git a/compiler/incremental-compilation-impl/test/org/jetbrains/kotlin/incremental/classpathDiff/ClasspathChangesComputerTest.kt b/compiler/incremental-compilation-impl/test/org/jetbrains/kotlin/incremental/classpathDiff/ClasspathChangesComputerTest.kt index 23d6fb05023..cd9fc9d294d 100644 --- a/compiler/incremental-compilation-impl/test/org/jetbrains/kotlin/incremental/classpathDiff/ClasspathChangesComputerTest.kt +++ b/compiler/incremental-compilation-impl/test/org/jetbrains/kotlin/incremental/classpathDiff/ClasspathChangesComputerTest.kt @@ -194,34 +194,25 @@ class JavaOnlyClasspathChangesComputerTest(private val protoBased: Boolean) : Cl // ModifiedClassChangedMembers LookupSymbol(name = "modifiedField", scope = "com.example.ModifiedClassChangedMembers"), + LookupSymbol(name = "addedField", scope = "com.example.ModifiedClassChangedMembers"), LookupSymbol(name = "removedField", scope = "com.example.ModifiedClassChangedMembers"), LookupSymbol(name = "modifiedMethod", scope = "com.example.ModifiedClassChangedMembers"), + LookupSymbol(name = "addedMethod", scope = "com.example.ModifiedClassChangedMembers"), LookupSymbol(name = "removedMethod", scope = "com.example.ModifiedClassChangedMembers"), LookupSymbol(name = SAM_LOOKUP_NAME.asString(), scope = "com.example.ModifiedClassChangedMembers"), + // AddedClass + LookupSymbol(name = "AddedClass", scope = "com.example"), + // RemovedClass LookupSymbol(name = "RemovedClass", scope = "com.example") - ) + if (protoBased) { - setOf( - // ModifiedClassChangedMembers - LookupSymbol(name = "addedField", scope = "com.example.ModifiedClassChangedMembers"), - LookupSymbol(name = "addedMethod", scope = "com.example.ModifiedClassChangedMembers"), - - // AddedClass - LookupSymbol(name = "AddedClass", scope = "com.example"), - ) - } else { - emptySet() - }, + ), fqNames = setOf( "com.example.ModifiedClassUnchangedMembers", "com.example.ModifiedClassChangedMembers", + "com.example.AddedClass", "com.example.RemovedClass" - ) + if (protoBased) { - setOf("com.example.AddedClass") - } else { - emptySet() - } + ) ).assertEquals(changes) }