Use CheckNotNull in the Throw deserialization hack.

sjrd · sjrd · commit f0c74cb450df · 2025-01-07T20:46:55.000+01:00
Back in Scala.js 1.11, we did not have `CheckNotNull` as a separate
node. The deserialization hack was therefore pretty complicated,
but all it wanted to do was check for null values.

Unfortunately we broke that deserialization hack in Scala.js 1.18.0
because `UnwrapFromThrowable` now demands a non-nullable argument.
So we have to insert an explicit `CheckNotNull`.

It turns out we can dramatically simplify the whole hack with
nothing but `CheckNotNull`.
diff --git a/ir/shared/src/main/scala/org/scalajs/ir/Serializers.scala b/ir/shared/src/main/scala/org/scalajs/ir/Serializers.scala
@@ -1424,69 +1424,32 @@ object Serializers {
      *  so that `Throw(e)` only ever throws the value of `e`, while the NPE UB
      *  is specified by `UnwrapFromThrowable`. Among other things, this allows
      *  the user-space code `js.special.throw(e)` to indiscriminately throw `e`
-     *  even if it is `null`.
+     *  even if it is `null`. Later, in Scala.js 1.18, we further separated the
+     *  null check of `UnwrapFromThrowable` to be an explicit `CheckNotNull`.
      *
-     *  With this hack, we patch `Throw(e)` where `e` is a nullable `Throwable`
-     *  by inserting an appropriate `UnwrapFromThrowable`.
+     *  With this hack, we patch `Throw(e)` by inserting an appropriate
+     *  `CheckNotNull`.
      *
-     *  Naively, we would just return `UnwrapFromThrowable(e)`. Unfortunately,
-     *  we cannot prove that this is type-correct when the type of `e` is a
-     *  `ClassType(cls)`, as we cannot test whether `cls` is a subclass of
-     *  `java.lang.Throwable`. So we have to produce the following instead:
-     *
-     *  {{{
-     *    if (expr === null) unwrapFromThrowable(null) else expr
-     *  }}}
-     *
-     *  except that evaluates `expr` twice. If it is a `VarRef`, which is a
-     *  common case, that is fine. Otherwise, we have to wrap this pattern in
-     *  an IIFE.
-     *
-     *  We also have to avoid the transformation altogether when the `expr` is
-     *  an `AnyType`. This happens when the previous Scala.js compiler already
-     *  provides the unwrapped exception, which is either
+     *  However, we must not do that when the previous Scala.js compiler
+     *  already provides the *unwrapped* exception. This happened in two
+     *  situations:
      *
      *  - when automatically re-throwing an unhandled exception at the end of a
      *    `try..catch`, or
      *  - when throwing a maybe-JavaScriptException, with an explicit call to
      *    `runtime.package$.unwrapJavaScriptException(x)`.
+     *
+     *  Fortunately, in both situations, the type of the `expr` is always
+     *  `AnyType`. We can accurately use that test to know whether we need to
+     *  apply the patch.
      */
     private def throwArgumentHack8(expr: Tree)(implicit pos: Position): Tree = {
-      def unwrapFromThrowable(t: Tree): Tree =
-        UnaryOp(UnaryOp.UnwrapFromThrowable, t)
-
-      expr.tpe match {
-        case NullType =>
-          // Evaluate the expression then definitely run into an NPE UB
-          unwrapFromThrowable(expr)
-
-        case ClassType(_, _) =>
-          expr match {
-            case New(_, _, _) =>
-              // Common case (`throw new SomeException(...)`) that is known not to be `null`
-              expr
-            case VarRef(_) =>
-              /* Common case (explicit re-throw of the form `throw th`) where we don't need the IIFE.
-               * if (expr === null) unwrapFromThrowable(null) else expr
-               */
-              If(BinaryOp(BinaryOp.===, expr, Null()), unwrapFromThrowable(Null()), expr)(AnyType)
-            case _ =>
-              /* General case where we need to avoid evaluating `expr` twice.
-               * ((x) => if (x === null) unwrapFromThrowable(null) else x)(expr)
-               */
-              val x = LocalIdent(LocalName("x"))
-              val xParamDef = ParamDef(x, OriginalName.NoOriginalName, AnyType, mutable = false)
-              val xRef = xParamDef.ref
-              val closure = Closure(arrow = true, Nil, List(xParamDef), None, {
-                If(BinaryOp(BinaryOp.===, xRef, Null()), unwrapFromThrowable(Null()), xRef)(AnyType)
-              }, Nil)
-              JSFunctionApply(closure, List(expr))
-          }
-
-        case _ =>
-          // Do not transform expressions of other types, in particular `AnyType`
-          expr
-      }
+      if (expr.tpe == AnyType)
+        expr
+      else if (!expr.tpe.isNullable)
+        expr // no CheckNotNull needed; common case because of `throw new ...`
+      else
+        UnaryOp(UnaryOp.CheckNotNull, expr)
     }
 
     def readTrees(): List[Tree] =
diff --git a/linker/shared/src/test/scala/org/scalajs/linker/BackwardsCompatTest.scala b/linker/shared/src/test/scala/org/scalajs/linker/BackwardsCompatTest.scala
@@ -75,6 +75,40 @@ class BackwardsCompatTest {
     test(classDefs, MainTestModuleInitializers)
   }
 
+  @Test
+  def testThrowHackWithVariable_Issue5107(): AsyncResult = await {
+    val Base64Class = ClassName("java.util.Base64")
+    val DecoderClass = ClassName("java.util.Base64$Decoder")
+    val ByteBufferClass = ClassName("java.nio.ByteBuffer")
+
+    val DecoderTypeRef = ClassRef(DecoderClass)
+    val ByteBufferTypeRef = ClassRef(ByteBufferClass)
+    val AB = ArrayTypeRef(ByteRef, 1)
+
+    val DecoderType = ClassType(DecoderClass, nullable = true)
+    val ByteBufferType = ClassType(ByteBufferClass, nullable = true)
+
+    /* java.util.Base64.getDecoder().decode(java.nio.ByteBuffer.wrap(Array(65, 81, 73, 61)))
+     * That is the only method I found in our javalib that contains a `throw e`,
+     * as opposed to a `throw new ...`.
+     */
+    val classDefs = Seq(
+      mainTestClassDef(systemOutPrintln {
+        Apply(
+          EAF,
+          ApplyStatic(EAF, Base64Class, m("getDecoder", Nil, DecoderTypeRef), Nil)(DecoderType),
+          m("decode", List(ByteBufferTypeRef), ByteBufferTypeRef),
+          List(
+            ApplyStatic(EAF, ByteBufferClass, m("wrap", List(AB), ByteBufferTypeRef),
+                List(ArrayValue(AB, List[Byte](65, 81, 73, 61).map(ByteLiteral(_)))))(ByteBufferType)
+          )
+        )(ByteBufferType)
+      })
+    )
+
+    test(classDefs, MainTestModuleInitializers)
+  }
+
   private def test(classDefs: Seq[ClassDef],
       moduleInitializers: Seq[ModuleInitializer]): Future[_] = {
     val classDefFiles = classDefs.map(MemClassDefIRFile(_))
