Merge pull request scala#5140 from lrytz/inlineDefaultMethods

SD-140 inline the correct default method

Author: retronym

src/compiler/scala/tools/nsc/backend/jvm/BackendReporting.scala

@@ -42,15 +42,15 @@ object BackendReporting {
   def assertionError(message: String): Nothing = throw new AssertionError(message)
 
   implicit class RightBiasedEither[A, B](val v: Either[A, B]) extends AnyVal {
-    def map[U](f: B => U) = v.right.map(f)
-    def flatMap[BB](f: B => Either[A, BB]) = v.right.flatMap(f)
+    def map[C](f: B => C): Either[A, C] = v.right.map(f)
+    def flatMap[C](f: B => Either[A, C]): Either[A, C] = v.right.flatMap(f)
     def withFilter(f: B => Boolean)(implicit empty: A): Either[A, B] = v match {
       case Left(_)  => v
       case Right(e) => if (f(e)) v else Left(empty) // scalaz.\/ requires an implicit Monoid m to get m.empty
     }
-    def foreach[U](f: B => U) = v.right.foreach(f)
+    def foreach[U](f: B => U): Unit = v.right.foreach(f)
 
-    def getOrElse[BB >: B](alt: => BB): BB = v.right.getOrElse(alt)
+    def getOrElse[C >: B](alt: => C): C = v.right.getOrElse(alt)
 
     /**
      * Get the value, fail with an assertion if this is an error.
@@ -101,11 +101,14 @@ object BackendReporting {
           else ""
         }
 
-      case MethodNotFound(name, descriptor, ownerInternalName, missingClasses) =>
-        val (javaDef, others) = missingClasses.partition(_.definedInJavaSource)
-        s"The method $name$descriptor could not be found in the class $ownerInternalName or any of its parents." +
-          (if (others.isEmpty) "" else others.map(_.internalName).mkString("\nNote that the following parent classes could not be found on the classpath: ", ", ", "")) +
-          (if (javaDef.isEmpty) "" else javaDef.map(_.internalName).mkString("\nNote that the following parent classes are defined in Java sources (mixed compilation), no bytecode is available: ", ",", ""))
+      case MethodNotFound(name, descriptor, ownerInternalName, missingClass) =>
+        val missingClassWarning = missingClass match {
+          case None => ""
+          case Some(c) =>
+            if (c.definedInJavaSource) s"\nNote that the parent class ${c.internalName} is defined in a Java source (mixed compilation), no bytecode is available."
+            else s"\nNote that the parent class ${c.internalName} could not be found on the classpath."
+        }
+        s"The method $name$descriptor could not be found in the class $ownerInternalName or any of its parents." + missingClassWarning
 
       case FieldNotFound(name, descriptor, ownerInternalName, missingClass) =>
         s"The field node $name$descriptor could not be found because the classfile $ownerInternalName cannot be found on the classpath." +
@@ -127,7 +130,7 @@ object BackendReporting {
   }
 
   case class ClassNotFound(internalName: InternalName, definedInJavaSource: Boolean) extends MissingBytecodeWarning
-  case class MethodNotFound(name: String, descriptor: String, ownerInternalNameOrArrayDescriptor: InternalName, missingClasses: List[ClassNotFound]) extends MissingBytecodeWarning {
+  case class MethodNotFound(name: String, descriptor: String, ownerInternalNameOrArrayDescriptor: InternalName, missingClass: Option[ClassNotFound]) extends MissingBytecodeWarning {
     def isArrayMethod = ownerInternalNameOrArrayDescriptor.charAt(0) == '['
   }
   case class FieldNotFound(name: String, descriptor: String, ownerInternalName: InternalName, missingClass: Option[ClassNotFound]) extends MissingBytecodeWarning

src/compiler/scala/tools/nsc/backend/jvm/CoreBTypes.scala

@@ -107,6 +107,7 @@ class CoreBTypes[BTFS <: BTypesFromSymbols[_ <: Global]](val bTypes: BTFS) {
   lazy val juHashMapRef              : ClassBType = classBTypeFromSymbol(JavaUtilHashMap)           // java/util/HashMap
   lazy val sbScalaBeanInfoRef        : ClassBType = classBTypeFromSymbol(requiredClass[scala.beans.ScalaBeanInfo])
   lazy val jliSerializedLambdaRef    : ClassBType = classBTypeFromSymbol(requiredClass[java.lang.invoke.SerializedLambda])
+  lazy val jliMethodHandleRef        : ClassBType = classBTypeFromSymbol(requiredClass[java.lang.invoke.MethodHandle])
   lazy val jliMethodHandlesRef       : ClassBType = classBTypeFromSymbol(requiredClass[java.lang.invoke.MethodHandles])
   lazy val jliMethodHandlesLookupRef : ClassBType = classBTypeFromSymbol(exitingPickler(getRequiredClass("java.lang.invoke.MethodHandles.Lookup"))) // didn't find a reliable non-stringly-typed way that works for inner classes in the backend
   lazy val jliMethodTypeRef          : ClassBType = classBTypeFromSymbol(requiredClass[java.lang.invoke.MethodType])
@@ -320,6 +321,7 @@ trait CoreBTypesProxyGlobalIndependent[BTS <: BTypes] {
   def jliCallSiteRef            : ClassBType
   def jliMethodTypeRef          : ClassBType
   def jliSerializedLambdaRef    : ClassBType
+  def jliMethodHandleRef        : ClassBType
   def jliMethodHandlesLookupRef : ClassBType
   def srBoxesRunTimeRef         : ClassBType
   def srBoxedUnitRef            : ClassBType
@@ -383,6 +385,7 @@ final class CoreBTypesProxy[BTFS <: BTypesFromSymbols[_ <: Global]](val bTypes:
   def juHashMapRef              : ClassBType = _coreBTypes.juHashMapRef
   def sbScalaBeanInfoRef        : ClassBType = _coreBTypes.sbScalaBeanInfoRef
   def jliSerializedLambdaRef    : ClassBType = _coreBTypes.jliSerializedLambdaRef
+  def jliMethodHandleRef        : ClassBType = _coreBTypes.jliMethodHandleRef
   def jliMethodHandlesRef       : ClassBType = _coreBTypes.jliMethodHandlesRef
   def jliMethodHandlesLookupRef : ClassBType = _coreBTypes.jliMethodHandlesLookupRef
   def jliMethodTypeRef          : ClassBType = _coreBTypes.jliMethodTypeRef

src/compiler/scala/tools/nsc/backend/jvm/opt/ByteCodeRepository.scala

@@ -10,7 +10,7 @@ package opt
 import scala.tools.asm
 import asm.tree._
 import scala.collection.JavaConverters._
-import scala.collection.concurrent
+import scala.collection.{concurrent, mutable}
 import scala.tools.asm.Attribute
 import scala.tools.nsc.backend.jvm.BackendReporting._
 import scala.tools.nsc.io.AbstractFile
@@ -132,38 +132,135 @@ class ByteCodeRepository[BT <: BTypes](val classPath: ClassFileLookup[AbstractFi
    * The method node for a method matching `name` and `descriptor`, accessed in class `ownerInternalNameOrArrayDescriptor`.
    * The declaration of the method may be in one of the parents.
    *
-   * TODO: make sure we always return the right method, the one being invoked. write tests.
-   *   - if there's an abstract and a concrete one. could possibly somehow the abstract be returned?
-   *   - with traits and default methods, if there is more than one default method inherited and
-   *     no override: what should be returned? We should not just inline one of the two.
+   * Note that the JVM spec performs method lookup in two steps: resolution and selection.
+   *
+   * Method resolution, defined in jvms-5.4.3.3 and jvms-5.4.3.4, is the first step and is identical
+   * for all invocation styles (virtual, interface, special, static). If C is the receiver class
+   * in the invocation instruction:
+   *   1 find a matching method (name and descriptor) in C
+   *   2 then in C's superclasses
+   *   3 then find the maximally-specific matching superinterface methods, succeed if there's a
+   *     single non-abstract one. static and private methods in superinterfaces are not considered.
+   *   4 then pick a random non-static, non-private superinterface method.
+   *   5 then fail.
+   *
+   * Note that for an `invokestatic` instruction, a method reference `B.m` may resolve to `A.m`, if
+   * class `B` doesn't specify a matching method `m`, but the parent `A` does.
+   *
+   * Selection depends on the invocation style and is defined in jvms-6.5.
+   *   - invokestatic: invokes the resolved method
+   *   - invokevirtual / invokeinterface: searches for an override of the resolved method starting
+   *     at the dynamic receiver type. the search procedure is basically the same as in resolution,
+   *     but it fails at 4 instead of picking a superinterface method at random.
+   *   - invokespecial: if C is the receiver in the invocation instruction, searches for an override
+   *     of the resolved method starting at
+   *       - the superclass of the current class, if C is a superclass of the current class
+   *       - C otherwise
+   *     again, the search procedure is the same.
+   *
+   * In the method here we implement method *resolution*. Whether or not the returned method is
+   * actually invoked at runtime depends on the invocation instruction and the class hierarchy, so
+   * the users (e.g. the inliner) have to be aware of method selection.
+   *
+   * Note that the returned method may be abstract (ACC_ABSTRACT), native (ACC_NATIVE) or signature
+   * polymorphic (methods `invoke` and `invokeExact` in class `MehtodHandles`).
    *
    * @return The [[MethodNode]] of the requested method and the [[InternalName]] of its declaring
-   *         class, or an error message if the method could not be found.
+   *         class, or an error message if the method could not be found. An error message is also
+   *         returned if method resolution results in multiple default methods.
    */
   def methodNode(ownerInternalNameOrArrayDescriptor: String, name: String, descriptor: String): Either[MethodNotFound, (MethodNode, InternalName)] = {
-    // on failure, returns a list of class names that could not be found on the classpath
-    def methodNodeImpl(ownerInternalName: InternalName): Either[List[ClassNotFound], (MethodNode, InternalName)] = {
-      classNode(ownerInternalName) match {
-        case Left(e)  => Left(List(e))
-        case Right(c) =>
-          c.methods.asScala.find(m => m.name == name && m.desc == descriptor) match {
-            case Some(m) => Right((m, ownerInternalName))
-            case None    => findInParents(Option(c.superName) ++: c.interfaces.asScala.toList, Nil)
-          }
+    def findMethod(c: ClassNode): Option[MethodNode] = c.methods.asScala.find(m => m.name == name && m.desc == descriptor)
+
+    // https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-2.html#jvms-2.9: "In Java SE 8, the only
+    // signature polymorphic methods are the invoke and invokeExact methods of the class MethodHandle.
+    def isSignaturePolymorphic(owner: InternalName) = owner == coreBTypes.jliMethodHandleRef.internalName && (name == "invoke" || name == "invokeExact")
+
+    // Note: if `owner` is an interface, in the first iteration we search for a matching member in the interface itself.
+    // If that fails, the recursive invocation checks in the superclass (which is Object) with `publicInstanceOnly == true`.
+    // This is specified in jvms-5.4.3.4: interface method resolution only returns public, non-static methods of Object.
+    def findInSuperClasses(owner: ClassNode, publicInstanceOnly: Boolean = false): Either[ClassNotFound, Option[(MethodNode, InternalName)]] = {
+      findMethod(owner) match {
+        case Some(m) if !publicInstanceOnly || (isPublicMethod(m) && !isStaticMethod(m)) => Right(Some((m, owner.name)))
+        case None =>
+          if (isSignaturePolymorphic(owner.name)) Right(Some((owner.methods.asScala.find(_.name == name).get, owner.name)))
+          else if (owner.superName == null) Right(None)
+          else classNode(owner.superName).flatMap(findInSuperClasses(_, isInterface(owner)))
       }
     }
 
-    // find the MethodNode in one of the parent classes
-    def findInParents(parents: List[InternalName], failedClasses: List[ClassNotFound]): Either[List[ClassNotFound], (MethodNode, InternalName)] = parents match {
-      case x :: xs => methodNodeImpl(x).left.flatMap(failed => findInParents(xs, failed ::: failedClasses))
-      case Nil     => Left(failedClasses)
+    def findInInterfaces(initialOwner: ClassNode): Either[ClassNotFound, Option[(MethodNode, InternalName)]] = {
+      val visited = mutable.Set.empty[InternalName]
+      val found = mutable.ListBuffer.empty[(MethodNode, ClassNode)]
+
+      def findIn(owner: ClassNode): Option[ClassNotFound] = {
+        for (i <- owner.interfaces.asScala if !visited(i)) classNode(i) match {
+          case Left(e) => return Some(e)
+          case Right(c) =>
+            visited += i
+            // abstract and static methods are excluded, see jvms-5.4.3.3
+            for (m <- findMethod(c) if !isPrivateMethod(m) && !isStaticMethod(m)) found += ((m, c))
+            val recusionResult = findIn(c)
+            if (recusionResult.isDefined) return recusionResult
+        }
+        None
+      }
+
+      findIn(initialOwner)
+
+      val result =
+        if (found.size <= 1) found.headOption
+        else {
+          val maxSpecific = found.filterNot({
+            case (method, owner) =>
+              isAbstractMethod(method) || {
+                val ownerTp = classBTypeFromClassNode(owner)
+                found exists {
+                  case (other, otherOwner) =>
+                    (other ne method) && {
+                      val otherTp = classBTypeFromClassNode(otherOwner)
+                      otherTp.isSubtypeOf(ownerTp).get
+                    }
+                }
+              }
+          })
+          // (*) note that if there's no single, non-abstract, maximally-specific method, the jvm
+          // method resolution (jvms-5.4.3.3) returns any of the non-private, non-static parent
+          // methods at random (abstract or concrete).
+          // we chose not to do this here, to prevent the inliner from potentially inlining the
+          // wrong method. in other words, we guarantee that a concrete method is only returned if
+          // it resolves deterministically.
+          // however, there may be multiple abstract methods inherited. in this case we *do* want
+          // to return a result to allow performing accessibility checks in the inliner. note that
+          // for accessibility it does not matter which of these methods is return, as they are all
+          // non-private (i.e., public, protected is not possible, jvms-4.1).
+          // the remaining case (when there's no max-specific method, but some non-abstract one)
+          // does not occur in bytecode generated by scalac or javac. we return no result in this
+          // case. this may at worst prevent some optimizations from happening.
+          if (maxSpecific.size == 1) maxSpecific.headOption
+          else if (found.forall(p => isAbstractMethod(p._1))) found.headOption // (*)
+          else None
+        }
+      Right(result.map(p => (p._1, p._2.name)))
     }
 
     // In a MethodInsnNode, the `owner` field may be an array descriptor, for example when invoking `clone`. We don't have a method node to return in this case.
-    if (ownerInternalNameOrArrayDescriptor.charAt(0) == '[')
-      Left(MethodNotFound(name, descriptor, ownerInternalNameOrArrayDescriptor, Nil))
-    else
-      methodNodeImpl(ownerInternalNameOrArrayDescriptor).left.map(MethodNotFound(name, descriptor, ownerInternalNameOrArrayDescriptor, _))
+    if (ownerInternalNameOrArrayDescriptor.charAt(0) == '[') {
+      Left(MethodNotFound(name, descriptor, ownerInternalNameOrArrayDescriptor, None))
+    } else {
+      def notFound(cnf: Option[ClassNotFound]) = Left(MethodNotFound(name, descriptor, ownerInternalNameOrArrayDescriptor, cnf))
+      val res: Either[ClassNotFound, Option[(MethodNode, InternalName)]] = classNode(ownerInternalNameOrArrayDescriptor).flatMap(c =>
+        findInSuperClasses(c) flatMap {
+          case None => findInInterfaces(c)
+          case res => Right(res)
+        }
+      )
+      res match {
+        case Left(e) => notFound(Some(e))
+        case Right(None) => notFound(None)
+        case Right(Some(res)) => Right(res)
+      }
+    }
   }
 
   private def parseClass(internalName: InternalName): Either[ClassNotFound, ClassNode] = {

src/compiler/scala/tools/nsc/backend/jvm/opt/BytecodeUtils.scala

@@ -99,6 +99,10 @@ object BytecodeUtils {
     methodNode.name == INSTANCE_CONSTRUCTOR_NAME || methodNode.name == CLASS_CONSTRUCTOR_NAME
   }
 
+  def isPublicMethod(methodNode: MethodNode): Boolean = (methodNode.access & ACC_PUBLIC) != 0
+
+  def isPrivateMethod(methodNode: MethodNode): Boolean = (methodNode.access & ACC_PRIVATE) != 0
+
   def isStaticMethod(methodNode: MethodNode): Boolean = (methodNode.access & ACC_STATIC) != 0
 
   def isAbstractMethod(methodNode: MethodNode): Boolean = (methodNode.access & ACC_ABSTRACT) != 0
@@ -107,10 +111,12 @@ object BytecodeUtils {
 
   def isNativeMethod(methodNode: MethodNode): Boolean = (methodNode.access & ACC_NATIVE) != 0
 
-  def hasCallerSensitiveAnnotation(methodNode: MethodNode) = methodNode.visibleAnnotations != null && methodNode.visibleAnnotations.asScala.exists(_.desc == "Lsun/reflect/CallerSensitive;")
+  def hasCallerSensitiveAnnotation(methodNode: MethodNode): Boolean = methodNode.visibleAnnotations != null && methodNode.visibleAnnotations.asScala.exists(_.desc == "Lsun/reflect/CallerSensitive;")
 
   def isFinalClass(classNode: ClassNode): Boolean = (classNode.access & ACC_FINAL) != 0
 
+  def isInterface(classNode: ClassNode): Boolean = (classNode.access & ACC_INTERFACE) != 0
+
   def isFinalMethod(methodNode: MethodNode): Boolean = (methodNode.access & (ACC_FINAL | ACC_PRIVATE | ACC_STATIC)) != 0
 
   def isStrictfpMethod(methodNode: MethodNode): Boolean = (methodNode.access & ACC_STRICT) != 0

src/compiler/scala/tools/nsc/backend/jvm/opt/CallGraph.scala

@@ -131,19 +131,19 @@ class CallGraph[BT <: BTypes](val btypes: BT) {
               (method, declarationClass)     <- byteCodeRepository.methodNode(call.owner, call.name, call.desc): Either[OptimizerWarning, (MethodNode, InternalName)]
               (declarationClassNode, source) <- byteCodeRepository.classNodeAndSource(declarationClass): Either[OptimizerWarning, (ClassNode, Source)]
             } yield {
-                val declarationClassBType = classBTypeFromClassNode(declarationClassNode)
-                val info = analyzeCallsite(method, declarationClassBType, call, source)
-                import info._
-                Callee(
-                  callee = method,
-                  calleeDeclarationClass = declarationClassBType,
-                  safeToInline = safeToInline,
-                  canInlineFromSource = canInlineFromSource,
-                  annotatedInline = annotatedInline,
-                  annotatedNoInline = annotatedNoInline,
-                  samParamTypes = info.samParamTypes,
-                  calleeInfoWarning = warning)
-              }
+              val declarationClassBType = classBTypeFromClassNode(declarationClassNode)
+              val info = analyzeCallsite(method, declarationClassBType, call, source)
+              import info._
+              Callee(
+                callee = method,
+                calleeDeclarationClass = declarationClassBType,
+                safeToInline = safeToInline,
+                canInlineFromSource = canInlineFromSource,
+                annotatedInline = annotatedInline,
+                annotatedNoInline = annotatedNoInline,
+                samParamTypes = info.samParamTypes,
+                calleeInfoWarning = warning)
+            }
 
             val argInfos = computeArgInfos(callee, call, prodCons)
 
@@ -388,12 +388,11 @@ class CallGraph[BT <: BTypes](val btypes: BT) {
    * @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: btypes.ClassBType,
-                           safeToInline: Boolean, canInlineFromSource: Boolean,
-                           annotatedInline: Boolean, annotatedNoInline: Boolean,
-                           samParamTypes: IntMap[btypes.ClassBType],
-                           calleeInfoWarning: Option[CalleeInfoWarning]) {
+  final case class Callee(callee: MethodNode, calleeDeclarationClass: btypes.ClassBType,
+                          safeToInline: Boolean, canInlineFromSource: Boolean,
+                          annotatedInline: Boolean, annotatedNoInline: Boolean,
+                          samParamTypes: IntMap[btypes.ClassBType],
+                          calleeInfoWarning: Option[CalleeInfoWarning]) {
     override def toString = s"Callee($calleeDeclarationClass.${callee.name})"
   }