Ensure to re-write only trait method calls of actual trait methods

The inliner would incorrectly treat trait field accessors like
ordinary trait member methods and try to re-write invocations to the
corresponding static method in the implementation class. This rewrite
usually failed because no method was found in the impl class.

However, for lazy val fields, there exists a member in the impl class
with the same name, and the rewrite was performed. The result was that
every field access would execute the lazy initializer instead of
reading the field.

This commit checks the traitMethodWithStaticImplementation field of
the ScalaInlineInfo classfile attribute and puts an explicit
`safeToRewrite` flag to each call site in the call graph. This cleans
up the code in the inliner that deals with rewriting trait callsites.

4 files changed, 119 insertions, 34 deletions

diff --git a/src/compiler/scala/tools/nsc/backend/jvm/BTypes.scala b/src/compiler/scala/tools/nsc/backend/jvm/BTypes.scala
index d2ee944916..d8a17e975e 100644
--- a/src/compiler/scala/tools/nsc/backend/jvm/BTypes.scala
+++ b/src/compiler/scala/tools/nsc/backend/jvm/BTypes.scala
@@ -14,7 +14,6 @@ import asm.Opcodes
 import scala.tools.asm.tree.{MethodInsnNode, InnerClassNode, ClassNode}
 import scala.tools.nsc.backend.jvm.BTypes.{InlineInfo, MethodInlineInfo}
 import scala.tools.nsc.backend.jvm.BackendReporting._
-import BackendReporting.RightBiasedEither
 import scala.tools.nsc.backend.jvm.opt._
 import scala.collection.convert.decorateAsScala._
 
