Avoid generating ACONST_NULL; POP; ACONST_NULL when loading null

When loading a value of type scala.runtime.Null$ we need to add
POP; ACONST_NULL, see comment in BCodeBodyBuilder.adapt.
This is however not necessary if the null value is a simple ACONST_NULL.
This patch eliminates that redundancy.

4 files changed, 69 insertions, 12 deletions

diff --git a/src/compiler/scala/tools/nsc/backend/jvm/BCodeBodyBuilder.scala b/src/compiler/scala/tools/nsc/backend/jvm/BCodeBodyBuilder.scala
index 1e7ea0c09d..8a90eb9780 100644
--- a/src/compiler/scala/tools/nsc/backend/jvm/BCodeBodyBuilder.scala
+++ b/src/compiler/scala/tools/nsc/backend/jvm/BCodeBodyBuilder.scala
@@ -717,7 +717,7 @@ abstract class BCodeBodyBuilder extends BCodeSkelBuilder {
                 // for callsites marked `f(): @inline/noinline`. For nullary calls, the attachment
                 // is on the Select node (not on the Apply node added by UnCurry).
                 def checkInlineAnnotated(t: Tree): Unit = {
-                  if (t.hasAttachment[InlineAnnotatedAttachment]) bc.jmethod.instructions.getLast match {
+                  if (t.hasAttachment[InlineAnnotatedAttachment]) lastInsn match {
                     case m: MethodInsnNode =>
                       if (app.hasAttachment[NoInlineCallsiteAttachment.type]) noInlineAnnotatedCallsites += m
                       else inlineAnnotatedCallsites += m
@@ -888,10 +888,24 @@ abstract class BCodeBodyBuilder extends BCodeSkelBuilder {
          * emitted instruction was an ATHROW. As explained above, it is OK to emit a second ATHROW,
          * the verifiers will be happy.
          */
-        emit(asm.Opcodes.ATHROW)
+        if (lastInsn.getOpcode != asm.Opcodes.ATHROW)
+          emit(asm.Opcodes.ATHROW)
       } else if (from.isNullType) {
-        bc drop from
-        emit(asm.Opcodes.ACONST_NULL)
+        /* After loading an expression of type `scala.runtime.Null$`, introduce POP; ACONST_NULL.
+         * This is required to pass the verifier: in Scala's type system, Null conforms to any
+         * reference type. In bytecode, the type Null is represented by scala.runtime.Null$, which
+         * is not a subtype of all reference types. Example:
+         *
+         *   def nl: Null = null // in bytecode, nl has return type scala.runtime.Null$
+         *   val a: String = nl  // OK for Scala but not for the JVM, scala.runtime.Null$ does not conform to String
+         *
+         * In order to fix the above problem, the value returned by nl is dropped and ACONST_NULL is
+         * inserted instead - after all, an expression of type scala.runtime.Null$ can only be null.
+         */
+        if (lastInsn.getOpcode != asm.Opcodes.ACONST_NULL) {
+          bc drop from
+          emit(asm.Opcodes.ACONST_NULL)
+        }
       }
       else (from, to) match  {
         case (BYTE, LONG) | (SHORT, LONG) | (CHAR, LONG) | (INT, LONG) => bc.emitT2T(INT, LONG)
diff --git a/src/compiler/scala/tools/nsc/backend/jvm/BCodeSkelBuilder.scala b/src/compiler/scala/tools/nsc/backend/jvm/BCodeSkelBuilder.scala
index 9dea21154f..96796b3244 100644
--- a/src/compiler/scala/tools/nsc/backend/jvm/BCodeSkelBuilder.scala
+++ b/src/compiler/scala/tools/nsc/backend/jvm/BCodeSkelBuilder.scala
@@ -437,9 +437,7 @@ abstract class BCodeSkelBuilder extends BCodeHelpers {
     var varsInScope: List[Tuple2[Symbol, asm.Label]] = null // (local-var-sym -> start-of-scope)
 
     // helpers around program-points.
-    def lastInsn: asm.tree.AbstractInsnNode = {
-      mnode.instructions.getLast
-    }
+    def lastInsn: asm.tree.AbstractInsnNode = mnode.instructions.getLast
     def currProgramPoint(): asm.Label = {
       lastInsn match {
         case labnode: asm.tree.LabelNode => labnode.getLabel
@@ -598,13 +596,11 @@ abstract class BCodeSkelBuilder extends BCodeHelpers {
           genLoad(rhs, returnType)
 
           rhs match {
-            case Block(_, Return(_)) => ()
-            case Return(_) => ()
+            case Return(_) | Block(_, Return(_)) | Throw(_) | Block(_, Throw(_)) => ()
             case EmptyTree =>
               globalError("Concrete method has no definition: " + dd + (
                 if (settings.debug) "(found: " + methSymbol.owner.info.decls.toList.mkString(", ") + ")"
-                else "")
-              )
+                else ""))
             case _ =>
               bc emitRETURN returnType
           }
diff --git a/test/junit/scala/tools/nsc/backend/jvm/opt/InlinerTest.scala b/test/junit/scala/tools/nsc/backend/jvm/opt/InlinerTest.scala
index 942b62b32c..70f3060027 100644
--- a/test/junit/scala/tools/nsc/backend/jvm/opt/InlinerTest.scala
+++ b/test/junit/scala/tools/nsc/backend/jvm/opt/InlinerTest.scala
@@ -1507,7 +1507,6 @@ class InlinerTest extends ClearAfterClass {
         |}
       """.stripMargin
     val List(c) = compile(code)
-    val t = getSingleMethod(c, "t")
 
     // box-unbox will clean it up
     assertEquals(getSingleMethod(c, "t").instructions.summary,
diff --git a/test/junit/scala/tools/nsc/backend/jvm/opt/UnreachableCodeTest.scala b/test/junit/scala/tools/nsc/backend/jvm/opt/UnreachableCodeTest.scala
index 86c8baa3c6..62ee09e9de 100644
--- a/test/junit/scala/tools/nsc/backend/jvm/opt/UnreachableCodeTest.scala
+++ b/test/junit/scala/tools/nsc/backend/jvm/opt/UnreachableCodeTest.scala
@@ -215,4 +215,52 @@ class UnreachableCodeTest extends ClearAfterClass {
     assertTrue(List(FrameEntry(F_FULL, List(INTEGER, DOUBLE, Label(3)), List("java/lang/Object", Label(4))), Label(3), Label(4)) ===
                List(FrameEntry(F_FULL, List(INTEGER, DOUBLE, Label(1)), List("java/lang/Object", Label(3))), Label(1), Label(3)))
   }
+
+  @Test
+  def loadNullNothingBytecode(): Unit = {
+    val code =
+      """class C {
+        |  def nl: Null = null
+        |  def nt: Nothing = throw new Error("")
+        |  def cons(a: Any) = ()
+        |
+        |  def t1 = cons(null)
+        |  def t2 = cons(nl)
+        |  def t3 = cons(throw new Error(""))
+        |  def t4 = cons(nt)
+        |}
+      """.stripMargin
+    val List(c) = compileClasses(noOptCompiler)(code)
+
+    assertEquals(getSingleMethod(c, "nl").instructions.summary, List(ACONST_NULL, ARETURN))
+
+    assertEquals(getSingleMethod(c, "nt").instructions.summary, List(
+      NEW, DUP, LDC, "<init>", ATHROW))
+
+    assertEquals(getSingleMethod(c, "t1").instructions.summary, List(
+      ALOAD, ACONST_NULL, "cons", RETURN))
+
+    // GenBCode introduces POP; ACONST_NULL after loading an expression of type scala.runtime.Null$,
+    // see comment in BCodeBodyBuilder.adapt
+    assertEquals(getSingleMethod(c, "t2").instructions.summary, List(
+      ALOAD, ALOAD, "nl", POP, ACONST_NULL, "cons", RETURN))
+
+    // the bytecode generated by GenBCode is ... ATHROW; INVOKEVIRTUAL C.cons; RETURN
+    // the ASM classfile writer creates a new basic block (creates a label) right after the ATHROW
+    // and replaces all instructions by NOP*; ATHROW, see comment in BCodeBodyBuilder.adapt
+    // NOTE: DCE is enabled by default and gets rid of the redundant code (tested below)
+    assertEquals(getSingleMethod(c, "t3").instructions.summary, List(
+      ALOAD, NEW, DUP, LDC, "<init>", ATHROW, NOP, NOP, NOP, ATHROW))
+
+    // GenBCode introduces an ATHROW after the invocation of C.nt, see BCodeBodyBuilder.adapt
+    // NOTE: DCE is enabled by default and gets rid of the redundant code (tested below)
+    assertEquals(getSingleMethod(c, "t4").instructions.summary, List(
+      ALOAD, ALOAD, "nt", ATHROW, NOP, NOP, NOP, ATHROW))
+
+    val List(cDCE) = compileClasses(dceCompiler)(code)
+    assertEquals(getSingleMethod(cDCE, "t3").instructions.summary, List(
+      ALOAD, NEW, DUP, LDC, "<init>", ATHROW))
+    assertEquals(getSingleMethod(cDCE, "t4").instructions.summary, List(
+      ALOAD, ALOAD, "nt", ATHROW))
+  }
 }