From e1f8bac4d5037b7347ab401e121725d23bd2f09c Mon Sep 17 00:00:00 2001 From: Alexey Andreev Date: Thu, 19 Oct 2017 12:19:34 +0300 Subject: [PATCH] JS: several bugfixes in incremental compilation 1. Fix exported packages sometimes being mixed up 2. Fix metadata losing package fragments sometimes 3. Don't serialize empty packages to .meta.js 4. Preserve order of package fragments in .meta.js 5. In IC tests, compare textual representation of metadata rather than binary representation --- .../js/KotlinJavascriptSerializationUtil.kt | 14 ++++----- ...kotlinJavascriptPackageFragmentProvider.kt | 25 +++++++++++---- js/js.tests/build.gradle.kts | 2 ++ .../jetbrains/kotlin/js/test/BasicBoxTest.kt | 21 +++++++++++-- .../js/test/semantics/BoxJsTestGenerated.java | 6 ++++ .../kotlin/js/translate/general/Merger.kt | 2 +- .../box/incremental/packagesWithSameName.kt | 31 +++++++++++++++++++ 7 files changed, 84 insertions(+), 17 deletions(-) create mode 100644 js/js.translator/testData/box/incremental/packagesWithSameName.kt diff --git a/js/js.serializer/src/org/jetbrains/kotlin/serialization/js/KotlinJavascriptSerializationUtil.kt b/js/js.serializer/src/org/jetbrains/kotlin/serialization/js/KotlinJavascriptSerializationUtil.kt index ccce9e1c083..4f402c462aa 100644 --- a/js/js.serializer/src/org/jetbrains/kotlin/serialization/js/KotlinJavascriptSerializationUtil.kt +++ b/js/js.serializer/src/org/jetbrains/kotlin/serialization/js/KotlinJavascriptSerializationUtil.kt @@ -39,7 +39,6 @@ import org.jetbrains.kotlin.utils.KotlinJavascriptMetadataUtils import java.io.ByteArrayInputStream import java.io.ByteArrayOutputStream import java.io.DataOutputStream -import java.util.* import java.util.zip.GZIPInputStream import java.util.zip.GZIPOutputStream @@ -109,7 +108,7 @@ object KotlinJavascriptSerializationUtil { for (fqName in getPackagesFqNames(module)) { val fragment = serializePackageFragment(bindingContext, module, fqName) - if (fragment.hasPackage() || fragment.class_Count > 0) { + if (!fragment.isEmpty()) { builder.addPackageFragment(fragment) } } @@ -212,13 +211,11 @@ object KotlinJavascriptSerializationUtil { module: ModuleDescriptor, languageVersionSettings: LanguageVersionSettings ): Map { - val contentMap = hashMapOf() + val contentMap = mutableMapOf() for (fqName in getPackagesFqNames(module)) { val part = serializePackageFragment(bindingContext, module, fqName) - if (part.class_Count == 0 && part.`package`.let { packageProto -> - packageProto.functionCount == 0 && packageProto.propertyCount == 0 && packageProto.typeAliasCount == 0 - }) continue + if (part.isEmpty()) continue val stream = ByteArrayOutputStream() with(DataOutputStream(stream)) { @@ -236,6 +233,9 @@ object KotlinJavascriptSerializationUtil { return contentMap } + private fun ProtoBuf.PackageFragment.isEmpty(): Boolean = + class_Count == 0 && `package`.let { it.functionCount == 0 && it.propertyCount == 0 && it.typeAliasCount == 0 } + fun serializeHeader(packageFqName: FqName?, languageVersionSettings: LanguageVersionSettings): JsProtoBuf.Header { val header = JsProtoBuf.Header.newBuilder() @@ -253,7 +253,7 @@ object KotlinJavascriptSerializationUtil { } private fun getPackagesFqNames(module: ModuleDescriptor): Set { - return HashSet().apply { + return mutableSetOf().apply { getSubPackagesFqNames(module.getPackage(FqName.ROOT), this) add(FqName.ROOT) } diff --git a/js/js.serializer/src/org/jetbrains/kotlin/serialization/js/kotlinJavascriptPackageFragmentProvider.kt b/js/js.serializer/src/org/jetbrains/kotlin/serialization/js/kotlinJavascriptPackageFragmentProvider.kt index eca0828610b..ca484411922 100644 --- a/js/js.serializer/src/org/jetbrains/kotlin/serialization/js/kotlinJavascriptPackageFragmentProvider.kt +++ b/js/js.serializer/src/org/jetbrains/kotlin/serialization/js/kotlinJavascriptPackageFragmentProvider.kt @@ -17,13 +17,12 @@ package org.jetbrains.kotlin.serialization.js import org.jetbrains.kotlin.contracts.ContractDeserializerImpl -import org.jetbrains.kotlin.descriptors.ModuleDescriptor -import org.jetbrains.kotlin.descriptors.NotFoundClasses -import org.jetbrains.kotlin.descriptors.PackageFragmentProvider -import org.jetbrains.kotlin.descriptors.PackageFragmentProviderImpl +import org.jetbrains.kotlin.descriptors.* import org.jetbrains.kotlin.descriptors.deserialization.PlatformDependentDeclarationFilter +import org.jetbrains.kotlin.descriptors.impl.EmptyPackageFragmentDescriptor import org.jetbrains.kotlin.incremental.components.LookupTracker import org.jetbrains.kotlin.name.FqName +import org.jetbrains.kotlin.name.parentOrNull import org.jetbrains.kotlin.serialization.ProtoBuf import org.jetbrains.kotlin.serialization.deserialization.* import org.jetbrains.kotlin.storage.StorageManager @@ -36,12 +35,26 @@ fun createKotlinJavascriptPackageFragmentProvider( configuration: DeserializationConfiguration, lookupTracker: LookupTracker ): PackageFragmentProvider { - val packageFragments = packageFragmentProtos.mapNotNull { proto -> + val packageFragments: MutableList = packageFragmentProtos.mapNotNullTo(mutableListOf()) { proto -> proto.fqName?.let { fqName -> KotlinJavascriptPackageFragment(fqName, storageManager, module, proto, header, configuration) } } + // Generate empty PackageFragmentDescriptor instances for packages that aren't mentioned in compilation units directly. + // For example, if there's `package foo.bar` directive, we'll get only PackageFragmentDescriptor for `foo.bar`, but + // none for `foo`. Various descriptor/scope code relies on presence of such package fragments, and currently we + // don't know if it's possible to fix this. + // TODO: think about fixing issues in descriptors/scopes + val packageFqNames = packageFragmentProtos.mapNotNullTo(mutableSetOf()) { it.fqName } + for (packageFqName in packageFqNames.mapNotNull { it.parentOrNull() }) { + var ancestorFqName = packageFqName + while (!ancestorFqName.isRoot && packageFqNames.add(ancestorFqName)) { + packageFragments += EmptyPackageFragmentDescriptor(module, ancestorFqName) + ancestorFqName = ancestorFqName.parent() + } + } + val provider = PackageFragmentProviderImpl(packageFragments) val notFoundClasses = NotFoundClasses(storageManager, module) @@ -63,7 +76,7 @@ fun createKotlinJavascriptPackageFragmentProvider( platformDependentDeclarationFilter = PlatformDependentDeclarationFilter.NoPlatformDependent ) - for (packageFragment in packageFragments) { + for (packageFragment in packageFragments.filterIsInstance()) { packageFragment.components = components } diff --git a/js/js.tests/build.gradle.kts b/js/js.tests/build.gradle.kts index 818e372f0d9..95f0255e0c1 100644 --- a/js/js.tests/build.gradle.kts +++ b/js/js.tests/build.gradle.kts @@ -2,6 +2,7 @@ apply { plugin("kotlin") } dependencies { + testCompile(protobufFull()) testCompile(project(":compiler.tests-common")) testCompileOnly(project(":compiler:frontend")) testCompileOnly(project(":compiler:cli")) @@ -19,6 +20,7 @@ dependencies { testRuntime(project(":compiler:backend-common")) // testRuntime(ideaSdkDeps("*.jar")) testRuntime(commonDep("org.fusesource.jansi", "jansi")) + testCompile(projectTests(":kotlin-build-common")) } sourceSets { diff --git a/js/js.tests/test/org/jetbrains/kotlin/js/test/BasicBoxTest.kt b/js/js.tests/test/org/jetbrains/kotlin/js/test/BasicBoxTest.kt index e247999a393..e953c7afdaa 100644 --- a/js/js.tests/test/org/jetbrains/kotlin/js/test/BasicBoxTest.kt +++ b/js/js.tests/test/org/jetbrains/kotlin/js/test/BasicBoxTest.kt @@ -56,6 +56,8 @@ import org.jetbrains.kotlin.js.util.TextOutputImpl import org.jetbrains.kotlin.psi.KtFile import org.jetbrains.kotlin.psi.KtNamedFunction import org.jetbrains.kotlin.psi.KtPsiFactory +import org.jetbrains.kotlin.serialization.DebugProtoBuf +import org.jetbrains.kotlin.serialization.js.JsSerializerProtocol import org.jetbrains.kotlin.serialization.js.KotlinJavascriptSerializationUtil import org.jetbrains.kotlin.serialization.js.ModuleKind import org.jetbrains.kotlin.test.InTextDirectivesUtils @@ -64,6 +66,7 @@ import org.jetbrains.kotlin.test.KotlinTestUtils.TestFileFactory import org.jetbrains.kotlin.test.KotlinTestWithEnvironment import org.jetbrains.kotlin.test.TargetBackend import org.jetbrains.kotlin.utils.DFS +import org.jetbrains.kotlin.utils.KotlinJavascriptMetadata import org.jetbrains.kotlin.utils.KotlinJavascriptMetadataUtils import java.io.* import java.nio.charset.Charset @@ -343,18 +346,30 @@ abstract class BasicBoxTest( val originalOutput = FileUtil.loadFile(outputFile) val recompiledOutput = removeRecompiledSuffix(FileUtil.loadFile(recompiledOutputFile)) - TestCase.assertEquals("Output file changed after recompilation", originalOutput, recompiledOutput) + assertEquals("Output file changed after recompilation", originalOutput, recompiledOutput) val originalSourceMap = FileUtil.loadFile(File(outputFile.parentFile, outputFile.name + ".map")) val recompiledSourceMap = removeRecompiledSuffix( FileUtil.loadFile(File(recompiledOutputFile.parentFile, recompiledOutputFile.name + ".map"))) - TestCase.assertEquals("Source map file changed after recompilation", originalSourceMap, recompiledSourceMap) + assertEquals("Source map file changed after recompilation", originalSourceMap, recompiledSourceMap) if (multiModule) { val originalMetadata = FileUtil.loadFile(File(outputFile.parentFile, outputFile.nameWithoutExtension + ".meta.js")) val recompiledMetadata = removeRecompiledSuffix( FileUtil.loadFile(File(recompiledOutputFile.parentFile, recompiledOutputFile.nameWithoutExtension + ".meta.js"))) - TestCase.assertEquals("Metadata file changed after recompilation", originalMetadata, recompiledMetadata) + assertEquals("Metadata file changed after recompilation", + metadataAsString(originalMetadata, module.name), + metadataAsString(recompiledMetadata, module.name)) + } + } + + private fun metadataAsString(metadata: String, moduleName: String): String { + val containers = mutableListOf() + KotlinJavascriptMetadataUtils.parseMetadata(metadata, containers) + val metadataParts = KotlinJavascriptSerializationUtil.readModuleAsProto(containers.single().body, moduleName).data.body + return metadataParts.joinToString("-----\n") { + val binary = it.toByteArray() + DebugProtoBuf.PackageFragment.parseFrom(binary, JsSerializerProtocol.extensionRegistry).toString() } } diff --git a/js/js.tests/test/org/jetbrains/kotlin/js/test/semantics/BoxJsTestGenerated.java b/js/js.tests/test/org/jetbrains/kotlin/js/test/semantics/BoxJsTestGenerated.java index 3454a678989..429f591ce2f 100644 --- a/js/js.tests/test/org/jetbrains/kotlin/js/test/semantics/BoxJsTestGenerated.java +++ b/js/js.tests/test/org/jetbrains/kotlin/js/test/semantics/BoxJsTestGenerated.java @@ -3761,6 +3761,12 @@ public class BoxJsTestGenerated extends AbstractBoxJsTest { doTest(fileName); } + @TestMetadata("packagesWithSameName.kt") + public void testPackagesWithSameName() throws Exception { + String fileName = KotlinTestUtils.navigationMetadata("js/js.translator/testData/box/incremental/packagesWithSameName.kt"); + doTest(fileName); + } + @TestMetadata("simple.kt") public void testSimple() throws Exception { String fileName = KotlinTestUtils.navigationMetadata("js/js.translator/testData/box/incremental/simple.kt"); diff --git a/js/js.translator/src/org/jetbrains/kotlin/js/translate/general/Merger.kt b/js/js.translator/src/org/jetbrains/kotlin/js/translate/general/Merger.kt index 23775f79011..505401508b1 100644 --- a/js/js.translator/src/org/jetbrains/kotlin/js/translate/general/Merger.kt +++ b/js/js.translator/src/org/jetbrains/kotlin/js/translate/general/Merger.kt @@ -109,7 +109,7 @@ class Merger(private val rootFunction: JsFunction, val internalModuleName: JsNam val exportedTag = statement.exportedTag if (exportedTag != null && !exportedTags.add(exportedTag)) continue } - exportBlock.statements += nameMap.rename(statement) + exportBlock.statements += nameMap.rename(statement.deepCopy()) } } diff --git a/js/js.translator/testData/box/incremental/packagesWithSameName.kt b/js/js.translator/testData/box/incremental/packagesWithSameName.kt new file mode 100644 index 00000000000..13bd35ae4f5 --- /dev/null +++ b/js/js.translator/testData/box/incremental/packagesWithSameName.kt @@ -0,0 +1,31 @@ +// EXPECTED_REACHABLE_NODES: 1126 +// MODULE: lib +// FILE: a.kt +package a.p + +fun foo() = "foo" + +// FILE: b1.kt +// RECOMPILE +package b.p + +fun bar() = "bar" + +// FILE: b2.kt +package b.p + +fun baz() = "baz" + +// MODULE: main(lib) +// FILE: main.kt +import a.p.* +import b.p.* + +fun box(): String { + val r = foo() + bar() + baz() + + if (r != "foobarbaz") return "fail: $r" + + return "OK" +} +