Skip to content

Commit

Permalink
Fix scala#14168: Do not elide fields required for Scala.js interop se…
Browse files Browse the repository at this point in the history
…mantics.
  • Loading branch information
sjrd committed Oct 14, 2022
1 parent 62684d0 commit 08159ee
Show file tree
Hide file tree
Showing 2 changed files with 96 additions and 2 deletions.
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
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 @@ -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 {
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit 08159ee

Please sign in to comment.