[JS IR] Fix generating property accessors

TL;DR, before we were emitting this JS code: Object.defineProperty(Base.prototype, 'bar', { configurable: true, get: Base.prototype._get_bar__0_k$, set: Base.prototype._set_bar__a4enbm_k$ }); Now we emit this code instead: Object.defineProperty(Base.prototype, 'bar', { configurable: true, get: function () { return this._get_bar__0_k$(); }, set: function (value) { this._set_bar__a4enbm_k$(value); } }); This fixes the issue where if we had a @JsExport'ed base class with a public property overridden in a non-exported derived class, we couldn't access that property from JS if we were given an instance of the derived class. #KT-41912 Fixed
2021-09-09 17:37:28 +03:00
parent 841f5a583e
commit f0f60907e2
6 changed files with 251 additions and 6 deletions
@@ -430,7 +430,7 @@ private fun shouldDeclarationBeExported(declaration: IrDeclarationWithName, cont
    }
 }

-private fun IrSimpleFunction.isOverriddenExported(context: JsIrBackendContext?): Boolean =
+fun IrSimpleFunction.isOverriddenExported(context: JsIrBackendContext?): Boolean =
    overriddenSymbols
        .any { shouldDeclarationBeExported(it.owner, context) }

@@ -8,6 +8,7 @@ package org.jetbrains.kotlin.ir.backend.js.transformers.irToJs
 import org.jetbrains.kotlin.descriptors.DescriptorVisibilities
 import org.jetbrains.kotlin.descriptors.Modality
 import org.jetbrains.kotlin.ir.backend.js.export.isExported
+import org.jetbrains.kotlin.ir.backend.js.export.isOverriddenExported
 import org.jetbrains.kotlin.ir.backend.js.utils.*
 import org.jetbrains.kotlin.ir.declarations.*
 import org.jetbrains.kotlin.ir.symbols.IrClassSymbol
