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
2023-01-19 11:21:29 +00:00
parent dc0cd61b6f
commit f172c50f27
7 changed files with 87 additions and 61 deletions
@@ -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<AccessibleClassSnapshot>): ProgramSymbolSet {
-        val changedProgramSymbols = dirtyLookupSymbols.toProgramSymbolSet(changedClasses)
+    private fun DirtyData.toProgramSymbols(allClasses: Iterable<AccessibleClassSnapshot>): 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=<SAM-CONSTRUCTOR>, 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=<SAM-CONSTRUCTOR>, 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.
@@ -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() {
@@ -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<ClassFile> {
            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<File> = emptyList()): List<ClassFile> {
-            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<File>, tmpDir: TemporaryFolder): List<ClassFile> {
-            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<File>()
-
        /**
-         * If <kotlin-repo>/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 <kotlin-repo>/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<File>,
-            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<File>()
+
+        private fun compileKotlin(srcDir: File, classesDir: File, classpath: List<File>): List<ClassFile> {
+            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<File>, tmpDir: TemporaryFolder): List<ClassFile> {
+        private fun doCompileKotlin(srcDir: File, classesDir: File, classpath: List<File>) {
+            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<File>): List<ClassFile> {
+            doCompileJava(srcDir, classesDir, classpath)
            return getClassFilesInDir(classesDir)
        }

-        private fun compileJava(srcDir: File, classpath: List<File>, tmpDir: TemporaryFolder): List<ClassFile> {
+        private fun doCompileJava(srcDir: File, classesDir: File, classpath: List<File>) {
+            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<ClassFile> {
@@ -0,0 +1,6 @@
+@file:JvmName("NewFileFacadeName")
+
+package com.example
+
+val someProperty = 0
+fun someFunction() = 0
@@ -0,0 +1,6 @@
+@file:JvmName("OldFileFacadeName")
+
+package com.example
+
+val someProperty = 0
+fun someFunction() = 0