Skip to content

Commit ecf4e9f

Browse files
committed
Fix #14168: Do not elide fields required for Scala.js interop semantics.
1 parent 62684d0 commit ecf4e9f

File tree

3 files changed

+113
-3
lines changed

3 files changed

+113
-3
lines changed

compiler/src/dotty/tools/backend/sjs/JSCodeGen.scala

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3144,7 +3144,23 @@ class JSCodeGen()(using genCtx: Context) {
31443144
val tpe = atPhase(elimErasedValueTypePhase) {
31453145
sym.info.finalResultType
31463146
}
3147-
unbox(boxedResult, tpe)
3147+
if (tpe.isRef(defn.BoxedUnitClass) && sym.isGetter) {
3148+
/* Work around to reclaim Scala 2 erasure behavior, assumed by the test
3149+
* NonNativeJSTypeTest.defaultValuesForFields.
3150+
* Scala 2 erases getters of `Unit`-typed fields as returning `Unit`
3151+
* (not `BoxedUnit`). Therefore, when called in expression position,
3152+
* the call site introduces an explicit `BoxedUnit.UNIT`. Even if the
3153+
* field has not been initialized at all (with `= _`), this results in
3154+
* an actual `()` value.
3155+
* In Scala 3, the same pattern returns `null`, as a `BoxedUnit`, so we
3156+
* introduce here an explicit `()` value.
3157+
* TODO We should remove this branch if the upstream test is updated
3158+
* not to assume such a strict interpretation of erasure.
3159+
*/
3160+
js.Block(boxedResult, js.Undefined())
3161+
} else {
3162+
unbox(boxedResult, tpe)
3163+
}
31483164
}
31493165
}
31503166

compiler/src/dotty/tools/dotc/transform/Memoize.scala

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,12 @@ import Flags._
1616
import Decorators._
1717
import StdNames.nme
1818

19+
import sjs.JSSymUtils._
20+
1921
import util.Store
2022

