JVM IR: do not generate hidden constructor for inline classes more than once

SyntheticAccessorLowering was initially implemented under the assumption that any access to an invisible declaration will cause an accessor to be generated _in the same file_. Moreover, it's declared in the group of phases that are performed by file. But this assumption is incorrect for constructors which need to be hidden (those which take parameters of inline class types), since such constructor is public and can be called from anywhere. In this case, SyntheticAccessorLowering actually generated a new accessor for the hidden constructor for each (!) source file where that constructor is called, which led to ClassFormatError because of the class file having multiple methods with the same signature. The internal `functionMap` cache didn't help because it's not shared among phase instances for different files (well, it helped to generate not more than one accessor per usage-file). In this change, we use the global cache, stored in JvmBackendContext, for accessors to hidden constructors. Note that after this change, calls to hidden constructors are always transformed to the corresponding accessor in SyntheticAccessorLowering right away, but that accessor might be orphaned for a while (not declared in any parent's declarations). Only when SyntheticAccessorLowering encounters the original constructor which needs to be hidden, it adds the accessor beside it. The test is sensitive to the file order, so both variants are added.
2020-01-02 14:39:32 +01:00
parent a339e7af19
commit 957b100cd1
11 changed files with 137 additions and 50 deletions
@@ -29,6 +29,7 @@ import org.jetbrains.kotlin.ir.builders.IrBuilderWithScope
 import org.jetbrains.kotlin.ir.builders.irBlock
 import org.jetbrains.kotlin.ir.builders.irNull
 import org.jetbrains.kotlin.ir.declarations.*
+import org.jetbrains.kotlin.ir.declarations.impl.IrConstructorImpl
 import org.jetbrains.kotlin.ir.descriptors.IrBuiltIns
 import org.jetbrains.kotlin.ir.expressions.IrExpression
 import org.jetbrains.kotlin.ir.expressions.IrFunctionReference
