From f172c50f279a09b54e9fe0140d76142f479e4530 Mon Sep 17 00:00:00 2001 From: hungvietnguyen Date: Thu, 19 Jan 2023 11:21:29 +0000 Subject: [PATCH] New IC: Ignore LookupSymbols that refer to file facades (#5069) A LookupSymbol should only refer to either a class, a class member, or a package member. When a LookupSymbol refers to a file facade (e.g., LookupSymbol(name="FooKt", scope="com.example")), it is redundant as it doesn't impact the IC analysis to find files to recompile. Previously, the new IC (ClasspathChangesComputer) would fail when detecting that IncrementalJvmCache reported these redundant LookupSymbols. With this change, the new IC will just ignore them. Note: A better approach would be to fix IncrementalJvmCache to not report these LookupSymbols, but it will require some significant cleanup/refactoring work, so we can consider it later. Test: New KotlinOnlyClasspathChangesComputerTest.testRenameFileFacade ^KT-55021 Fixed --- .../classpathDiff/ClasspathChangesComputer.kt | 45 ++++++---- .../ClasspathChangesComputerTest.kt | 13 +++ .../ClasspathSnapshotTestCommon.kt | 78 ++++++++---------- .../com/example/NewFileFacadeName.class | Bin 0 -> 657 bytes .../com/example/OldFileFacadeName.class | Bin 0 -> 657 bytes .../com/example/FileFacade.kt | 6 ++ .../com/example/FileFacade.kt | 6 ++ 7 files changed, 87 insertions(+), 61 deletions(-) create mode 100644 compiler/incremental-compilation-impl/testData/org/jetbrains/kotlin/incremental/classpathDiff/ClasspathChangesComputerTest/KotlinOnly/testRenameFileFacade/classes/current-classpath/com/example/NewFileFacadeName.class create mode 100644 compiler/incremental-compilation-impl/testData/org/jetbrains/kotlin/incremental/classpathDiff/ClasspathChangesComputerTest/KotlinOnly/testRenameFileFacade/classes/previous-classpath/com/example/OldFileFacadeName.class create mode 100644 compiler/incremental-compilation-impl/testData/org/jetbrains/kotlin/incremental/classpathDiff/ClasspathChangesComputerTest/KotlinOnly/testRenameFileFacade/src/current-classpath/com/example/FileFacade.kt create mode 100644 compiler/incremental-compilation-impl/testData/org/jetbrains/kotlin/incremental/classpathDiff/ClasspathChangesComputerTest/KotlinOnly/testRenameFileFacade/src/previous-classpath/com/example/FileFacade.kt diff --git a/compiler/incremental-compilation-impl/src/org/jetbrains/kotlin/incremental/classpathDiff/ClasspathChangesComputer.kt b/compiler/incremental-compilation-impl/src/org/jetbrains/kotlin/incremental/classpathDiff/ClasspathChangesComputer.kt index 9b70a5bbc61..3b1594fd3b5 100644 --- a/compiler/incremental-compilation-impl/src/org/jetbrains/kotlin/incremental/classpathDiff/ClasspathChangesComputer.kt +++ b/compiler/incremental-compilation-impl/src/org/jetbrains/kotlin/incremental/classpathDiff/ClasspathChangesComputer.kt @@ -120,7 +120,8 @@ object ClasspathChangesComputer { // changes = changesOnPreviousAndCurrentClasspath, // allClasses = classesOnPreviousClasspath + classesOnCurrentClasspath) // Note: We will replace `classesOnCurrentClasspath` with `changedClassesOnCurrentClasspath` to avoid listing unchanged classes - // twice. `allClasses` may contain overlapping ClassIds of modified classes, but it won't be an issue. + // twice. `allClasses` may still contain duplicate ClassIds (because it contains the previous and current version of each + // modified class), but this is not an issue. computeImpactedSymbols( changes = changedSet, allClasses = (previousClassSnapshots.asSequence() + changedCurrentClasses.asSequence()).asIterable() @@ -252,11 +253,13 @@ object ClasspathChangesComputer { // Normalize the changes (convert DirtyData to `ProgramSymbol`s) // Note: // - DirtyData may contain added symbols (they can also impact recompilation -- see examples in JavaClassChangesComputer). - // Therefore, we need to consider classes on both the previous and current classpath when converting DirtyData. - // - We have removed unchanged classes earlier in computeChangedAndImpactedSet method, so here we only have changed classes. - // - `changedClasses` may contain overlapping ClassIds of modified classes, but it won't be an issue. - val changedClasses = (previousClassSnapshots.asSequence() + currentClassSnapshots.asSequence()).asIterable() - return dirtyData.toProgramSymbols(changedClasses) + // Therefore, we need to consider classes on both the previous and current classpath (`allClasses`) when converting DirtyData. + // - `allClasses` actually contains only deleted/modified/added classes as we have removed unchanged classes earlier in the + // computeChangedAndImpactedSet method. This doesn't affect correctness as here we don't care about unchanged classes. + // - `allClasses` may contain duplicate ClassIds (because it contains the previous and current version of each modified class), + // but this is not an issue. + val allClasses = (previousClassSnapshots.asSequence() + currentClassSnapshots.asSequence()).asIterable() + return dirtyData.toProgramSymbols(allClasses) } /** @@ -274,8 +277,8 @@ object ClasspathChangesComputer { * 2. `dirtyClassesFqNames` and `dirtyClassesFqNamesForceRecompile` must not contain new information that can't be derived from * `dirtyLookupSymbols`. */ - private fun DirtyData.toProgramSymbols(changedClasses: Iterable): ProgramSymbolSet { - val changedProgramSymbols = dirtyLookupSymbols.toProgramSymbolSet(changedClasses) + private fun DirtyData.toProgramSymbols(allClasses: Iterable): ProgramSymbolSet { + val changedProgramSymbols = dirtyLookupSymbols.toProgramSymbolSet(allClasses) // Check whether there is any info in this DirtyData that has not yet been converted to `changedProgramSymbols` val (changedLookupSymbols, changedFqNames) = changedProgramSymbols.toChangesEither().let { @@ -303,8 +306,8 @@ object ClasspathChangesComputer { // class member LookupSymbol is redundant. When converting DirtyData to ProgramSymbols, we remove redundant class member // `ProgramSymbol`s, so here we will find that LookupSymbol("com.example.A", "someProperty") is not yet matched. Ignore these // `LookupSymbol`s for now. - val classesFqNames = changedProgramSymbols.classes.mapTo(mutableSetOf()) { it.asSingleFqName() } - unmatchedLookupSymbols.removeAll { FqName(it.scope) in classesFqNames } + val changedClassesFqNames = changedProgramSymbols.classes.mapTo(mutableSetOf()) { it.asSingleFqName() } + unmatchedLookupSymbols.removeAll { FqName(it.scope) in changedClassesFqNames } // Known issue 2: If class A has a companion object containing a constant `CONSTANT`, and if the value of `CONSTANT` has changed, // then only `A.class` will change, not `A.Companion.class` (see `ConstantsInCompanionObjectImpact`). Since we distinguish between @@ -322,7 +325,7 @@ object ClasspathChangesComputer { // // Note: Once we are able to remove this workaround, we can remove RegularKotlinClassSnapshot.companionObjectName as this is the only // usage of that property. - val companionObjectFqNames = changedClasses.mapNotNullTo(mutableSetOf()) { clazz -> + val companionObjectFqNames = allClasses.mapNotNullTo(mutableSetOf()) { clazz -> (clazz as? RegularKotlinClassSnapshot)?.companionObjectName?.let { it -> clazz.classId.createNestedClassId(Name.identifier(it)).asSingleFqName() } @@ -330,15 +333,21 @@ object ClasspathChangesComputer { unmatchedLookupSymbols.removeAll { FqName(it.scope) in companionObjectFqNames } unmatchedFqNames.removeAll(companionObjectFqNames) - // Known issue 3: LookupSymbol(name=, scope=com.example) reported by IncrementalJvmCache is invalid (detected by - // KotlinOnlyClasspathChangesComputerTest.testTopLevelMembers): SAM-CONSTRUCTOR should have a class scope, not a package scope. + // Known issue 3: LookupSymbol(name=, scope=com.example) reported by IncrementalJvmCache is invalid: + // SAM-CONSTRUCTOR should have a class scope, not a package scope. + // This issue was detected by KotlinOnlyClasspathChangesComputerTest.testTopLevelMembers. + val classesFqNames = allClasses.filter { it is RegularKotlinClassSnapshot || it is JavaClassSnapshot } + .mapTo(mutableSetOf()) { it.classId.asSingleFqName() } unmatchedLookupSymbols.removeAll { it.name == SAM_LOOKUP_NAME.asString() && FqName(it.scope) !in classesFqNames } - // Known issue 4: LookupSymbol(name=BarUseABKt, scope=bar) reported by IncrementalJvmCache is invalid (detected by - // IncrementalCompilationClasspathSnapshotJvmMultiProjectIT.testMoveFunctionFromLibToApp): The name of a LookupSymbol should not - // end with "Kt" (unless there is a Kotlin class (CLASS kind) whose name ends with "Kt", which is almost never the case). - unmatchedLookupSymbols.removeAll { it.name.endsWith("Kt") } - unmatchedFqNames.removeAll { it.asString().endsWith("Kt") } + // Known issue 4: LookupSymbol(name=FooKt, scope=com.example) reported by IncrementalJvmCache is invalid: LookupSymbol should not + // refer to a package facade; it should only refer to either a class, a class member, or a package member (see KT-55021). + // This issue was detected by KotlinOnlyClasspathChangesComputerTest.testRenameFileFacade and + // IncrementalCompilationClasspathSnapshotJvmMultiProjectIT.testMoveFunctionFromLibToApp. + val packageFacadeFqNames = allClasses.filter { it is KotlinClassSnapshot && it !is RegularKotlinClassSnapshot } + .mapTo(mutableSetOf()) { it.classId.asSingleFqName() } + unmatchedLookupSymbols.removeAll { FqName(it.scope).child(Name.identifier(it.name)) in packageFacadeFqNames } + unmatchedFqNames.removeAll(packageFacadeFqNames) /* * End of known issues, throw an Exception. 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 72d5c064130..9125e1bafca 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 @@ -256,6 +256,19 @@ class KotlinOnlyClasspathChangesComputerTest : ClasspathChangesComputerTest() { ).assertEquals(changes) } + /** Regression test for KT-55021. */ + @Test + fun testRenameFileFacade() { + val changes = computeClasspathChanges(File(testDataDir, "KotlinOnly/testRenameFileFacade/src"), tmpDir) + Changes( + lookupSymbols = setOf( + LookupSymbol(name = "someFunction", scope = "com.example"), + LookupSymbol(name = "someProperty", scope = "com.example"), + ), + fqNames = setOf("com.example") + ).assertEquals(changes) + } + /** Tests [SupertypesInheritorsImpact]. */ @Test override fun testImpactComputation_SupertypesInheritors() { diff --git a/compiler/incremental-compilation-impl/test/org/jetbrains/kotlin/incremental/classpathDiff/ClasspathSnapshotTestCommon.kt b/compiler/incremental-compilation-impl/test/org/jetbrains/kotlin/incremental/classpathDiff/ClasspathSnapshotTestCommon.kt index 518b1b289bf..8f662faf169 100644 --- a/compiler/incremental-compilation-impl/test/org/jetbrains/kotlin/incremental/classpathDiff/ClasspathSnapshotTestCommon.kt +++ b/compiler/incremental-compilation-impl/test/org/jetbrains/kotlin/incremental/classpathDiff/ClasspathSnapshotTestCommon.kt @@ -7,7 +7,6 @@ package org.jetbrains.kotlin.incremental.classpathDiff import com.google.gson.GsonBuilder import org.jetbrains.kotlin.cli.common.isWindows -import org.jetbrains.kotlin.incremental.classpathDiff.ClasspathSnapshotTestCommon.ClassFileUtil.asFile import org.jetbrains.kotlin.incremental.classpathDiff.ClasspathSnapshotTestCommon.ClassFileUtil.snapshot import org.jetbrains.kotlin.incremental.classpathDiff.ClasspathSnapshotTestCommon.CompileUtil.compile import org.jetbrains.kotlin.incremental.classpathDiff.ClasspathSnapshotTestCommon.CompileUtil.compileAll @@ -71,7 +70,7 @@ abstract class ClasspathSnapshotTestCommon { fun SourceFile.compile(tmpDir: TemporaryFolder): List { return if (this is KotlinSourceFile) { preCompiledClassFiles.forEach { - preCompileKotlinFilesIfNecessary(baseDir, it.classRoot, classpath = emptyList(), tmpDir) + compileKotlin(srcDir = baseDir, classesDir = it.classRoot, classpath = emptyList()) } preCompiledClassFiles } else { @@ -83,58 +82,49 @@ abstract class ClasspathSnapshotTestCommon { /** Compiles the source files in the given directory and returns all generated .class files. */ fun compileAll(srcDir: File, tmpDir: TemporaryFolder, classpath: List = emptyList()): List { - val kotlinClasses = compileKotlin(srcDir, classpath, tmpDir) + val classesDir = srcDir.path.let { + File(it.substringBeforeLast("src") + "classes" + it.substringAfterLast("src")) + } + val kotlinClasses = compileKotlin(srcDir, classesDir, classpath) val javaClasspath = classpath + listOfNotNull(kotlinClasses.firstOrNull()?.classRoot) - val javaClasses = compileJava(srcDir, javaClasspath, tmpDir) + val javaClasses = compileJava(srcDir, classesDir = tmpDir.newFolder(), javaClasspath) return kotlinClasses + javaClasses } - private fun compileKotlin(srcDir: File, classpath: List, tmpDir: TemporaryFolder): List { - val preCompiledKotlinClassesDir = srcDir.path.let { - File(it.substringBeforeLast("src") + "classes" + it.substringAfterLast("src")) - } - preCompileKotlinFilesIfNecessary(srcDir, preCompiledKotlinClassesDir, classpath, tmpDir) - return getClassFilesInDir(preCompiledKotlinClassesDir) - } - - private val preCompiledKotlinClassesDirs = mutableSetOf() - /** - * If /dist/kotlinc/lib/kotlin-compiler.jar is available (e.g., by running ./gradlew dist), we will be able to call the - * Kotlin compiler to generate classes. However, kotlin-compiler.jar is currently not available in CI builds, so we need to - * pre-compile the classes locally and put them in the test data to check in. + * Set to `true` to (re)generate Kotlin .class files locally which can then be checked in (remember to set this value back to + * `false` afterwards, DO NOT check in this code when this value = `true`). + * + * Reason for this flag: If /dist/kotlinc/lib/kotlin-compiler.jar is available (e.g., by running ./gradlew dist), we + * will be able to call the Kotlin compiler to generate classes. However, kotlin-compiler.jar is currently not available in CI + * builds, so we need to pre-compile the classes locally and put them in the test data to check in. */ - @Synchronized // To safeguard shared variable preCompiledKotlinClassesDirs - private fun preCompileKotlinFilesIfNecessary( - srcDir: File, - preCompiledKotlinClassesDir: File, - classpath: List, - tmpDir: TemporaryFolder, - preCompile: Boolean = false // Set to `true` to pre-compile Kotlin class files locally (DO NOT check in with preCompile = true) - ) { - if (preCompile) { - if (!preCompiledKotlinClassesDirs.contains(preCompiledKotlinClassesDir)) { - val classFiles = doCompileKotlin(srcDir, classpath, tmpDir) - preCompiledKotlinClassesDir.deleteRecursively() - for (classFile in classFiles) { - File(preCompiledKotlinClassesDir, classFile.unixStyleRelativePath).apply { - parentFile.mkdirs() - classFile.asFile().copyTo(this) - } + private const val GENERATE_KOTLIN_CLASS_FILES = false + + private val alreadyCompiledKotlinSrcDirs = mutableSetOf() + + private fun compileKotlin(srcDir: File, classesDir: File, classpath: List): List { + if (GENERATE_KOTLIN_CLASS_FILES) { + // This block may be called concurrently so add this synchronization to be safe + synchronized(alreadyCompiledKotlinSrcDirs) { + if (!alreadyCompiledKotlinSrcDirs.contains(srcDir)) { + doCompileKotlin(srcDir, classesDir, classpath) + alreadyCompiledKotlinSrcDirs.add(srcDir) } - preCompiledKotlinClassesDirs.add(preCompiledKotlinClassesDir) } } + return getClassFilesInDir(classesDir) } - private fun doCompileKotlin(srcDir: File, classpath: List, tmpDir: TemporaryFolder): List { + private fun doCompileKotlin(srcDir: File, classesDir: File, classpath: List) { + classesDir.deleteRecursively() + classesDir.mkdirs() if (srcDir.walk().none { it.path.endsWith(".kt") }) { - return emptyList() + return } - val classesDir = tmpDir.newFolder() // Note: Calling the following is simpler: // org.jetbrains.kotlin.test.MockLibraryUtil.compileKotlin( // srcDir.path, classesDir, extraClasspath = classpath.map { it.path }.toTypedArray()) @@ -148,22 +138,24 @@ abstract class ClasspathSnapshotTestCommon { "-classpath", (listOf(srcDir) + classpath).joinToString(File.pathSeparator) { it.path } ) runCommandInNewProcess(commandAndArgs) + } + private fun compileJava(srcDir: File, classesDir: File, classpath: List): List { + doCompileJava(srcDir, classesDir, classpath) return getClassFilesInDir(classesDir) } - private fun compileJava(srcDir: File, classpath: List, tmpDir: TemporaryFolder): List { + private fun doCompileJava(srcDir: File, classesDir: File, classpath: List) { + classesDir.deleteRecursively() + classesDir.mkdirs() val javaFiles = srcDir.walk().toList().filter { it.path.endsWith(".java") } if (javaFiles.isEmpty()) { - return emptyList() + return } - val classesDir = tmpDir.newFolder() val classpathOption = if (classpath.isNotEmpty()) listOf("-classpath", classpath.joinToString(File.pathSeparator)) else emptyList() - KotlinTestUtils.compileJavaFiles(javaFiles, listOf("-d", classesDir.path) + classpathOption) - return getClassFilesInDir(classesDir) } private fun getClassFilesInDir(classesDir: File): List { diff --git a/compiler/incremental-compilation-impl/testData/org/jetbrains/kotlin/incremental/classpathDiff/ClasspathChangesComputerTest/KotlinOnly/testRenameFileFacade/classes/current-classpath/com/example/NewFileFacadeName.class b/compiler/incremental-compilation-impl/testData/org/jetbrains/kotlin/incremental/classpathDiff/ClasspathChangesComputerTest/KotlinOnly/testRenameFileFacade/classes/current-classpath/com/example/NewFileFacadeName.class new file mode 100644 index 0000000000000000000000000000000000000000..194f2da80bc5c58d8f5d7c9bbf7395f297b8fbfa GIT binary patch literal 657 zcmZWlO>fgc5Ph?D9oJ2W3*jSf%SYh=7n~wafRGR}-kb5v{P_9pJAj9{L)c2IGFHd3JStQ?Q1APBq53kF znHtDaH9&-LEax&Vq&|#akH#uB4LF4Mp)z|_soqShBQ-T22+rMm387j4lcc1H2K-eB ztS-s^Os6KVG{Jw87P-#N(?#(*wa<-0zB4%E(L_ z$#PN79o~aE9_Kddag^;4dZ#Brz$QXJ5WW+6p&N-FZFj=c6B`_yeSC5D z$@5tgxxNT_bCEqD+p_idR`(}{r+i*zJY{<~*J>~;M{4>`jtb5Md(~{3Dk~>+-_Eqj zOSPZZIkV5Su8g$js0kZOF46OIEOz*W?qZk7UX&vTU97<4QEu?pCZm1sR|KCK2{+kW z7Od)G?T<2ADuwgE&f~sKw3xRYTWt2xmjyVu#VBwc+l-G`w9bt$4&fxYl%SK~GOi@J lifak~E*gSrY+!SUl^Ts2TeS_mng_Yf^=n*zfvsI5`~^RDfK31Z literal 0 HcmV?d00001 diff --git a/compiler/incremental-compilation-impl/testData/org/jetbrains/kotlin/incremental/classpathDiff/ClasspathChangesComputerTest/KotlinOnly/testRenameFileFacade/classes/previous-classpath/com/example/OldFileFacadeName.class b/compiler/incremental-compilation-impl/testData/org/jetbrains/kotlin/incremental/classpathDiff/ClasspathChangesComputerTest/KotlinOnly/testRenameFileFacade/classes/previous-classpath/com/example/OldFileFacadeName.class new file mode 100644 index 0000000000000000000000000000000000000000..2bc0b8a6b721d2133feeb80b2bfadc6b90197842 GIT binary patch literal 657 zcmZWlO>fgc5Ph?D6W2|M3*jSf%SY*ZSQO_q*baGBTA$ zvRo9k!~2nSo^>G5#0G-N)67PFj?x`M|NPAN*+k&^!gIn#;D(}4o8932%mzmnA75R3 zYIv*(T~7qOxkw+8ZQ1^NtA`WAQ@$)yp0cx>X|*>iMr!(Aj&jcU2jy&6}C==;1>`?fHwdD literal 0 HcmV?d00001 diff --git a/compiler/incremental-compilation-impl/testData/org/jetbrains/kotlin/incremental/classpathDiff/ClasspathChangesComputerTest/KotlinOnly/testRenameFileFacade/src/current-classpath/com/example/FileFacade.kt b/compiler/incremental-compilation-impl/testData/org/jetbrains/kotlin/incremental/classpathDiff/ClasspathChangesComputerTest/KotlinOnly/testRenameFileFacade/src/current-classpath/com/example/FileFacade.kt new file mode 100644 index 00000000000..604a3c27ffa --- /dev/null +++ b/compiler/incremental-compilation-impl/testData/org/jetbrains/kotlin/incremental/classpathDiff/ClasspathChangesComputerTest/KotlinOnly/testRenameFileFacade/src/current-classpath/com/example/FileFacade.kt @@ -0,0 +1,6 @@ +@file:JvmName("NewFileFacadeName") + +package com.example + +val someProperty = 0 +fun someFunction() = 0 \ No newline at end of file diff --git a/compiler/incremental-compilation-impl/testData/org/jetbrains/kotlin/incremental/classpathDiff/ClasspathChangesComputerTest/KotlinOnly/testRenameFileFacade/src/previous-classpath/com/example/FileFacade.kt b/compiler/incremental-compilation-impl/testData/org/jetbrains/kotlin/incremental/classpathDiff/ClasspathChangesComputerTest/KotlinOnly/testRenameFileFacade/src/previous-classpath/com/example/FileFacade.kt new file mode 100644 index 00000000000..7d67c13366b --- /dev/null +++ b/compiler/incremental-compilation-impl/testData/org/jetbrains/kotlin/incremental/classpathDiff/ClasspathChangesComputerTest/KotlinOnly/testRenameFileFacade/src/previous-classpath/com/example/FileFacade.kt @@ -0,0 +1,6 @@ +@file:JvmName("OldFileFacadeName") + +package com.example + +val someProperty = 0 +fun someFunction() = 0 \ No newline at end of file