From f0f60907e2d9b6b7249deba5d95d23b489924622 Mon Sep 17 00:00:00 2001 From: Sergej Jaskiewicz Date: Thu, 9 Sep 2021 17:37:28 +0300 Subject: [PATCH] [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 --- .../backend/js/export/ExportModelGenerator.kt | 2 +- .../transformers/irToJs/JsClassGenerator.kt | 66 ++++++- .../semantics/IrBoxJsES6TestGenerated.java | 5 + .../ir/semantics/IrBoxJsTestGenerated.java | 5 + .../js/test/semantics/BoxJsTestGenerated.java | 5 + .../box/propertyOverride/exportedBaseClass.kt | 174 ++++++++++++++++++ 6 files changed, 251 insertions(+), 6 deletions(-) create mode 100644 js/js.translator/testData/box/propertyOverride/exportedBaseClass.kt diff --git a/compiler/ir/backend.js/src/org/jetbrains/kotlin/ir/backend/js/export/ExportModelGenerator.kt b/compiler/ir/backend.js/src/org/jetbrains/kotlin/ir/backend/js/export/ExportModelGenerator.kt index 3fdfbb09e88..e6f71e774dd 100644 --- a/compiler/ir/backend.js/src/org/jetbrains/kotlin/ir/backend/js/export/ExportModelGenerator.kt +++ b/compiler/ir/backend.js/src/org/jetbrains/kotlin/ir/backend/js/export/ExportModelGenerator.kt @@ -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) } diff --git a/compiler/ir/backend.js/src/org/jetbrains/kotlin/ir/backend/js/transformers/irToJs/JsClassGenerator.kt b/compiler/ir/backend.js/src/org/jetbrains/kotlin/ir/backend/js/transformers/irToJs/JsClassGenerator.kt index eb4cf81e7fa..f2ab9298e39 100644 --- a/compiler/ir/backend.js/src/org/jetbrains/kotlin/ir/backend/js/transformers/irToJs/JsClassGenerator.kt +++ b/compiler/ir/backend.js/src/org/jetbrains/kotlin/ir/backend/js/transformers/irToJs/JsClassGenerator.kt @@ -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 ) ) } diff --git a/js/js.tests/tests-gen/org/jetbrains/kotlin/js/test/es6/semantics/IrBoxJsES6TestGenerated.java b/js/js.tests/tests-gen/org/jetbrains/kotlin/js/test/es6/semantics/IrBoxJsES6TestGenerated.java index aad16b3ef84..f8b5b8e2818 100644 --- a/js/js.tests/tests-gen/org/jetbrains/kotlin/js/test/es6/semantics/IrBoxJsES6TestGenerated.java +++ b/js/js.tests/tests-gen/org/jetbrains/kotlin/js/test/es6/semantics/IrBoxJsES6TestGenerated.java @@ -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"); diff --git a/js/js.tests/tests-gen/org/jetbrains/kotlin/js/test/ir/semantics/IrBoxJsTestGenerated.java b/js/js.tests/tests-gen/org/jetbrains/kotlin/js/test/ir/semantics/IrBoxJsTestGenerated.java index a9e810ed8f9..7d9e7dc1879 100644 --- a/js/js.tests/tests-gen/org/jetbrains/kotlin/js/test/ir/semantics/IrBoxJsTestGenerated.java +++ b/js/js.tests/tests-gen/org/jetbrains/kotlin/js/test/ir/semantics/IrBoxJsTestGenerated.java @@ -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"); diff --git a/js/js.tests/tests-gen/org/jetbrains/kotlin/js/test/semantics/BoxJsTestGenerated.java b/js/js.tests/tests-gen/org/jetbrains/kotlin/js/test/semantics/BoxJsTestGenerated.java index fe2c33daa6e..168a68e795b 100644 --- a/js/js.tests/tests-gen/org/jetbrains/kotlin/js/test/semantics/BoxJsTestGenerated.java +++ b/js/js.tests/tests-gen/org/jetbrains/kotlin/js/test/semantics/BoxJsTestGenerated.java @@ -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"); diff --git a/js/js.translator/testData/box/propertyOverride/exportedBaseClass.kt b/js/js.translator/testData/box/propertyOverride/exportedBaseClass.kt new file mode 100644 index 00000000000..a8d400ec358 --- /dev/null +++ b/js/js.translator/testData/box/propertyOverride/exportedBaseClass.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'; +} \ No newline at end of file