Merge pull request #5529 from lrytz/sd259

Better inliner support for 2.12 trait encoding

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$")
+  }
 }