Fix scala#14168: Do not elide fields required for Scala.js interop semantics.

Author: sjrd

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)
+      }
     }
   }
 

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

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.*
 
@@ -90,6 +98,39 @@ class RegressionTestScala3 {
 
     assertEquals(5, Issue16173.bar1())
   }
+
+  @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 {
@@ -174,6 +215,39 @@ object RegressionTestScala3 {
     @JSGlobal("RegressionTestScala3_Issue16173_bar")
     def bar1(): 5 = js.native
   }
+
+  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