From 3c9d6535954930e801f25388d0e42bad070b9051 Mon Sep 17 00:00:00 2001 From: Alexander Korepanov Date: Mon, 20 Feb 2023 11:54:47 +0100 Subject: [PATCH] [JS IR] Ignore unbound symbols in IC infrastructure The IR linker is responsible for detecting unbound symbols, if it skips them for some reason, IC infrastructure must not fail with unbound symbols exceptions. Anyway, the IR validator verifies later if there are unbound symbols in reachable IR and generates the corresponding error. ^KT-56602 Fixed --- .../kotlin/ir/backend/js/ic/IdSignatureSource.kt | 10 +++++++++- .../org/jetbrains/kotlin/codegen/TestModel.kt | 11 +++++++++-- .../incremental/AbstractInvalidationTest.kt | 8 ++++---- .../JsIrES6InvalidationTestGenerated.java | 5 +++++ .../JsIrInvalidationTestGenerated.java | 5 +++++ .../lib2/module.info | 3 +++ .../lib1/l1.0.kt | 5 +++++ .../lib1/l1.1.kt | 5 +++++ .../lib1/module.info | 9 +++++++++ .../lib2/l2.0.kt | 5 +++++ .../lib2/module.info | 8 ++++++++ .../main/m.kt | 16 ++++++++++++++++ .../main/module.info | 6 ++++++ .../project.info | 8 ++++++++ .../genericInlineFunctions/libProxy/module.info | 1 + .../Kotlin2JsIrBeIncrementalCompilationIT.kt | 9 +++------ 16 files changed, 101 insertions(+), 13 deletions(-) create mode 100644 js/js.translator/testData/incremental/invalidation/breakKlibBinaryCompatibilityWithVariance/lib1/l1.0.kt create mode 100644 js/js.translator/testData/incremental/invalidation/breakKlibBinaryCompatibilityWithVariance/lib1/l1.1.kt create mode 100644 js/js.translator/testData/incremental/invalidation/breakKlibBinaryCompatibilityWithVariance/lib1/module.info create mode 100644 js/js.translator/testData/incremental/invalidation/breakKlibBinaryCompatibilityWithVariance/lib2/l2.0.kt create mode 100644 js/js.translator/testData/incremental/invalidation/breakKlibBinaryCompatibilityWithVariance/lib2/module.info create mode 100644 js/js.translator/testData/incremental/invalidation/breakKlibBinaryCompatibilityWithVariance/main/m.kt create mode 100644 js/js.translator/testData/incremental/invalidation/breakKlibBinaryCompatibilityWithVariance/main/module.info create mode 100644 js/js.translator/testData/incremental/invalidation/breakKlibBinaryCompatibilityWithVariance/project.info diff --git a/compiler/ir/backend.js/src/org/jetbrains/kotlin/ir/backend/js/ic/IdSignatureSource.kt b/compiler/ir/backend.js/src/org/jetbrains/kotlin/ir/backend/js/ic/IdSignatureSource.kt index d57305c3e76..8da49483bc6 100644 --- a/compiler/ir/backend.js/src/org/jetbrains/kotlin/ir/backend/js/ic/IdSignatureSource.kt +++ b/compiler/ir/backend.js/src/org/jetbrains/kotlin/ir/backend/js/ic/IdSignatureSource.kt @@ -115,7 +115,15 @@ internal sealed class FileSignatureProvider(val irFile: IrFile) { } override fun getImplementedSymbols(): Map { - return collectImplementedSymbol(fileDeserializer.symbolDeserializer.deserializedSymbols) + // Sometimes linker may leave unbound symbols in IrSymbolDeserializer::deserializedSymbols map. + // Generally, all unbound symbols must be caught in KotlinIrLinker::checkNoUnboundSymbols, + // unfortunately it does not work properly in the current implementation. + // Also, reachable unbound symbols are caught by IrValidator, it works fine, but it works after this place. + // Filter unbound symbols here, because an error from IC infrastructure about the unbound symbols looks pretty wired + // and if the unbound symbol is really reachable from IR the error will be fired from IrValidator later. + // Otherwise, the unbound symbol is unreachable, and it cannot appear in IC dependency graph, so we can ignore them. + val deserializedSymbols = fileDeserializer.symbolDeserializer.deserializedSymbols.filter { it.value.isBound } + return collectImplementedSymbol(deserializedSymbols) } } diff --git a/compiler/tests-compiler-utils/tests/org/jetbrains/kotlin/codegen/TestModel.kt b/compiler/tests-compiler-utils/tests/org/jetbrains/kotlin/codegen/TestModel.kt index 262970831f2..20134d21a49 100644 --- a/compiler/tests-compiler-utils/tests/org/jetbrains/kotlin/codegen/TestModel.kt +++ b/compiler/tests-compiler-utils/tests/org/jetbrains/kotlin/codegen/TestModel.kt @@ -48,7 +48,8 @@ class ModuleInfo(val moduleName: String) { val dependencies: Collection, val modifications: List, val expectedFileStats: Map>, - val expectedDTS: Set + val expectedDTS: Set, + val rebuildKlib: Boolean ) val steps = mutableListOf() @@ -67,6 +68,7 @@ private const val MODIFICATIONS = "modifications" private const val MODIFICATION_UPDATE = "U" private const val MODIFICATION_DELETE = "D" private const val EXPECTED_DTS_LIST = "expected dts" +private const val REBUILD_KLIB = "rebuild klib" private val STEP_PATTERN = Pattern.compile("^\\s*STEP\\s+(\\d+)\\.*(\\d+)?\\s*:?$") @@ -213,6 +215,7 @@ class ModuleInfoParser(infoFile: File) : InfoParser(infoFile) { val friendDependencies = mutableSetOf() val modifications = mutableListOf() val expectedDTS = mutableSetOf() + var rebuildKlib = true loop { line -> if (line.matches(STEP_PATTERN.toRegex())) @@ -234,6 +237,9 @@ class ModuleInfoParser(infoFile: File) : InfoParser(infoFile) { FRIENDS -> getOpArgs().forEach { friendDependencies += it } MODIFICATIONS -> modifications += parseModifications() EXPECTED_DTS_LIST -> getOpArgs().forEach { expectedDTS += it } + REBUILD_KLIB -> getOpArgs().singleOrNull()?.toBooleanStrictOrNull()?.let { + rebuildKlib = it + } ?: error(diagnosticMessage("$op expects true or false", line)) else -> error(diagnosticMessage("Unknown op $op", line)) } } @@ -255,7 +261,8 @@ class ModuleInfoParser(infoFile: File) : InfoParser(infoFile) { dependencies = dependencies, modifications = modifications, expectedFileStats = expectedFileStats, - expectedDTS = expectedDTS + expectedDTS = expectedDTS, + rebuildKlib = rebuildKlib ) } } diff --git a/js/js.tests/test/org/jetbrains/kotlin/incremental/AbstractInvalidationTest.kt b/js/js.tests/test/org/jetbrains/kotlin/incremental/AbstractInvalidationTest.kt index 7f7f8804199..e308bfe07b9 100644 --- a/js/js.tests/test/org/jetbrains/kotlin/incremental/AbstractInvalidationTest.kt +++ b/js/js.tests/test/org/jetbrains/kotlin/incremental/AbstractInvalidationTest.kt @@ -152,7 +152,7 @@ abstract class AbstractInvalidationTest( val expectedDTS: String? ) - private fun setupTestStep(projStep: ProjectInfo.ProjectBuildStep, module: String, buildKlib: Boolean): TestStepInfo { + private fun setupTestStep(projStep: ProjectInfo.ProjectBuildStep, module: String): TestStepInfo { val projStepId = projStep.id val moduleTestDir = File(testDir, module) val moduleSourceDir = File(sourceDir, module) @@ -172,7 +172,7 @@ abstract class AbstractInvalidationTest( val outputKlibFile = resolveModuleArtifact(module, buildDir) val friends = mutableListOf() - if (buildKlib) { + if (moduleStep.rebuildKlib) { val dependencies = mutableListOf(File(STDLIB_KLIB)) for (dep in moduleStep.dependencies) { val klibFile = resolveModuleArtifact(dep.moduleName, buildDir) @@ -313,7 +313,7 @@ abstract class AbstractInvalidationTest( fun execute() { for (projStep in projectInfo.steps) { - val testInfo = projStep.order.map { setupTestStep(projStep, it, true) } + val testInfo = projStep.order.map { setupTestStep(projStep, it) } val mainModuleInfo = testInfo.last() testInfo.find { it != mainModuleInfo && it.friends.isNotEmpty() }?.let { @@ -341,7 +341,7 @@ abstract class AbstractInvalidationTest( } ) - val removedModulesInfo = (projectInfo.modules - projStep.order.toSet()).map { setupTestStep(projStep, it, false) } + val removedModulesInfo = (projectInfo.modules - projStep.order.toSet()).map { setupTestStep(projStep, it) } val icCaches = cacheUpdater.actualizeCaches() verifyCacheUpdateStats(projStep.id, cacheUpdater.getDirtyFileLastStats(), testInfo + removedModulesInfo) diff --git a/js/js.tests/tests-gen/org/jetbrains/kotlin/incremental/JsIrES6InvalidationTestGenerated.java b/js/js.tests/tests-gen/org/jetbrains/kotlin/incremental/JsIrES6InvalidationTestGenerated.java index 415c18f9127..9778e6bd4d8 100644 --- a/js/js.tests/tests-gen/org/jetbrains/kotlin/incremental/JsIrES6InvalidationTestGenerated.java +++ b/js/js.tests/tests-gen/org/jetbrains/kotlin/incremental/JsIrES6InvalidationTestGenerated.java @@ -40,6 +40,11 @@ public class JsIrES6InvalidationTestGenerated extends AbstractJsIrES6Invalidatio KtTestUtil.assertAllTestsPresentByMetadataWithExcluded(this.getClass(), new File("js/js.translator/testData/incremental/invalidation"), Pattern.compile("^([^_](.+))$"), null, TargetBackend.JS_IR_ES6, false); } + @TestMetadata("breakKlibBinaryCompatibilityWithVariance") + public void testBreakKlibBinaryCompatibilityWithVariance() throws Exception { + runTest("js/js.translator/testData/incremental/invalidation/breakKlibBinaryCompatibilityWithVariance/"); + } + @TestMetadata("circleExportsUpdate") public void testCircleExportsUpdate() throws Exception { runTest("js/js.translator/testData/incremental/invalidation/circleExportsUpdate/"); diff --git a/js/js.tests/tests-gen/org/jetbrains/kotlin/incremental/JsIrInvalidationTestGenerated.java b/js/js.tests/tests-gen/org/jetbrains/kotlin/incremental/JsIrInvalidationTestGenerated.java index 420db0724ea..ed6cddee232 100644 --- a/js/js.tests/tests-gen/org/jetbrains/kotlin/incremental/JsIrInvalidationTestGenerated.java +++ b/js/js.tests/tests-gen/org/jetbrains/kotlin/incremental/JsIrInvalidationTestGenerated.java @@ -40,6 +40,11 @@ public class JsIrInvalidationTestGenerated extends AbstractJsIrInvalidationTest KtTestUtil.assertAllTestsPresentByMetadataWithExcluded(this.getClass(), new File("js/js.translator/testData/incremental/invalidation"), Pattern.compile("^([^_](.+))$"), null, TargetBackend.JS_IR, false); } + @TestMetadata("breakKlibBinaryCompatibilityWithVariance") + public void testBreakKlibBinaryCompatibilityWithVariance() throws Exception { + runTest("js/js.translator/testData/incremental/invalidation/breakKlibBinaryCompatibilityWithVariance/"); + } + @TestMetadata("circleExportsUpdate") public void testCircleExportsUpdate() throws Exception { runTest("js/js.translator/testData/incremental/invalidation/circleExportsUpdate/"); diff --git a/js/js.translator/testData/incremental/invalidation/addUpdateRemoveDependentModule/lib2/module.info b/js/js.translator/testData/incremental/invalidation/addUpdateRemoveDependentModule/lib2/module.info index 551c2f0d50e..6256d9c3f99 100644 --- a/js/js.translator/testData/incremental/invalidation/addUpdateRemoveDependentModule/lib2/module.info +++ b/js/js.translator/testData/incremental/invalidation/addUpdateRemoveDependentModule/lib2/module.info @@ -1,4 +1,5 @@ STEP 0: + rebuild klib: false STEP 1: dependencies: lib1 modifications: @@ -10,6 +11,8 @@ STEP 2: U : l2.2.kt -> l2.kt modified ir: l2.kt STEP 3: + rebuild klib: false modifications: D : l2.kt STEP 4: + rebuild klib: false diff --git a/js/js.translator/testData/incremental/invalidation/breakKlibBinaryCompatibilityWithVariance/lib1/l1.0.kt b/js/js.translator/testData/incremental/invalidation/breakKlibBinaryCompatibilityWithVariance/lib1/l1.0.kt new file mode 100644 index 00000000000..9fe43152ef6 --- /dev/null +++ b/js/js.translator/testData/incremental/invalidation/breakKlibBinaryCompatibilityWithVariance/lib1/l1.0.kt @@ -0,0 +1,5 @@ +interface GenericInterface + +interface TestInterface { + fun test(f: () -> GenericInterface) {} +} diff --git a/js/js.translator/testData/incremental/invalidation/breakKlibBinaryCompatibilityWithVariance/lib1/l1.1.kt b/js/js.translator/testData/incremental/invalidation/breakKlibBinaryCompatibilityWithVariance/lib1/l1.1.kt new file mode 100644 index 00000000000..fcd9728dba2 --- /dev/null +++ b/js/js.translator/testData/incremental/invalidation/breakKlibBinaryCompatibilityWithVariance/lib1/l1.1.kt @@ -0,0 +1,5 @@ +interface GenericInterface + +interface TestInterface { + fun test(f: () -> GenericInterface) {} +} diff --git a/js/js.translator/testData/incremental/invalidation/breakKlibBinaryCompatibilityWithVariance/lib1/module.info b/js/js.translator/testData/incremental/invalidation/breakKlibBinaryCompatibilityWithVariance/lib1/module.info new file mode 100644 index 00000000000..cfe5f544e59 --- /dev/null +++ b/js/js.translator/testData/incremental/invalidation/breakKlibBinaryCompatibilityWithVariance/lib1/module.info @@ -0,0 +1,9 @@ +STEP 0: + modifications: + U : l1.0.kt -> l1.kt + added file: l1.kt +STEP 1: + modifications: + U : l1.1.kt -> l1.kt + modified ir: l1.kt + updated exports: l1.kt diff --git a/js/js.translator/testData/incremental/invalidation/breakKlibBinaryCompatibilityWithVariance/lib2/l2.0.kt b/js/js.translator/testData/incremental/invalidation/breakKlibBinaryCompatibilityWithVariance/lib2/l2.0.kt new file mode 100644 index 00000000000..2991eeecc73 --- /dev/null +++ b/js/js.translator/testData/incremental/invalidation/breakKlibBinaryCompatibilityWithVariance/lib2/l2.0.kt @@ -0,0 +1,5 @@ +class TestClass : TestInterface { + override fun test(f: () -> GenericInterface) { + f() + } +} diff --git a/js/js.translator/testData/incremental/invalidation/breakKlibBinaryCompatibilityWithVariance/lib2/module.info b/js/js.translator/testData/incremental/invalidation/breakKlibBinaryCompatibilityWithVariance/lib2/module.info new file mode 100644 index 00000000000..ee248f0e59d --- /dev/null +++ b/js/js.translator/testData/incremental/invalidation/breakKlibBinaryCompatibilityWithVariance/lib2/module.info @@ -0,0 +1,8 @@ +STEP 0: + dependencies: lib1 + modifications: + U : l2.0.kt -> l2.kt + added file: l2.kt +STEP 1: + rebuild klib: false + updated imports: l2.kt diff --git a/js/js.translator/testData/incremental/invalidation/breakKlibBinaryCompatibilityWithVariance/main/m.kt b/js/js.translator/testData/incremental/invalidation/breakKlibBinaryCompatibilityWithVariance/main/m.kt new file mode 100644 index 00000000000..94857eabb73 --- /dev/null +++ b/js/js.translator/testData/incremental/invalidation/breakKlibBinaryCompatibilityWithVariance/main/m.kt @@ -0,0 +1,16 @@ +fun box(stepId: Int): String { + var x = -1 + TestClass().test { + object : GenericInterface { + init { + x = stepId + } + } + } + + when (stepId) { + in 0..1 -> if (x != stepId) return "Fail, got $x" + else -> return "Unknown" + } + return "OK" +} diff --git a/js/js.translator/testData/incremental/invalidation/breakKlibBinaryCompatibilityWithVariance/main/module.info b/js/js.translator/testData/incremental/invalidation/breakKlibBinaryCompatibilityWithVariance/main/module.info new file mode 100644 index 00000000000..a7fcdfeed8d --- /dev/null +++ b/js/js.translator/testData/incremental/invalidation/breakKlibBinaryCompatibilityWithVariance/main/module.info @@ -0,0 +1,6 @@ +STEP 0: + dependencies: lib1, lib2 + added file: m.kt +STEP 1: + dependencies: lib1, lib2 + updated imports: m.kt diff --git a/js/js.translator/testData/incremental/invalidation/breakKlibBinaryCompatibilityWithVariance/project.info b/js/js.translator/testData/incremental/invalidation/breakKlibBinaryCompatibilityWithVariance/project.info new file mode 100644 index 00000000000..446b4d0374c --- /dev/null +++ b/js/js.translator/testData/incremental/invalidation/breakKlibBinaryCompatibilityWithVariance/project.info @@ -0,0 +1,8 @@ +MODULES: lib1, lib2, main + +STEP 0: + libs: lib1, lib2, main + dirty js: lib1, lib2, main +STEP 1: + libs: lib1, lib2, main + dirty js: lib1, lib2, main diff --git a/js/js.translator/testData/incremental/invalidation/genericInlineFunctions/libProxy/module.info b/js/js.translator/testData/incremental/invalidation/genericInlineFunctions/libProxy/module.info index 678e80eef6a..c07c0ddd43c 100644 --- a/js/js.translator/testData/incremental/invalidation/genericInlineFunctions/libProxy/module.info +++ b/js/js.translator/testData/incremental/invalidation/genericInlineFunctions/libProxy/module.info @@ -1,4 +1,5 @@ STEP 0..7: + rebuild klib: false STEP 8: dependencies: lib1 modifications: diff --git a/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/kotlin/org/jetbrains/kotlin/gradle/Kotlin2JsIrBeIncrementalCompilationIT.kt b/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/kotlin/org/jetbrains/kotlin/gradle/Kotlin2JsIrBeIncrementalCompilationIT.kt index bac21517ffb..8bf816d9c6b 100644 --- a/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/kotlin/org/jetbrains/kotlin/gradle/Kotlin2JsIrBeIncrementalCompilationIT.kt +++ b/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/kotlin/org/jetbrains/kotlin/gradle/Kotlin2JsIrBeIncrementalCompilationIT.kt @@ -43,6 +43,7 @@ class Kotlin2JsIrBeIncrementalCompilationIT : KGPBaseTest() { } val srcFile = projectPath.resolve("app/src/main/kotlin/App.kt").toFile() + val guardFile = projectPath.resolve("app/build/klib/cache/cache.guard").toFile() val badCode = srcFile.readText() var successfulBuildCacheFiles = emptyMap() @@ -56,15 +57,10 @@ class Kotlin2JsIrBeIncrementalCompilationIT : KGPBaseTest() { srcFile.writeText(badCode) - val guardFile = projectPath.resolve("app/build/klib/cache/cache.guard").toFile() for (i in 0..1) { buildAndFail("nodeDevelopmentRun") { assertTasksFailed(":app:compileDevelopmentExecutableKotlinJs") - val failedBuildCacheFiles = readCacheFiles() - val expectedFiles = successfulBuildCacheFiles.toMutableMap() - expectedFiles[guardFile.absolutePath] = ByteArray(0).contentHashCode() - assertEquals(expectedFiles, failedBuildCacheFiles, "The cache files should not be modified") - guardFile.delete() + assertTrue("guard file after compilation error expected") { guardFile.exists() } } } @@ -75,6 +71,7 @@ class Kotlin2JsIrBeIncrementalCompilationIT : KGPBaseTest() { val successfulRebuildCacheFiles = readCacheFiles() assertEquals(successfulBuildCacheFiles.size, successfulRebuildCacheFiles.size, "The number of files must be the same") assertNotEquals(successfulBuildCacheFiles, successfulRebuildCacheFiles, "The cache files should be modified") + assertFalse("guard file after successful compilation must be removed") { guardFile.exists() } } } }