23+
import dotty.tools.backend.sjs.JSDefinitions.jsdefn
24+
2125
object Memoize {
2226
val name: String = "memoize"
2327
val description: String = "add private fields to getters and setters"
@@ -142,14 +146,30 @@ class Memoize extends MiniPhase with IdentityDenotTransformer { thisPhase =>
142146
}
143147

144148
if sym.is(Accessor, butNot = NoFieldNeeded) then
149+
/* Tests whether the semantics of Scala.js require a field for this symbol, irrespective of any
150+
* optimization we think we can do. This is the case if one of the following is true:
151+
* - it is a member of a JS type, since it needs to be visible as a JavaScript field
152+
* - is is exported as static member of the companion class, since it needs to be visible as a JavaScript static field
153+
* - it is exported to the top-level, since that can only be done as a true top-level variable, i.e., a field
154+
*/
155+
def sjsNeedsField: Boolean =
156+
ctx.settings.scalajs.value && (
157+
sym.owner.isJSType
158+
|| sym.hasAnnotation(jsdefn.JSExportTopLevelAnnot)
159+
|| sym.hasAnnotation(jsdefn.JSExportStaticAnnot)
160+
)
161+
145162
def adaptToField(field: Symbol, tree: Tree): Tree =
146163
if (tree.isEmpty) tree else tree.ensureConforms(field.info.widen)
147164

148165
def isErasableBottomField(field: Symbol, cls: Symbol): Boolean =
149-
!field.isVolatile && ((cls eq defn.NothingClass) || (cls eq defn.NullClass) || (cls eq defn.BoxedUnitClass))
166+
!field.isVolatile
167+
&& ((cls eq defn.NothingClass) || (cls eq defn.NullClass) || (cls eq defn.BoxedUnitClass))
168+
&& !sjsNeedsField
150169

151170
if sym.isGetter then
152-
val constantFinalVal = sym.isAllOf(Accessor | Final, butNot = Mutable) && tree.rhs.isInstanceOf[Literal]
171+
val constantFinalVal =
172+
sym.isAllOf(Accessor | Final, butNot = Mutable) && tree.rhs.isInstanceOf[Literal] && !sjsNeedsField
153173
if constantFinalVal then
154174
// constant final vals do not need to be transformed at all, and do not need a field
155175
tree

tests/sjs-junit/test/org/scalajs/testsuite/compiler/RegressionTestScala3.scala

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,17 @@ package org.scalajs.testsuite.compiler
33
import org.junit.Assert.*
44
import org.junit.Test
55

6+
import scala.concurrent.ExecutionContext.Implicits.{global => globalEc}
7+
import scala.concurrent.Future
8+
69
import scala.scalajs.js
710
import scala.scalajs.js.annotation._
811

12+
import org.scalajs.junit.async._
13+
14+
import org.scalajs.testsuite.jsinterop.ExportLoopback
15+
import org.scalajs.testsuite.utils.Platform._
16+
917
class RegressionTestScala3 {
1018
import RegressionTestScala3.*
1119

@@ -78,6 +86,39 @@ class RegressionTestScala3 {
7886
val f3 = { () => i += 1 }
7987
assertSame(f3, Thunk.asFunction0(f3()))
8088
}
89+
90+
@Test def mandatoryFieldsForSJSSemanticsInNonNativeJSClassIssue14168(): Unit = {
91+
val nonNativeJS = new Issue14168.NonNativeJSClass().asInstanceOf[js.Dynamic]
92+
assertEquals("string", nonNativeJS.stringField)
93+
assertEquals(null, nonNativeJS.nullField)
94+
assertEquals((), nonNativeJS.unitField)
95+
assertEquals(true, nonNativeJS.hasOwnProperty("unitField"))
96+
}
97+
98+
@Test def mandatoryFieldsForSJSSemanticsInStaticExportsIssue14168(): Unit = {
99+
val staticExports = js.constructorOf[Issue14168.StaticExports]
100+
assertEquals("string", staticExports.stringField)
101+
assertEquals(null, staticExports.nullField)
102+
assertEquals((), staticExports.unitField)
103+
assertEquals(true, staticExports.hasOwnProperty("unitField"))
104+
}
105+
106+
@Test def mandatoryFieldsForSJSSemanticsInTopLevelExportsIssue14168(): AsyncResult = await {
107+
if (isNoModule) {
108+
import js.Dynamic.global
109+
Future {
110+
assertEquals("string", global.RegressionTestScala3_Issue14168_stringField)
111+
assertEquals(null, global.RegressionTestScala3_Issue14168_nullField)
112+
assertEquals((), global.RegressionTestScala3_Issue14168_unitField)
113+
}
114+
} else {
115+
for (exports <- ExportLoopback.exportsNamespace) yield {
116+
assertEquals("string", exports.RegressionTestScala3_Issue14168_stringField)
117+
assertEquals(null, exports.RegressionTestScala3_Issue14168_nullField)
118+
assertEquals((), exports.RegressionTestScala3_Issue14168_unitField)
119+
}
120+
}
121+
}
81122
}
82123

83124
object RegressionTestScala3 {
@@ -148,6 +189,39 @@ object RegressionTestScala3 {
148189
val entries = js.Object.entries(obj)
149190
val js.Tuple2(k, v) = entries(0): @unchecked
150191
}
192+
193+
object Issue14168 {
194+
class NonNativeJSClass extends js.Object {
195+
val stringField: "string" = "string"
196+
val nullField: Null = null
197+
val unitField: Unit = ()
198+
final val finalValField = "finalVal"
199+
}
200+
201+
class StaticExports extends js.Object
202+
203+
object StaticExports {
204+
@JSExportStatic
205+
val stringField: "string" = "string"
206+
@JSExportStatic
207+
val nullField: Null = null
208+
@JSExportStatic
209+
val unitField: Unit = ()
210+
@JSExportStatic
211+
final val finalValField = "finalVal"
212+
}
213+
214+
object TopLevelExports {
215+
@JSExportTopLevel("RegressionTestScala3_Issue14168_stringField")
216+
val stringField: "string" = "string"
217+
@JSExportTopLevel("RegressionTestScala3_Issue14168_nullField")
218+
val nullField: Null = null
219+
@JSExportTopLevel("RegressionTestScala3_Issue14168_unitField")
220+
val unitField: Unit = ()
221+
@JSExportTopLevel("RegressionTestScala3_Issue14168_finalValField")
222+
final val finalValField = "finalVal"
223+
}
224+
}
151225
}
152226

153227
// This class needs to be at the top-level, not in an object, to reproduce the issue

0 commit comments

Comments
 (0)