[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.
2019-11-14 10:43:25 +01:00
parent d1c2862e27
commit 6f8682c950
17 changed files with 149 additions and 37 deletions
@@ -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)
@@ -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)
        }
@@ -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<IrClass>()?.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)

@@ -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()
    }
 }

@@ -1,3 +1,4 @@
+// IGNORE_BACKEND_FIR: JVM_IR
 // !JVM_DEFAULT_MODE: compatibility
 // TARGET_BACKEND: JVM
 // JVM_TARGET: 1.8
@@ -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'

@@ -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
+
+// IR only generates the accessors actually needed
+// JVM_IR_TEMPLATES
+// 1 GETSTATIC My.my
@@ -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.

@@ -9,7 +9,7 @@ interface KInterface {
    }
 }

-// 1 INVOKESTATIC KInterface.access\$test\$
+// 1 INVOKESTATIC KInterface.access\$test\$jd
 // 1 INVOKESTATIC KInterface.test\$default

 // from $default
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
+
@@ -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)
@@ -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)