@@ -107,6 +108,19 @@ class JsClassGenerator(private val irClass: IrClass, val context: JsGenerationCo
                if (property.isFakeOverride)
                    continue

+                fun IrSimpleFunction.propertyAccessorForwarder(
+                    description: String,
+                    callActualAccessor: (JsNameRef) -> JsStatement
+                ): JsFunction? =
+                    when (visibility) {
+                        DescriptorVisibilities.PRIVATE -> null
+                        else -> JsFunction(
+                            emptyScope,
+                            JsBlock(callActualAccessor(JsNameRef(context.getNameForMemberFunction(this), JsThisRef()))),
+                            description
+                        )
+                    }
+
                fun IrSimpleFunction.accessorRef(): JsNameRef? =
                    when (visibility) {
                        DescriptorVisibilities.PRIVATE -> null
@@ -116,18 +130,60 @@ class JsClassGenerator(private val irClass: IrClass, val context: JsGenerationCo
                        )
                    }

-                if (irClass.isExported(context.staticContext.backendContext) ||
+                // Don't generate `defineProperty` if the property overrides a property from an exported class,
+                // because we've already generated `defineProperty` for the base class property.
+                // In other words, we only want to generate `defineProperty` once for each property.
+                if (irClass.isExported(context.staticContext.backendContext) &&
+                    property.getter?.isOverriddenExported(context.staticContext.backendContext) == false ||
                    property.getter?.overridesExternal() == true ||
                    property.getJsName() != null
                ) {
-                    val getterRef = property.getter?.accessorRef()
-                    val setterRef = property.setter?.accessorRef()
+
+                    // Use "direct dispatch" for final properties, i. e. instead of this:
+                    //     Object.defineProperty(Foo.prototype, 'prop', {
+                    //         configurable: true,
+                    //         get: function() { return this._get_prop__0_k$(); },
+                    //         set: function(v) { this._set_prop__a4enbm_k$(v); }
+                    //     });
+                    // emit this:
+                    //     Object.defineProperty(Foo.prototype, 'prop', {
+                    //         configurable: true,
+                    //         get: Foo.prototype._get_prop__0_k$,
+                    //         set: Foo.prototype._set_prop__a4enbm_k$
+                    //     });
+
+                    val getterForwarder = if (property.getter?.modality == Modality.FINAL) property.getter?.accessorRef()
+                    else {
+                        property.getter?.propertyAccessorForwarder("getter forwarder") { getterRef ->
+                            JsReturn(
+                                JsInvocation(
+                                    getterRef
+                                )
+                            )
+                        }
+                    }
+
+                    val setterForwarder = if (property.setter?.modality == Modality.FINAL) property.setter?.accessorRef()
+                    else {
+                        property.setter?.let {
+                            val setterArgName = JsName("value")
+                            it.propertyAccessorForwarder("setter forwarder") { setterRef ->
+                                JsInvocation(
+                                    setterRef,
+                                    JsNameRef(setterArgName)
+                                ).makeStmt()
+                            }?.apply {
+                                parameters.add(JsParameter(setterArgName))
+                            }
+                        }
+                    }
+
                    classBlock.statements += JsExpressionStatement(
                        defineProperty(
                            classPrototypeRef,
                            context.getNameForProperty(property).ident,
-                            getter = getterRef,
-                            setter = setterRef
+                            getter = getterForwarder,
+                            setter = setterForwarder
                        )
                    )
                }
@@ -6990,6 +6990,11 @@ public class IrBoxJsES6TestGenerated extends AbstractIrBoxJsES6Test {
            runTest("js/js.translator/testData/box/propertyOverride/checkSupertypeOrder.kt");
        }

+        @TestMetadata("exportedBaseClass.kt")
+        public void testExportedBaseClass() throws Exception {
+            runTest("js/js.translator/testData/box/propertyOverride/exportedBaseClass.kt");
+        }
+
        @TestMetadata("initOverrideInConstructor.kt")
        public void testInitOverrideInConstructor() throws Exception {
            runTest("js/js.translator/testData/box/propertyOverride/initOverrideInConstructor.kt");
@@ -6990,6 +6990,11 @@ public class IrBoxJsTestGenerated extends AbstractIrBoxJsTest {
            runTest("js/js.translator/testData/box/propertyOverride/checkSupertypeOrder.kt");
        }

+        @TestMetadata("exportedBaseClass.kt")
+        public void testExportedBaseClass() throws Exception {
+            runTest("js/js.translator/testData/box/propertyOverride/exportedBaseClass.kt");
+        }
+
        @TestMetadata("initOverrideInConstructor.kt")
        public void testInitOverrideInConstructor() throws Exception {
            runTest("js/js.translator/testData/box/propertyOverride/initOverrideInConstructor.kt");
@@ -7010,6 +7010,11 @@ public class BoxJsTestGenerated extends AbstractBoxJsTest {
            runTest("js/js.translator/testData/box/propertyOverride/checkSupertypeOrder.kt");
        }

+        @TestMetadata("exportedBaseClass.kt")
+        public void testExportedBaseClass() throws Exception {
+            runTest("js/js.translator/testData/box/propertyOverride/exportedBaseClass.kt");
+        }
+
        @TestMetadata("initOverrideInConstructor.kt")
        public void testInitOverrideInConstructor() throws Exception {
            runTest("js/js.translator/testData/box/propertyOverride/initOverrideInConstructor.kt");
@@ -0,0 +1,174 @@
+// RUN_PLAIN_BOX_FUNCTION
+// INFER_MAIN_MODULE
+// SKIP_MINIFICATION
+// SKIP_DCE_DRIVEN
+// SKIP_NODE_JS
+
+// MODULE: exportedBaseClass
+// FILE: lib.kt
+@JsExport
+abstract class ExportedBase {
+    abstract val foo: String
+    abstract var bar: String
+
+    val fooFinal: String
+        get() = foo
+}
+
+@JsExport
+class ExportedDerived1 : ExportedBase() {
+    override val foo: String
+        get() = "ExportedDerived1.foo"
+
+    private var _bar = "ExportedDerived1.bar"
+
+    override var bar: String
+        get() = _bar
+        set(value) { _bar = value }
+}
+
+abstract class NonExportedBase {
+    abstract val foo: String
+    abstract var bar: String
+}
+
+@JsExport
+class ExportedDerived2 : NonExportedBase() {
+    override val foo: String
+        get() = "ExportedDerived2.foo"
+
+    private var _bar = "ExportedDerived2.bar"
+
+    override var bar: String
+        get() = _bar
+        set(value) { _bar = value }
+}
+
+// Non-exported
+open class Derived1 : ExportedBase() {
+    override val foo: String
+        get() = "1"
+
+    private var _bar = "1"
+
+    override var bar: String
+        get() = _bar
+        set(value) { _bar = value }
+}
+
+class Derived2 : Derived1() {
+    override val foo: String
+        get() = "2"
+
+    private var _bar = "2"
+
+    override var bar: String
+        get() = _bar
+        set(value) { _bar = value }
+}
+
+@JsExport
+fun getDerived1(): ExportedBase = Derived1()
+
+@JsExport
+fun getDerived2(): ExportedBase = Derived2()
+
+@JsExport
+fun isLegacyBackend() = testUtils.isLegacyBackend()
+
+// FILE: test.js
+function assertEquals(expected, actual, msg) {
+    if (expected !== actual) {
+        throw "Unexpected value: expected = '" + expected + "', actual = '" + actual + "' — " + msg;
+    }
+}
+
+function box() {
+
+    var d = exportedBaseClass;
+
+    if (!d.isLegacyBackend()) {
+        assertEquals(
+            true,
+            d.ExportedBase.prototype.hasOwnProperty('foo'),
+            'Property foo should be defined for ExportedBase.prototype'
+        );
+        assertEquals(
+            true,
+            d.ExportedBase.prototype.hasOwnProperty('bar'),
+            'Property bar should be defined for ExportedBase.prototype'
+        );
+    }
+
+    assertEquals(
+        true,
+        d.ExportedBase.prototype.hasOwnProperty('fooFinal'),
+        'Property fooFinal should be defined for ExportedBase.prototype'
+    );
+
+    var derived1 = d.getDerived1();
+    assertEquals('1', derived1.foo, "derived1.foo");
+    assertEquals('1', derived1.fooFinal, "derived1.fooFinal");
+    assertEquals('1', derived1.bar, "derived1.bar initial value");
+    derived1.bar = '11';
+    assertEquals('11', derived1.bar, "derived1.bar after write");
+    if (!d.isLegacyBackend()) {
+        assertEquals(
+            false,
+            Object.getPrototypeOf(derived1).hasOwnProperty('foo'),
+            'Property foo of Derived1 should be inherited from ExportedBase.prototype'
+        );
+        assertEquals(
+            false,
+            Object.getPrototypeOf(derived1).hasOwnProperty('bar'),
+            'Property bar should of Derived1 be inherited from ExportedBase.prototype'
+        );
+    }
+
+    var derived2 = d.getDerived2();
+    assertEquals('2', derived2.foo, "derived2.foo");
+    assertEquals('2', derived2.fooFinal, "derived2.fooFinal");
+    assertEquals('2', derived2.bar, "derived2.bar initial value");
+    derived2.bar = '22';
+    assertEquals('22', derived2.bar, "derived2.bar after write");
+    if (!d.isLegacyBackend()) {
+        assertEquals(
+            false,
+            Object.getPrototypeOf(derived2).hasOwnProperty('foo'),
+            'Property foo of Derived2 should be inherited from ExportedBase.prototype'
+        );
+        assertEquals(
+            false,
+            Object.getPrototypeOf(derived2).hasOwnProperty('bar'),
+            'Property bar of Derived2 should be inherited from ExportedBase.prototype'
+        );
+    }
+
+    var exportedDerived1 = new d.ExportedDerived1();
+    assertEquals('ExportedDerived1.foo', exportedDerived1.foo, "exportedDerived1.foo");
+    assertEquals('ExportedDerived1.bar', exportedDerived1.bar, "exportedDerived1.bar initial value");
+    exportedDerived1.bar = 'ExportedDerived1.bar (updated)';
+    assertEquals('ExportedDerived1.bar (updated)', exportedDerived1.bar, "exportedDerived1.bar after write");
+    if (!d.isLegacyBackend()) {
+        assertEquals(
+            false,
+            d.ExportedDerived1.prototype.hasOwnProperty('foo'),
+            'Property foo of ExportedDerived1 should be inherited from ExportedBase.prototype'
+        );
+        assertEquals(
+            false,
+            d.ExportedDerived1.prototype.hasOwnProperty('bar'),
+            'Property bar of ExportedDerived1 should be inherited from ExportedBase.prototype'
+        );
+    }
+
+    var exportedDerived2 = new d.ExportedDerived2();
+    assertEquals('ExportedDerived2.foo', exportedDerived2.foo, "exportedDerived2.foo");
+    assertEquals('ExportedDerived2.bar', exportedDerived2.bar, "exportedDerived2.bar initial value");
+    exportedDerived2.bar = 'ExportedDerived2.bar (updated)';
+    assertEquals('ExportedDerived2.bar (updated)', exportedDerived2.bar, "exportedDerived2.bar after write");
+    assertEquals(true, d.ExportedDerived2.prototype.hasOwnProperty('foo'), 'Property foo should be defined for ExportedDerived2.prototype');
+    assertEquals(true, d.ExportedDerived2.prototype.hasOwnProperty('bar'), 'Property bar should be defined for ExportedDerived2.prototype');
+
+    return 'OK';
+}