From ecf4e9f6533df31f5b9b3afa669f520d65c8eda6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20Doeraene?= Date: Fri, 14 Oct 2022 12:04:51 +0200 Subject: [PATCH] Fix #14168: Do not elide fields required for Scala.js interop semantics. --- .../dotty/tools/backend/sjs/JSCodeGen.scala | 18 ++++- .../dotty/tools/dotc/transform/Memoize.scala | 24 +++++- .../compiler/RegressionTestScala3.scala | 74 +++++++++++++++++++ 3 files changed, 113 insertions(+), 3 deletions(-) diff --git a/compiler/src/dotty/tools/backend/sjs/JSCodeGen.scala b/compiler/src/dotty/tools/backend/sjs/JSCodeGen.scala index 8ec19bb994b8..6714f664620b 100644 --- a/compiler/src/dotty/tools/backend/sjs/JSCodeGen.scala +++ b/compiler/src/dotty/tools/backend/sjs/JSCodeGen.scala @@ -3144,7 +3144,23 @@ class JSCodeGen()(using genCtx: Context) { val tpe = atPhase(elimErasedValueTypePhase) { sym.info.finalResultType } - unbox(boxedResult, tpe) + if (tpe.isRef(defn.BoxedUnitClass) && sym.isGetter) { + /* Work around to reclaim Scala 2 erasure behavior, assumed by the test + * NonNativeJSTypeTest.defaultValuesForFields. + * Scala 2 erases getters of `Unit`-typed fields as returning `Unit` + * (not `BoxedUnit`). Therefore, when called in expression position, + * the call site introduces an explicit `BoxedUnit.UNIT`. Even if the + * field has not been initialized at all (with `= _`), this results in + * an actual `()` value. + * In Scala 3, the same pattern returns `null`, as a `BoxedUnit`, so we + * introduce here an explicit `()` value. + * TODO We should remove this branch if the upstream test is updated + * not to assume such a strict interpretation of erasure. + */ + js.Block(boxedResult, js.Undefined()) + } else { + unbox(boxedResult, tpe) + } } } diff --git a/compiler/src/dotty/tools/dotc/transform/Memoize.scala b/compiler/src/dotty/tools/dotc/transform/Memoize.scala index d20f3e1a8da4..6456066bfdb0 100644 --- a/compiler/src/dotty/tools/dotc/transform/Memoize.scala +++ b/compiler/src/dotty/tools/dotc/transform/Memoize.scala @@ -16,8 +16,12 @@ import Flags._ import Decorators._ import StdNames.nme +import sjs.JSSymUtils._ + import util.Store +import dotty.tools.backend.sjs.JSDefinitions.jsdefn + object Memoize { val name: String = "memoize" val description: String = "add private fields to getters and setters" @@ -142,14 +146,30 @@ class Memoize extends MiniPhase with IdentityDenotTransformer { thisPhase => } if sym.is(Accessor, butNot = NoFieldNeeded) then + /* Tests whether the semantics of Scala.js require a field for this symbol, irrespective of any + * optimization we think we can do. This is the case if one of the following is true: + * - it is a member of a JS type, since it needs to be visible as a JavaScript field + * - is is exported as static member of the companion class, since it needs to be visible as a JavaScript static field + * - it is exported to the top-level, since that can only be done as a true top-level variable, i.e., a field + */ + def sjsNeedsField: Boolean = + ctx.settings.scalajs.value && ( + sym.owner.isJSType + || sym.hasAnnotation(jsdefn.JSExportTopLevelAnnot) + || sym.hasAnnotation(jsdefn.JSExportStaticAnnot) + ) + def adaptToField(field: Symbol, tree: Tree): Tree = if (tree.isEmpty) tree else tree.ensureConforms(field.info.widen) def isErasableBottomField(field: Symbol, cls: Symbol): Boolean = - !field.isVolatile && ((cls eq defn.NothingClass) || (cls eq defn.NullClass) || (cls eq defn.BoxedUnitClass)) + !field.isVolatile + && ((cls eq defn.NothingClass) || (cls eq defn.NullClass) || (cls eq defn.BoxedUnitClass)) + && !sjsNeedsField if sym.isGetter then - val constantFinalVal = sym.isAllOf(Accessor | Final, butNot = Mutable) && tree.rhs.isInstanceOf[Literal] + val constantFinalVal = + sym.isAllOf(Accessor | Final, butNot = Mutable) && tree.rhs.isInstanceOf[Literal] && !sjsNeedsField if constantFinalVal then // constant final vals do not need to be transformed at all, and do not need a field tree diff --git a/tests/sjs-junit/test/org/scalajs/testsuite/compiler/RegressionTestScala3.scala b/tests/sjs-junit/test/org/scalajs/testsuite/compiler/RegressionTestScala3.scala index e8e2ef0b0746..3a4f4d5e4b0a 100644 --- a/tests/sjs-junit/test/org/scalajs/testsuite/compiler/RegressionTestScala3.scala +++ b/tests/sjs-junit/test/org/scalajs/testsuite/compiler/RegressionTestScala3.scala @@ -3,9 +3,17 @@ package org.scalajs.testsuite.compiler import org.junit.Assert.* import org.junit.Test +import scala.concurrent.ExecutionContext.Implicits.{global => globalEc} +import scala.concurrent.Future + import scala.scalajs.js import scala.scalajs.js.annotation._ +import org.scalajs.junit.async._ + +import org.scalajs.testsuite.jsinterop.ExportLoopback +import org.scalajs.testsuite.utils.Platform._ + class RegressionTestScala3 { import RegressionTestScala3.* @@ -78,6 +86,39 @@ class RegressionTestScala3 { val f3 = { () => i += 1 } assertSame(f3, Thunk.asFunction0(f3())) } + + @Test def mandatoryFieldsForSJSSemanticsInNonNativeJSClassIssue14168(): Unit = { + val nonNativeJS = new Issue14168.NonNativeJSClass().asInstanceOf[js.Dynamic] + assertEquals("string", nonNativeJS.stringField) + assertEquals(null, nonNativeJS.nullField) + assertEquals((), nonNativeJS.unitField) + assertEquals(true, nonNativeJS.hasOwnProperty("unitField")) + } + + @Test def mandatoryFieldsForSJSSemanticsInStaticExportsIssue14168(): Unit = { + val staticExports = js.constructorOf[Issue14168.StaticExports] + assertEquals("string", staticExports.stringField) + assertEquals(null, staticExports.nullField) + assertEquals((), staticExports.unitField) + assertEquals(true, staticExports.hasOwnProperty("unitField")) + } + + @Test def mandatoryFieldsForSJSSemanticsInTopLevelExportsIssue14168(): AsyncResult = await { + if (isNoModule) { + import js.Dynamic.global + Future { + assertEquals("string", global.RegressionTestScala3_Issue14168_stringField) + assertEquals(null, global.RegressionTestScala3_Issue14168_nullField) + assertEquals((), global.RegressionTestScala3_Issue14168_unitField) + } + } else { + for (exports <- ExportLoopback.exportsNamespace) yield { + assertEquals("string", exports.RegressionTestScala3_Issue14168_stringField) + assertEquals(null, exports.RegressionTestScala3_Issue14168_nullField) + assertEquals((), exports.RegressionTestScala3_Issue14168_unitField) + } + } + } } object RegressionTestScala3 { @@ -148,6 +189,39 @@ object RegressionTestScala3 { val entries = js.Object.entries(obj) val js.Tuple2(k, v) = entries(0): @unchecked } + + object Issue14168 { + class NonNativeJSClass extends js.Object { + val stringField: "string" = "string" + val nullField: Null = null + val unitField: Unit = () + final val finalValField = "finalVal" + } + + class StaticExports extends js.Object + + object StaticExports { + @JSExportStatic + val stringField: "string" = "string" + @JSExportStatic + val nullField: Null = null + @JSExportStatic + val unitField: Unit = () + @JSExportStatic + final val finalValField = "finalVal" + } + + object TopLevelExports { + @JSExportTopLevel("RegressionTestScala3_Issue14168_stringField") + val stringField: "string" = "string" + @JSExportTopLevel("RegressionTestScala3_Issue14168_nullField") + val nullField: Null = null + @JSExportTopLevel("RegressionTestScala3_Issue14168_unitField") + val unitField: Unit = () + @JSExportTopLevel("RegressionTestScala3_Issue14168_finalValField") + final val finalValField = "finalVal" + } + } } // This class needs to be at the top-level, not in an object, to reproduce the issue