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 00000000000..194f2da80bc Binary files /dev/null and b/compiler/incremental-compilation-impl/testData/org/jetbrains/kotlin/incremental/classpathDiff/ClasspathChangesComputerTest/KotlinOnly/testRenameFileFacade/classes/current-classpath/com/example/NewFileFacadeName.class differ 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 00000000000..2bc0b8a6b72 Binary files /dev/null and b/compiler/incremental-compilation-impl/testData/org/jetbrains/kotlin/incremental/classpathDiff/ClasspathChangesComputerTest/KotlinOnly/testRenameFileFacade/classes/previous-classpath/com/example/OldFileFacadeName.class differ 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