Fix precise Java IC for multi-module projects

The problem may happen in case of multi-module projects: If a Java class was effectively unused in one module (A), but it's used in kt-files from the dependent module (B) then we wouldn't track the changes of the class while compiling A and don't recompile its usages in B. It happens because now we track only Java classes that were resolved by out frontend instead of all classes in the module that would be more correct but it might be rather slow. The idea is that whenever we see change in an untracked Java file we start tracking the classes in it and for the first time we mark all its content as changed.
2017-12-12 12:03:19 +03:00
parent d9cfdf2f63
commit a978c6be35
12 changed files with 238 additions and 24 deletions
@@ -72,13 +72,13 @@ class ChangesCollector {
        }
    }

-    fun collectProtoChanges(oldData: ProtoData?, newData: ProtoData?) {
+    fun collectProtoChanges(oldData: ProtoData?, newData: ProtoData?, collectAllMembersForNewClass: Boolean = false) {
        if (oldData == null && newData == null) {
            throw IllegalStateException("Old and new value are null")
        }

        if (oldData == null) {
-            newData!!.collectAll(isRemoved = false)
+            newData!!.collectAll(isRemoved = false, collectAllMembersForNewClass = collectAllMembersForNewClass)
            return
        }

@@ -120,10 +120,10 @@ class ChangesCollector {
    private fun <T> T.getNonPrivateNames(nameResolver: NameResolver, vararg members: T.() -> List<MessageLite>): Set<String> =
            members.flatMap { this.it().filterNot { it.isPrivate }.names(nameResolver) }.toSet()

-    private fun ProtoData.collectAll(isRemoved: Boolean) =
+    private fun ProtoData.collectAll(isRemoved: Boolean, collectAllMembersForNewClass: Boolean = false) =
        when (this) {
            is PackagePartProtoData -> collectAllFromPackage(isRemoved)
-            is ClassProtoData -> collectAllFromClass(isRemoved)
+            is ClassProtoData -> collectAllFromClass(isRemoved, collectAllMembersForNewClass)
        }

    private fun PackagePartProtoData.collectAllFromPackage(isRemoved: Boolean) {
@@ -142,28 +142,36 @@ class ChangesCollector {
        }
    }

-    private fun ClassProtoData.collectAllFromClass(isRemoved: Boolean) {
+    private fun ClassProtoData.collectAllFromClass(isRemoved: Boolean, collectAllMembersForNewClass: Boolean = false) {
        val classFqName = nameResolver.getClassId(proto.fqName).asSingleFqName()
        val kind = Flags.CLASS_KIND.get(proto.flags)

        if (kind == ProtoBuf.Class.Kind.COMPANION_OBJECT) {
-            val memberNames =
-                    proto.getNonPrivateNames(
-                            nameResolver,
-                            ProtoBuf.Class::getConstructorList,
-                            ProtoBuf.Class::getFunctionList,
-                            ProtoBuf.Class::getPropertyList
-                    ) + proto.enumEntryList.map { nameResolver.getString(it.name) }
+            val memberNames = getNonPrivateMemberNames()

            val collectMember = if (isRemoved) this@ChangesCollector::collectRemovedMember else this@ChangesCollector::collectChangedMember
            collectMember(classFqName.parent(), classFqName.shortName().asString())
            memberNames.forEach { collectMember(classFqName, it) }
        }
        else {
+            if (!isRemoved && collectAllMembersForNewClass) {
+                val memberNames = getNonPrivateMemberNames()
+                memberNames.forEach { this@ChangesCollector.collectChangedMember(classFqName, it) }
+            }
+
            collectSignature(classFqName, areSubclassesAffected = true)
        }
    }

+    private fun ClassProtoData.getNonPrivateMemberNames(): Set<String> {
+        return proto.getNonPrivateNames(
+                nameResolver,
+                ProtoBuf.Class::getConstructorList,
+                ProtoBuf.Class::getFunctionList,
+                ProtoBuf.Class::getPropertyList
+        ) + proto.enumEntryList.map { nameResolver.getString(it.name) }
+    }
+
    fun collectMemberIfValueWasChanged(scope: FqName, name: String, oldValue: Any?, newValue: Any?) {
        if (oldValue == null && newValue == null) {
            throw IllegalStateException("Old and new value are null for $scope#$name")
@@ -186,4 +194,4 @@ class ChangesCollector {
        val prevValue = this.areSubclassesAffected[fqName] ?: false
        this.areSubclassesAffected[fqName] = prevValue || areSubclassesAffected
    }
-}
+}
@@ -87,6 +87,8 @@ open class IncrementalJvmCache(
        }
    }

+    fun isTrackedFile(file: File) = sourceToClassesMap.contains(file)
+
    // used in gradle
    @Suppress("unused")
    fun classesBySources(sources: Iterable<File>): Iterable<JvmClassName> =
@@ -335,7 +337,10 @@ open class IncrementalJvmCache(
            val oldData = storage[key]
            storage[key] = newData

-            changesCollector.collectProtoChanges(oldData?.toProtoData(), newData.toProtoData())
+            changesCollector.collectProtoChanges(
+                    oldData?.toProtoData(), newData.toProtoData(),
+                    collectAllMembersForNewClass = true
+            )
        }

        fun remove(className: JvmClassName, changesCollector: ChangesCollector) {
@@ -454,6 +459,8 @@ open class IncrementalJvmCache(
            storage.append(sourceFile.absolutePath, className.internalName)
        }

+        fun contains(sourceFile: File) = sourceFile.absolutePath in storage
+
        operator fun get(sourceFile: File): Collection<JvmClassName> =
                storage[sourceFile.absolutePath].orEmpty().map { JvmClassName.byInternalName(it) }

@@ -41,7 +41,10 @@ import java.io.File

 val CONVERTING_JAVA_CLASSES_TO_PROTO = PerformanceCounter.create("Converting Java sources to proto")

-class JavaClassesTrackerImpl(private val cache: IncrementalJvmCache) : JavaClassesTracker {
+class JavaClassesTrackerImpl(
+        private val cache: IncrementalJvmCache,
+        private val untrackedJavaClasses: Set<ClassId>
+) : JavaClassesTracker {
    private val classToSourceSerialized: MutableMap<ClassId, SerializedJavaClassWithSource> = hashMapOf()

    val javaClassesUpdates: Collection<SerializedJavaClassWithSource>
@@ -57,7 +60,7 @@ class JavaClassesTrackerImpl(private val cache: IncrementalJvmCache) : JavaClass
    }

    override fun onCompletedAnalysis(module: ModuleDescriptor) {
-        for (classId in cache.getObsoleteJavaClasses()) {
+        for (classId in cache.getObsoleteJavaClasses() + untrackedJavaClasses) {
            // Just force the loading obsolete classes
            // We assume here that whenever an LazyJavaClassDescriptor instances is created
            // it's being passed to JavaClassesTracker::reportClass
@@ -66,7 +69,7 @@ class JavaClassesTrackerImpl(private val cache: IncrementalJvmCache) : JavaClass

        for (classDescriptor in classDescriptors.toList()) {
            val classId = classDescriptor.classId!!
-            if (cache.isJavaClassAlreadyInCache(classId) || classDescriptor.wasContentRequested()) {
+            if (cache.isJavaClassAlreadyInCache(classId) || classId in untrackedJavaClasses || classDescriptor.wasContentRequested()) {
                assert(classId !in classToSourceSerialized) {
                    "Duplicated JavaClassDescriptor $classId reported to IC"
                }
@@ -16,7 +16,13 @@

 package org.jetbrains.kotlin.incremental

+import com.intellij.lang.java.JavaLanguage
+import com.intellij.openapi.util.Disposer
 import com.intellij.openapi.util.io.FileUtil
+import com.intellij.psi.PsiClass
+import com.intellij.psi.PsiFile
+import com.intellij.psi.PsiFileFactory
+import com.intellij.psi.PsiJavaFile
 import org.jetbrains.kotlin.annotation.AnnotationFileUpdater
 import org.jetbrains.kotlin.build.GeneratedFile
 import org.jetbrains.kotlin.build.GeneratedJvmClass
@@ -25,7 +31,10 @@ import org.jetbrains.kotlin.cli.common.ExitCode
 import org.jetbrains.kotlin.cli.common.arguments.K2JVMCompilerArguments
 import org.jetbrains.kotlin.cli.common.messages.MessageCollector
 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.compilerRunner.ArgumentUtils
+import org.jetbrains.kotlin.config.CompilerConfiguration
 import org.jetbrains.kotlin.config.IncrementalCompilation
 import org.jetbrains.kotlin.config.Services
 import org.jetbrains.kotlin.incremental.components.LookupTracker
@@ -35,7 +44,9 @@ import org.jetbrains.kotlin.load.java.JavaClassesTracker
 import org.jetbrains.kotlin.load.kotlin.header.KotlinClassHeader
 import org.jetbrains.kotlin.load.kotlin.incremental.components.IncrementalCompilationComponents
 import org.jetbrains.kotlin.modules.TargetId
+import org.jetbrains.kotlin.name.ClassId
 import org.jetbrains.kotlin.name.FqName
+import org.jetbrains.kotlin.name.Name
 import java.io.File

 fun makeIncrementally(
@@ -108,6 +119,16 @@ class IncrementalJvmCompilerRunner(
    override fun destinationDir(args: K2JVMCompilerArguments): File =
            args.destinationAsFile

+    private val psiFileFactory: PsiFileFactory by lazy {
+        val rootDisposable = Disposer.newDisposable()
+        val configuration = CompilerConfiguration()
+        val environment = KotlinCoreEnvironment.createForProduction(rootDisposable, configuration, EnvironmentConfigFiles.JVM_CONFIG_FILES)
+        val project = environment.project
+        PsiFileFactory.getInstance(project)
+    }
+
+    private val changedUntrackedJavaClasses = mutableSetOf<ClassId>()
+
    override fun calculateSourcesToCompile(caches: IncrementalJvmCachesManager, changedFiles: ChangedFiles.Known, args: K2JVMCompilerArguments): CompilationMode {
        val dirtyFiles = getDirtyFiles(changedFiles)

@@ -166,7 +187,9 @@ class IncrementalJvmCompilerRunner(
            return CompilationMode.Rebuild { "could not get changes from modified classpath entries: ${reporter.pathsAsString(modifiedClasspathEntries)}" }
        }

-        processChangedJava(changedFiles, caches)
+        if (!processChangedJava(changedFiles, caches)) {
+            return CompilationMode.Rebuild { "Could not get changes for java files" }
+        }

        if ((changedFiles.modified + changedFiles.removed).any { it.extension.toLowerCase() == "xml" }) {
            return CompilationMode.Rebuild { "XML resource files were changed" }
@@ -178,8 +201,42 @@ class IncrementalJvmCompilerRunner(
        return CompilationMode.Incremental(dirtyFiles)
    }

-    private fun processChangedJava(changedFiles: ChangedFiles.Known, caches: IncrementalJvmCachesManager) {
-        caches.platformCache.markDirty((changedFiles.modified + changedFiles.removed).filter(File::isJavaFile))
+    private fun processChangedJava(changedFiles: ChangedFiles.Known, caches: IncrementalJvmCachesManager): Boolean {
+        val javaFiles = (changedFiles.modified + changedFiles.removed).filter(File::isJavaFile)
+
+        for (javaFile in javaFiles) {
+            if (!caches.platformCache.isTrackedFile(javaFile)) {
+                val psiFile = javaFile.psiFile()
+                if (psiFile !is PsiJavaFile) {
+                    reporter.report { "[Precise Java tracking] Expected PsiJavaFile, got ${psiFile?.javaClass}" }
+                    return false
+                }
+
+                for (psiClass in psiFile.classes) {
+                    val qualifiedName = psiClass.qualifiedName
+                    if (qualifiedName == null) {
+                        reporter.report { "[Precise Java tracking] Class with unknown qualified name in $javaFile" }
+                        return false
+                    }
+
+                    processChangedUntrackedJavaClass(psiClass, ClassId.topLevel(FqName(qualifiedName)))
+                }
+            }
+        }
+
+        caches.platformCache.markDirty(javaFiles)
+        return true
+    }
+
+    private fun File.psiFile(): PsiFile? =
+            psiFileFactory.createFileFromText(nameWithoutExtension, JavaLanguage.INSTANCE, readText())
+
+    private fun processChangedUntrackedJavaClass(psiClass: PsiClass, classId: ClassId) {
+        changedUntrackedJavaClasses.add(classId)
+        for (innerClass in psiClass.innerClasses) {
+            val name = innerClass.name ?: continue
+            processChangedUntrackedJavaClass(innerClass, classId.createNestedClassId(Name.identifier(name)))
+        }
    }

    private fun getClasspathChanges(
@@ -251,7 +308,7 @@ class IncrementalJvmCompilerRunner(
    }

    override fun runWithNoDirtyKotlinSources(caches: IncrementalJvmCachesManager): Boolean =
-            caches.platformCache.getObsoleteJavaClasses().isNotEmpty()
+            caches.platformCache.getObsoleteJavaClasses().isNotEmpty() || changedUntrackedJavaClasses.isNotEmpty()

    override fun additionalDirtyFiles(
            caches: IncrementalJvmCachesManager,
@@ -321,7 +378,8 @@ class IncrementalJvmCompilerRunner(
            val targetToCache = mapOf(targetId to caches.platformCache)
            val incrementalComponents = IncrementalCompilationComponentsImpl(targetToCache)
            register(IncrementalCompilationComponents::class.java, incrementalComponents)
-            val changesTracker = JavaClassesTrackerImpl(caches.platformCache)
+            val changesTracker = JavaClassesTrackerImpl(caches.platformCache, changedUntrackedJavaClasses.toSet())
+            changedUntrackedJavaClasses.clear()
            register(JavaClassesTracker::class.java, changesTracker)
        }

@@ -100,7 +100,27 @@ open class A {

        project.build("build") {
            assertSuccessful()
-            val affectedSources = project.projectDir.getFilesByNames("JavaClassChild.kt", "useJavaClass.kt")
+            val affectedSources = project.projectDir.getFilesByNames("JavaClassChild.kt", "useJavaClass.kt", "useJavaClassFooMethodUsage.kt")
+            val relativePaths = project.relativize(affectedSources)
+            assertCompiledKotlinSources(relativePaths, weakTesting = false)
+        }
+    }
+
+    @Test
+    fun testModifyTrackedJavaClassInLib() {
+        val project = Project("incrementalMultiproject", GRADLE_VERSION)
+        project.build("build") {
+            assertSuccessful()
+        }
+
+        val javaClassJava = project.projectDir.getFileByName("TrackedJavaClass.java")
+        javaClassJava.modify { it.replace("String getString", "Object getString") }
+
+        project.build("build") {
+            assertSuccessful()
+            val affectedSources = project.projectDir.getFilesByNames(
+                    "TrackedJavaClassChild.kt", "useTrackedJavaClass.kt", "useTrackedJavaClassSameModule.kt"
+            )
            val relativePaths = project.relativize(affectedSources)
            assertCompiledKotlinSources(relativePaths, weakTesting = false)
        }
@@ -0,0 +1,21 @@
+/*
+ * Copyright 2010-2017 JetBrains s.r.o.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package foo
+
+import bar.*
+
+class TrackedJavaClassChild : TrackedJavaClass() {}
@@ -0,0 +1,23 @@
+/*
+ * Copyright 2010-2017 JetBrains s.r.o.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package foo
+
+import bar.*
+
+fun useJavaClassFooMethodUsage(jc: JavaClass) {
+    jc.foo()
+}
@@ -0,0 +1,23 @@
+/*
+ * Copyright 2010-2017 JetBrains s.r.o.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package foo
+
+import bar.*
+
+fun useTrackedJavaClass(jc: TrackedJavaClass) {
+    jc.getString()
+}
@@ -0,0 +1,23 @@
+/*
+ * Copyright 2010-2017 JetBrains s.r.o.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package foo
+
+import bar.*
+
+fun useTrackedJavaClassFooMethodUsage(jc: TrackedJavaClass) {
+    jc.foo()
+}
@@ -2,4 +2,5 @@ package bar;

 public class JavaClass {
    public String getString() { return "Hello, World!"; }
-}
+    public String foo() { return ""; }
+}
@@ -0,0 +1,22 @@
+/*
+ * Copyright 2010-2017 JetBrains s.r.o.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package bar;
+
+public class TrackedJavaClass {
+    public String getString() { return "Hello, World!"; }
+    public String foo() { return ""; }
+}
@@ -0,0 +1,5 @@
+package bar
+
+private fun useTrackedJavaClass() {
+    TrackedJavaClass().getString()
+}