@@ -92,6 +93,8 @@ class JvmBackendContext(
    internal val multifileFacadeForPart = mutableMapOf<IrClass, JvmClassName>()
    internal val multifileFacadeMemberToPartMember = mutableMapOf<IrFunction, IrFunction>()

+    internal val hiddenConstructors = mutableMapOf<IrConstructor, IrConstructorImpl>()
+
    override var inVerbosePhase: Boolean = false

    override val configuration get() = state.configuration
@@ -11,7 +11,6 @@ import org.jetbrains.kotlin.backend.jvm.JvmSymbols
 import org.jetbrains.kotlin.backend.jvm.codegen.isInlineOnly
 import org.jetbrains.kotlin.backend.jvm.codegen.isJvmInterface
 import org.jetbrains.kotlin.backend.jvm.descriptors.JvmDeclarationFactory
-import org.jetbrains.kotlin.backend.jvm.lower.inlineclasses.hasMangledParameters
 import org.jetbrains.kotlin.backend.jvm.lower.inlineclasses.unboxInlineClass
 import org.jetbrains.kotlin.builtins.KotlinBuiltIns
 import org.jetbrains.kotlin.descriptors.Visibilities
@@ -141,10 +140,6 @@ fun IrType.getArrayElementType(irBuiltIns: IrBuiltIns): IrType =
 val IrStatementOrigin?.isLambda: Boolean
    get() = this == IrStatementOrigin.LAMBDA || this == IrStatementOrigin.ANONYMOUS_FUNCTION

-val IrConstructor.shouldBeHidden: Boolean
-    get() = !Visibilities.isPrivate(visibility) && !constructedClass.isInline && hasMangledParameters &&
-            origin != IrDeclarationOrigin.FUNCTION_FOR_DEFAULT_PARAMETER
-
 // An IR builder with a reference to the JvmBackendContext
 class JvmIrBuilder(
    val backendContext: JvmBackendContext,
@@ -15,7 +15,7 @@ import org.jetbrains.kotlin.backend.jvm.intrinsics.receiverAndArgs
 import org.jetbrains.kotlin.backend.jvm.ir.IrInlineReferenceLocator
 import org.jetbrains.kotlin.backend.jvm.ir.hasJvmDefault
 import org.jetbrains.kotlin.backend.jvm.ir.isLambda
-import org.jetbrains.kotlin.backend.jvm.ir.shouldBeHidden
+import org.jetbrains.kotlin.backend.jvm.lower.inlineclasses.hasMangledParameters
 import org.jetbrains.kotlin.descriptors.Modality
 import org.jetbrains.kotlin.descriptors.Visibilities
 import org.jetbrains.kotlin.descriptors.Visibility
@@ -57,20 +57,30 @@ internal class SyntheticAccessorLowering(val context: JvmBackendContext) : IrEle
            return super.visitFunctionAccess(expression)
        }

+        val callee = expression.symbol.owner
        val withSuper = (expression as? IrCall)?.superQualifierSymbol != null
        val thisSymbol = (expression as? IrCall)?.dispatchReceiver?.type?.classifierOrNull as? IrClassSymbol
-        if (expression.symbol.isAccessible(withSuper, thisSymbol)) {
-            return super.visitFunctionAccess(expression)
-        }
-        return super.visitExpression(
-            modifyFunctionAccessExpression(expression, functionMap.getOrPut(expression.symbol) {
-                when (val symbol = expression.symbol) {
-                    is IrConstructorSymbol -> symbol.owner.makeConstructorAccessor().symbol
-                    is IrSimpleFunctionSymbol -> symbol.owner.makeSimpleFunctionAccessor(expression as IrCall).symbol
-                    else -> error("Unknown subclass of IrFunctionSymbol")
+
+        val accessor = when {
+            callee is IrConstructor && callee.isOrShouldBeHidden ->
+                handleHiddenConstructor(callee).symbol
+
+            !expression.symbol.isAccessible(withSuper, thisSymbol) ->
+                functionMap.getOrPut(expression.symbol) {
+                    when (val symbol = expression.symbol) {
+                        is IrConstructorSymbol -> symbol.owner.makeConstructorAccessor().also { accessor ->
+                            pendingTransformations.add {
+                                (accessor.parent as IrDeclarationContainer).declarations.add(accessor)
+                            }
+                        }.symbol
+                        is IrSimpleFunctionSymbol -> symbol.owner.makeSimpleFunctionAccessor(expression as IrCall).symbol
+                        else -> error("Unknown subclass of IrFunctionSymbol")
+                    }
                }
-            })
-        )
+
+            else -> return super.visitFunctionAccess(expression)
+        }
+        return super.visitExpression(modifyFunctionAccessExpression(expression, accessor))
    }

    override fun visitGetField(expression: IrGetField) = super.visitExpression(
@@ -90,25 +100,26 @@ internal class SyntheticAccessorLowering(val context: JvmBackendContext) : IrEle
    )

    override fun visitConstructor(declaration: IrConstructor): IrStatement {
-        handleHiddenConstructor(declaration)
-        return super.visitConstructor(declaration)
-    }
+        if (declaration.isOrShouldBeHidden) {
+            pendingTransformations.add {
+                declaration.parentAsClass.declarations.add(handleHiddenConstructor(declaration))
+            }
+            declaration.visibility = Visibilities.PRIVATE
+        }

-    override fun visitConstructorCall(expression: IrConstructorCall): IrExpression {
-        handleHiddenConstructor(expression.symbol.owner)
-        return super.visitConstructorCall(expression)
+        return super.visitConstructor(declaration)
    }

    override fun visitFunctionReference(expression: IrFunctionReference): IrExpression {
        val function = expression.symbol.owner

-        if (!expression.origin.isLambda && function is IrConstructor) {
-            handleHiddenConstructor(function)?.let { accessor ->
+        if (!expression.origin.isLambda && function is IrConstructor && function.isOrShouldBeHidden) {
+            handleHiddenConstructor(function).let { accessor ->
                expression.transformChildrenVoid()
                return IrFunctionReferenceImpl(
                    expression.startOffset, expression.endOffset, expression.type,
-                    accessor, accessor.owner.typeParameters.size,
-                    accessor.owner.valueParameters.size, expression.origin
+                    accessor.symbol, accessor.typeParameters.size,
+                    accessor.valueParameters.size, expression.origin
                )
            }
        }
@@ -116,28 +127,28 @@ internal class SyntheticAccessorLowering(val context: JvmBackendContext) : IrEle
        return super.visitFunctionReference(expression)
    }

-    private fun handleHiddenConstructor(declaration: IrConstructor): IrConstructorSymbol? {
-        functionMap[declaration.symbol]?.let { return it as IrConstructorSymbol }
+    private val IrConstructor.isOrShouldBeHidden: Boolean
+        get() = this in context.hiddenConstructors || (
+                !Visibilities.isPrivate(visibility) && !constructedClass.isInline && hasMangledParameters &&
+                        origin != IrDeclarationOrigin.FUNCTION_FOR_DEFAULT_PARAMETER &&
+                        origin != JvmLoweredDeclarationOrigin.SYNTHETIC_ACCESSOR)

-        if (!declaration.shouldBeHidden)
-            return null
-
-        declaration.visibility = Visibilities.PRIVATE
-
-        return declaration.makeConstructorAccessor().also { accessor ->
-            functionMap[declaration.symbol] = accessor.symbol
-
-            // There's a special case in the JVM backend for serializing the metadata of hidden
-            // constructors - we serialize the descriptor of the original constructor, but the
-            // signature of the bridge. We implement this special case in the JVM IR backend by
-            // attaching the metadata directly to the bridge. We also have to move all annotations
-            // to the bridge method. Parameter annotations are already moved by the copyTo method.
-            accessor.metadata = declaration.metadata
-            declaration.safeAs<IrConstructorImpl>()?.metadata = null
-            accessor.annotations += declaration.annotations
-            declaration.annotations.clear()
-            declaration.valueParameters.forEach { it.annotations.clear() }
-        }.symbol
+    private fun handleHiddenConstructor(declaration: IrConstructor): IrConstructorImpl {
+        require(declaration.isOrShouldBeHidden, declaration::render)
+        return context.hiddenConstructors.getOrPut(declaration) {
+            declaration.makeConstructorAccessor().also { accessor ->
+                // There's a special case in the JVM backend for serializing the metadata of hidden
+                // constructors - we serialize the descriptor of the original constructor, but the
+                // signature of the accessor. We implement this special case in the JVM IR backend by
+                // attaching the metadata directly to the accessor. We also have to move all annotations
+                // to the accessor. Parameter annotations are already moved by the copyTo method.
+                accessor.metadata = declaration.metadata
+                declaration.safeAs<IrConstructorImpl>()?.metadata = null
+                accessor.annotations += declaration.annotations
+                declaration.annotations.clear()
+                declaration.valueParameters.forEach { it.annotations.clear() }
+            }
+        }
    }

    // In case of Java `protected static`, access could be done from a public inline function in the same package,
@@ -155,10 +166,8 @@ internal class SyntheticAccessorLowering(val context: JvmBackendContext) : IrEle
            origin = JvmLoweredDeclarationOrigin.SYNTHETIC_ACCESSOR
            name = source.name
            visibility = Visibilities.PUBLIC
-
        }.also { accessor ->
            accessor.parent = source.parent
-            pendingTransformations.add { (accessor.parent as IrDeclarationContainer).declarations.add(accessor) }

            accessor.copyTypeParametersFrom(source, JvmLoweredDeclarationOrigin.SYNTHETIC_ACCESSOR)
            accessor.copyValueParametersToStatic(source, JvmLoweredDeclarationOrigin.SYNTHETIC_ACCESSOR)
@@ -0,0 +1,10 @@
+// IGNORE_BACKEND_FIR: JVM_IR
+// FILE: 1.kt
+
+fun box(): String = X(Z("OK")).z.result
+
+// FILE: 2.kt
+
+inline class Z(val result: String)
+
+class X(val z: Z)
@@ -0,0 +1,10 @@
+// IGNORE_BACKEND_FIR: JVM_IR
+// FILE: 2.kt
+
+fun box(): String = X(Z("OK")).z.result
+
+// FILE: 1.kt
+
+inline class Z(val result: String)
+
+class X(val z: Z)
@@ -13723,6 +13723,16 @@ public class BlackBoxCodegenTestGenerated extends AbstractBlackBoxCodegenTest {
                KotlinTestUtils.assertAllTestsPresentByMetadataWithExcluded(this.getClass(), new File("compiler/testData/codegen/box/inlineClasses/hiddenConstructor"), Pattern.compile("^(.+)\\.kt$"), null, TargetBackend.JVM, true);
            }

+            @TestMetadata("constructorReferencedFromOtherFile1.kt")
+            public void testConstructorReferencedFromOtherFile1() throws Exception {
+                runTest("compiler/testData/codegen/box/inlineClasses/hiddenConstructor/constructorReferencedFromOtherFile1.kt");
+            }
+
+            @TestMetadata("constructorReferencedFromOtherFile2.kt")
+            public void testConstructorReferencedFromOtherFile2() throws Exception {
+                runTest("compiler/testData/codegen/box/inlineClasses/hiddenConstructor/constructorReferencedFromOtherFile2.kt");
+            }
+
            @TestMetadata("constructorWithDefaultParameters.kt")
            public void testConstructorWithDefaultParameters() throws Exception {
                runTest("compiler/testData/codegen/box/inlineClasses/hiddenConstructor/constructorWithDefaultParameters.kt");
@@ -13723,6 +13723,16 @@ public class LightAnalysisModeTestGenerated extends AbstractLightAnalysisModeTes
                KotlinTestUtils.assertAllTestsPresentByMetadataWithExcluded(this.getClass(), new File("compiler/testData/codegen/box/inlineClasses/hiddenConstructor"), Pattern.compile("^(.+)\\.kt$"), null, TargetBackend.JVM, true);
            }

+            @TestMetadata("constructorReferencedFromOtherFile1.kt")
+            public void testConstructorReferencedFromOtherFile1() throws Exception {
+                runTest("compiler/testData/codegen/box/inlineClasses/hiddenConstructor/constructorReferencedFromOtherFile1.kt");
+            }
+
+            @TestMetadata("constructorReferencedFromOtherFile2.kt")
+            public void testConstructorReferencedFromOtherFile2() throws Exception {
+                runTest("compiler/testData/codegen/box/inlineClasses/hiddenConstructor/constructorReferencedFromOtherFile2.kt");
+            }
+
            @TestMetadata("constructorWithDefaultParameters.kt")
            public void testConstructorWithDefaultParameters() throws Exception {
                runTest("compiler/testData/codegen/box/inlineClasses/hiddenConstructor/constructorWithDefaultParameters.kt");
@@ -12573,6 +12573,16 @@ public class FirBlackBoxCodegenTestGenerated extends AbstractFirBlackBoxCodegenT
                KotlinTestUtils.assertAllTestsPresentByMetadataWithExcluded(this.getClass(), new File("compiler/testData/codegen/box/inlineClasses/hiddenConstructor"), Pattern.compile("^(.+)\\.kt$"), null, TargetBackend.JVM_IR, true);
            }

+            @TestMetadata("constructorReferencedFromOtherFile1.kt")
+            public void testConstructorReferencedFromOtherFile1() throws Exception {
+                runTest("compiler/testData/codegen/box/inlineClasses/hiddenConstructor/constructorReferencedFromOtherFile1.kt");
+            }
+
+            @TestMetadata("constructorReferencedFromOtherFile2.kt")
+            public void testConstructorReferencedFromOtherFile2() throws Exception {
+                runTest("compiler/testData/codegen/box/inlineClasses/hiddenConstructor/constructorReferencedFromOtherFile2.kt");
+            }
+
            @TestMetadata("constructorWithDefaultParameters.kt")
            public void testConstructorWithDefaultParameters() throws Exception {
                runTest("compiler/testData/codegen/box/inlineClasses/hiddenConstructor/constructorWithDefaultParameters.kt");
@@ -12573,6 +12573,16 @@ public class IrBlackBoxCodegenTestGenerated extends AbstractIrBlackBoxCodegenTes
                KotlinTestUtils.assertAllTestsPresentByMetadataWithExcluded(this.getClass(), new File("compiler/testData/codegen/box/inlineClasses/hiddenConstructor"), Pattern.compile("^(.+)\\.kt$"), null, TargetBackend.JVM_IR, true);
            }

+            @TestMetadata("constructorReferencedFromOtherFile1.kt")
+            public void testConstructorReferencedFromOtherFile1() throws Exception {
+                runTest("compiler/testData/codegen/box/inlineClasses/hiddenConstructor/constructorReferencedFromOtherFile1.kt");
+            }
+
+            @TestMetadata("constructorReferencedFromOtherFile2.kt")
+            public void testConstructorReferencedFromOtherFile2() throws Exception {
+                runTest("compiler/testData/codegen/box/inlineClasses/hiddenConstructor/constructorReferencedFromOtherFile2.kt");
+            }
+
            @TestMetadata("constructorWithDefaultParameters.kt")
            public void testConstructorWithDefaultParameters() throws Exception {
                runTest("compiler/testData/codegen/box/inlineClasses/hiddenConstructor/constructorWithDefaultParameters.kt");
@@ -10888,6 +10888,16 @@ public class IrJsCodegenBoxTestGenerated extends AbstractIrJsCodegenBoxTest {
                KotlinTestUtils.assertAllTestsPresentByMetadataWithExcluded(this.getClass(), new File("compiler/testData/codegen/box/inlineClasses/hiddenConstructor"), Pattern.compile("^(.+)\\.kt$"), null, TargetBackend.JS_IR, true);
            }

+            @TestMetadata("constructorReferencedFromOtherFile1.kt")
+            public void testConstructorReferencedFromOtherFile1() throws Exception {
+                runTest("compiler/testData/codegen/box/inlineClasses/hiddenConstructor/constructorReferencedFromOtherFile1.kt");
+            }
+
+            @TestMetadata("constructorReferencedFromOtherFile2.kt")
+            public void testConstructorReferencedFromOtherFile2() throws Exception {
+                runTest("compiler/testData/codegen/box/inlineClasses/hiddenConstructor/constructorReferencedFromOtherFile2.kt");
+            }
+
            @TestMetadata("constructorWithDefaultParameters.kt")
            public void testConstructorWithDefaultParameters() throws Exception {
                runTest("compiler/testData/codegen/box/inlineClasses/hiddenConstructor/constructorWithDefaultParameters.kt");
@@ -12028,6 +12028,16 @@ public class JsCodegenBoxTestGenerated extends AbstractJsCodegenBoxTest {
                KotlinTestUtils.assertAllTestsPresentByMetadataWithExcluded(this.getClass(), new File("compiler/testData/codegen/box/inlineClasses/hiddenConstructor"), Pattern.compile("^(.+)\\.kt$"), null, TargetBackend.JS, true);
            }

+            @TestMetadata("constructorReferencedFromOtherFile1.kt")
+            public void testConstructorReferencedFromOtherFile1() throws Exception {
+                runTest("compiler/testData/codegen/box/inlineClasses/hiddenConstructor/constructorReferencedFromOtherFile1.kt");
+            }
+
+            @TestMetadata("constructorReferencedFromOtherFile2.kt")
+            public void testConstructorReferencedFromOtherFile2() throws Exception {
+                runTest("compiler/testData/codegen/box/inlineClasses/hiddenConstructor/constructorReferencedFromOtherFile2.kt");
+            }
+
            @TestMetadata("constructorWithDefaultParameters.kt")
            public void testConstructorWithDefaultParameters() throws Exception {
                runTest("compiler/testData/codegen/box/inlineClasses/hiddenConstructor/constructorWithDefaultParameters.kt");