From 26e3237b8c38fa876b8f243c3ec6b96c6907a2b4 Mon Sep 17 00:00:00 2001 From: pyos Date: Thu, 24 Jun 2021 11:26:31 +0200 Subject: [PATCH] JVM_IR: never rename public/protected fields And if that causes a platform declaration clash, that's not a problem that can be solved without breaking the ABI anyway. #KT-47412 Fixed --- .../FirBlackBoxCodegenTestGenerated.java | 6 ++ .../backend/jvm/lower/RenameFieldsLowering.kt | 80 +++++++------------ .../box/enum/nameClashWithCompanion.kt | 18 +++++ .../codegen/BlackBoxCodegenTestGenerated.java | 6 ++ .../IrBlackBoxCodegenTestGenerated.java | 6 ++ .../LightAnalysisModeTestGenerated.java | 5 ++ 6 files changed, 71 insertions(+), 50 deletions(-) create mode 100644 compiler/testData/codegen/box/enum/nameClashWithCompanion.kt diff --git a/compiler/fir/fir2ir/tests-gen/org/jetbrains/kotlin/test/runners/codegen/FirBlackBoxCodegenTestGenerated.java b/compiler/fir/fir2ir/tests-gen/org/jetbrains/kotlin/test/runners/codegen/FirBlackBoxCodegenTestGenerated.java index 82f859d26d0..7c07742b835 100644 --- a/compiler/fir/fir2ir/tests-gen/org/jetbrains/kotlin/test/runners/codegen/FirBlackBoxCodegenTestGenerated.java +++ b/compiler/fir/fir2ir/tests-gen/org/jetbrains/kotlin/test/runners/codegen/FirBlackBoxCodegenTestGenerated.java @@ -14836,6 +14836,12 @@ public class FirBlackBoxCodegenTestGenerated extends AbstractFirBlackBoxCodegenT runTest("compiler/testData/codegen/box/enum/modifierFlags.kt"); } + @Test + @TestMetadata("nameClashWithCompanion.kt") + public void testNameClashWithCompanion() throws Exception { + runTest("compiler/testData/codegen/box/enum/nameClashWithCompanion.kt"); + } + @Test @TestMetadata("nameConflict.kt") public void testNameConflict() throws Exception { diff --git a/compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/lower/RenameFieldsLowering.kt b/compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/lower/RenameFieldsLowering.kt index aec7c21889d..ad94d4c67e6 100644 --- a/compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/lower/RenameFieldsLowering.kt +++ b/compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/lower/RenameFieldsLowering.kt @@ -11,7 +11,6 @@ import org.jetbrains.kotlin.backend.common.phaser.makeIrFilePhase import org.jetbrains.kotlin.ir.IrElement import org.jetbrains.kotlin.ir.IrStatement import org.jetbrains.kotlin.ir.builders.declarations.buildField -import org.jetbrains.kotlin.ir.declarations.IrDeclarationOrigin import org.jetbrains.kotlin.ir.declarations.IrDeclarationParent import org.jetbrains.kotlin.ir.declarations.IrField import org.jetbrains.kotlin.ir.declarations.IrFile @@ -21,13 +20,9 @@ import org.jetbrains.kotlin.ir.expressions.IrSetField import org.jetbrains.kotlin.ir.expressions.impl.IrGetFieldImpl import org.jetbrains.kotlin.ir.expressions.impl.IrSetFieldImpl import org.jetbrains.kotlin.ir.symbols.IrFieldSymbol -import org.jetbrains.kotlin.ir.util.hasAnnotation import org.jetbrains.kotlin.ir.util.patchDeclarationParents import org.jetbrains.kotlin.ir.visitors.IrElementTransformerVoid -import org.jetbrains.kotlin.ir.visitors.IrElementVisitorVoid -import org.jetbrains.kotlin.ir.visitors.acceptChildrenVoid -import org.jetbrains.kotlin.ir.visitors.acceptVoid -import org.jetbrains.kotlin.load.java.JvmAbi +import org.jetbrains.kotlin.ir.visitors.IrElementVisitor import org.jetbrains.kotlin.name.Name internal val renameFieldsPhase = makeIrFilePhase( @@ -38,63 +33,48 @@ internal val renameFieldsPhase = makeIrFilePhase( private class RenameFieldsLowering(val context: CommonBackendContext) : FileLoweringPass { override fun lower(irFile: IrFile) { - val collector = FieldNameCollector() - irFile.acceptVoid(collector) + val fields = mutableListOf() + irFile.accept(FieldCollector, fields) + fields.sortBy { + when { + // We never rename public ABI fields (public and protected visibility) since they are accessible from Java + // even in cases when Kotlin code would prefer an accessor. (And in some cases, such as enum entries and const + // fields, Kotlin references the fields directly too.) Therefore we consider such fields first, in order to make + // sure it'll claim its original name. There can be multiple such fields, in which case they will cause a platform + // declaration clash if they map to the same JVM type - nothing we can do about that. + it.visibility.isPublicAPI -> 0 + // If there are non-public non-static and static (moved from companion) fields with the same name, we try to make + // static properties retain their original names first, since this is what the old JVM backend did. However this + // can easily be changed without any major binary compatibility consequences (ignoring Java reflection). + it.isStatic -> 1 + else -> 2 + } + } val newNames = mutableMapOf() - for ((_, fields) in collector.nameToField) { - if (fields.size < 2) continue - - var count = 0 - // We never rename fields that are part of public ABI (backing fields of const, lateinit, and JvmField properties). - // Therefore we consider such fields first, in order to make sure it'll claim its original name. - // If there are non-static and static (moved from companion) fields with the same name, we try to make static properties retain - // their original names first, since this is what the old JVM backend did. However this can easily be changed without any - // major binary compatibility consequences (modulo access to private members via Java reflection). - for (field in fields.sortedBy { - when { - it.isPublicAbi() -> 0 - it.isStatic -> 1 - else -> 2 - } - }) { - val oldName = field.name - val newName = if (count == 0) oldName else Name.identifier(oldName.asString() + "$$count") - count++ - - if (field.isPublicAbi()) continue - - newNames[field] = newName + val count = hashMapOf, Int>() + for (field in fields) { + val key = field.parent to field.name + val index = count[key] ?: 0 + if (index != 0 && !field.visibility.isPublicAPI) { + newNames[field] = Name.identifier("${field.name}$$index") } + count[key] = index + 1 } val renamer = FieldRenamer(newNames) irFile.transform(renamer, null) - irFile.transform(FieldAccessTransformer(renamer.newSymbols), null) } - - private fun IrField.isPublicAbi(): Boolean { - if (!visibility.isPublicAPI) return false - val correspondingProperty = correspondingPropertySymbol?.owner - return isJvmField || - origin == IrDeclarationOrigin.FIELD_FOR_OBJECT_INSTANCE || - correspondingProperty != null && (correspondingProperty.isLateinit || correspondingProperty.isConst) - } - - private val IrField.isJvmField: Boolean - get() = hasAnnotation(JvmAbi.JVM_FIELD_ANNOTATION_FQ_NAME) } -private class FieldNameCollector : IrElementVisitorVoid { - val nameToField = mutableMapOf, MutableList>() - - override fun visitElement(element: IrElement) { - element.acceptChildrenVoid(this) +private object FieldCollector : IrElementVisitor> { + override fun visitElement(element: IrElement, data: MutableList) { + element.acceptChildren(this, data) } - override fun visitField(declaration: IrField) { - nameToField.getOrPut(declaration.parent to declaration.name) { mutableListOf() }.add(declaration) + override fun visitField(declaration: IrField, data: MutableList) { + data.add(declaration) } } diff --git a/compiler/testData/codegen/box/enum/nameClashWithCompanion.kt b/compiler/testData/codegen/box/enum/nameClashWithCompanion.kt new file mode 100644 index 00000000000..1aa35bf84ae --- /dev/null +++ b/compiler/testData/codegen/box/enum/nameClashWithCompanion.kt @@ -0,0 +1,18 @@ +// TARGET_BACKEND: JVM +// IGNORE_BACKEND_FIR: JVM_IR +// WITH_RUNTIME +// MODULE: lib +// FILE: lib.kt + +enum class E(val value: String) { + OK("K"); + + companion object { + @JvmField + val OK = "O" + } +} + +// MODULE: main(lib) +// FILE: main.kt +fun box() = E.OK + E.OK.value diff --git a/compiler/tests-common-new/tests-gen/org/jetbrains/kotlin/test/runners/codegen/BlackBoxCodegenTestGenerated.java b/compiler/tests-common-new/tests-gen/org/jetbrains/kotlin/test/runners/codegen/BlackBoxCodegenTestGenerated.java index 9c9ad346b81..995266ebbef 100644 --- a/compiler/tests-common-new/tests-gen/org/jetbrains/kotlin/test/runners/codegen/BlackBoxCodegenTestGenerated.java +++ b/compiler/tests-common-new/tests-gen/org/jetbrains/kotlin/test/runners/codegen/BlackBoxCodegenTestGenerated.java @@ -14836,6 +14836,12 @@ public class BlackBoxCodegenTestGenerated extends AbstractBlackBoxCodegenTest { runTest("compiler/testData/codegen/box/enum/modifierFlags.kt"); } + @Test + @TestMetadata("nameClashWithCompanion.kt") + public void testNameClashWithCompanion() throws Exception { + runTest("compiler/testData/codegen/box/enum/nameClashWithCompanion.kt"); + } + @Test @TestMetadata("nameConflict.kt") public void testNameConflict() throws Exception { diff --git a/compiler/tests-common-new/tests-gen/org/jetbrains/kotlin/test/runners/codegen/IrBlackBoxCodegenTestGenerated.java b/compiler/tests-common-new/tests-gen/org/jetbrains/kotlin/test/runners/codegen/IrBlackBoxCodegenTestGenerated.java index 127c56eaaf0..a771a8b6e1c 100644 --- a/compiler/tests-common-new/tests-gen/org/jetbrains/kotlin/test/runners/codegen/IrBlackBoxCodegenTestGenerated.java +++ b/compiler/tests-common-new/tests-gen/org/jetbrains/kotlin/test/runners/codegen/IrBlackBoxCodegenTestGenerated.java @@ -14836,6 +14836,12 @@ public class IrBlackBoxCodegenTestGenerated extends AbstractIrBlackBoxCodegenTes runTest("compiler/testData/codegen/box/enum/modifierFlags.kt"); } + @Test + @TestMetadata("nameClashWithCompanion.kt") + public void testNameClashWithCompanion() throws Exception { + runTest("compiler/testData/codegen/box/enum/nameClashWithCompanion.kt"); + } + @Test @TestMetadata("nameConflict.kt") public void testNameConflict() throws Exception { diff --git a/compiler/tests-gen/org/jetbrains/kotlin/codegen/LightAnalysisModeTestGenerated.java b/compiler/tests-gen/org/jetbrains/kotlin/codegen/LightAnalysisModeTestGenerated.java index 460ccf498c7..e0a7ff20abd 100644 --- a/compiler/tests-gen/org/jetbrains/kotlin/codegen/LightAnalysisModeTestGenerated.java +++ b/compiler/tests-gen/org/jetbrains/kotlin/codegen/LightAnalysisModeTestGenerated.java @@ -12147,6 +12147,11 @@ public class LightAnalysisModeTestGenerated extends AbstractLightAnalysisModeTes runTest("compiler/testData/codegen/box/enum/modifierFlags.kt"); } + @TestMetadata("nameClashWithCompanion.kt") + public void testNameClashWithCompanion() throws Exception { + runTest("compiler/testData/codegen/box/enum/nameClashWithCompanion.kt"); + } + @TestMetadata("nameConflict.kt") public void testNameConflict() throws Exception { runTest("compiler/testData/codegen/box/enum/nameConflict.kt");