Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Oct 18, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion compiler/src/dotty/tools/backend/sjs/JSCodeGen.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we instead change the behavior of Scala 3? We already have some logic for "unboxing" nulls from value classes: https://github.com/lampepfl/dotty/blob/3636ee6d0fda29daf9ef6e4e0491e2e504b227ed/compiler/src/dotty/tools/dotc/transform/Erasure.scala#L292-L317

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We cannot change it without breaking binary compatibility ;)

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

Expand Down
24 changes: 22 additions & 2 deletions compiler/src/dotty/tools/dotc/transform/Memoize.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand Down
25 changes: 25 additions & 0 deletions tests/run/uninitialized-field-values.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import scala.compiletime.uninitialized

object Test:
def main(args: Array[String]): Unit =
val foo = new Foo
assertEquals(0, foo.int)
assertEquals(false, foo.bool)
assertEquals(null, foo.str)
assertEquals((), foo.unit)
assertEquals(ValueClass(0), foo.vc)
end main

def assertEquals(expected: Any, actual: Any): Unit =
assert(expected == actual)

class Foo:
var int: Int = uninitialized
var bool: Boolean = uninitialized
var str: String = uninitialized
var unit: Unit = uninitialized
var vc: ValueClass = uninitialized
end Foo

class ValueClass(val i: Int) extends AnyVal
end Test
Original file line number Diff line number Diff line change
Expand Up @@ -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.*

Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand Down