diff --git a/src/compiler/scala/tools/nsc/backend/jvm/opt/CallGraph.scala b/src/compiler/scala/tools/nsc/backend/jvm/opt/CallGraph.scala
index cd204e8043..47d32c94cb 100644
--- a/src/compiler/scala/tools/nsc/backend/jvm/opt/CallGraph.scala
+++ b/src/compiler/scala/tools/nsc/backend/jvm/opt/CallGraph.scala
@@ -27,7 +27,9 @@ class CallGraph[BT <: BTypes](val btypes: BT) {
 
   def analyzeCallsites(methodNode: MethodNode, definingClass: ClassBType): List[Callsite] = {
 
-    case class CallsiteInfo(safeToInline: Boolean, annotatedInline: Boolean, annotatedNoInline: Boolean, warning: Option[CalleeInfoWarning])
+    case class CallsiteInfo(safeToInline: Boolean, safeToRewrite: Boolean,
+                            annotatedInline: Boolean, annotatedNoInline: Boolean,
+                            warning: Option[CalleeInfoWarning])
 
     /**
      * Analyze a callsite and gather meta-data that can be used for inlining decisions.
@@ -42,25 +44,44 @@ class CallGraph[BT <: BTypes](val btypes: BT) {
         calleeDeclarationClassBType.info.orThrow.inlineInfo.methodInfos.get(methodSignature) match {
           case Some(methodInlineInfo) =>
             val canInlineFromSource = inlineGlobalEnabled || calleeSource == CompilationUnit
-            // A non-final method can be inline if the receiver type is a final subclass. Example:
-            //   class A { @inline def f = 1 }; object B extends A; B.f // can be inlined
-            def isStaticallyResolved: Boolean = {
-              // TODO: type analysis can render more calls statically resolved
-              // Example: `new A.f` can be inlined, the receiver type is known to be exactly A.
-              methodInlineInfo.effectivelyFinal || classBTypeFromParsedClassfile(receiverTypeInternalName).info.orThrow.inlineInfo.isEffectivelyFinal
+
+            val isAbstract = BytecodeUtils.isAbstractMethod(calleeMethodNode)
+
+            // (1) A non-final method can be safe to inline if the receiver type is a final subclass. Example:
+            //   class A { @inline def f = 1 }; object B extends A; B.f  // can be inlined
+            //
+            // TODO: type analysis can render more calls statically resolved. Example˜∫
+            //   new A.f  // can be inlined, the receiver type is known to be exactly A.
+            val isStaticallyResolved: Boolean = {
+              methodInlineInfo.effectivelyFinal ||
+                classBTypeFromParsedClassfile(receiverTypeInternalName).info.orThrow.inlineInfo.isEffectivelyFinal // (1)
             }
+
+            val isRewritableTraitCall = isStaticallyResolved && methodInlineInfo.traitMethodWithStaticImplementation
+
             val warning = calleeDeclarationClassBType.info.orThrow.inlineInfo.warning.map(
               MethodInlineInfoIncomplete(calleeDeclarationClassBType.internalName, calleeMethodNode.name, calleeMethodNode.desc, _))
-            CallsiteInfo(canInlineFromSource && isStaticallyResolved, methodInlineInfo.annotatedInline, methodInlineInfo.annotatedNoInline, warning)
+
+            // (1) For invocations of final trait methods, the callee isStaticallyResolved but also
+            //     abstract. Such a callee is not safe to inline - it needs to be re-written to the
+            //     static impl method first (safeToRewrite).
+            // (2) Final trait methods can be rewritten from the interface to the static implementation
+            //     method to enable inlining.
+            CallsiteInfo(
+              safeToInline      = canInlineFromSource && isStaticallyResolved && !isAbstract, // (1)
+              safeToRewrite     = canInlineFromSource && isRewritableTraitCall,               // (2)
+              annotatedInline   = methodInlineInfo.annotatedInline,
+              annotatedNoInline = methodInlineInfo.annotatedNoInline,
+              warning           = warning)
 
           case None =>
             val warning = MethodInlineInfoMissing(calleeDeclarationClassBType.internalName, calleeMethodNode.name, calleeMethodNode.desc, calleeDeclarationClassBType.info.orThrow.inlineInfo.warning)
-            CallsiteInfo(false, false, false, Some(warning))
+            CallsiteInfo(false, false, false, false, Some(warning))
         }
       } catch {
         case Invalid(noInfo: NoClassBTypeInfo) =>
           val warning = MethodInlineInfoError(calleeDeclarationClassBType.internalName, calleeMethodNode.name, calleeMethodNode.desc, noInfo)
-          CallsiteInfo(false, false, false, Some(warning))
+          CallsiteInfo(false, false, false, false, Some(warning))
       }
     }
 
@@ -80,11 +101,12 @@ class CallGraph[BT <: BTypes](val btypes: BT) {
           (declarationClassNode, source) <- byteCodeRepository.classNodeAndSource(declarationClass): Either[OptimizerWarning, (ClassNode, Source)]
           declarationClassBType          =  classBTypeFromClassNode(declarationClassNode)
         } yield {
-          val CallsiteInfo(safeToInline, annotatedInline, annotatedNoInline, warning) = analyzeCallsite(method, declarationClassBType, call.owner, source)
+          val CallsiteInfo(safeToInline, safeToRewrite, annotatedInline, annotatedNoInline, warning) = analyzeCallsite(method, declarationClassBType, call.owner, source)
           Callee(
             callee = method,
             calleeDeclarationClass = declarationClassBType,
             safeToInline = safeToInline,
+            safeToRewrite = safeToRewrite,
             annotatedInline = annotatedInline,
             annotatedNoInline = annotatedNoInline,
             calleeInfoWarning = warning)
@@ -151,13 +173,17 @@ class CallGraph[BT <: BTypes](val btypes: BT) {
    * @param calleeDeclarationClass The class in which the callee is declared
    * @param safeToInline           True if the callee can be safely inlined: it cannot be overridden,
    *                               and the inliner settings (project / global) allow inlining it.
+   * @param safeToRewrite          True if the callee the interface method of a concrete trait method
+   *                               that can be safely re-written to the static implementation method.
    * @param annotatedInline        True if the callee is annotated @inline
    * @param annotatedNoInline      True if the callee is annotated @noinline
    * @param calleeInfoWarning      An inliner warning if some information was not available while
    *                               gathering the information about this callee.
    */
   final case class Callee(callee: MethodNode, calleeDeclarationClass: ClassBType,
-                          safeToInline: Boolean,
+                          safeToInline: Boolean, safeToRewrite: Boolean,
                           annotatedInline: Boolean, annotatedNoInline: Boolean,
-                          calleeInfoWarning: Option[CalleeInfoWarning])
+                          calleeInfoWarning: Option[CalleeInfoWarning]) {
+    assert(!(safeToInline && safeToRewrite), s"A callee of ${callee.name} can be either safeToInline or safeToRewrite, but not both.")
+  }
 }
