From 6551d25b7d4b573cd22d5588f751a5b51ff4fd73 Mon Sep 17 00:00:00 2001 From: Som Snytt Date: Sat, 27 May 2023 06:46:25 -0700 Subject: [PATCH] Fix interface method lookup in optimizer --- .../backend/jvm/opt/ByteCodeRepository.scala | 77 ++++++++++--------- test/files/pos/t12787.scala | 18 +++++ .../nsc/backend/jvm/opt/InlinerTest.scala | 15 ++++ 3 files changed, 74 insertions(+), 36 deletions(-) create mode 100644 test/files/pos/t12787.scala diff --git a/src/compiler/scala/tools/nsc/backend/jvm/opt/ByteCodeRepository.scala b/src/compiler/scala/tools/nsc/backend/jvm/opt/ByteCodeRepository.scala index 2d08d3ea5d8b..b43c338b091a 100644 --- a/src/compiler/scala/tools/nsc/backend/jvm/opt/ByteCodeRepository.scala +++ b/src/compiler/scala/tools/nsc/backend/jvm/opt/ByteCodeRepository.scala @@ -204,7 +204,7 @@ 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 @@ -212,42 +212,46 @@ abstract class ByteCodeRepository extends PerRunInit { 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) diff --git a/test/files/pos/t12787.scala b/test/files/pos/t12787.scala new file mode 100644 index 000000000000..e32f880264c4 --- /dev/null +++ b/test/files/pos/t12787.scala @@ -0,0 +1,18 @@ + +// scalac: -opt:inline: -Wopt -Werror +// skalac: -opt:inline: -Vopt:C -Wopt -Werror + +// > using scala 2.13.nightly +// > using options -opt:inline:, -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) +} 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 54171b3041cf..4360d9534384 100644 --- a/test/junit/scala/tools/nsc/backend/jvm/opt/InlinerTest.scala +++ b/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) + } }