Merge pull request #10404 from som-snytt/issue/12787-inliner

Inliner method resolution keeps concrete methods

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

@@ -204,50 +204,54 @@ abstract class ByteCodeRepository extends PerRunInit {
           case Left(e) => return Some(e)
           case Right(c) =>
             visited += i
-            // abstract and static methods are excluded, see jvms-5.4.3.3
+            // private and static methods are excluded, see jvms-5.4.3.3
             for (m <- findMethod(c) if !isPrivateMethod(m) && !isStaticMethod(m)) found += ((m, c))
             val recursionResult = findIn(c)
             if (recursionResult.isDefined) return recursionResult
         }
         None
       }
 
-      findIn(initialOwner)
-
-      val result =
-        if (found.size <= 1) found.headOption
-        else {
-          val maxSpecific = found.filterNot({
-            case (method, owner) =>
-              isAbstractMethod(method) || {
-                val ownerTp = bTypesFromClassfile.classBTypeFromClassNode(owner)
-                found exists {
-                  case (other, otherOwner) =>
-                    (other ne method) && {
-                      val otherTp = bTypesFromClassfile.classBTypeFromClassNode(otherOwner)
-                      otherTp.isSubtypeOf(ownerTp).get
-                    }
-                }
+      findIn(initialOwner) match {
+        case Some(cnf) => Left(cnf)
+        case _ =>
+          val result =
+            if (found.sizeIs <= 1) found.headOption
+            else {
+              val maxSpecific = found.filterNot {
+                case (method, owner) =>
+                  val ownerTp = bTypesFromClassfile.classBTypeFromClassNode(owner)
+                  found.exists {
+                    case (other, otherOwner) =>
+                      (other ne method) && {
+                        val otherTp = bTypesFromClassfile.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)))
+              // (*) if there's no single, non-abstract, maximally-specific method, JVM method resolution
+              // (jvms-5.4.3.3) returns any of the non-private, non-static parent methods arbitrarily
+              // (abstract or concrete).
+              // we chose not to do this here, to prevent the inlining the wrong method. in other words,
+              // a concrete method is only returned if it resolves deterministically.
+              // if there's no non-abstract method, we *do* want to return a result to allow performing
+              // accessibility checks in the inliner. for accessibility it does not matter which of these
+              // methods is returned, they are all public (protected is not possible, jvms-4.1).
+              // TODO: it would be cleaner to make `methodNode` return a list of methods and deal
+              // with it at the call sites, but it's a bigger refactoring that affects the
+              // `CallGraph`. in any case, it should not occur in Scala bytecode as we emit mixin
+              // forwarders.
+              val nonAbs = maxSpecific.filterNot(p => isAbstractMethod(p._1))
+              if (nonAbs.sizeIs == 1) nonAbs.headOption
+              else {
+                val foundNonAbs = found.filterNot(p => isAbstractMethod(p._1))
+                if (foundNonAbs.sizeIs == 1) foundNonAbs.headOption
+                else if (foundNonAbs.isEmpty) 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.
@@ -256,6 +260,7 @@ abstract class ByteCodeRepository extends PerRunInit {
     } else {
       def notFound(cnf: Option[ClassNotFound]) = Left(MethodNotFound(name, descriptor, ownerInternalNameOrArrayDescriptor, cnf))
       val res: Either[ClassNotFound, Option[(MethodNode, InternalName)]] = classNode(ownerInternalNameOrArrayDescriptor).flatMap(c =>
+        // TODO: if `c` is an interface, should directly go to `findInInterfaces`
         findInSuperClasses(c) flatMap {
           case None => findInInterfaces(c)
           case res => Right(res)

test/files/pos/t12787.scala

@@ -0,0 +1,18 @@
+
+// scalac: -opt:inline:<sources> -Wopt -Werror
+// skalac: -opt:inline:<sources> -Vopt:C -Wopt -Werror
+
+// > using scala 2.13.nightly
+// > using options -opt:inline:<sources>, -Wopt
+
+import scala.collection.LinearSeq
+
+final class C {
+  val iterators: LinearSeq[Int] = List(42)
+  @inline def current: Int = iterators.head
+  val asString = current.toString
+}
+object Test extends App {
+  val c = new C
+  println(c.asString)
+}

test/junit/scala/tools/nsc/backend/jvm/opt/InlinerTest.scala

@@ -2276,4 +2276,19 @@ class InlinerTest extends BytecodeTesting {
     assertInvoke(getMethod(c, "t2a"), "T", "p")
     assertInvoke(getMethod(c, "t2b"), "T", "p")
   }
+
+  @Test def t10404(): Unit = {
+    val c1 =
+      """trait T { def f = 1 }
+        |trait U extends T { def f: Int }
+        |trait V extends U
+        |""".stripMargin
+    val c2 =
+      """class K {
+        |  @inline final def u(c: V) = c.f
+        |  def r = u(new V { })
+        |}
+        |""".stripMargin
+    compileClassesSeparately(List(c1, c2), compilerArgs)
+  }
 }