Better inliner support for 2.12 trait encoding

Some changes to the trait encoding came late in the 2.12 cycle, and the
inliner was not adapted to support it in the best possible way.

In 2.12.0 concrete trait methods are encoded as

    interface T {
      default int m() { return 1 }
      static int m$(T $this) { <invokespecial $this.m()> }
    }
    class C implements T {
      public int m() { return T.m$(this) }
    }

If a trait method is selected for inlining, the 2.12.0 inliner would
copy its body into the static super accessor `T.m$`, and from there into
the mixin forwarder `C.m`.

This commit special-cases the inliner:
  - We don't inline into static super accessors and mixin forwarders.
  - Insted, when inlining an invocation of a mixin forwarder, the
    inliner also follows through the two forwarders and inlines the
    trait method body.

There was a difficulty implementing this: inlining the static static
super accessor would copy an `invokespecial` instruction into a
different classfile, which is not legal / may change semantics. That
`invokespecial` is supposed to disappear when inlining the actual
default method body. However, this last step may fail, for example
because the trait method body itself contains instructions that are not
legal in a different classfile.

It is very difficult to perform all necessary checks ahead of time. So
instead, this commit implements the ability to speculatively inline a
callsite and roll back if necessary.

The commit also cleans up the implementation of inliner warnings a
little. The previous code would always emit a warning when a method
annotated `@inline` was not picked by the heuristics - this was a
problem when the callsite in the static super accessor was no longer
chosen.

6 files changed, 157 insertions, 36 deletions

diff --git a/test/files/neg/sealed-final-neg.check b/test/files/neg/sealed-final-neg.check
index e135f38f8b..5e47c69ed8 100644
--- a/test/files/neg/sealed-final-neg.check
+++ b/test/files/neg/sealed-final-neg.check
@@ -1,7 +1,9 @@
-sealed-final-neg.scala:17: warning: neg1/Foo::bar(I)I is annotated @inline but cannot be inlined: the method is not final and may be overridden.
+sealed-final-neg.scala:17: warning: neg1/Foo::bar(I)I is annotated @inline but could not be inlined:
+The method is not final and may be overridden.
     def f = Foo.mkFoo() bar 10
                         ^
-sealed-final-neg.scala:37: warning: neg2/Foo::bar(I)I is annotated @inline but cannot be inlined: the method is not final and may be overridden.
+sealed-final-neg.scala:37: warning: neg2/Foo::bar(I)I is annotated @inline but could not be inlined:
+The method is not final and may be overridden.
     def f = Foo.mkFoo() bar 10
                         ^
 error: No warnings can be incurred under -Xfatal-warnings.
