From 6f8682c950008271c59017c5fd6e3950b5434414 Mon Sep 17 00:00:00 2001 From: Kristoffer Andersen Date: Thu, 14 Nov 2019 10:43:25 +0100 Subject: [PATCH] [JVM IR] Stabilize accesor names in IR Backend - introduce a scoped counter instead of a global one for name generation for accessors. Naive solution not working. - Introduced hardcoded "jd" suffix for accessors on interfaces, under the assumption that the only such accessors are due to JvmDefault and their bridges from `$DefaultImpls`. Removed all associated templated tests, so the old and IR backend correspond on this matter again. - Respecialized writeFlags from regexps to string-equality: we are going for exact matches now! - Fixed package calculation in `IrUtils.kt`. - Accessors for static members must be due to accessing super classes. Actual super-qualified calls are naturally also accessing super classes. Hence the `$s+{hashcode(superClassName)}` suffix. Added test to affirm this naming scheme. - Field getters/setters for static fields must be companion accessors, otherwise just labelled as accessors. They are also tagged with `s` suffix when accessing static fields. - For naming of accessors to coincide with the old backend, field renaming to avoid JVM signature clashes must be done _after_ generation of accessors for those fields. --- .../jetbrains/kotlin/backend/jvm/JvmLower.kt | 2 +- .../backend/jvm/lower/InterfaceLowering.kt | 2 +- .../jvm/lower/SyntheticAccessorLowering.kt | 50 ++++++++++++++----- .../org/jetbrains/kotlin/ir/util/IrUtils.kt | 3 +- .../compatibility/interfaceExtension.kt | 1 + .../bytecodeText/companion/kt14258_2.kt | 1 - .../bytecodeText/companion/kt14258_3.kt | 13 +++-- .../bytecodeText/companion/kt14258_4.kt | 1 - .../jvmDefault/compatibility/defaultArgs.kt | 2 +- .../compatibility/simpleFunction.kt | 4 +- .../simpleFunctionWithAbstractOverride.kt | 12 ++--- .../compatibility/simpleProperty.kt | 8 +-- .../compatibility/propertyAccessors.kt | 2 - .../writeFlags/protectedAccessToBaseMethod.kt | 28 +++++++++++ ...ectedAccessToBaseMethodDifferentPackage.kt | 37 ++++++++++++++ .../flags/WriteFlagsTestGenerated.java | 10 ++++ .../codegen/ir/IrWriteFlagsTestGenerated.java | 10 ++++ 17 files changed, 149 insertions(+), 37 deletions(-) create mode 100644 compiler/testData/writeFlags/protectedAccessToBaseMethod.kt create mode 100644 compiler/testData/writeFlags/protectedAccessToBaseMethodDifferentPackage.kt diff --git a/compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/JvmLower.kt b/compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/JvmLower.kt index c81c3a00612..6ea1574fc00 100644 --- a/compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/JvmLower.kt +++ b/compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/JvmLower.kt @@ -244,7 +244,6 @@ private val jvmFilePhases = propertiesToFieldsPhase then remapObjectFieldAccesses then propertiesPhase then - renameFieldsPhase then anonymousObjectSuperConstructorPhase then tailrecPhase then @@ -308,6 +307,7 @@ private val jvmFilePhases = checkLocalNamesWithOldBackendPhase then mainMethodGenerationPhase then + renameFieldsPhase then fakeInliningLocalVariablesLowering then makePatchParentsPhase(3) diff --git a/compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/lower/InterfaceLowering.kt b/compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/lower/InterfaceLowering.kt index 7673f286d88..764cee8f5ea 100644 --- a/compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/lower/InterfaceLowering.kt +++ b/compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/lower/InterfaceLowering.kt @@ -175,7 +175,7 @@ internal class InterfaceLowering(val context: JvmBackendContext) : IrElementTran } private fun createDefaultImpl(function: IrSimpleFunction): IrSimpleFunction = - context.declarationFactory.getDefaultImplsFunction(function).also {newFunction -> + context.declarationFactory.getDefaultImplsFunction(function).also { newFunction -> newFunction.body = function.body?.patchDeclarationParents(newFunction) newFunction.parentAsClass.declarations.add(newFunction) } diff --git a/compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/lower/SyntheticAccessorLowering.kt b/compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/lower/SyntheticAccessorLowering.kt index d55f4c7f13d..17147db5710 100644 --- a/compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/lower/SyntheticAccessorLowering.kt +++ b/compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/lower/SyntheticAccessorLowering.kt @@ -7,10 +7,7 @@ package org.jetbrains.kotlin.backend.jvm.lower import org.jetbrains.kotlin.backend.common.FileLoweringPass import org.jetbrains.kotlin.backend.common.IrElementTransformerVoidWithContext -import org.jetbrains.kotlin.backend.common.ir.copyTypeParametersFrom -import org.jetbrains.kotlin.backend.common.ir.copyValueParametersToStatic -import org.jetbrains.kotlin.backend.common.ir.passTypeArgumentsFrom -import org.jetbrains.kotlin.backend.common.ir.remapTypeParameters +import org.jetbrains.kotlin.backend.common.ir.* import org.jetbrains.kotlin.backend.jvm.JvmBackendContext import org.jetbrains.kotlin.backend.jvm.JvmLoweredDeclarationOrigin import org.jetbrains.kotlin.backend.jvm.codegen.isJvmInterface @@ -228,7 +225,7 @@ internal class SyntheticAccessorLowering(val context: JvmBackendContext) : IrEle return buildFun { origin = JvmLoweredDeclarationOrigin.SYNTHETIC_ACCESSOR - name = source.accessorName() + name = source.accessorName(expression.superQualifierSymbol) visibility = Visibilities.PUBLIC modality = if (parent is IrClass && parent.isJvmInterface) Modality.OPEN else Modality.FINAL isSuspend = source.isSuspend // synthetic accessors of suspend functions are handled in codegen @@ -447,24 +444,53 @@ internal class SyntheticAccessorLowering(val context: JvmBackendContext) : IrEle } } - // Counter used to disambiguate accessors to functions with the same base name. - private var nameCounter = 0 - - private fun IrFunction.accessorName(): Name { + private fun IrFunction.accessorName(superQualifier: IrClassSymbol?): Name { val jvmName = context.methodSignatureMapper.mapFunctionName(this) - return Name.identifier("access\$$jvmName\$${nameCounter++}") + val suffix = when { + // The only function accessors placed on interfaces are for JvmDefault implementations + parent.safeAs()?.isJvmInterface == true -> "\$jd" + + // Accessor for _s_uper-qualified call + superQualifier != null -> "\$s" + superQualifier.descriptor.name.asString().hashCode() + + // Access to static members that need an accessor must be because they are inherited, + // hence accessed on a _s_upertype. + isStatic -> "\$s" + hashForAccessorDisambiguation() + + else -> "" + } + return Name.identifier("access\$$jvmName$suffix") } private fun IrField.accessorNameForGetter(): Name { val getterName = JvmAbi.getterName(name.asString()) - return Name.identifier("access\$prop\$$getterName\$${nameCounter++}") + return Name.identifier("access\$$getterName\$${fieldAccessorSuffix()}") } private fun IrField.accessorNameForSetter(): Name { val setterName = JvmAbi.setterName(name.asString()) - return Name.identifier("access\$prop\$$setterName\$${nameCounter++}") + return Name.identifier("access\$$setterName\$${fieldAccessorSuffix()}") } + private fun IrField.fieldAccessorSuffix(): String { + // The only static field accessors are those for fields moved from companion objects, hence a + // _c_ompanion _p_roperty suffix. However, companion objects for interfaces keep their fields + // (as interfaces cannot own fields), hence are simple _p_roperty accessors, like everything else. + val companionSuffix = if (isStatic && !parentAsClass.isCompanion) "cp" else "p" + + // Static accesses that need an accessor must be due to being inherited, hence accessed on a + // _s_upertype + val staticSuffix = if (isStatic) "\$s"+ hashForAccessorDisambiguation() else "" + return companionSuffix + staticSuffix + } + + private fun IrDeclaration.hashForAccessorDisambiguation() = + if (parentAsClass.name.isSpecial) { + context.getLocalClassType(parentAsClass)?.className.hashCode() ?: parentAsClass.name.hashCode() + } else { + parentAsClass.name.identifier.hashCode() + } + private val Visibility.isPrivate get() = Visibilities.isPrivate(this) diff --git a/compiler/ir/ir.tree/src/org/jetbrains/kotlin/ir/util/IrUtils.kt b/compiler/ir/ir.tree/src/org/jetbrains/kotlin/ir/util/IrUtils.kt index 3f73fe6d668..eb9cc6b06a2 100644 --- a/compiler/ir/ir.tree/src/org/jetbrains/kotlin/ir/util/IrUtils.kt +++ b/compiler/ir/ir.tree/src/org/jetbrains/kotlin/ir/util/IrUtils.kt @@ -347,8 +347,7 @@ tailrec fun IrElement.getPackageFragment(): IrPackageFragment? { val vParent = (this as? IrDeclaration)?.parent return when (vParent) { is IrPackageFragment -> vParent - is IrClass -> vParent.getPackageFragment() - else -> null + else -> vParent?.getPackageFragment() } } diff --git a/compiler/testData/codegen/box/jvm8/defaults/compatibility/interfaceExtension.kt b/compiler/testData/codegen/box/jvm8/defaults/compatibility/interfaceExtension.kt index 99206085bd7..72a6660b7da 100644 --- a/compiler/testData/codegen/box/jvm8/defaults/compatibility/interfaceExtension.kt +++ b/compiler/testData/codegen/box/jvm8/defaults/compatibility/interfaceExtension.kt @@ -1,3 +1,4 @@ +// IGNORE_BACKEND_FIR: JVM_IR // !JVM_DEFAULT_MODE: compatibility // TARGET_BACKEND: JVM // JVM_TARGET: 1.8 diff --git a/compiler/testData/codegen/bytecodeText/companion/kt14258_2.kt b/compiler/testData/codegen/bytecodeText/companion/kt14258_2.kt index b32e89b564c..5a8b0bf65c0 100644 --- a/compiler/testData/codegen/bytecodeText/companion/kt14258_2.kt +++ b/compiler/testData/codegen/bytecodeText/companion/kt14258_2.kt @@ -1,4 +1,3 @@ -// IGNORE_BACKEND: JVM_IR // Checks that methods 'access$getMy$p' and 'getMy' are not generated and // that backed field 'my' is accessed through 'access$getMy$cp' diff --git a/compiler/testData/codegen/bytecodeText/companion/kt14258_3.kt b/compiler/testData/codegen/bytecodeText/companion/kt14258_3.kt index df8a63ac1a3..e64775f8a2d 100644 --- a/compiler/testData/codegen/bytecodeText/companion/kt14258_3.kt +++ b/compiler/testData/codegen/bytecodeText/companion/kt14258_3.kt @@ -1,4 +1,3 @@ -// IGNORE_BACKEND: JVM_IR // Checks that accessor methods are always used due to the overriding of the default setter of 'my' property. class My { @@ -19,11 +18,17 @@ class My { // GETSTATIC My.my into 'access$getMy$cp' } -// 2 GETSTATIC My.my // 2 PUTSTATIC My.my // 0 INVOKESTATIC My\$Companion.access\$getMy\$p -// 1 INVOKESTATIC My\$Companion.access\$setMy\$p // 1 INVOKESTATIC My.access\$setMy\$cp +// 1 INVOKESPECIAL My\$Companion.setMy + +// JVM_TEMPLATES +// 2 GETSTATIC My.my +// 1 INVOKESTATIC My\$Companion.access\$setMy\$p // 1 INVOKESTATIC My.access\$getMy\$cp // 1 INVOKESPECIAL My\$Companion.getMy -// 1 INVOKESPECIAL My\$Companion.setMy \ No newline at end of file + +// IR only generates the accessors actually needed +// JVM_IR_TEMPLATES +// 1 GETSTATIC My.my diff --git a/compiler/testData/codegen/bytecodeText/companion/kt14258_4.kt b/compiler/testData/codegen/bytecodeText/companion/kt14258_4.kt index 3a93bd0c20e..12c3790139e 100644 --- a/compiler/testData/codegen/bytecodeText/companion/kt14258_4.kt +++ b/compiler/testData/codegen/bytecodeText/companion/kt14258_4.kt @@ -1,4 +1,3 @@ -// IGNORE_BACKEND: JVM_IR // Checks that accessor 'I$Companion.access$getBar\$p' is always used because the property is kept // into the companion object. diff --git a/compiler/testData/codegen/bytecodeText/jvm8/jvmDefault/compatibility/defaultArgs.kt b/compiler/testData/codegen/bytecodeText/jvm8/jvmDefault/compatibility/defaultArgs.kt index e135cac740b..79f757c0152 100644 --- a/compiler/testData/codegen/bytecodeText/jvm8/jvmDefault/compatibility/defaultArgs.kt +++ b/compiler/testData/codegen/bytecodeText/jvm8/jvmDefault/compatibility/defaultArgs.kt @@ -9,7 +9,7 @@ interface KInterface { } } -// 1 INVOKESTATIC KInterface.access\$test\$ +// 1 INVOKESTATIC KInterface.access\$test\$jd // 1 INVOKESTATIC KInterface.test\$default // from $default diff --git a/compiler/testData/codegen/bytecodeText/jvm8/jvmDefault/compatibility/simpleFunction.kt b/compiler/testData/codegen/bytecodeText/jvm8/jvmDefault/compatibility/simpleFunction.kt index 523733c4279..6ace7993aab 100644 --- a/compiler/testData/codegen/bytecodeText/jvm8/jvmDefault/compatibility/simpleFunction.kt +++ b/compiler/testData/codegen/bytecodeText/jvm8/jvmDefault/compatibility/simpleFunction.kt @@ -12,8 +12,8 @@ interface KInterface2 : KInterface { } -// 1 INVOKESTATIC KInterface2.access\$test2\$ -// 1 INVOKESTATIC KInterface.access\$test2\$ +// 1 INVOKESTATIC KInterface2.access\$test2\$jd +// 1 INVOKESTATIC KInterface.access\$test2\$jd // 1 INVOKESPECIAL KInterface2.test2 // 1 INVOKESPECIAL KInterface.test2 diff --git a/compiler/testData/codegen/bytecodeText/jvm8/jvmDefault/compatibility/simpleFunctionWithAbstractOverride.kt b/compiler/testData/codegen/bytecodeText/jvm8/jvmDefault/compatibility/simpleFunctionWithAbstractOverride.kt index 32fe14cee28..227eb96c28f 100644 --- a/compiler/testData/codegen/bytecodeText/jvm8/jvmDefault/compatibility/simpleFunctionWithAbstractOverride.kt +++ b/compiler/testData/codegen/bytecodeText/jvm8/jvmDefault/compatibility/simpleFunctionWithAbstractOverride.kt @@ -13,11 +13,11 @@ interface KInterface2 : KInterface { abstract override fun test2(): String } +// 1 INVOKESTATIC KInterface.access\$test2\$jd +// + +// 0 INVOKESTATIC KInterface2.access\$test2\$jd +// = +// 1 INVOKESTATIC KInterface + // 1 INVOKESPECIAL KInterface.test2 // 0 INVOKESPECIAL KInterface2.test2 - -// 1 INVOKESTATIC KInterface.access\$test2\$ -// + -// 0 INVOKESTATIC KInterface2.access\$test2\$ -// = -// 1 INVOKESTATIC KInterface \ No newline at end of file diff --git a/compiler/testData/codegen/bytecodeText/jvm8/jvmDefault/compatibility/simpleProperty.kt b/compiler/testData/codegen/bytecodeText/jvm8/jvmDefault/compatibility/simpleProperty.kt index 673e8c8e188..04d4e7b0a37 100644 --- a/compiler/testData/codegen/bytecodeText/jvm8/jvmDefault/compatibility/simpleProperty.kt +++ b/compiler/testData/codegen/bytecodeText/jvm8/jvmDefault/compatibility/simpleProperty.kt @@ -13,10 +13,10 @@ interface KInterface2 : KInterface { } -// 1 INVOKESTATIC KInterface2.access\$getBar\$ -// 1 INVOKESTATIC KInterface2.access\$setBar\$ -// 1 INVOKESTATIC KInterface.access\$getBar\$ -// 1 INVOKESTATIC KInterface.access\$setBar\$ +// 1 INVOKESTATIC KInterface2.access\$getBar\$jd +// 1 INVOKESTATIC KInterface2.access\$setBar\$jd +// 1 INVOKESTATIC KInterface.access\$getBar\$jd +// 1 INVOKESTATIC KInterface.access\$setBar\$jd // 1 INVOKESPECIAL KInterface2.getBar // 1 INVOKESPECIAL KInterface2.setBar diff --git a/compiler/testData/writeFlags/jvm8/defaults/compatibility/propertyAccessors.kt b/compiler/testData/writeFlags/jvm8/defaults/compatibility/propertyAccessors.kt index a35efca57a7..66a4ec7f612 100644 --- a/compiler/testData/writeFlags/jvm8/defaults/compatibility/propertyAccessors.kt +++ b/compiler/testData/writeFlags/jvm8/defaults/compatibility/propertyAccessors.kt @@ -1,5 +1,4 @@ // !JVM_DEFAULT_MODE: compatibility -// IGNORE_BACKEND: JVM_IR // JVM_TARGET: 1.8 // WITH_RUNTIME @@ -22,7 +21,6 @@ interface Test { // TESTED_OBJECTS: Test, access$getTest2$jd // FLAGS: ACC_PUBLIC, ACC_STATIC, ACC_SYNTHETIC - // TESTED_OBJECT_KIND: function // TESTED_OBJECTS: Test, access$setTest2$jd // FLAGS: ACC_PUBLIC, ACC_STATIC, ACC_SYNTHETIC diff --git a/compiler/testData/writeFlags/protectedAccessToBaseMethod.kt b/compiler/testData/writeFlags/protectedAccessToBaseMethod.kt new file mode 100644 index 00000000000..8f8d14e0026 --- /dev/null +++ b/compiler/testData/writeFlags/protectedAccessToBaseMethod.kt @@ -0,0 +1,28 @@ +// IGNORE_BACKEND_FIR: JVM_IR +// TARGET_BACKEND: JVM +// FILE: Base.java +public class Base { + + protected static String BASE_ONLY = "BASE"; + + protected static String baseOnly() { + return BASE_ONLY; + } + + public static class Derived extends Base { + + } +} + +// FILE: Kotlin.kt + +class Kotlin : Base.Derived() { + fun doTest(): String { + if ({ Base.baseOnly() }() != "BASE") return "fail 8" + if ({ baseOnly() }() != "BASE") return "fail 10" + return "FAIL" + } +} +// TESTED_OBJECT_KIND: function +// TESTED_OBJECTS: Kotlin, access$baseOnly$s2063089 +// ABSENT: True \ No newline at end of file diff --git a/compiler/testData/writeFlags/protectedAccessToBaseMethodDifferentPackage.kt b/compiler/testData/writeFlags/protectedAccessToBaseMethodDifferentPackage.kt new file mode 100644 index 00000000000..7c8ce2414dc --- /dev/null +++ b/compiler/testData/writeFlags/protectedAccessToBaseMethodDifferentPackage.kt @@ -0,0 +1,37 @@ +// IGNORE_BACKEND_FIR: JVM_IR +// TARGET_BACKEND: JVM +// FILE: Base.java +public class Base { + + protected static String BASE_ONLY = "BASE"; + + protected static String baseOnly() { + return BASE_ONLY; + } + + public static class Derived extends Base { + + } +} + +// FILE: Kotlin.kt +package differentPackage; + +import Base.Derived +import Base + +class Kotlin : Base.Derived() { + fun doTest(): String { + if ({ Base.baseOnly() }() != "BASE") return "fail 8" + if ({ baseOnly() }() != "BASE") return "fail 10" + return "FAIL" + } +} +// TESTED_OBJECT_KIND: function +// TESTED_OBJECTS: differentPackage/Kotlin, access$baseOnly$s2063089 +// FLAGS: ACC_PUBLIC, ACC_STATIC, ACC_FINAL, ACC_SYNTHETIC + +// TESTED_OBJECT_KIND: function +// TESTED_OBJECTS: differentPackage/Kotlin, access$baseOnly$s-1074188803 +// FLAGS: ACC_PUBLIC, ACC_STATIC, ACC_FINAL, ACC_SYNTHETIC + diff --git a/compiler/tests/org/jetbrains/kotlin/codegen/flags/WriteFlagsTestGenerated.java b/compiler/tests/org/jetbrains/kotlin/codegen/flags/WriteFlagsTestGenerated.java index cd942d33fbd..136767d797b 100644 --- a/compiler/tests/org/jetbrains/kotlin/codegen/flags/WriteFlagsTestGenerated.java +++ b/compiler/tests/org/jetbrains/kotlin/codegen/flags/WriteFlagsTestGenerated.java @@ -29,6 +29,16 @@ public class WriteFlagsTestGenerated extends AbstractWriteFlagsTest { KotlinTestUtils.assertAllTestsPresentByMetadata(this.getClass(), new File("compiler/testData/writeFlags"), Pattern.compile("^(.+)\\.kt$"), TargetBackend.JVM, true); } + @TestMetadata("protectedAccessToBaseMethod.kt") + public void testProtectedAccessToBaseMethod() throws Exception { + runTest("compiler/testData/writeFlags/protectedAccessToBaseMethod.kt"); + } + + @TestMetadata("protectedAccessToBaseMethodDifferentPackage.kt") + public void testProtectedAccessToBaseMethodDifferentPackage() throws Exception { + runTest("compiler/testData/writeFlags/protectedAccessToBaseMethodDifferentPackage.kt"); + } + @TestMetadata("compiler/testData/writeFlags/callableReference") @TestDataPath("$PROJECT_ROOT") @RunWith(JUnit3RunnerWithInners.class) diff --git a/compiler/tests/org/jetbrains/kotlin/codegen/ir/IrWriteFlagsTestGenerated.java b/compiler/tests/org/jetbrains/kotlin/codegen/ir/IrWriteFlagsTestGenerated.java index 34d3ed8eac0..f9d2335d438 100644 --- a/compiler/tests/org/jetbrains/kotlin/codegen/ir/IrWriteFlagsTestGenerated.java +++ b/compiler/tests/org/jetbrains/kotlin/codegen/ir/IrWriteFlagsTestGenerated.java @@ -29,6 +29,16 @@ public class IrWriteFlagsTestGenerated extends AbstractIrWriteFlagsTest { KotlinTestUtils.assertAllTestsPresentByMetadata(this.getClass(), new File("compiler/testData/writeFlags"), Pattern.compile("^(.+)\\.kt$"), TargetBackend.JVM_IR, true); } + @TestMetadata("protectedAccessToBaseMethod.kt") + public void testProtectedAccessToBaseMethod() throws Exception { + runTest("compiler/testData/writeFlags/protectedAccessToBaseMethod.kt"); + } + + @TestMetadata("protectedAccessToBaseMethodDifferentPackage.kt") + public void testProtectedAccessToBaseMethodDifferentPackage() throws Exception { + runTest("compiler/testData/writeFlags/protectedAccessToBaseMethodDifferentPackage.kt"); + } + @TestMetadata("compiler/testData/writeFlags/callableReference") @TestDataPath("$PROJECT_ROOT") @RunWith(JUnit3RunnerWithInners.class)