From 0074e0209cf42ae09e223c915b21bac1102882a9 Mon Sep 17 00:00:00 2001 From: Ivan Gavrilovic Date: Sat, 19 Feb 2022 15:06:35 +0000 Subject: [PATCH] KT-41456: Incremental KAPT - check compiled sources before running incrementally Because incremental KAPT tries to reuse .class files produced by kotlinc and javac, it should check for their existence before starting an incremental run. Otherwise, annotation processors that perform type validation will fail to run. Current check counts the number of declared types in processed .java sources, and it makes sure the total number of .class files in compiled sources dirs is equal or higher. Otherwise, KAPT runs non-incrementally. Tests: KaptIncrementalWithIsolatingApt.testMissingKotlinOutputForcesNonIncrementalRun ^KT-41456 In Progress --- .../gradle/KaptIncrementalWithIsolatingApt.kt | 17 ++++++++++ .../kotlin/kapt3/base/KaptContext.kt | 10 ++++-- .../kotlin/kapt3/base/incremental/cache.kt | 26 ++++++++++++++- .../base/incremental/classStructureCache.kt | 12 +++++++ .../base/incremental/IncrementalKaptTest.kt | 5 ++- .../incremental/JavaClassCacheManagerTest.kt | 28 +++++++++++++--- .../TestSimpleIncrementalAptCache.kt | 32 ++++++++++++++----- 7 files changed, 112 insertions(+), 18 deletions(-) diff --git a/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/kotlin/org/jetbrains/kotlin/gradle/KaptIncrementalWithIsolatingApt.kt b/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/kotlin/org/jetbrains/kotlin/gradle/KaptIncrementalWithIsolatingApt.kt index 66edec8c525..fe111b75cb8 100644 --- a/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/kotlin/org/jetbrains/kotlin/gradle/KaptIncrementalWithIsolatingApt.kt +++ b/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/kotlin/org/jetbrains/kotlin/gradle/KaptIncrementalWithIsolatingApt.kt @@ -403,6 +403,23 @@ class KaptIncrementalWithIsolatingApt : KaptIncrementalIT() { } } } + + @DisplayName("KT-41456: detect missing kotlin compilation output") + @GradleTest + @DisabledOnOs(OS.WINDOWS, disabledReason = "https://youtrack.jetbrains.com/issue/KTI-405") + fun testMissingKotlinOutputForcesNonIncrementalRun(gradleVersion: GradleVersion) { + kaptProject(gradleVersion) { + build("clean", "assemble") + + // Explicitly remove all kotlinc generated .class files + projectPath.resolve("build/classes/kotlin/main").toFile().listFiles()!!.forEach { it.deleteRecursively() } + javaSourcesDir().resolve("bar/useB.kt").modify { current -> "$current\nfun otherFunction() {}" } + + build("assemble") { + assertOutputContains("Unable to run incrementally, processing all sources.") + } + } + } } private const val patternApt = "Processing java sources with annotation processors:" diff --git a/plugins/kapt3/kapt3-base/src/org/jetbrains/kotlin/kapt3/base/KaptContext.kt b/plugins/kapt3/kapt3-base/src/org/jetbrains/kotlin/kapt3/base/KaptContext.kt index 5cf85417ec6..8432f613ce1 100644 --- a/plugins/kapt3/kapt3-base/src/org/jetbrains/kotlin/kapt3/base/KaptContext.kt +++ b/plugins/kapt3/kapt3-base/src/org/jetbrains/kotlin/kapt3/base/KaptContext.kt @@ -74,10 +74,14 @@ open class KaptContext(val options: KaptOptions, val withJdk: Boolean, val logge sourcesToReprocess = run { val start = System.currentTimeMillis() cacheManager?.invalidateAndGetDirtyFiles( - options.changedFiles, options.classpathChanges - ).also { + options.changedFiles, options.classpathChanges, options.compiledSources + ).also { result -> if (logger.isVerbose) { - logger.info("Computing sources to reprocess took ${System.currentTimeMillis() - start}[ms].") + if (result == SourcesToReprocess.FullRebuild) { + logger.info("Unable to run incrementally, processing all sources.") + } else { + logger.info("Computing sources to reprocess took ${System.currentTimeMillis() - start}[ms].") + } } } }?: SourcesToReprocess.FullRebuild diff --git a/plugins/kapt3/kapt3-base/src/org/jetbrains/kotlin/kapt3/base/incremental/cache.kt b/plugins/kapt3/kapt3-base/src/org/jetbrains/kotlin/kapt3/base/incremental/cache.kt index 3d25d87f9aa..94b658347a0 100644 --- a/plugins/kapt3/kapt3-base/src/org/jetbrains/kotlin/kapt3/base/incremental/cache.kt +++ b/plugins/kapt3/kapt3-base/src/org/jetbrains/kotlin/kapt3/base/incremental/cache.kt @@ -36,11 +36,35 @@ class JavaClassCacheManager(val file: File) : Closeable { * From set of changed sources, get list of files to recompile using structural information and dependency information from * annotation processing. */ - fun invalidateAndGetDirtyFiles(changedSources: Collection, dirtyClasspathJvmNames: Collection): SourcesToReprocess { + fun invalidateAndGetDirtyFiles( + changedSources: Collection, + dirtyClasspathJvmNames: Collection, + compiledSources: List + ): SourcesToReprocess { if (!aptCache.isIncremental) { return SourcesToReprocess.FullRebuild } + /** + * Incremental annotation processing tries to limit the number of .java files that are passes to annotation processors, and it + * uses already existing .class files to resolve types. This relies on outputs of kotlinc and javac being present. + * + * The check below verifies that the number of types defined in the processed .java files matches the number of .class files + * in the compiled sources. This check does not guarantee that .class file will indeed exist for every defined type, but + * even if such check is introduced, there is no guarantee that the actual content of the source file matches the .class file. E.g: + * - clean build i.e non-incremental KAPT + * - modify B.java with error in the method body: KAPT runs successfully, but java compilation will fail (assume old B.class is kept) + * - add C.java that references B as method parameter type: KAPT wil only process C.java, and it will use old version of B + * To properly fix this, KAPT needs to keep track of kotlinc and javac compilation outcome, and there is no clean API to do that. + * + * However, comparing total numbers is probably good enough, and it should eliminate most of the issues. See KT-41456 for details. + */ + val totalCompiledClasses = compiledSources.sumOf { it.walk().filter { it.extension == "class" }.count() } + val totalDeclaredTypes = javaCache.getSourceFileDefinedTypesCount() + if (totalCompiledClasses < totalDeclaredTypes) { + return SourcesToReprocess.FullRebuild + } + val dirtyClasspathFqNames = HashSet(dirtyClasspathJvmNames.size) dirtyClasspathJvmNames.forEach { dirtyClasspathFqNames.add(it.replace("$", ".").replace("/", ".")) diff --git a/plugins/kapt3/kapt3-base/src/org/jetbrains/kotlin/kapt3/base/incremental/classStructureCache.kt b/plugins/kapt3/kapt3-base/src/org/jetbrains/kotlin/kapt3/base/incremental/classStructureCache.kt index 8cdd14f6d1d..27dd111cfe8 100644 --- a/plugins/kapt3/kapt3-base/src/org/jetbrains/kotlin/kapt3/base/incremental/classStructureCache.kt +++ b/plugins/kapt3/kapt3-base/src/org/jetbrains/kotlin/kapt3/base/incremental/classStructureCache.kt @@ -158,6 +158,18 @@ class JavaClassCache() : Serializable { allSources.forEach { sourceCache.remove(it) } } + + /** Returns total number of declared types in .java source files that were processed. */ + fun getSourceFileDefinedTypesCount(): Int { + return sourceCache.values.sumOf { + val structure = it as? SourceFileStructure ?: return@sumOf 0 + if (structure.declaredTypes.size == 1 && structure.declaredTypes.single() == "error.NonExistentClass") { + // never report package for error.NonExistentClass, as it is never compiled by javac/kotlinc + return@sumOf 0 + } + return@sumOf structure.declaredTypes.size + } + } } diff --git a/plugins/kapt3/kapt3-base/test/org/jetbrains/kotlin/kapt3/base/incremental/IncrementalKaptTest.kt b/plugins/kapt3/kapt3-base/test/org/jetbrains/kotlin/kapt3/base/incremental/IncrementalKaptTest.kt index 5fdb35a877f..236b1afa84d 100644 --- a/plugins/kapt3/kapt3-base/test/org/jetbrains/kotlin/kapt3/base/incremental/IncrementalKaptTest.kt +++ b/plugins/kapt3/kapt3-base/test/org/jetbrains/kotlin/kapt3/base/incremental/IncrementalKaptTest.kt @@ -129,7 +129,10 @@ class IncrementalKaptTest { val classesOutput = tmp.newFolder() compileSources(sourcesDir.listFiles().asIterable(), classesOutput) - compileSources(listOf(outputDir.resolve("test/UserGenerated.java")), classesOutput) + compileSources( + listOf(outputDir.resolve("test/UserGenerated.java"), outputDir.resolve("test/AddressGenerated.java")), + classesOutput + ) val optionsForSecondRun = KaptOptions.Builder().apply { projectBaseDir = tmp.newFolder() diff --git a/plugins/kapt3/kapt3-base/test/org/jetbrains/kotlin/kapt3/base/incremental/JavaClassCacheManagerTest.kt b/plugins/kapt3/kapt3-base/test/org/jetbrains/kotlin/kapt3/base/incremental/JavaClassCacheManagerTest.kt index f3c704c24eb..998c600e66e 100644 --- a/plugins/kapt3/kapt3-base/test/org/jetbrains/kotlin/kapt3/base/incremental/JavaClassCacheManagerTest.kt +++ b/plugins/kapt3/kapt3-base/test/org/jetbrains/kotlin/kapt3/base/incremental/JavaClassCacheManagerTest.kt @@ -21,10 +21,12 @@ class JavaClassCacheManagerTest { private lateinit var cache: JavaClassCacheManager private lateinit var cacheDir: File + private lateinit var compiledSources: List @Before fun setUp() { cacheDir = tmp.newFolder() + compiledSources = listOf(tmp.newFolder().also { it.resolve(TEST_PACKAGE_NAME).mkdir() }) cache = JavaClassCacheManager(cacheDir) } @@ -42,21 +44,24 @@ class JavaClassCacheManagerTest { SourceFileStructure(File("Mentioned.java").toURI()).also { it.addDeclaredType("test.Mentioned") cache.javaCache.addSourceStructure(it) + compiledSources.single().resolve("test/Mentioned.class").createNewFile() } SourceFileStructure(File("Src.java").toURI()).also { it.addDeclaredType("test.Src") it.addMentionedType("test.Mentioned") cache.javaCache.addSourceStructure(it) + compiledSources.single().resolve("test/Src.class").createNewFile() } SourceFileStructure(File("ReferencesSrc.java").toURI()).also { it.addDeclaredType("test.ReferencesSrc") it.addPrivateType("test.Src") cache.javaCache.addSourceStructure(it) + compiledSources.single().resolve("test/ReferencesSrc.class").createNewFile() } prepareForIncremental() val dirtyFiles = - cache.invalidateAndGetDirtyFiles(listOf(File("Mentioned.java").absoluteFile), emptyList()) as SourcesToReprocess.Incremental + cache.invalidateAndGetDirtyFiles(listOf(File("Mentioned.java").absoluteFile), emptyList(), compiledSources) as SourcesToReprocess.Incremental assertEquals( listOf( File("Mentioned.java").absoluteFile, @@ -71,21 +76,24 @@ class JavaClassCacheManagerTest { SourceFileStructure(File("Mentioned.java").toURI()).also { it.addDeclaredType("test.Mentioned") cache.javaCache.addSourceStructure(it) + compiledSources.single().resolve("test/Mentioned.class").createNewFile() } SourceFileStructure(File("Src.java").toURI()).also { it.addDeclaredType("test.Src") it.addPrivateType("test.Mentioned") cache.javaCache.addSourceStructure(it) + compiledSources.single().resolve("test/Src.class").createNewFile() } SourceFileStructure(File("ReferencesSrc.java").toURI()).also { it.addDeclaredType("test.ReferencesSrc") it.addPrivateType("test.Src") cache.javaCache.addSourceStructure(it) + compiledSources.single().resolve("test/ReferencesSrc.class").createNewFile() } prepareForIncremental() val dirtyFiles = - cache.invalidateAndGetDirtyFiles(listOf(File("Mentioned.java").absoluteFile), emptyList()) as SourcesToReprocess.Incremental + cache.invalidateAndGetDirtyFiles(listOf(File("Mentioned.java").absoluteFile), emptyList(), compiledSources) as SourcesToReprocess.Incremental assertEquals( listOf( File("Mentioned.java").absoluteFile, @@ -100,21 +108,25 @@ class JavaClassCacheManagerTest { it.addDeclaredType("test.TwoTypes") it.addDeclaredType("test.AnotherType") cache.javaCache.addSourceStructure(it) + compiledSources.single().resolve("test/TwoTypes.class").createNewFile() + compiledSources.single().resolve("test/AnotherType.class").createNewFile() } SourceFileStructure(File("ReferencesTwoTypes.java").toURI()).also { it.addDeclaredType("test.ReferencesTwoTypes") it.addPrivateType("test.TwoTypes") cache.javaCache.addSourceStructure(it) + compiledSources.single().resolve("test/ReferencesTwoTypes.class").createNewFile() } SourceFileStructure(File("ReferencesAnotherType.java").toURI()).also { it.addDeclaredType("test.ReferencesAnotherType") it.addPrivateType("test.AnotherType") cache.javaCache.addSourceStructure(it) + compiledSources.single().resolve("test/ReferencesAnotherType.class").createNewFile() } prepareForIncremental() val dirtyFiles = - cache.invalidateAndGetDirtyFiles(listOf(File("TwoTypes.java").absoluteFile), emptyList()) as SourcesToReprocess.Incremental + cache.invalidateAndGetDirtyFiles(listOf(File("TwoTypes.java").absoluteFile), emptyList(), compiledSources) as SourcesToReprocess.Incremental assertEquals( listOf( File("TwoTypes.java").absoluteFile, @@ -130,10 +142,11 @@ class JavaClassCacheManagerTest { it.addDeclaredType("test.Src") it.addMentionedType("test.Mentioned") cache.javaCache.addSourceStructure(it) + compiledSources.single().resolve("test/Src.class").createNewFile() } prepareForIncremental() - val dirtyFiles = cache.invalidateAndGetDirtyFiles(listOf(), listOf("test/Mentioned")) as SourcesToReprocess.Incremental + val dirtyFiles = cache.invalidateAndGetDirtyFiles(listOf(), listOf("test/Mentioned"), compiledSources) as SourcesToReprocess.Incremental assertEquals(listOf(File("Src.java").absoluteFile), dirtyFiles.toReprocess) } @@ -142,22 +155,25 @@ class JavaClassCacheManagerTest { SourceFileStructure(File("Constants.java").toURI()).also { it.addDeclaredType("test.Constants") cache.javaCache.addSourceStructure(it) + compiledSources.single().resolve("test/Constants.class").createNewFile() } SourceFileStructure(File("MentionsConst.java").toURI()).also { it.addDeclaredType("test.MentionsConst") it.addMentionedConstant("test.Constants", "CONST") cache.javaCache.addSourceStructure(it) + compiledSources.single().resolve("test/MentionsConst.class").createNewFile() } SourceFileStructure(File("MentionsOtherConst.java").toURI()).also { it.addDeclaredType("test.MentionsOtherConst") it.addMentionedConstant("test.OtherConstants", "CONST") cache.javaCache.addSourceStructure(it) + compiledSources.single().resolve("test/MentionsOtherConst.class").createNewFile() } prepareForIncremental() val dirtyFiles = cache.invalidateAndGetDirtyFiles( - listOf(File("Constants.java").absoluteFile), emptyList() + listOf(File("Constants.java").absoluteFile), emptyList(), compiledSources ) as SourcesToReprocess.Incremental assertEquals( listOf(File("Constants.java").absoluteFile, File("MentionsConst.java").absoluteFile), @@ -170,3 +186,5 @@ class JavaClassCacheManagerTest { cache = JavaClassCacheManager(cacheDir) } } + +private const val TEST_PACKAGE_NAME = "test" \ No newline at end of file diff --git a/plugins/kapt3/kapt3-base/test/org/jetbrains/kotlin/kapt3/base/incremental/TestSimpleIncrementalAptCache.kt b/plugins/kapt3/kapt3-base/test/org/jetbrains/kotlin/kapt3/base/incremental/TestSimpleIncrementalAptCache.kt index abb1259d2d2..7e786c5cf9b 100644 --- a/plugins/kapt3/kapt3-base/test/org/jetbrains/kotlin/kapt3/base/incremental/TestSimpleIncrementalAptCache.kt +++ b/plugins/kapt3/kapt3-base/test/org/jetbrains/kotlin/kapt3/base/incremental/TestSimpleIncrementalAptCache.kt @@ -5,10 +5,7 @@ package org.jetbrains.kotlin.kapt.base.test.org.jetbrains.kotlin.kapt3.base.incremental -import org.jetbrains.kotlin.kapt3.base.incremental.IncrementalProcessor -import org.jetbrains.kotlin.kapt3.base.incremental.JavaClassCacheManager -import org.jetbrains.kotlin.kapt3.base.incremental.MentionedTypesTaskListener -import org.jetbrains.kotlin.kapt3.base.incremental.SourcesToReprocess +import org.jetbrains.kotlin.kapt3.base.incremental.* import org.junit.Assert.* import org.junit.Before import org.junit.Rule @@ -24,11 +21,13 @@ class TestSimpleIncrementalAptCache { private lateinit var cache: JavaClassCacheManager private lateinit var generatedSources: File + private lateinit var compiledSources: List @Before fun setUp() { cache = JavaClassCacheManager(tmp.newFolder()) generatedSources = tmp.newFolder() + compiledSources = listOf(tmp.newFolder().also { it.resolve(TEST_PACKAGE_NAME).mkdir() }) cache.close() } @@ -36,7 +35,11 @@ class TestSimpleIncrementalAptCache { fun testAggregatingAnnotations() { runProcessor(SimpleProcessor().toAggregating()) - val dirtyFiles = cache.invalidateAndGetDirtyFiles(listOf(TEST_DATA_DIR.resolve("User.java").absoluteFile), emptyList()) as SourcesToReprocess.Incremental + val dirtyFiles = cache.invalidateAndGetDirtyFiles( + listOf(TEST_DATA_DIR.resolve("User.java").absoluteFile), + emptyList(), + compiledSources + ) as SourcesToReprocess.Incremental assertEquals( listOf(TEST_DATA_DIR.resolve("User.java").absoluteFile, TEST_DATA_DIR.resolve("Address.java").absoluteFile), dirtyFiles.toReprocess @@ -49,7 +52,11 @@ class TestSimpleIncrementalAptCache { fun testIsolatingAnnotations() { runProcessor(SimpleProcessor().toIsolating()) - val dirtyFiles = cache.invalidateAndGetDirtyFiles(listOf(TEST_DATA_DIR.resolve("User.java").absoluteFile), emptyList()) as SourcesToReprocess.Incremental + val dirtyFiles = cache.invalidateAndGetDirtyFiles( + listOf(TEST_DATA_DIR.resolve("User.java").absoluteFile), + emptyList(), + compiledSources + ) as SourcesToReprocess.Incremental assertFalse(generatedSources.resolve("test/UserGenerated.java").exists()) assertEquals( listOf(TEST_DATA_DIR.resolve("User.java").absoluteFile), @@ -61,7 +68,7 @@ class TestSimpleIncrementalAptCache { fun testNonIncremental() { runProcessor(SimpleProcessor().toNonIncremental()) - val dirtyFiles = cache.invalidateAndGetDirtyFiles(listOf(TEST_DATA_DIR.resolve("User.java").absoluteFile), emptyList()) + val dirtyFiles = cache.invalidateAndGetDirtyFiles(listOf(TEST_DATA_DIR.resolve("User.java").absoluteFile), emptyList(), compiledSources) assertTrue(dirtyFiles is SourcesToReprocess.FullRebuild) } @@ -73,5 +80,14 @@ class TestSimpleIncrementalAptCache { generatedSources ) { elementUtils, trees -> MentionedTypesTaskListener(cache.javaCache, elementUtils, trees) } cache.updateCache(listOf(processor), false) + + // add mock compiled source files + compiledSources.single().resolve("test/User.class").createNewFile() + compiledSources.single().resolve("test/Address.class").createNewFile() + compiledSources.single().resolve("test/Observable.class").createNewFile() + compiledSources.single().resolve("test/UserGenerated.class").createNewFile() + compiledSources.single().resolve("test/AddressGenerated.class").createNewFile() } -} \ No newline at end of file +} + +private const val TEST_PACKAGE_NAME = "test" \ No newline at end of file