diff --git a/src/compiler/scala/tools/nsc/backend/jvm/opt/Inliner.scala b/src/compiler/scala/tools/nsc/backend/jvm/opt/Inliner.scala
index 7ce98ecff1..0f4c7d5287 100644
--- a/src/compiler/scala/tools/nsc/backend/jvm/opt/Inliner.scala
+++ b/src/compiler/scala/tools/nsc/backend/jvm/opt/Inliner.scala
@@ -71,7 +71,7 @@ class Inliner[BT <: BTypes](val btypes: BT) {
    */
   def selectCallsitesForInlining: List[Callsite] = {
     callsites.valuesIterator.filter({
-      case callsite @ Callsite(_, _, _, Right(Callee(callee, calleeDeclClass, safeToInline, annotatedInline, _, warning)), _, _, pos) =>
+      case callsite @ Callsite(_, _, _, Right(Callee(callee, calleeDeclClass, safeToInline, _, annotatedInline, _, warning)), _, _, pos) =>
         val res = doInlineCallsite(callsite)
 
         if (!res) {
@@ -80,10 +80,10 @@ class Inliner[BT <: BTypes](val btypes: BT) {
             // reason is, for example, mixed compilation (which has a separate -Yopt-warning flag).
             def initMsg = s"${BackendReporting.methodSignature(calleeDeclClass.internalName, callee)} is annotated @inline but cannot be inlined"
             def warnMsg = warning.map(" Possible reason:\n" + _).getOrElse("")
-            if (!safeToInline)
-              backendReporting.inlinerWarning(pos, s"$initMsg: the method is not final and may be overridden." + warnMsg)
-            else if (doRewriteTraitCallsite(callsite) && isAbstractMethod(callee))
+            if (doRewriteTraitCallsite(callsite))
               backendReporting.inlinerWarning(pos, s"$initMsg: the trait method call could not be rewritten to the static implementation method." + warnMsg)
+            else if (!safeToInline)
+              backendReporting.inlinerWarning(pos, s"$initMsg: the method is not final and may be overridden." + warnMsg)
             else
               backendReporting.inlinerWarning(pos, s"$initMsg." + warnMsg)
           } else if (warning.isDefined && warning.get.emitWarning(warnSettings)) {
@@ -105,13 +105,8 @@ class Inliner[BT <: BTypes](val btypes: BT) {
    * The current inlining heuristics are simple: inline calls to methods annotated @inline.
    */
   def doInlineCallsite(callsite: Callsite): Boolean = callsite match {
-    case Callsite(_, _, _, Right(Callee(callee, calleeDeclClass, safeToInline, annotatedInline, _, warning)), _, _, pos) =>
-      // Usually, safeToInline implies that the callee is not abstract.
-      // But for final trait methods, the callee is abstract: "trait T { @inline final def f = 1}".
-      // A callsite (t: T).f is `safeToInline`, but the callee is the abstract method in the interface.
-      // We try to rewrite these calls to the static impl method, but that may not always succeed,
-      // in which case we cannot inline the call.
-      annotatedInline && safeToInline && !isAbstractMethod(callee)
+    case Callsite(_, _, _, Right(Callee(callee, calleeDeclClass, safeToInline, _, annotatedInline, _, warning)), _, _, pos) =>
+      annotatedInline && safeToInline
 
     case _ => false
   }
@@ -127,13 +122,7 @@ class Inliner[BT <: BTypes](val btypes: BT) {
    * True for statically resolved trait callsites that should be rewritten to the static implementation method.
    */
   def doRewriteTraitCallsite(callsite: Callsite) = callsite.callee match {
-    case Right(Callee(callee, calleeDeclarationClass, true, annotatedInline, annotatedNoInline, infoWarning)) if isAbstractMethod(callee) =>
-      // The pattern matches abstract methods that are `safeToInline`. This can only match the interface method of a final, concrete
-      // trait method. An abstract method (in a trait or abstract class) is never `safeToInline` (abstract methods cannot be final).
-      // See also comment in `doInlineCallsite`
-      for (i <- calleeDeclarationClass.isInterface) assert(i, s"expected interface call (final trait method) when inlining abstract method: $callsite")
-      true
-
+    case Right(Callee(callee, calleeDeclarationClass, safeToInline, true, annotatedInline, annotatedNoInline, infoWarning)) => true
     case _ => false
   }
 
@@ -146,7 +135,7 @@ class Inliner[BT <: BTypes](val btypes: BT) {
    */
   def rewriteFinalTraitMethodInvocation(callsite: Callsite): Unit = {
     if (doRewriteTraitCallsite(callsite)) {
-      val Right(Callee(callee, calleeDeclarationClass, safeToInline, annotatedInline, annotatedNoInline, infoWarning)) = callsite.callee
+      val Right(Callee(callee, calleeDeclarationClass, _, _, annotatedInline, annotatedNoInline, infoWarning)) = callsite.callee
 
       val traitMethodArgumentTypes = asm.Type.getArgumentTypes(callee.desc)
 
@@ -198,7 +187,8 @@ class Inliner[BT <: BTypes](val btypes: BT) {
           callee              = Right(Callee(
             callee                 = implClassMethod,
             calleeDeclarationClass = implClassBType,
-            safeToInline           = safeToInline,
+            safeToInline           = true,
+            safeToRewrite          = false,
             annotatedInline        = annotatedInline,
             annotatedNoInline      = annotatedNoInline,
             calleeInfoWarning      = infoWarning)),
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 d32c1b2958..caaa65bf7e 100644
--- a/test/junit/scala/tools/nsc/backend/jvm/opt/InlinerTest.scala
+++ b/test/junit/scala/tools/nsc/backend/jvm/opt/InlinerTest.scala
@@ -791,4 +791,74 @@ class InlinerTest extends ClearAfterClass {
     compile(code, allowMessage = info => {i += 1; info.msg contains err})
     assert(i == 2, i)
   }
+
+  @Test
+  def noInlineTraitFieldAccessors(): Unit = {
+    val code =
+      """sealed trait T {
+        |  lazy val a = 0
+        |  val b = 1
+        |  final lazy val c = 2
+        |  final val d = 3
+        |  final val d1: Int = 3
+        |
+        |  @noinline def f = 5       // re-written to T$class
+        |  @noinline final def g = 6 // re-written
+        |
+        |  @noinline def h: Int
+        |  @inline def i: Int
+        |}
+        |
+        |trait U { // not sealed
+        |  lazy val a = 0
+        |  val b = 1
+        |  final lazy val c = 2
+        |  final val d = 3
+        |  final val d1: Int = 3
+        |
+        |  @noinline def f = 5       // not re-written (not final)
+        |  @noinline final def g = 6 // re-written
+        |
+        |  @noinline def h: Int
+        |  @inline def i: Int
+        |}
+        |
+        |class C {
+        |  def m1(t: T) = t.a + t.b + t.c + t.d1
+        |  def m2(t: T) = t.d // inlined by the type-checker's constant folding
+        |  def m3(t: T) = t.f + t.g + t.h + t.i
+        |
+        |  def m4(u: U) = u.a + u.b + u.c + u.d1
+        |  def m5(u: U) = u.d
+        |  def m6(u: U) = u.f + u.g + u.h + u.i
+        |}
+      """.stripMargin
+
+    val List(c, t, tClass, u, uClass) = compile(code, allowMessage = _.msg contains "i()I is annotated @inline but cannot be inlined")
+    val m1 = getSingleMethod(c, "m1")
+    assertInvoke(m1, "T", "a")
+    assertInvoke(m1, "T", "b")
+    assertInvoke(m1, "T", "c")
+
+    assertNoInvoke(getSingleMethod(c, "m2"))
+
+    val m3 = getSingleMethod(c, "m3")
+    assertInvoke(m3, "T$class", "f")
+    assertInvoke(m3, "T$class", "g")
+    assertInvoke(m3, "T", "h")
+    assertInvoke(m3, "T", "i")
+
+    val m4 = getSingleMethod(c, "m4")
+    assertInvoke(m4, "U", "a")
+    assertInvoke(m4, "U", "b")
+    assertInvoke(m4, "U", "c")
+
+    assertNoInvoke(getSingleMethod(c, "m5"))
+
+    val m6 = getSingleMethod(c, "m6")
+    assertInvoke(m6, "U", "f")
+    assertInvoke(m6, "U$class", "g")
+    assertInvoke(m6, "U", "h")
+    assertInvoke(m6, "U", "i")
+  }
 }