Merge pull request tanishiking#93 from sjrd/fix-type-tests-for-ancestors-of-hijacked-classes

Fix static type tests for ancestors of hijacked classes.

Author: sjrd

build.sbt

@@ -228,8 +228,6 @@ lazy val IgnoredTestNames: Set[String] = {
   Set(
     // reflective call: should be throw an exception when reflective proxy not found
     "org.scalajs.testsuite.compiler.WasPublicBeforeTyperTestScala2",
-    // javaLangNumber failed: java.lang.AssertionError: 1, class java.lang.Number expected:<true> but was:<false>
-    "org.scalajs.testsuite.compiler.RuntimeTypeTestsTest",
     // Various run-time errors and JS exceptions
     "org.scalajs.testsuite.compiler.InteroperabilityTest",
     "org.scalajs.testsuite.compiler.RegressionJSTest",
@@ -242,17 +240,13 @@ lazy val IgnoredTestNames: Set[String] = {
     "org.scalajs.testsuite.compiler.ReflectionTest",
     "org.scalajs.testsuite.compiler.RuntimeTypeTestsJSTest",
     "org.scalajs.testsuite.jsinterop.ModulesTest",
-    // eqEqJLFloat/eqEqJLDouble failed: java.lang.AssertionError: null
-    "org.scalajs.testsuite.compiler.RegressionTest",
     // TypeError: WebAssembly objects are opaque
     "org.scalajs.testsuite.javalib.lang.SystemJSTest",
     // throwablesAreTrueErrors failed: org.junit.ComparisonFailure: expected:<[object [Error]]> but was:<[object [Object]]>
     // throwablesAreJSErrors failed: java.lang.AssertionError: null
     "org.scalajs.testsuite.javalib.lang.ThrowableJSTest",
     // keepBreakToLabelWithinFinallyBlock_Issue2689 failed: java.lang.AssertionError: expected:<2> but was:<1>
     "org.scalajs.testsuite.compiler.OptimizerTest",
-    // nonUnitBoxedPrimitiveValuesAreSerializable failed: java.lang.AssertionError: Boolean
-    "org.scalajs.testsuite.javalib.io.SerializableTest",
     // No support for stack traces
     "org.scalajs.testsuite.library.StackTraceTest",
   )

wasm/src/main/scala/ir2wasm/HelperFunctions.scala

@@ -690,6 +690,9 @@ object HelperFunctions {
        *
        * Since computing `jsValueType` is somewhat expensive, we first test
        * whether `specialInstanceTypes != 0` before calling `jsValueType`.
+       *
+       * There is a more elaborated concrete example of this algorithm in
+       * `genInstanceTest`.
        */
       instrs += LOCAL_GET(typeDataParam)
       instrs += STRUCT_GET(WasmStructTypeName.typeData, specialInstanceTypesIdx)
@@ -1636,6 +1639,8 @@ object HelperFunctions {
   def genInstanceTest(clazz: LinkedClass)(implicit ctx: WasmContext): Unit = {
     assert(clazz.kind == ClassKind.Interface)
 
+    val classInfo = ctx.getClassInfo(clazz.className)
+
     val fctx = WasmFunctionContext(
       Names.WasmFunctionName.instanceTest(clazz.name.name),
       List("expr" -> WasmRefType.anyref),
@@ -1645,9 +1650,9 @@ object HelperFunctions {
 
     import fctx.instrs
 
-    val itables = fctx.addSyntheticLocal(
-      Types.WasmRefType.nullable(WasmArrayType.itables.name)
-    )
+    val itables = fctx.addLocal("itables", WasmRefType.nullable(WasmArrayType.itables.name))
+    val exprNonNullLocal = fctx.addLocal("exprNonNull", WasmRefType.any)
+
     val itableIdx = ctx.getItableIdx(clazz.name.name)
     fctx.block(WasmRefType.anyref) { testFail =>
       // if expr is not an instance of Object, return false
@@ -1677,8 +1682,48 @@ object HelperFunctions {
       instrs += I32_CONST(1)
       instrs += RETURN
     } // test fail
-    instrs += DROP
-    instrs += I32_CONST(0) // false
+
+    if (classInfo.isAncestorOfHijackedClass) {
+      /* It could be a hijacked class instance that implements this interface.
+       * Test whether `jsValueType(expr)` is in the `specialInstanceTypes` bitset.
+       * In other words, return `((1 << jsValueType(expr)) & specialInstanceTypes) != 0`.
+       *
+       * For example, if this class is `Comparable`,
+       * `specialInstanceTypes == 0b00001111`, since `jl.Boolean`, `jl.String`
+       * and `jl.Double` implement `Comparable`, but `jl.Void` does not.
+       * If `expr` is a `number`, `jsValueType(expr) == 3`. We then test whether
+       * `(1 << 3) & 0b00001111 != 0`, which is true because `(1 << 3) == 0b00001000`.
+       * If `expr` is `undefined`, it would be `(1 << 4) == 0b00010000`, which
+       * would give `false`.
+       */
+      val anyRefToVoidSig =
+        WasmFunctionSignature(List(WasmRefType.anyref), Nil)
+
+      fctx.block(anyRefToVoidSig) { isNullLabel =>
+        // exprNonNull := expr; branch to isNullLabel if it is null
+        instrs += BR_ON_NULL(isNullLabel)
+        instrs += LOCAL_SET(exprNonNullLocal)
+
+        // Load 1 << jsValueType(expr)
+        instrs += I32_CONST(1)
+        instrs += LOCAL_GET(exprNonNullLocal)
+        instrs += CALL(WasmFunctionName.jsValueType)
+        instrs += I32_SHL
+
+        // return (... & specialInstanceTypes) != 0
+        instrs += I32_CONST(classInfo.specialInstanceTypes)
+        instrs += I32_AND
+        instrs += I32_CONST(0)
+        instrs += I32_NE
+        instrs += RETURN
+      }
+
+      instrs += I32_CONST(0) // false
+    } else {
+      instrs += DROP
+      instrs += I32_CONST(0) // false
+    }
+
     fctx.buildAndAddToContext()
   }
 

wasm/src/main/scala/ir2wasm/Preprocessor.scala

@@ -10,6 +10,7 @@ import org.scalajs.ir.Traversers
 
 import org.scalajs.linker.standard.LinkedClass
 
+import EmbeddedConstants._
 import WasmContext._
 
 object Preprocessor {
@@ -101,6 +102,26 @@ object Preprocessor {
       )
     )
 
+    // Update specialInstanceTypes for ancestors of hijacked classes
+    if (clazz.kind == ClassKind.HijackedClass) {
+      def addSpecialInstanceTypeOnAllAncestors(jsValueType: Int): Unit =
+        clazz.ancestors.foreach(ctx.getClassInfo(_).addSpecialInstanceType(jsValueType))
+
+      clazz.className match {
+        case IRNames.BoxedBooleanClass =>
+          addSpecialInstanceTypeOnAllAncestors(JSValueTypeFalse)
+          addSpecialInstanceTypeOnAllAncestors(JSValueTypeTrue)
+        case IRNames.BoxedStringClass =>
+          addSpecialInstanceTypeOnAllAncestors(JSValueTypeString)
+        case IRNames.BoxedDoubleClass =>
+          addSpecialInstanceTypeOnAllAncestors(JSValueTypeNumber)
+        case IRNames.BoxedUnitClass =>
+          addSpecialInstanceTypeOnAllAncestors(JSValueTypeUndefined)
+        case _ =>
+          ()
+      }
+    }
+
     /* Work around https://github.com/scala-js/scala-js/issues/4972
      * Manually mark all ancestors of instantiated classes as having instances.
      */

wasm/src/main/scala/ir2wasm/WasmBuilder.scala

@@ -158,35 +158,7 @@ class WasmBuilder {
     val className = clazz.className
     val classInfo = ctx.getClassInfo(className)
 
-    /* See the `isInstance` helper. `specialInstanceTypes` is a bitset of the
-     * `jsValueType`s corresponding to hijacked classes that extend this class.
-     * For example, if this class is `Comparable`, we want the bitset to contain
-     * the values for `boolean`, `string` and `number` (but not `undefined`),
-     * because `jl.Boolean`, `jl.String` and `jl.Double` implement `Comparable`.
-     *
-     * When testing whether a `value` is a `Comparable`, `isInstance` will
-     * compute the `jsValueType(value)` and test whether it is part of the bit
-     * set, using `((1 << jsValueType(value)) & specialInstanceTypes) != 0`.
-     */
-    val specialInstanceTypes = {
-      if (!classInfo.isAncestorOfHijackedClass) {
-        // fast path
-        0
-      } else {
-        var bits = 0
-        if (ctx.getClassInfo(IRNames.BoxedBooleanClass).ancestors.contains(className))
-          bits |= ((1 << JSValueTypeFalse) | (1 << JSValueTypeTrue))
-        if (ctx.getClassInfo(IRNames.BoxedStringClass).ancestors.contains(className))
-          bits |= (1 << JSValueTypeString)
-        if (ctx.getClassInfo(IRNames.BoxedDoubleClass).ancestors.contains(className))
-          bits |= (1 << JSValueTypeNumber)
-        if (ctx.getClassInfo(IRNames.BoxedUnitClass).ancestors.contains(className))
-          bits |= (1 << JSValueTypeUndefined)
-        bits
-      }
-    }
-
-    val kind = clazz.className match {
+    val kind = className match {
       case IRNames.ObjectClass         => KindObject
       case IRNames.BoxedUnitClass      => KindBoxedUnit
       case IRNames.BoxedBooleanClass   => KindBoxedBoolean
@@ -209,7 +181,7 @@ class WasmBuilder {
 
     genTypeDataFieldValues(
       kind,
-      specialInstanceTypes,
+      classInfo.specialInstanceTypes,
       IRTypes.ClassRef(clazz.className),
       vtableElems
     )

wasm/src/main/scala/ir2wasm/WasmExpressionBuilder.scala

@@ -1300,6 +1300,20 @@ private class WasmExpressionBuilder private (
         instrs += I32_CONST(1)
         instrs += I32_XOR
 
+      case IRTypes.ClassType(JLNumberClass) =>
+        /* Special case: the only non-Object *class* that is an ancestor of a
+         * hijacked class. We need to accept `number` primitives here.
+         */
+        val tempLocal = fctx.addSyntheticLocal(Types.WasmRefType.anyref)
+        instrs += LOCAL_TEE(tempLocal)
+        instrs += REF_TEST(Types.WasmRefType(WasmStructTypeName.forClass(JLNumberClass)))
+        fctx.ifThenElse(Types.WasmInt32) {
+          instrs += I32_CONST(1)
+        } {
+          instrs += LOCAL_GET(tempLocal)
+          instrs += CALL(WasmFunctionName.typeTest(IRTypes.DoubleRef))
+        }
+
       case IRTypes.ClassType(testClassName) =>
         IRTypes.BoxedClassToPrimType.get(testClassName) match {
           case Some(primType) =>

wasm/src/main/scala/wasm4s/WasmContext.scala

@@ -702,20 +702,6 @@ class WasmContext(val module: WasmModule) extends TypeDefinableWasmContext {
 object WasmContext {
   private val classFieldOffset = 2 // vtable, itables
 
-  private val AncestorsOfHijackedClasses: Set[IRNames.ClassName] = {
-    // We hard-code this for now, but ideally we should derive it
-    IRNames.HijackedClasses ++
-      Set(
-        IRNames.ObjectClass,
-        IRNames.SerializableClass,
-        IRNames.ClassName("java.lang.CharSequence"),
-        IRNames.ClassName("java.lang.Comparable"),
-        IRNames.ClassName("java.lang.Number"),
-        IRNames.ClassName("java.lang.constant.Constable"),
-        IRNames.ClassName("java.lang.constant.ConstantDesc")
-      )
-  }
-
   final class WasmClassInfo(
       val name: IRNames.ClassName,
       val kind: ClassKind,
@@ -741,7 +727,47 @@ object WasmContext {
 
     def hasInstances: Boolean = _hasInstances
 
-    def isAncestorOfHijackedClass: Boolean = AncestorsOfHijackedClasses.contains(name)
+    private var _specialInstanceTypes: Int = 0
+
+    def addSpecialInstanceType(jsValueType: Int): Unit =
+      _specialInstanceTypes |= (1 << jsValueType)
+
+    /** A bitset of the `jsValueType`s corresponding to hijacked classes that extend this class.
+      *
+      * This value is used for instance tests against this class. A JS value `x` is an instance of
+      * this type iff `jsValueType(x)` is a member of this bitset. Because of how a bitset works,
+      * this means testing the following formula:
+      *
+      * {{{
+      * ((1 << jsValueType(x)) & specialInstanceTypes) != 0
+      * }}}
+      *
+      * For example, if this class is `Comparable`, we want the bitset to contain the values for
+      * `boolean`, `string` and `number` (but not `undefined`), because `jl.Boolean`, `jl.String`
+      * and `jl.Double` implement `Comparable`.
+      *
+      * This field is initialized with 0, and augmented during preprocessing by calls to
+      * `addSpecialInstanceType`.
+      *
+      * This technique is used both for static `isInstanceOf` tests as well as reflective tests
+      * through `Class.isInstance`. For the latter, this value is stored in
+      * `typeData.specialInstanceTypes`. For the former, it is embedded as a constant in the
+      * generated code.
+      *
+      * See the `isInstance` and `genInstanceTest` helpers.
+      *
+      * Special cases: this value remains 0 for all the numeric hijacked classes except `jl.Double`,
+      * since `jsValueType(x) == JSValueTypeNumber` is not enough to deduce that
+      * `x.isInstanceOf[Int]`, for example.
+      */
+    def specialInstanceTypes: Int = _specialInstanceTypes
+
+    /** Is this class an ancestor of any hijacked class?
+      *
+      * This includes but is not limited to the hijacked classes themselves, as well as `jl.Object`.
+      */
+    def isAncestorOfHijackedClass: Boolean =
+      specialInstanceTypes != 0 || kind == ClassKind.HijackedClass
 
     def isInterface = kind == ClassKind.Interface