diff --git a/test/junit/scala/tools/nsc/backend/jvm/opt/CallGraphTest.scala b/test/junit/scala/tools/nsc/backend/jvm/opt/CallGraphTest.scala
index 80fbba133e..a74e73afc9 100644
--- a/test/junit/scala/tools/nsc/backend/jvm/opt/CallGraphTest.scala
+++ b/test/junit/scala/tools/nsc/backend/jvm/opt/CallGraphTest.scala
@@ -72,7 +72,7 @@ class CallGraphTest extends BytecodeTesting {
         |  @noinline def f5         = try { 0 } catch { case _: Throwable => 1 }
         |  @noinline final def f6   = try { 0 } catch { case _: Throwable => 1 }
         |
-        |  @inline @noinline def f7 = try { 0 } catch { case _: Throwable => 1 }
+        |  @inline @noinline def f7 = try { 0 } catch { case _: Throwable => 1 } // no warning, @noinline takes precedence
         |}
         |class D extends C {
         |  @inline override def f1  = try { 0 } catch { case _: Throwable => 1 }
@@ -91,18 +91,17 @@ class CallGraphTest extends BytecodeTesting {
     // The callGraph.callsites map is indexed by instructions of those ClassNodes.
 
     val ok = Set(
-      "D::f1()I is annotated @inline but cannot be inlined: the method is not final and may be overridden", // only one warning for D.f1: C.f1 is not annotated @inline
-      "C::f3()I is annotated @inline but cannot be inlined: the method is not final and may be overridden", // only one warning for C.f3: D.f3 does not have @inline (and it would also be safe to inline)
-      "C::f7()I is annotated @inline but cannot be inlined: the method is not final and may be overridden", // two warnings (the error message mentions C.f7 even if the receiver type is D, because f7 is inherited from C)
-      "operand stack at the callsite in Test::t1(LC;)I contains more values",
-      "operand stack at the callsite in Test::t2(LD;)I contains more values")
+      "D::f1()I is annotated @inline but could not be inlined:\nThe method is not final and may be overridden.", // only one warning for D.f1: C.f1 is not annotated @inline
+      "C::f3()I is annotated @inline but could not be inlined:\nThe method is not final and may be overridden.", // only one warning for C.f3: D.f3 does not have @inline (and it would also be safe to inline)
+      "C::f4()I is annotated @inline but could not be inlined:\nThe operand stack at the callsite in Test::t1(LC;)I contains more values",
+      "C::f4()I is annotated @inline but could not be inlined:\nThe operand stack at the callsite in Test::t2(LD;)I contains more values")
     var msgCount = 0
     val checkMsg = (m: StoreReporter#Info) => {
       msgCount += 1
       ok exists (m.msg contains _)
     }
     val List(cCls, cMod, dCls, testCls) = compile(code, checkMsg)
-    assert(msgCount == 6, msgCount)
+    assert(msgCount == 4, msgCount)
 
     val List(cf1, cf2, cf3, cf4, cf5, cf6, cf7) = getAsmMethods(cCls, _.startsWith("f"))
     val List(df1, df3) = getAsmMethods(dCls, _.startsWith("f"))
diff --git a/test/junit/scala/tools/nsc/backend/jvm/opt/InlineWarningTest.scala b/test/junit/scala/tools/nsc/backend/jvm/opt/InlineWarningTest.scala
index 95b47f7d04..b1aa27fd27 100644
--- a/test/junit/scala/tools/nsc/backend/jvm/opt/InlineWarningTest.scala
+++ b/test/junit/scala/tools/nsc/backend/jvm/opt/InlineWarningTest.scala
@@ -35,9 +35,9 @@ class InlineWarningTest extends BytecodeTesting {
       """.stripMargin
     var count = 0
     val warns = Set(
-      "C::m1()I is annotated @inline but cannot be inlined: the method is not final and may be overridden",
-      "T::m2()I is annotated @inline but cannot be inlined: the method is not final and may be overridden",
-      "D::m2()I is annotated @inline but cannot be inlined: the method is not final and may be overridden")
+      "C::m1()I is annotated @inline but could not be inlined:\nThe method is not final and may be overridden.",
+      "T::m2()I is annotated @inline but could not be inlined:\nThe method is not final and may be overridden.",
+      "D::m2()I is annotated @inline but could not be inlined:\nThe method is not final and may be overridden.")
     compileToBytes(code, allowMessage = i => {count += 1; warns.exists(i.msg contains _)})
     assert(count == 4, count)
   }
diff --git a/test/junit/scala/tools/nsc/backend/jvm/opt/InlinerIllegalAccessTest.scala b/test/junit/scala/tools/nsc/backend/jvm/opt/InlinerIllegalAccessTest.scala
index 3e0b889e9c..bf9da0f48f 100644
--- a/test/junit/scala/tools/nsc/backend/jvm/opt/InlinerIllegalAccessTest.scala
+++ b/test/junit/scala/tools/nsc/backend/jvm/opt/InlinerIllegalAccessTest.scala
@@ -20,7 +20,7 @@ class InlinerIllegalAccessTest extends BytecodeTesting {
   import global.genBCode.bTypes._
 
   def addToRepo(cls: List[ClassNode]): Unit = for (c <- cls) byteCodeRepository.add(c, None)
-  def assertEmpty(ins: Option[AbstractInsnNode]) = for (i <- ins)
+  def assertEmpty(ins: List[AbstractInsnNode]) = for (i <- ins)
     throw new AssertionError(textify(i))
 
   @Test
@@ -28,7 +28,7 @@ class InlinerIllegalAccessTest extends BytecodeTesting {
     val code =
       """package a {
         |  private class C {            // the Scala compiler makes all classes public
-        |    def f1 = new C                   // NEW a/C
+        |    def f1 = new C                   // NEW a/C, INVOKESPECIAL a/C.<init> ()V
         |    def f2 = new Array[C](0)         // ANEWARRAY a/C
         |    def f3 = new Array[Array[C]](0)  // ANEWARRAY [La/C;
         |  }
@@ -46,9 +46,9 @@ class InlinerIllegalAccessTest extends BytecodeTesting {
 
     val methods = cClass.methods.asScala.filter(_.name(0) == 'f').toList
 
-    def check(classNode: ClassNode, test: Option[AbstractInsnNode] => Unit) = {
+    def check(classNode: ClassNode, test: List[AbstractInsnNode] => Unit) = {
       for (m <- methods)
-        test(inliner.findIllegalAccess(m.instructions, classBTypeFromParsedClassfile(cClass.name), classBTypeFromParsedClassfile(classNode.name)).map(_._1))
+        test(inliner.findIllegalAccess(m.instructions, classBTypeFromParsedClassfile(cClass.name), classBTypeFromParsedClassfile(classNode.name)).right.get)
     }
 
     check(cClass, assertEmpty)
@@ -65,7 +65,11 @@ class InlinerIllegalAccessTest extends BytecodeTesting {
     check(cClass, assertEmpty)
     check(dClass, assertEmpty) // accessing a private class in the same package is OK
     check(eClass, {
-      case Some(ti: TypeInsnNode) if Set("a/C", "[La/C;")(ti.desc) => ()
+      case (ti: TypeInsnNode) :: is if Set("a/C", "[La/C;")(ti.desc) =>
+        is match {
+          case List(mi: MethodInsnNode) => assert(mi.owner == "a/C" && mi.name == "<init>")
+          case Nil =>
+        }
       // MatchError otherwise
     })
   }
@@ -141,12 +145,12 @@ class InlinerIllegalAccessTest extends BytecodeTesting {
 
     val List(rbD, rcD, rfD, rgD) = dCl.methods.asScala.toList.filter(_.name(0) == 'r').sortBy(_.name)
 
-    def check(method: MethodNode, decl: ClassNode, dest: ClassNode, test: Option[AbstractInsnNode] => Unit): Unit = {
-      test(inliner.findIllegalAccess(method.instructions, classBTypeFromParsedClassfile(decl.name), classBTypeFromParsedClassfile(dest.name)).map(_._1))
+    def check(method: MethodNode, decl: ClassNode, dest: ClassNode, test: List[AbstractInsnNode] => Unit): Unit = {
+      test(inliner.findIllegalAccess(method.instructions, classBTypeFromParsedClassfile(decl.name), classBTypeFromParsedClassfile(dest.name)).right.get)
     }
 
-    val cOrDOwner = (_: Option[AbstractInsnNode] @unchecked) match {
-      case Some(mi: MethodInsnNode) if Set("a/C", "a/D")(mi.owner) => ()
+    val cOrDOwner = (_: List[AbstractInsnNode] @unchecked) match {
+      case List(mi: MethodInsnNode) if Set("a/C", "a/D")(mi.owner) => ()
       // MatchError otherwise
     }
 
diff --git a/test/junit/scala/tools/nsc/backend/jvm/opt/InlinerSeparateCompilationTest.scala b/test/junit/scala/tools/nsc/backend/jvm/opt/InlinerSeparateCompilationTest.scala
index 5362585642..9b1609a130 100644
--- a/test/junit/scala/tools/nsc/backend/jvm/opt/InlinerSeparateCompilationTest.scala
+++ b/test/junit/scala/tools/nsc/backend/jvm/opt/InlinerSeparateCompilationTest.scala
@@ -31,7 +31,7 @@ class InlinerSeparateCompilationTest {
         |}
       """.stripMargin
 
-    val warn = "T::f()I is annotated @inline but cannot be inlined: the method is not final and may be overridden"
+    val warn = "T::f()I is annotated @inline but could not be inlined:\nThe method is not final and may be overridden."
     val List(c, o, oMod, t) = compileClassesSeparately(List(codeA, codeB), args + " -opt-warnings", _.msg contains warn)
     assertInvoke(getMethod(c, "t1"), "T", "f")
     assertNoInvoke(getMethod(c, "t2"))
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 a844c20a7f..90a938be35 100644
--- a/test/junit/scala/tools/nsc/backend/jvm/opt/InlinerTest.scala
+++ b/test/junit/scala/tools/nsc/backend/jvm/opt/InlinerTest.scala
@@ -67,7 +67,7 @@ class InlinerTest extends BytecodeTesting {
 
   def canInlineTest(code: String, mod: ClassNode => Unit = _ => ()): Option[OptimizerWarning] = {
     val cs = gMethAndFCallsite(code, mod)._2
-    inliner.earlyCanInlineCheck(cs) orElse inliner.canInlineBody(cs)
+    inliner.earlyCanInlineCheck(cs) orElse inliner.canInlineCallsite(cs).map(_._1)
   }
 
   def inlineTest(code: String, mod: ClassNode => Unit = _ => ()): MethodNode = {
@@ -199,8 +199,8 @@ class InlinerTest extends BytecodeTesting {
     val List(c, d) = compile(code)
     val hMeth = getAsmMethod(d, "h")
     val gCall = getCallsite(hMeth, "g")
-    val r = inliner.canInlineBody(gCall)
-    assert(r.nonEmpty && r.get.isInstanceOf[IllegalAccessInstruction], r)
+    val r = inliner.canInlineCallsite(gCall)
+    assert(r.nonEmpty && r.get._1.isInstanceOf[IllegalAccessInstruction], r)
   }
 
   @Test
@@ -340,7 +340,7 @@ class InlinerTest extends BytecodeTesting {
     val fMeth = getAsmMethod(c, "f")
     val call = getCallsite(fMeth, "lowestOneBit")
 
-    val warning = inliner.canInlineBody(call)
+    val warning = inliner.canInlineCallsite(call)
     assert(warning.isEmpty, warning)
 
     inliner.inline(InlineRequest(call, Nil, null))
@@ -475,7 +475,7 @@ class InlinerTest extends BytecodeTesting {
         |  def t2 = this.f
         |}
       """.stripMargin
-    val warn = "::f()I is annotated @inline but cannot be inlined: the method is not final and may be overridden"
+    val warn = "::f()I is annotated @inline but could not be inlined:\nThe method is not final and may be overridden."
     var count = 0
     val List(c, t) = compile(code, allowMessage = i => {count += 1; i.msg contains warn})
     assert(count == 2, count)
@@ -513,7 +513,7 @@ class InlinerTest extends BytecodeTesting {
         |  def t3(t: T) = t.f // no inlining here
         |}
       """.stripMargin
-    val warn = "T::f()I is annotated @inline but cannot be inlined: the method is not final and may be overridden"
+    val warn = "T::f()I is annotated @inline but could not be inlined:\nThe method is not final and may be overridden."
     var count = 0
     val List(c, oMirror, oModule, t) = compile(code, allowMessage = i => {count += 1; i.msg contains warn})
     assert(count == 1, count)
@@ -617,7 +617,7 @@ class InlinerTest extends BytecodeTesting {
         |}
       """.stripMargin
 
-    val warning = "T1::f()I is annotated @inline but cannot be inlined: the method is not final and may be overridden"
+    val warning = "T1::f()I is annotated @inline but could not be inlined:\nThe method is not final and may be overridden."
     var count = 0
     val List(ca, cb, t1, t2a, t2b) = compile(code, allowMessage = i => {count += 1; i.msg contains warning})
     assert(count == 4, count) // see comments, f is not inlined 4 times
@@ -698,7 +698,7 @@ class InlinerTest extends BytecodeTesting {
         |  def t1(c: C) = c.foo
         |}
       """.stripMargin
-    val warn = "C::foo()I is annotated @inline but cannot be inlined: the method is not final and may be overridden"
+    val warn = "C::foo()I is annotated @inline but could not be inlined:\nThe method is not final and may be overridden."
     var c = 0
     compile(code, allowMessage = i => {c += 1; i.msg contains warn})
     assert(c == 1, c)
@@ -762,7 +762,7 @@ class InlinerTest extends BytecodeTesting {
         |}
       """.stripMargin
 
-    val List(c, t, u) = compile(code, allowMessage = _.msg contains "i()I is annotated @inline but cannot be inlined")
+    val List(c, t, u) = compile(code, allowMessage = _.msg contains "::i()I is annotated @inline but could not be inlined:\nThe method is not final and may be overridden.")
     val m1 = getMethod(c, "m1")
     assertInvoke(m1, "T", "a")
     assertInvoke(m1, "T", "b")
@@ -969,7 +969,7 @@ class InlinerTest extends BytecodeTesting {
     val gCall = getCallsite(hMeth, "g")
     val hCall = getCallsite(iMeth, "h")
 
-    val warning = inliner.canInlineBody(gCall)
+    val warning = inliner.canInlineCallsite(gCall)
     assert(warning.isEmpty, warning)
 
     inliner.inline(InlineRequest(hCall,
@@ -1053,7 +1053,7 @@ class InlinerTest extends BytecodeTesting {
         |  def t1 = f1(1)             // inlined
         |  def t2 = f2(1)             // not inlined
         |  def t3 = f1(1): @noinline  // not inlined
-        |  def t4 = f2(1): @inline    // not inlined (cannot override the def-site @noinline)
+        |  def t4 = f2(1): @inline    // inlined
         |  def t5 = f3(1): @inline    // inlined
         |  def t6 = f3(1): @noinline  // not inlined
         |
@@ -1067,7 +1067,7 @@ class InlinerTest extends BytecodeTesting {
     assertNoInvoke(getMethod(c, "t1"))
     assertInvoke(getMethod(c, "t2"), "C", "f2")
     assertInvoke(getMethod(c, "t3"), "C", "f1")
-    assertInvoke(getMethod(c, "t4"), "C", "f2")
+    assertNoInvoke(getMethod(c, "t4"))
     assertNoInvoke(getMethod(c, "t5"))
     assertInvoke(getMethod(c, "t6"), "C", "f3")
     assertNoInvoke(getMethod(c, "t7"))
@@ -1469,8 +1469,8 @@ class InlinerTest extends BytecodeTesting {
         |class C extends T1 with T2
       """.stripMargin
     val List(c, t1, t2) = compile(code, allowMessage = _ => true)
-    // the forwarder C.f is inlined, so there's no invocation
-    assertSameSummary(getMethod(c, "f"), List(ICONST_1, IRETURN))
+    // we never inline into mixin forwarders, see scala-dev#259
+    assertInvoke(getMethod(c, "f"), "T2", "f$")
   }
 
   @Test
@@ -1622,4 +1622,120 @@ class InlinerTest extends BytecodeTesting {
       ("oneLastMethodWithVeryVeryLongNam_yetAnotherMethodWithVeryVeryLong_oneMoreMethodWithVeryVeryLongNam_anotherMethodWithVeryVeryLongNam_param",10),
       ("oneLastMethodWithVeryVery_yetAnotherMethodWithVeryV_oneMoreMethodWithVeryVery_anotherMethodWithVeryVery_methodWithVeryVeryLongNam_param",11)))
   }
+
+  @Test
+  def sd259(): Unit = {
+    // - trait methods are not inlined into their static super accessors, and also not into mixin forwarders.
+    // - inlining an invocation of a mixin forwarder also inlines the static accessor and the trait method body.
+    val code =
+      """trait T {
+        |        def m1a = 1
+        |  final def m1b = 1
+        |
+        |  @inline       def m2a = 2
+        |  @inline final def m2b = 2
+        |
+        |        def m3a(f: Int => Int) = f(1)
+        |  final def m3b(f: Int => Int) = f(1)
+        |}
+        |final class A extends T
+        |class C {
+        |  def t1(t: T) = t.m1a
+        |  def t2(t: T) = t.m1b
+        |  def t3(t: T) = t.m2a
+        |  def t4(t: T) = t.m2b
+        |  def t5(t: T) = t.m3a(x => x)
+        |  def t6(t: T) = t.m3b(x => x)
+        |
+        |  def t7(a: A) = a.m1a
+        |  def t8(a: A) = a.m1b
+        |  def t9(a: A) = a.m2a
+        |  def t10(a: A) = a.m2b
+        |  def t11(a: A) = a.m3a(x => x)
+        |  def t12(a: A) = a.m3b(x => x)
+        |}
+      """.stripMargin
+    val warn = "T::m2a()I is annotated @inline but could not be inlined:\nThe method is not final and may be overridden."
+    var count = 0
+    val List(a, c, t) = compile(code, allowMessage = i => {count += 1; i.msg contains warn})
+    assert(count == 1)
+
+    assertInvoke(getMethod(t, "m1a$"), "T", "m1a")
+    assertInvoke(getMethod(t, "m1b$"), "T", "m1b")
+    assertInvoke(getMethod(t, "m2a$"), "T", "m2a")
+    assertInvoke(getMethod(t, "m2b$"), "T", "m2b")
+    assertInvoke(getMethod(t, "m3a$"), "T", "m3a")
+    assertInvoke(getMethod(t, "m3b$"), "T", "m3b")
+
+    assertInvoke(getMethod(a, "m1a"), "T", "m1a$")
+    assertInvoke(getMethod(a, "m1b"), "T", "m1b$")
+    assertInvoke(getMethod(a, "m2a"), "T", "m2a$")
+    assertInvoke(getMethod(a, "m2b"), "T", "m2b$")
+    assertInvoke(getMethod(a, "m3a"), "T", "m3a$")
+    assertInvoke(getMethod(a, "m3b"), "T", "m3b$")
+
+    assertInvoke(getMethod(c, "t1"), "T", "m1a")
+    assertInvoke(getMethod(c, "t2"), "T", "m1b")
+
+    assertInvoke(getMethod(c, "t3"), "T", "m2a") // could not inline
+    assertNoInvoke(getMethod(c, "t4"))
+
+    assertInvoke(getMethod(c, "t5"), "T", "m3a") // could not inline
+    assertInvoke(getMethod(c, "t6"), "C", "$anonfun$t6$1") // both forwarders inlined, closure eliminated
+
+    assertInvoke(getMethod(c, "t7"), "A", "m1a")
+    assertInvoke(getMethod(c, "t8"), "A", "m1b")
+
+    assertNoInvoke(getMethod(c, "t9"))
+    assertNoInvoke(getMethod(c, "t10"))
+
+    assertInvoke(getMethod(c, "t11"), "C", "$anonfun$t11$1") // both forwarders inlined, closure eliminated
+    assertInvoke(getMethod(c, "t12"), "C", "$anonfun$t12$1") // both forwarders inlined, closure eliminated
+  }
+
+  @Test
+  def sd259b(): Unit = {
+    val code =
+      """trait T {
+        |  def get = 1
+        |  @inline final def m = try { get } catch { case _: Throwable => 1 }
+        |}
+        |class A extends T
+        |class C {
+        |  def t(a: A) = 1 + a.m // cannot inline a try block onto a non-empty stack
+        |}
+      """.stripMargin
+    val warn =
+      """T::m()I is annotated @inline but could not be inlined:
+        |The operand stack at the callsite in C::t(LA;)I contains more values than the
+        |arguments expected by the callee T::m()I. These values would be discarded
+        |when entering an exception handler declared in the inlined method.""".stripMargin
+    val List(a, c, t) = compile(code, allowMessage = _.msg contains warn)
+
+    // inlinig of m$ is rolled back, because <invokespecial T.m> is not legal in class C.
+    assertInvoke(getMethod(c, "t"), "T", "m$")
+  }
+
+  @Test
+  def sd259c(): Unit = {
+    val code =
+      """trait T {
+        |  def bar = 1
+        |  @inline final def m = {
+        |    def impl = bar // private, non-static method
+        |    impl
+        |  }
+        |}
+        |class A extends T
+        |class C {
+        |  def t(a: A) = a.m
+        |}
+      """.stripMargin
+    val warn =
+      """T::m()I is annotated @inline but could not be inlined:
+        |The callee T::m()I contains the instruction INVOKESPECIAL T.impl$1 ()I
+        |that would cause an IllegalAccessError when inlined into class C.""".stripMargin
+    val List(a, c, t) = compile(code, allowMessage = _.msg contains warn)
+    assertInvoke(getMethod(c, "t"), "T", "m$")
+  }
 }