From b932419f07c0d747d868ebc3b51ccc54573a5827 Mon Sep 17 00:00:00 2001 From: Lukas Rytz Date: Wed, 6 Apr 2016 10:15:38 +0200 Subject: Rewrite JUnit tests to avoid `@Test` methods in traits JUnit 4 does not support running `@Test` methods defined as default methods in parent interfaces. JUnit 5 will, but is not yet available. Currently scalac emits a forwarder to every trait method inherited by a class, so tests are correctly executed. The fix for SD-98 will change this. --- .../scala/reflect/internal/PrintersTest.scala | 26 ++++++++++------------ 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/test/junit/scala/reflect/internal/PrintersTest.scala b/test/junit/scala/reflect/internal/PrintersTest.scala index 9bfe6eecb8..2305e7ea50 100644 --- a/test/junit/scala/reflect/internal/PrintersTest.scala +++ b/test/junit/scala/reflect/internal/PrintersTest.scala @@ -8,14 +8,6 @@ import scala.reflect.runtime.{currentMirror=>cm} import org.junit.runner.RunWith import org.junit.runners.JUnit4 -@RunWith(classOf[JUnit4]) -class PrintersTest extends BasePrintTests - with ClassPrintTests - with TraitPrintTests - with ValAndDefPrintTests - with QuasiTreesPrintTests - with PackagePrintTests - object PrinterHelper { val toolbox = cm.mkToolBox() @@ -73,7 +65,8 @@ object PrinterHelper { import PrinterHelper._ -trait BasePrintTests { +@RunWith(classOf[JUnit4]) +class BasePrintTest { @Test def testIdent = assertTreeCode(Ident("*"))("*") @Test def testConstant1 = assertTreeCode(Literal(Constant("*")))("\"*\"") @@ -348,7 +341,8 @@ trait BasePrintTests { @Test def testImport4 = assertPrintedCode("import scala.collection._") } -trait ClassPrintTests { +@RunWith(classOf[JUnit4]) +class ClassPrintTest { @Test def testClass = assertPrintedCode("class *") @Test def testClassWithBody = assertPrintedCode(sm""" @@ -833,7 +827,8 @@ trait ClassPrintTests { |}""") } -trait TraitPrintTests { +@RunWith(classOf[JUnit4]) +class TraitPrintTest { @Test def testTrait = assertPrintedCode("trait *") @Test def testTraitWithBody = assertPrintedCode(sm""" @@ -953,7 +948,8 @@ trait TraitPrintTests { |}""") } -trait ValAndDefPrintTests { +@RunWith(classOf[JUnit4]) +class ValAndDefPrintTest { @Test def testVal1 = assertPrintedCode("val a: scala.Unit = ()") @Test def testVal2 = assertPrintedCode("val * : scala.Unit = ()") @@ -1093,7 +1089,8 @@ trait ValAndDefPrintTests { |}""", wrapCode = true) } -trait PackagePrintTests { +@RunWith(classOf[JUnit4]) +class PackagePrintTest { @Test def testPackage1 = assertPrintedCode(sm""" |package foo.bar { | @@ -1131,7 +1128,8 @@ trait PackagePrintTests { |}""", checkTypedTree = false) } -trait QuasiTreesPrintTests { +@RunWith(classOf[JUnit4]) +class QuasiTreesPrintTest { @Test def testQuasiIdent = assertTreeCode(q"*")("*") @Test def testQuasiVal = assertTreeCode(q"val * : Unit = null")("val * : Unit = null") -- cgit v1.2.3 From 33e71061d63d08ee23e28d9a43ad601afa74e043 Mon Sep 17 00:00:00 2001 From: Lukas Rytz Date: Tue, 5 Apr 2016 22:01:43 +0200 Subject: SD-98 don't emit unnecessary mixin forwarders In most cases when a class inherits a concrete method from a trait we don't need to generate a forwarder to the default method in the class. t5148 is moved to pos as it compiles without error now. the error message ("missing or invalid dependency") is still tested by t6440b. --- src/compiler/scala/tools/nsc/transform/Mixin.scala | 38 +++++- test/files/neg/t4749.check | 2 +- test/files/neg/t5148.check | 16 --- test/files/neg/t5148.scala | 4 - test/files/pos/t5148.scala | 4 + test/files/run/mixin-signatures.check | 21 ++-- test/files/run/t5652.check | 1 - test/files/run/t6554.scala | 12 +- test/files/run/t7932.check | 5 +- test/files/run/t7932.scala | 25 +++- test/junit/scala/issues/BytecodeTest.scala | 133 +++++++++++++++++++++ .../tools/nsc/backend/jvm/opt/InlinerTest.scala | 23 +--- .../nsc/backend/jvm/opt/ScalaInlineInfoTest.scala | 4 - 13 files changed, 227 insertions(+), 61 deletions(-) delete mode 100644 test/files/neg/t5148.check delete mode 100644 test/files/neg/t5148.scala create mode 100644 test/files/pos/t5148.scala diff --git a/src/compiler/scala/tools/nsc/transform/Mixin.scala b/src/compiler/scala/tools/nsc/transform/Mixin.scala index ed7ef0d8fd..03935c3d67 100644 --- a/src/compiler/scala/tools/nsc/transform/Mixin.scala +++ b/src/compiler/scala/tools/nsc/transform/Mixin.scala @@ -47,7 +47,6 @@ abstract class Mixin extends InfoTransform with ast.TreeDSL { sym.isMethod && (!sym.hasFlag(DEFERRED | SUPERACCESSOR) || (sym hasFlag lateDEFERRED)) && sym.owner.isTrait - && sym.isMethod && (!sym.isModule || sym.hasFlag(PRIVATE | LIFTED)) && (!(sym hasFlag (ACCESSOR | SUPERACCESSOR)) || sym.isLazy) && !sym.isPrivate @@ -240,8 +239,41 @@ abstract class Mixin extends InfoTransform with ast.TreeDSL { for (member <- mixinClass.info.decls ; if isImplementedStatically(member)) { member overridingSymbol clazz match { case NoSymbol => - if (clazz.info.findMember(member.name, 0, 0L, stableOnly = false).alternatives contains member) - cloneAndAddMixinMember(mixinClass, member).asInstanceOf[TermSymbol] setAlias member + val isMemberOfClazz = clazz.info.findMember(member.name, 0, 0L, stableOnly = false).alternatives.contains(member) + if (isMemberOfClazz) { + val competingMethods = clazz.baseClasses.iterator + .filter(_ ne mixinClass) + .map(member.overriddenSymbol) + .filter(_.exists) + .toList + + // `member` is a concrete `method` defined in `mixinClass`, which is a base class of + // `clazz`, and the method is not overridden in `clazz`. A forwarder is needed if: + // + // - A non-trait base class defines matching method. Example: + // class C {def f: Int}; trait T extends C {def f = 1}; class D extends T + // Even if C.f is abstract, the forwarder in D is needed, otherwise the JVM would + // resolve `D.f` to `C.f`, see jvms-6.5.invokevirtual. + // + // - There exists another concrete, matching method in any of the base classes, and + // the `mixinClass` does not itself extend that base class. In this case the + // forwarder is needed to disambiguate. Example: + // trait T1 {def f = 1}; trait T2 extends T1 {override def f = 2}; class C extends T2 + // In C we don't need a forwarder for f because T2 extends T1, so the JVM resolves + // C.f to T2.f non-ambiguously. See jvms-5.4.3.3, "maximally-specific method". + // trait U1 {def f = 1}; trait U2 {self:U1 => override def f = 2}; class D extends U2 + // In D the forwarder is needed, the interfaces U1 and U2 are unrelated at the JVM + // level. + + lazy val mixinSuperInterfaces = mixinClass.ancestors.filter(_.isTraitOrInterface) + val needsForwarder = competingMethods.exists(m => { + !m.owner.isTraitOrInterface || + (!m.isDeferred && !mixinSuperInterfaces.contains(m.owner)) + }) + if (needsForwarder) + cloneAndAddMixinMember(mixinClass, member).asInstanceOf[TermSymbol] setAlias member + } + case _ => } } diff --git a/test/files/neg/t4749.check b/test/files/neg/t4749.check index 3539140954..6bd2550097 100644 --- a/test/files/neg/t4749.check +++ b/test/files/neg/t4749.check @@ -26,7 +26,7 @@ t4749.scala:26: warning: Fail6 has a main method with parameter type Array[Strin object Fail6 { ^ t4749.scala:42: warning: Win3 has a main method with parameter type Array[String], but bippy.Win3 will not be a runnable program. - Reason: main method must have exact signature (Array[String])Unit + Reason: main methods cannot refer to type parameters or abstract types. object Win3 extends WinBippy[Unit] { } ^ error: No warnings can be incurred under -Xfatal-warnings. diff --git a/test/files/neg/t5148.check b/test/files/neg/t5148.check deleted file mode 100644 index 1f58c235ce..0000000000 --- a/test/files/neg/t5148.check +++ /dev/null @@ -1,16 +0,0 @@ -error: missing or invalid dependency detected while loading class file 'Imports.class'. -Could not access term memberHandlers in class scala.tools.nsc.interpreter.IMain, -because it (or its dependencies) are missing. Check your build definition for -missing or conflicting dependencies. (Re-run with `-Ylog-classpath` to see the problematic classpath.) -A full rebuild may help if 'Imports.class' was compiled against an incompatible version of scala.tools.nsc.interpreter.IMain. -error: missing or invalid dependency detected while loading class file 'Imports.class'. -Could not access type Wrapper in class scala.tools.nsc.interpreter.IMain.Request, -because it (or its dependencies) are missing. Check your build definition for -missing or conflicting dependencies. (Re-run with `-Ylog-classpath` to see the problematic classpath.) -A full rebuild may help if 'Imports.class' was compiled against an incompatible version of scala.tools.nsc.interpreter.IMain.Request. -error: missing or invalid dependency detected while loading class file 'Imports.class'. -Could not access type Request in class scala.tools.nsc.interpreter.IMain, -because it (or its dependencies) are missing. Check your build definition for -missing or conflicting dependencies. (Re-run with `-Ylog-classpath` to see the problematic classpath.) -A full rebuild may help if 'Imports.class' was compiled against an incompatible version of scala.tools.nsc.interpreter.IMain. -three errors found diff --git a/test/files/neg/t5148.scala b/test/files/neg/t5148.scala deleted file mode 100644 index fca64e57df..0000000000 --- a/test/files/neg/t5148.scala +++ /dev/null @@ -1,4 +0,0 @@ -package scala.tools.nsc -package interpreter - -class IMain extends Imports diff --git a/test/files/pos/t5148.scala b/test/files/pos/t5148.scala new file mode 100644 index 0000000000..fca64e57df --- /dev/null +++ b/test/files/pos/t5148.scala @@ -0,0 +1,4 @@ +package scala.tools.nsc +package interpreter + +class IMain extends Imports diff --git a/test/files/run/mixin-signatures.check b/test/files/run/mixin-signatures.check index 77bff79ac8..9961992e2d 100644 --- a/test/files/run/mixin-signatures.check +++ b/test/files/run/mixin-signatures.check @@ -1,19 +1,23 @@ class Test$bar1$ { - public java.lang.String Test$bar1$.f(java.lang.Object) + public default java.lang.String Foo1.f(java.lang.Object) + generic: public default java.lang.String Foo1.f(T) public java.lang.Object Test$bar1$.f(java.lang.Object) public java.lang.String Test$bar1$.g(java.lang.String) public java.lang.Object Test$bar1$.g(java.lang.Object) public java.lang.String Test$bar1$.g(java.lang.Object) - public java.lang.Object Test$bar1$.h(java.lang.Object) + public default java.lang.Object Base.h(java.lang.Object) + generic: public default R Base.h(T) } class Test$bar2$ { - public java.lang.Object Test$bar2$.f(java.lang.String) + public default java.lang.Object Foo2.f(java.lang.String) + generic: public default R Foo2.f(java.lang.String) public java.lang.Object Test$bar2$.f(java.lang.Object) public java.lang.String Test$bar2$.g(java.lang.String) public java.lang.Object Test$bar2$.g(java.lang.Object) public java.lang.Object Test$bar2$.g(java.lang.String) - public java.lang.Object Test$bar2$.h(java.lang.Object) + public default java.lang.Object Base.h(java.lang.Object) + generic: public default R Base.h(T) } class Test$bar3$ { @@ -23,7 +27,8 @@ class Test$bar3$ { public java.lang.String Test$bar3$.g(java.lang.String) public java.lang.Object Test$bar3$.g(java.lang.Object) public java.lang.String Test$bar3$.g(java.lang.Object) - public java.lang.Object Foo3.h(java.lang.Object) + public default java.lang.Object Base.h(java.lang.Object) + generic: public default R Base.h(T) } class Test$bar4$ { @@ -33,7 +38,8 @@ class Test$bar4$ { public java.lang.String Test$bar4$.g(java.lang.String) public java.lang.Object Test$bar4$.g(java.lang.Object) public java.lang.Object Test$bar4$.g(java.lang.String) - public java.lang.Object Foo4.h(java.lang.Object) + public default java.lang.Object Base.h(java.lang.Object) + generic: public default R Base.h(T) } class Test$bar5$ { @@ -45,7 +51,8 @@ class Test$bar5$ { public java.lang.Object Test$bar5$.g(java.lang.Object) public java.lang.Object Test$bar5$.g(java.lang.String) public java.lang.String Test$bar5$.g(java.lang.Object) - public java.lang.Object Test$bar5$.h(java.lang.Object) + public default java.lang.Object Base.h(java.lang.Object) + generic: public default R Base.h(T) } interface Foo1 { diff --git a/test/files/run/t5652.check b/test/files/run/t5652.check index 326b22f9dd..a0fb6fe9b4 100644 --- a/test/files/run/t5652.check +++ b/test/files/run/t5652.check @@ -4,5 +4,4 @@ public default void T1.$init$() public final int A1.A1$$g$2() public int A1.f1() public final int A2.A2$$g$1() -public int A2.f0() public int A2.f2() diff --git a/test/files/run/t6554.scala b/test/files/run/t6554.scala index 5d29d16666..eed139fea6 100644 --- a/test/files/run/t6554.scala +++ b/test/files/run/t6554.scala @@ -1,8 +1,14 @@ -trait Foo[A] { +trait T1[A] { def minBy[B](b: B): A = ??? } - -class Bar extends Foo[Int] + +// The second trait is needed to make sure there's a forwarder generated in Bar. +// otherwise Bar.minBy is just the inherited default method from T1. +trait T2[A] { self: T1[A] => + override def minBy[B](b: B): A = ??? +} + +class Bar extends T1[Int] with T2[Int] object Test extends App { val sigs = classOf[Bar].getDeclaredMethods.map(m => s"${m.toString} / ${m.toGenericString}").sorted diff --git a/test/files/run/t7932.check b/test/files/run/t7932.check index 3f0a0c4f62..a2ad84cd46 100644 --- a/test/files/run/t7932.check +++ b/test/files/run/t7932.check @@ -1,3 +1,6 @@ -warning: there was one feature warning; re-run with -feature for details public Category C.category() public Category C.category1() +public default Category M1.category() +public default Category M1.category1() +public default Category M2.category() +public default Category M2.category1() diff --git a/test/files/run/t7932.scala b/test/files/run/t7932.scala index 8743abff88..e6bdbf2417 100644 --- a/test/files/run/t7932.scala +++ b/test/files/run/t7932.scala @@ -1,11 +1,28 @@ +import scala.language.higherKinds + class Category[M[_, _]] -trait M[F] { + +trait M1[F] { type X[a, b] = F def category: Category[X] = null def category1: Category[Tuple2] = null } -abstract class C extends M[Float] + +// The second trait is needed to make sure there's a forwarder generated in C. +// otherwise the trait methods are just the inherited default methods from M1. +trait M2[F] { self: M1[F] => + override def category: Category[X] = null + override def category1: Category[Tuple2] = null +} + +abstract class C extends M1[Float] with M2[Float] + object Test extends App { - val ms = classOf[C].getMethods.filter(_.getName.startsWith("category")) - println(ms.map(_.toGenericString).sorted.mkString("\n")) + def t(c: Class[_]) = { + val ms = c.getMethods.filter(_.getName.startsWith("category")) + println(ms.map(_.toGenericString).sorted.mkString("\n")) + } + t(classOf[C]) + t(classOf[M1[_]]) + t(classOf[M2[_]]) } diff --git a/test/junit/scala/issues/BytecodeTest.scala b/test/junit/scala/issues/BytecodeTest.scala index 18f8b44391..08b984a622 100644 --- a/test/junit/scala/issues/BytecodeTest.scala +++ b/test/junit/scala/issues/BytecodeTest.scala @@ -3,11 +3,14 @@ package scala.issues import org.junit.runner.RunWith import org.junit.runners.JUnit4 import org.junit.Test + import scala.tools.asm.Opcodes._ import scala.tools.nsc.backend.jvm.AsmUtils import scala.tools.nsc.backend.jvm.CodeGenTools._ import org.junit.Assert._ + import scala.collection.JavaConverters._ +import scala.tools.asm.tree.ClassNode import scala.tools.partest.ASMConverters._ import scala.tools.testing.ClearAfterClass @@ -208,4 +211,134 @@ class BytecodeTest extends ClearAfterClass { Label(14), Op(ICONST_0), Label(17), Op(IRETURN))) } + + object forwarderTestUtils { + def findMethods(cls: ClassNode, name: String): List[Method] = cls.methods.iterator.asScala.find(_.name == name).map(convertMethod).toList + + import language.implicitConversions + implicit def s2c(s: Symbol)(implicit classes: Map[String, ClassNode]): ClassNode = classes(s.name) + + def checkForwarder(c: ClassNode, target: String) = { + val List(f) = findMethods(c, "f") + assertSameCode(f, List(VarOp(ALOAD, 0), Invoke(INVOKESPECIAL, target, "f", "()I", false), Op(IRETURN))) + } + } + + @Test + def traitMethodForwarders(): Unit = { + import forwarderTestUtils._ + val code = + """trait T1 { def f = 1 } + |trait T2 extends T1 { override def f = 2 } + |trait T3 { self: T1 => override def f = 3 } + | + |abstract class A1 { def f: Int } + |class A2 { def f: Int = 4 } + | + |trait T4 extends A1 { def f = 5 } + |trait T5 extends A2 { override def f = 6 } + | + |trait T6 { def f: Int } + |trait T7 extends T6 { abstract override def f = super.f + 1 } + | + |trait T8 { override def clone() = super.clone() } + | + |class A3 extends T1 { override def f = 7 } + | + |class C1 extends T1 + |class C2 extends T2 + |class C3 extends T1 with T2 + |class C4 extends T2 with T1 + |class C5 extends T1 with T3 + | + |// traits extending a class that defines f + |class C6 extends T4 + |class C7 extends T5 + |class C8 extends A1 with T4 + |class C9 extends A2 with T5 + | + |// T6: abstract f in trait + |class C10 extends T6 with T1 + |class C11 extends T6 with T2 + |abstract class C12 extends A1 with T6 + |class C13 extends A2 with T6 + |class C14 extends T4 with T6 + |class C15 extends T5 with T6 + | + |// superclass overrides a trait method + |class C16 extends A3 + |class C17 extends A3 with T1 + | + |// abstract override + |class C18 extends T6 { def f = 22 } + |class C19 extends C18 with T7 + | + |class C20 extends T8 + """.stripMargin + + implicit val classes = compileClasses(compiler)(code).map(c => (c.name, c)).toMap + + val noForwarder = List('C1, 'C2, 'C3, 'C4, 'C10, 'C11, 'C12, 'C13, 'C16, 'C17) + for (c <- noForwarder) assertEquals(findMethods(c, "f"), Nil) + + checkForwarder('C5, "T3") + checkForwarder('C6, "T4") + checkForwarder('C7, "T5") + checkForwarder('C8, "T4") + checkForwarder('C9, "T5") + checkForwarder('C14, "T4") + checkForwarder('C15, "T5") + assertSameSummary(getSingleMethod('C18, "f"), List(BIPUSH, IRETURN)) + checkForwarder('C19, "T7") + assertSameCode(getSingleMethod('C19, "T7$$super$f"), List(VarOp(ALOAD, 0), Invoke(INVOKESPECIAL, "C18", "f", "()I", false), Op(IRETURN))) + assertInvoke(getSingleMethod('C20, "clone"), "T8", "clone") // mixin forwarder + } + + @Test + def noTraitMethodForwardersForOverloads(): Unit = { + import forwarderTestUtils._ + val code = + """trait T1 { def f(x: Int) = 0 } + |trait T2 { def f(x: String) = 1 } + |class C extends T1 with T2 + """.stripMargin + val List(c, t1, t2) = compileClasses(compiler)(code) + assertEquals(findMethods(c, "f"), Nil) + } + + @Test + def traitMethodForwardersForJavaDefaultMethods(): Unit = { + import forwarderTestUtils._ + val j1 = ("interface J1 { int f(); }", "J1.java") + val j2 = ("interface J2 { default int f() { return 1; } }", "J2.java") + val j3 = ("interface J3 extends J1 { default int f() { return 2; } }", "J3.java") + val j4 = ("interface J4 extends J2 { default int f() { return 3; } }", "J4.java") + val code = + """trait T1 extends J2 { override def f = 4 } + |trait T2 { self: J2 => override def f = 5 } + | + |class K1 extends J2 + |class K2 extends J1 with J2 + |class K3 extends J2 with J1 + | + |class K4 extends J3 + |class K5 extends J3 with J1 + |class K6 extends J1 with J3 + | + |class K7 extends J4 + |class K8 extends J4 with J2 + |class K9 extends J2 with J4 + | + |class K10 extends T1 with J2 + |class K11 extends J2 with T1 + | + |class K12 extends J2 with T2 + """.stripMargin + implicit val classes = compileClasses(compiler)(code, List(j1, j2, j3, j4)).map(c => (c.name, c)).toMap + + val noForwarder = List('K1, 'K2, 'K3, 'K4, 'K5, 'K6, 'K7, 'K8, 'K9, 'K10, 'K11) + for (c <- noForwarder) assertEquals(findMethods(c, "f"), Nil) + + checkForwarder('K12, "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 9079ca248a..15665fca33 100644 --- a/test/junit/scala/tools/nsc/backend/jvm/opt/InlinerTest.scala +++ b/test/junit/scala/tools/nsc/backend/jvm/opt/InlinerTest.scala @@ -457,19 +457,6 @@ class InlinerTest extends ClearAfterClass { assertNoInvoke(getSingleMethod(c, "t2")) } - @Test - def inlineMixinMethods(): Unit = { - val code = - """trait T { - | @inline final def f = 1 - |} - |class C extends T - """.stripMargin - val List(c, t) = compile(code) - // the static implementation method is inlined into the mixin, so there's no invocation in the mixin - assertNoInvoke(getSingleMethod(c, "f")) - } - @Test def inlineTraitInherited(): Unit = { val code = @@ -545,7 +532,7 @@ class InlinerTest extends ClearAfterClass { val List(c, oMirror, oModule, t) = compile(code, allowMessage = i => {count += 1; i.msg contains warn}) assert(count == 1, count) - assertNoInvoke(getSingleMethod(oModule, "f")) + assertNoInvoke(getSingleMethod(t, "f")) assertNoInvoke(getSingleMethod(c, "t1")) assertNoInvoke(getSingleMethod(c, "t2")) @@ -1497,10 +1484,12 @@ class InlinerTest extends ClearAfterClass { @Test def sd86(): Unit = { val code = - """trait T { @inline def f = 1 } // note that f is not final - |class C extends T + """trait T1 { @inline def f = 999 } + |trait T2 { self: T1 => @inline override def f = 1 } // note that f is not final + |class C extends T1 with T2 """.stripMargin - val List(c, t) = compile(code, allowMessage = _ => true) + val List(c, t1, t2) = compile(code, allowMessage = _ => true) + // the forwarder C.f is inlined, so there's no invocation assertSameSummary(getSingleMethod(c, "f"), List(ICONST_1, IRETURN)) } } diff --git a/test/junit/scala/tools/nsc/backend/jvm/opt/ScalaInlineInfoTest.scala b/test/junit/scala/tools/nsc/backend/jvm/opt/ScalaInlineInfoTest.scala index 4db2657c1b..46b3ad3f8e 100644 --- a/test/junit/scala/tools/nsc/backend/jvm/opt/ScalaInlineInfoTest.scala +++ b/test/junit/scala/tools/nsc/backend/jvm/opt/ScalaInlineInfoTest.scala @@ -113,10 +113,6 @@ class ScalaInlineInfoTest extends ClearAfterClass { val infoC = inlineInfo(c) val expectC = InlineInfo(false, None, Map( "O()LT$O$;" -> MethodInlineInfo(true ,false,false), - "f1()I" -> MethodInlineInfo(false,false,false), - "f3()I" -> MethodInlineInfo(false,false,false), - "f4()Ljava/lang/String;" -> MethodInlineInfo(false,true ,false), - "f5()I" -> MethodInlineInfo(true ,false,false), "f6()I" -> MethodInlineInfo(false,false,false), "x1()I" -> MethodInlineInfo(false,false,false), "T$_setter_$x1_$eq(I)V" -> MethodInlineInfo(false,false,false), -- cgit v1.2.3 From d176f102bb6d0a6467e510583e61f363ff76d75e Mon Sep 17 00:00:00 2001 From: Lukas Rytz Date: Mon, 11 Apr 2016 13:24:46 +0200 Subject: Move test run/origins.scala to pending It tests an internal debugging tool which does not appear to work as intented. If anyone can compile and run that test and get an output that looks like the check file, I'd be interested to know. Origins does not seem to support the kind of stack traces that scalac currently emits. --- test/files/run/origins.check | 6 ------ test/files/run/origins.flags | 1 - test/files/run/origins.scala | 21 --------------------- test/pending/run/origins.check | 6 ++++++ test/pending/run/origins.flags | 1 + test/pending/run/origins.scala | 21 +++++++++++++++++++++ 6 files changed, 28 insertions(+), 28 deletions(-) delete mode 100644 test/files/run/origins.check delete mode 100644 test/files/run/origins.flags delete mode 100644 test/files/run/origins.scala create mode 100644 test/pending/run/origins.check create mode 100644 test/pending/run/origins.flags create mode 100644 test/pending/run/origins.scala diff --git a/test/files/run/origins.check b/test/files/run/origins.check deleted file mode 100644 index b12cb6e38f..0000000000 --- a/test/files/run/origins.check +++ /dev/null @@ -1,6 +0,0 @@ - ->> Origins tag 'boop' logged 65 calls from 3 distinguished sources. - - 50 Test$$anonfun$f3$1.apply(origins.scala:16) - 10 Test$$anonfun$f2$1.apply(origins.scala:15) - 5 Test$$anonfun$f1$1.apply(origins.scala:14) diff --git a/test/files/run/origins.flags b/test/files/run/origins.flags deleted file mode 100644 index 690753d807..0000000000 --- a/test/files/run/origins.flags +++ /dev/null @@ -1 +0,0 @@ --no-specialization -Ydelambdafy:inline \ No newline at end of file diff --git a/test/files/run/origins.scala b/test/files/run/origins.scala deleted file mode 100644 index 6529351d3c..0000000000 --- a/test/files/run/origins.scala +++ /dev/null @@ -1,21 +0,0 @@ -import scala.reflect.internal.util.Origins - -package goxbox { - object Socks { - val origins = Origins("boop") - - def boop(x: Int): Int = origins { 5 } - } -} - -object Test { - import goxbox.Socks.boop - - def f1() = 1 to 5 map boop - def f2() = 1 to 10 map boop - def f3() = 1 to 50 map boop - - def main(args: Array[String]): Unit = { - f1() ; f2() ; f3() - } -} diff --git a/test/pending/run/origins.check b/test/pending/run/origins.check new file mode 100644 index 0000000000..b12cb6e38f --- /dev/null +++ b/test/pending/run/origins.check @@ -0,0 +1,6 @@ + +>> Origins tag 'boop' logged 65 calls from 3 distinguished sources. + + 50 Test$$anonfun$f3$1.apply(origins.scala:16) + 10 Test$$anonfun$f2$1.apply(origins.scala:15) + 5 Test$$anonfun$f1$1.apply(origins.scala:14) diff --git a/test/pending/run/origins.flags b/test/pending/run/origins.flags new file mode 100644 index 0000000000..690753d807 --- /dev/null +++ b/test/pending/run/origins.flags @@ -0,0 +1 @@ +-no-specialization -Ydelambdafy:inline \ No newline at end of file diff --git a/test/pending/run/origins.scala b/test/pending/run/origins.scala new file mode 100644 index 0000000000..6529351d3c --- /dev/null +++ b/test/pending/run/origins.scala @@ -0,0 +1,21 @@ +import scala.reflect.internal.util.Origins + +package goxbox { + object Socks { + val origins = Origins("boop") + + def boop(x: Int): Int = origins { 5 } + } +} + +object Test { + import goxbox.Socks.boop + + def f1() = 1 to 5 map boop + def f2() = 1 to 10 map boop + def f3() = 1 to 50 map boop + + def main(args: Array[String]): Unit = { + f1() ; f2() ; f3() + } +} -- cgit v1.2.3 From 765eb29a769488d56f9ff2e4dde105961dbd55db Mon Sep 17 00:00:00 2001 From: Lukas Rytz Date: Mon, 11 Apr 2016 13:34:17 +0200 Subject: Clean up code gen for method invocations The code was patched many times in the history and became a bit scattered. When emitting a virtual call, the receiver in the bytecode cannot just be the method's owner (the class in which it is declared), because that class may not be accessible at the callsite. Instead we use the type of the receiver. This was basically done to fix - aladdin bug 455 (9954eaf) - SI-1430 (0bea2ab) - basically the same bug, slightly different - SI-4283 (8707c9e) - the same for field reads In this patch we extend the fix to field writes, and clean up the code. This patch basically reverts 6eb55d4b, the fix for SI-4560, which was rather a workaround than a fix. The underlying problem was that in some cases, in a method invocation `foo.bar()`, the method `bar` was not actually a member of `foo.tpe`, causing a NoSuchMethodErrors. The issue was related to trait implementation classes. The idea of the fix was to check, at code-gen time, `foo.tpe.member("bar")`, and if that returns `NoSymbol`, use `barSym.owner`. With the new trait encoding the underlying problem seems to be fixed - all tests still pass (run/t4560.scala and run/t4560b.scala). --- .../tools/nsc/backend/jvm/BCodeBodyBuilder.scala | 357 ++++++++++----------- .../tools/nsc/backend/jvm/BCodeSkelBuilder.scala | 28 +- .../scala/tools/nsc/backend/jvm/GenBCode.scala | 2 +- test/files/run/t2106.check | 2 +- test/junit/scala/issues/BytecodeTest.scala | 134 ++++++++ test/junit/scala/issues/RunTest.scala | 7 + .../jvm/analysis/NullnessAnalyzerTest.scala | 12 +- 7 files changed, 324 insertions(+), 218 deletions(-) diff --git a/src/compiler/scala/tools/nsc/backend/jvm/BCodeBodyBuilder.scala b/src/compiler/scala/tools/nsc/backend/jvm/BCodeBodyBuilder.scala index 6d3d458324..a429cfa0f2 100644 --- a/src/compiler/scala/tools/nsc/backend/jvm/BCodeBodyBuilder.scala +++ b/src/compiler/scala/tools/nsc/backend/jvm/BCodeBodyBuilder.scala @@ -34,14 +34,6 @@ abstract class BCodeBodyBuilder extends BCodeSkelBuilder { * Functionality to build the body of ASM MethodNode, except for `synchronized` and `try` expressions. */ abstract class PlainBodyBuilder(cunit: CompilationUnit) extends PlainSkelBuilder(cunit) { - /* If the selector type has a member with the right name, - * it is the host class; otherwise the symbol's owner. - */ - def findHostClass(selector: Type, sym: Symbol) = selector member sym.name match { - case NoSymbol => debuglog(s"Rejecting $selector as host class for $sym") ; sym.owner - case _ => selector.typeSymbol - } - /* ---------------- helper utils for generating methods and code ---------------- */ def emit(opc: Int) { mnode.visitInsn(opc) } @@ -69,12 +61,14 @@ abstract class BCodeBodyBuilder extends BCodeSkelBuilder { def genStat(tree: Tree) { lineNumber(tree) tree match { - case Assign(lhs @ Select(_, _), rhs) => + case Assign(lhs @ Select(qual, _), rhs) => val isStatic = lhs.symbol.isStaticMember if (!isStatic) { genLoadQualifier(lhs) } genLoad(rhs, symInfoTK(lhs.symbol)) lineNumber(tree) - fieldStore(lhs.symbol) + // receiverClass is used in the bytecode to access the field. using sym.owner may lead to IllegalAccessError, SI-4283 + val receiverClass = qual.tpe.typeSymbol + fieldStore(lhs.symbol, receiverClass) case Assign(lhs, rhs) => val s = lhs.symbol @@ -169,21 +163,13 @@ abstract class BCodeBodyBuilder extends BCodeSkelBuilder { genLoad(args.head, INT) generatedType = k.asArrayBType.componentType bc.aload(elementType) - } - else if (scalaPrimitives.isArraySet(code)) { - args match { - case a1 :: a2 :: Nil => - genLoad(a1, INT) - genLoad(a2) - // the following line should really be here, but because of bugs in erasure - // we pretend we generate whatever type is expected from us. - //generatedType = UNIT - bc.astore(elementType) - case _ => - abort(s"Too many arguments for array set operation: $tree") - } - } - else { + } else if (scalaPrimitives.isArraySet(code)) { + val List(a1, a2) = args + genLoad(a1, INT) + genLoad(a2) + generatedType = UNIT + bc.astore(elementType) + } else { generatedType = INT emit(asm.Opcodes.ARRAYLENGTH) } @@ -338,26 +324,22 @@ abstract class BCodeBodyBuilder extends BCodeSkelBuilder { assert(tree.symbol.isModule, s"Selection of non-module from empty package: $tree sym: ${tree.symbol} at: ${tree.pos}") genLoadModule(tree) - case Select(qualifier, selector) => + case Select(qualifier, _) => val sym = tree.symbol generatedType = symInfoTK(sym) - val hostClass = findHostClass(qualifier.tpe, sym) - debuglog(s"Host class of $sym with qual $qualifier (${qualifier.tpe}) is $hostClass") val qualSafeToElide = treeInfo isQualifierSafeToElide qualifier - def genLoadQualUnlessElidable() { if (!qualSafeToElide) { genLoadQualifier(tree) } } - + // receiverClass is used in the bytecode to access the field. using sym.owner may lead to IllegalAccessError, SI-4283 + def receiverClass = qualifier.tpe.typeSymbol if (sym.isModule) { genLoadQualUnlessElidable() genLoadModule(tree) - } - else if (sym.isStaticMember) { + } else if (sym.isStaticMember) { genLoadQualUnlessElidable() - fieldLoad(sym, hostClass) - } - else { + fieldLoad(sym, receiverClass) + } else { genLoadQualifier(tree) - fieldLoad(sym, hostClass) + fieldLoad(sym, receiverClass) } case Ident(name) => @@ -410,24 +392,18 @@ abstract class BCodeBodyBuilder extends BCodeSkelBuilder { /* * must-single-thread */ - def fieldLoad( field: Symbol, hostClass: Symbol = null) { - fieldOp(field, isLoad = true, hostClass) - } + def fieldLoad(field: Symbol, hostClass: Symbol): Unit = fieldOp(field, isLoad = true, hostClass) + /* * must-single-thread */ - def fieldStore(field: Symbol, hostClass: Symbol = null) { - fieldOp(field, isLoad = false, hostClass) - } + def fieldStore(field: Symbol, hostClass: Symbol): Unit = fieldOp(field, isLoad = false, hostClass) /* * must-single-thread */ - private def fieldOp(field: Symbol, isLoad: Boolean, hostClass: Symbol) { - // LOAD_FIELD.hostClass , CALL_METHOD.hostClass , and #4283 - val owner = - if (hostClass == null) internalName(field.owner) - else internalName(hostClass) + private def fieldOp(field: Symbol, isLoad: Boolean, hostClass: Symbol): Unit = { + val owner = internalName(if (hostClass == null) field.owner else hostClass) val fieldJName = field.javaSimpleName.toString val fieldDescr = symInfoTK(field).descriptor val isStatic = field.isStaticMember @@ -435,7 +411,6 @@ abstract class BCodeBodyBuilder extends BCodeSkelBuilder { if (isLoad) { if (isStatic) asm.Opcodes.GETSTATIC else asm.Opcodes.GETFIELD } else { if (isStatic) asm.Opcodes.PUTSTATIC else asm.Opcodes.PUTFIELD } mnode.visitFieldInsn(opc, owner, fieldJName, fieldDescr) - } // ---------------- emitting constant values ---------------- @@ -532,21 +507,6 @@ abstract class BCodeBodyBuilder extends BCodeSkelBuilder { var generatedType = expectedType lineNumber(app) - def genSuperApply(hostClass: Symbol, fun: Symbol, args: List[Tree]) = { - // 'super' call: Note: since constructors are supposed to - // return an instance of what they construct, we have to take - // special care. On JVM they are 'void', and Scala forbids (syntactically) - // to call super constructors explicitly and/or use their 'returned' value. - // therefore, we can ignore this fact, and generate code that leaves nothing - // on the stack (contrary to what the type in the AST says). - - val invokeStyle = InvokeStyle.Super - mnode.visitVarInsn(asm.Opcodes.ALOAD, 0) - genLoadArguments(args, paramTKs(app)) - genCallMethod(fun, invokeStyle, app.pos, hostClass) - generatedType = methodBTypeFromSymbol(fun).returnType - } - app match { case Apply(TypeApply(fun, targs), _) => @@ -594,19 +554,33 @@ abstract class BCodeBodyBuilder extends BCodeSkelBuilder { generatedType = genTypeApply() - case Apply(fun @ Select(Super(qual, mix), _), args) => - val hostClass = qual.symbol.parentSymbols.filter(_.name == mix) match { - case Nil => - // We get here for trees created by SuperSelect which use tpnme.EMPTY as the super qualifier - // Subsequent code uses the owner of fun.symbol to target the call. - null - case parent :: Nil=> - parent - case parents => - devWarning("ambiguous parent class qualifier: " + qual.symbol.parentSymbols) - null + case Apply(fun @ Select(Super(_, _), _), args) => + def initModule() { + // we initialize the MODULE$ field immediately after the super ctor + if (!isModuleInitialized && + jMethodName == INSTANCE_CONSTRUCTOR_NAME && + fun.symbol.javaSimpleName.toString == INSTANCE_CONSTRUCTOR_NAME && + isStaticModuleClass(claszSymbol)) { + isModuleInitialized = true + mnode.visitVarInsn(asm.Opcodes.ALOAD, 0) + mnode.visitFieldInsn( + asm.Opcodes.PUTSTATIC, + thisBType.internalName, + strMODULE_INSTANCE_FIELD, + thisBType.descriptor + ) + } } - genSuperApply(hostClass, fun.symbol, args) + // 'super' call: Note: since constructors are supposed to + // return an instance of what they construct, we have to take + // special care. On JVM they are 'void', and Scala forbids (syntactically) + // to call super constructors explicitly and/or use their 'returned' value. + // therefore, we can ignore this fact, and generate code that leaves nothing + // on the stack (contrary to what the type in the AST says). + mnode.visitVarInsn(asm.Opcodes.ALOAD, 0) + genLoadArguments(args, paramTKs(app)) + generatedType = genCallMethod(fun.symbol, InvokeStyle.Super, app.pos) + initModule() // 'new' constructor call: Note: since constructors are // thought to return an instance of what they construct, @@ -675,80 +649,73 @@ abstract class BCodeBodyBuilder extends BCodeSkelBuilder { case app @ Apply(fun, args) => val sym = fun.symbol - if (sym.isLabel) { // jump to a label + if (sym.isLabel) { // jump to a label genLoadLabelArguments(args, labelDef(sym), app.pos) bc goTo programPoint(sym) } else if (isPrimitive(sym)) { // primitive method call generatedType = genPrimitiveOp(app, expectedType) - } else { // normal method call - - def genNormalMethodCall() { - - val invokeStyle = - if (sym.isStaticMember) InvokeStyle.Static - else if (sym.isPrivate || sym.isClassConstructor) InvokeStyle.Special - else InvokeStyle.Virtual - - if (invokeStyle.hasInstance) { - genLoadQualifier(fun) + } else { // normal method call + val invokeStyle = + if (sym.isStaticMember) InvokeStyle.Static + else if (sym.isPrivate || sym.isClassConstructor) InvokeStyle.Special + else InvokeStyle.Virtual + + if (invokeStyle.hasInstance) genLoadQualifier(fun) + genLoadArguments(args, paramTKs(app)) + + val Select(qual, _) = fun // fun is a Select, also checked in genLoadQualifier + if (sym == definitions.Array_clone) { + // Special-case Array.clone, introduced in 36ef60e. The goal is to generate this call + // as "[I.clone" instead of "java/lang/Object.clone". This is consistent with javac. + // Arrays have a public method `clone` (jls 10.7). + // + // The JVMS is not explicit about this, but that receiver type can be an array type + // descriptor (instead of a class internal name): + // invokevirtual #2; //Method "[I".clone:()Ljava/lang/Object + // + // Note that using `Object.clone()` would work as well, but only because the JVM + // relaxes protected access specifically if the receiver is an array: + // http://hg.openjdk.java.net/jdk8/jdk8/hotspot/file/87ee5ee27509/src/share/vm/interpreter/linkResolver.cpp#l439 + // Example: `class C { override def clone(): Object = "hi" }` + // Emitting `def f(c: C) = c.clone()` as `Object.clone()` gives a VerifyError. + val target: String = tpeTK(qual).asRefBType.classOrArrayType + val methodBType = methodBTypeFromSymbol(sym) + bc.invokevirtual(target, sym.javaSimpleName.toString, methodBType.descriptor, app.pos) + generatedType = methodBType.returnType + } else { + val receiverClass = if (!invokeStyle.isVirtual) null else { + // receiverClass is used in the bytecode to as the method receiver. using sym.owner + // may lead to IllegalAccessErrors, see 9954eaf / aladdin bug 455. + val qualSym = qual.tpe.typeSymbol + if (qualSym == ArrayClass) { + // For invocations like `Array(1).hashCode` or `.wait()`, use Object as receiver + // in the bytecode. Using the array descriptor (like we do for clone above) seems + // to work as well, but it seems safer not to change this. Javac also uses Object. + // Note that array apply/update/length are handled by isPrimitive (above). + assert(sym.owner == ObjectClass, s"unexpected array call: ${show(app)}") + ObjectClass + } else qualSym } - genLoadArguments(args, paramTKs(app)) - - // In "a couple cases", squirrel away a extra information (hostClass, targetTypeKind). TODO Document what "in a couple cases" refers to. - var hostClass: Symbol = null - var targetTypeKind: BType = null - fun match { - case Select(qual, _) => - val qualSym = findHostClass(qual.tpe, sym) - if (qualSym == ArrayClass) { - targetTypeKind = tpeTK(qual) - log(s"Stored target type kind for ${sym.fullName} as $targetTypeKind") - } - else { - hostClass = qualSym - if (qual.tpe.typeSymbol != qualSym) { - log(s"Precisified host class for $sym from ${qual.tpe.typeSymbol.fullName} to ${qualSym.fullName}") - } - } - - case _ => - } - if ((targetTypeKind != null) && (sym == definitions.Array_clone) && invokeStyle.isVirtual) { - // An invokevirtual points to a CONSTANT_Methodref_info which in turn points to a - // CONSTANT_Class_info of the receiver type. - // The JVMS is not explicit about this, but that receiver type may be an array type - // descriptor (instead of a class internal name): - // invokevirtual #2; //Method "[I".clone:()Ljava/lang/Object - val target: String = targetTypeKind.asRefBType.classOrArrayType - bc.invokevirtual(target, "clone", "()Ljava/lang/Object;", app.pos) - } - else { - genCallMethod(sym, invokeStyle, app.pos, hostClass) - // Check if the Apply tree has an InlineAnnotatedAttachment, added by the typer - // for callsites marked `f(): @inline/noinline`. For nullary calls, the attachment - // is on the Select node (not on the Apply node added by UnCurry). - def checkInlineAnnotated(t: Tree): Unit = { - if (t.hasAttachment[InlineAnnotatedAttachment]) lastInsn match { - case m: MethodInsnNode => - if (app.hasAttachment[NoInlineCallsiteAttachment.type]) noInlineAnnotatedCallsites += m - else inlineAnnotatedCallsites += m - case _ => - } else t match { - case Apply(fun, _) => checkInlineAnnotated(fun) - case _ => - } + generatedType = genCallMethod(sym, invokeStyle, app.pos, receiverClass) + + // Check if the Apply tree has an InlineAnnotatedAttachment, added by the typer + // for callsites marked `f(): @inline/noinline`. For nullary calls, the attachment + // is on the Select node (not on the Apply node added by UnCurry). + def recordInlineAnnotated(t: Tree): Unit = { + if (t.hasAttachment[InlineAnnotatedAttachment]) lastInsn match { + case m: MethodInsnNode => + if (app.hasAttachment[NoInlineCallsiteAttachment.type]) noInlineAnnotatedCallsites += m + else inlineAnnotatedCallsites += m + case _ => + } else t match { + case Apply(fun, _) => recordInlineAnnotated(fun) + case _ => } - checkInlineAnnotated(app) } - - } // end of genNormalMethodCall() - - genNormalMethodCall() - - generatedType = methodBTypeFromSymbol(sym).returnType + recordInlineAnnotated(app) + } } - } generatedType @@ -1026,11 +993,11 @@ abstract class BCodeBodyBuilder extends BCodeSkelBuilder { def genStringConcat(tree: Tree): BType = { lineNumber(tree) liftStringConcat(tree) match { - // Optimization for expressions of the form "" + x. We can avoid the StringBuilder. case List(Literal(Constant("")), arg) => genLoad(arg, ObjectRef) genCallMethod(String_valueOf, InvokeStyle.Static, arg.pos) + case concatenations => bc.genStartConcat(tree.pos) for (elem <- concatenations) { @@ -1047,74 +1014,75 @@ abstract class BCodeBodyBuilder extends BCodeSkelBuilder { bc.genConcat(elemType, loadedElem.pos) } bc.genEndConcat(tree.pos) - } - StringRef } - def genCallMethod(method: Symbol, style: InvokeStyle, pos: Position, hostClass0: Symbol = null) { - - val siteSymbol = claszSymbol - val hostSymbol = if (hostClass0 == null) method.owner else hostClass0 + /** + * Generate a method invocation. If `specificReceiver != null`, it is used as receiver in the + * invocation instruction, otherwise `method.owner`. A specific receiver class is needed to + * prevent an IllegalAccessError, (aladdin bug 455). + */ + def genCallMethod(method: Symbol, style: InvokeStyle, pos: Position, specificReceiver: Symbol = null): BType = { val methodOwner = method.owner - // info calls so that types are up to date; erasure may add lateINTERFACE to traits - hostSymbol.info ; methodOwner.info - - def needsInterfaceCall(sym: Symbol) = ( - sym.isTraitOrInterface - || sym.isJavaDefined && sym.isNonBottomSubClass(definitions.ClassfileAnnotationClass) - ) + // the class used in the invocation's method descriptor in the classfile + val receiverClass = { + if (specificReceiver != null) + assert(style.isVirtual || specificReceiver == methodOwner, s"specificReceiver can only be specified for virtual calls. $method - $specificReceiver") + + val useSpecificReceiver = specificReceiver != null && !specificReceiver.isBottomClass + val receiver = if (useSpecificReceiver) specificReceiver else methodOwner + + // workaround for a JVM bug: https://bugs.openjdk.java.net/browse/JDK-8154587 + // when an interface method overrides a member of Object (note that all interfaces implicitly + // have superclass Object), the receiver needs to be the interface declaring the override (and + // not a sub-interface that inherits it). example: + // trait T { override def clone(): Object = "" } + // trait U extends T + // class C extends U + // class D { def f(u: U) = u.clone() } + // The invocation `u.clone()` needs `T` as a receiver: + // - using Object is illegal, as Object.clone is protected + // - using U results in a `NoSuchMethodError: U.clone. This is the JVM bug. + // Note that a mixin forwarder is generated, so the correct method is executed in the end: + // class C { override def clone(): Object = super[T].clone() } + val isTraitMethodOverridingObjectMember = { + receiver != methodOwner && // fast path - the boolean is used to pick either of these two, if they are the same it does not matter + style.isVirtual && + receiver.isTraitOrInterface && + ObjectTpe.decl(method.name).exists && // fast path - compute overrideChain on the next line only if necessary + method.overrideChain.last.owner == ObjectClass + } + if (isTraitMethodOverridingObjectMember) methodOwner else receiver + } - val isTraitCallToObjectMethod = - hostSymbol != methodOwner && methodOwner.isTraitOrInterface && ObjectTpe.decl(method.name) != NoSymbol && method.overrideChain.last.owner == ObjectClass + receiverClass.info // ensure types the type is up to date; erasure may add lateINTERFACE to traits + val receiverName = internalName(receiverClass) - // whether to reference the type of the receiver or - // the type of the method owner - val useMethodOwner = (( - !style.isVirtual - || hostSymbol.isBottomClass - || methodOwner == definitions.ObjectClass - ) && !(style.isSuper && hostSymbol != null)) || isTraitCallToObjectMethod - val receiver = if (useMethodOwner) methodOwner else hostSymbol - val jowner = internalName(receiver) + // super calls are only allowed to direct parents + if (style.isSuper && receiverClass.isTraitOrInterface && !cnode.interfaces.contains(receiverName)) + cnode.interfaces.add(receiverName) - if (style.isSuper && (isTraitCallToObjectMethod || receiver.isTraitOrInterface) && !cnode.interfaces.contains(jowner)) - cnode.interfaces.add(jowner) + def needsInterfaceCall(sym: Symbol) = { + sym.isTraitOrInterface || + sym.isJavaDefined && sym.isNonBottomSubClass(definitions.ClassfileAnnotationClass) + } val jname = method.javaSimpleName.toString val bmType = methodBTypeFromSymbol(method) val mdescr = bmType.descriptor - def initModule() { - // we initialize the MODULE$ field immediately after the super ctor - if (!isModuleInitialized && - jMethodName == INSTANCE_CONSTRUCTOR_NAME && - jname == INSTANCE_CONSTRUCTOR_NAME && - isStaticModuleClass(siteSymbol)) { - isModuleInitialized = true - mnode.visitVarInsn(asm.Opcodes.ALOAD, 0) - mnode.visitFieldInsn( - asm.Opcodes.PUTSTATIC, - thisName, - strMODULE_INSTANCE_FIELD, - "L" + thisName + ";" - ) - } - } - - if (style.isStatic) { bc.invokestatic (jowner, jname, mdescr, pos) } - else if (style.isSpecial) { bc.invokespecial (jowner, jname, mdescr, pos) } - else if (style.isVirtual) { - if (needsInterfaceCall(receiver)) { bc.invokeinterface(jowner, jname, mdescr, pos) } - else { bc.invokevirtual (jowner, jname, mdescr, pos) } - } - else { - assert(style.isSuper, s"An unknown InvokeStyle: $style") - bc.invokespecial(jowner, jname, mdescr, pos) - initModule() + import InvokeStyle._ + style match { + case Static => bc.invokestatic (receiverName, jname, mdescr, pos) + case Special => bc.invokespecial (receiverName, jname, mdescr, pos) + case Virtual => + if (needsInterfaceCall(receiverClass)) bc.invokeinterface(receiverName, jname, mdescr, pos) + else bc.invokevirtual (receiverName, jname, mdescr, pos) + case Super => bc.invokespecial (receiverName, jname, mdescr, pos) } + bmType.returnType } // end of genCallMethod() /* Generate the scala ## method. */ @@ -1122,7 +1090,6 @@ abstract class BCodeBodyBuilder extends BCodeSkelBuilder { genLoadModule(ScalaRunTimeModule) // TODO why load ScalaRunTimeModule if ## has InvokeStyle of Static(false) ? genLoad(tree, ObjectRef) genCallMethod(hashMethodSym, InvokeStyle.Static, applyPos) - INT } /* diff --git a/src/compiler/scala/tools/nsc/backend/jvm/BCodeSkelBuilder.scala b/src/compiler/scala/tools/nsc/backend/jvm/BCodeSkelBuilder.scala index 4c41cfc380..47884da560 100644 --- a/src/compiler/scala/tools/nsc/backend/jvm/BCodeSkelBuilder.scala +++ b/src/compiler/scala/tools/nsc/backend/jvm/BCodeSkelBuilder.scala @@ -59,7 +59,7 @@ abstract class BCodeSkelBuilder extends BCodeHelpers { // current class var cnode: asm.tree.ClassNode = null - var thisName: String = null // the internal name of the class being emitted + var thisBType: ClassBType = null var claszSymbol: Symbol = null var isCZParcelable = false @@ -91,9 +91,7 @@ abstract class BCodeSkelBuilder extends BCodeHelpers { isCZParcelable = isAndroidParcelableClass(claszSymbol) isCZStaticModule = isStaticModuleClass(claszSymbol) isCZRemote = isRemote(claszSymbol) - thisName = internalName(claszSymbol) - - val classBType = classBTypeFromSymbol(claszSymbol) + thisBType = classBTypeFromSymbol(claszSymbol) cnode = new asm.tree.ClassNode() @@ -123,7 +121,7 @@ abstract class BCodeSkelBuilder extends BCodeHelpers { if (shouldAddLambdaDeserialize) backendUtils.addLambdaDeserialize(cnode) - cnode.visitAttribute(classBType.inlineInfoAttribute.get) + cnode.visitAttribute(thisBType.inlineInfoAttribute.get) if (AsmUtils.traceClassEnabled && cnode.name.contains(AsmUtils.traceClassPattern)) AsmUtils.traceClass(cnode) @@ -144,7 +142,7 @@ abstract class BCodeSkelBuilder extends BCodeHelpers { val thisSignature = getGenericSignature(claszSymbol, claszSymbol.owner) cnode.visit(classfileVersion, flags, - thisName, thisSignature, + thisBType.internalName, thisSignature, superClass, interfaceNames.toArray) if (emitSource) { @@ -157,7 +155,7 @@ abstract class BCodeSkelBuilder extends BCodeHelpers { case _ => () } - val ssa = getAnnotPickle(thisName, claszSymbol) + val ssa = getAnnotPickle(thisBType.internalName, claszSymbol) cnode.visitAttribute(if (ssa.isDefined) pickleMarkerLocal else pickleMarkerForeign) emitAnnotations(cnode, claszSymbol.annotations ++ ssa) @@ -178,7 +176,7 @@ abstract class BCodeSkelBuilder extends BCodeHelpers { } if (isCandidateForForwarders) { log(s"Adding static forwarders from '$claszSymbol' to implementations in '$lmoc'") - addForwarders(isRemote(claszSymbol), cnode, thisName, lmoc.moduleClass) + addForwarders(isRemote(claszSymbol), cnode, thisBType.internalName, lmoc.moduleClass) } } } @@ -196,7 +194,7 @@ abstract class BCodeSkelBuilder extends BCodeHelpers { val fv = cnode.visitField(GenBCode.PublicStaticFinal, // TODO confirm whether we really don't want ACC_SYNTHETIC nor ACC_DEPRECATED strMODULE_INSTANCE_FIELD, - "L" + thisName + ";", + thisBType.descriptor, null, // no java-generic-signature null // no initial value ) @@ -220,11 +218,11 @@ abstract class BCodeSkelBuilder extends BCodeHelpers { /* "legacy static initialization" */ if (isCZStaticModule) { - clinit.visitTypeInsn(asm.Opcodes.NEW, thisName) + clinit.visitTypeInsn(asm.Opcodes.NEW, thisBType.internalName) clinit.visitMethodInsn(asm.Opcodes.INVOKESPECIAL, - thisName, INSTANCE_CONSTRUCTOR_NAME, "()V", false) + thisBType.internalName, INSTANCE_CONSTRUCTOR_NAME, "()V", false) } - if (isCZParcelable) { legacyAddCreatorCode(clinit, cnode, thisName) } + if (isCZParcelable) { legacyAddCreatorCode(clinit, cnode, thisBType.internalName) } clinit.visitInsn(asm.Opcodes.RETURN) clinit.visitMaxs(0, 0) // just to follow protocol, dummy arguments @@ -604,7 +602,7 @@ abstract class BCodeSkelBuilder extends BCodeHelpers { if (!hasStaticBitSet) { mnode.visitLocalVariable( "this", - "L" + thisName + ";", + thisBType.descriptor, null, veryFirstProgramPoint, onePastLastProgramPoint, @@ -686,8 +684,8 @@ abstract class BCodeSkelBuilder extends BCodeHelpers { val jname = callee.javaSimpleName.toString val jtype = methodBTypeFromSymbol(callee).descriptor insnParcA = new asm.tree.MethodInsnNode(asm.Opcodes.INVOKESTATIC, jowner, jname, jtype, false) - // PUTSTATIC `thisName`.CREATOR; - insnParcB = new asm.tree.FieldInsnNode(asm.Opcodes.PUTSTATIC, thisName, "CREATOR", andrFieldDescr) + // PUTSTATIC `thisBType.internalName`.CREATOR; + insnParcB = new asm.tree.FieldInsnNode(asm.Opcodes.PUTSTATIC, thisBType.internalName, "CREATOR", andrFieldDescr) } // insert a few instructions for initialization before each return instruction diff --git a/src/compiler/scala/tools/nsc/backend/jvm/GenBCode.scala b/src/compiler/scala/tools/nsc/backend/jvm/GenBCode.scala index b0ec37db97..65bd62a413 100644 --- a/src/compiler/scala/tools/nsc/backend/jvm/GenBCode.scala +++ b/src/compiler/scala/tools/nsc/backend/jvm/GenBCode.scala @@ -187,7 +187,7 @@ abstract class GenBCode extends BCodeSyncAndTry { // -------------- "plain" class -------------- val pcb = new PlainClassBuilder(cunit) pcb.genPlainClass(cd) - val outF = if (needsOutFolder) getOutFolder(claszSymbol, pcb.thisName, cunit) else null + val outF = if (needsOutFolder) getOutFolder(claszSymbol, pcb.thisBType.internalName, cunit) else null val plainC = pcb.cnode // -------------- bean info class, if needed -------------- diff --git a/test/files/run/t2106.check b/test/files/run/t2106.check index b19165824b..c8ebe575f0 100644 --- a/test/files/run/t2106.check +++ b/test/files/run/t2106.check @@ -1,5 +1,5 @@ t2106.scala:7: warning: A::foo()Ljava/lang/Object; is annotated @inline but could not be inlined: -The callee A::foo()Ljava/lang/Object; contains the instruction INVOKEVIRTUAL java/lang/Object.clone ()Ljava/lang/Object; +The callee A::foo()Ljava/lang/Object; contains the instruction INVOKEVIRTUAL A.clone ()Ljava/lang/Object; that would cause an IllegalAccessError when inlined into class Test$. def main(args: Array[String]): Unit = x.foo ^ diff --git a/test/junit/scala/issues/BytecodeTest.scala b/test/junit/scala/issues/BytecodeTest.scala index 08b984a622..cf5c7f9ec3 100644 --- a/test/junit/scala/issues/BytecodeTest.scala +++ b/test/junit/scala/issues/BytecodeTest.scala @@ -10,6 +10,7 @@ import scala.tools.nsc.backend.jvm.CodeGenTools._ import org.junit.Assert._ import scala.collection.JavaConverters._ +import scala.tools.asm.Opcodes import scala.tools.asm.tree.ClassNode import scala.tools.partest.ASMConverters._ import scala.tools.testing.ClearAfterClass @@ -341,4 +342,137 @@ class BytecodeTest extends ClearAfterClass { checkForwarder('K12, "T2") } + + @Test + def invocationReceivers(): Unit = { + val List(c1, c2, t, u) = compileClasses(compiler)(invocationReceiversTestCode.definitions("Object")) + // mixin forwarder in C1 + assertSameCode(getSingleMethod(c1, "clone"), List(VarOp(ALOAD, 0), Invoke(INVOKESPECIAL, "T", "clone", "()Ljava/lang/Object;", false), Op(ARETURN))) + assertInvoke(getSingleMethod(c1, "f1"), "T", "clone") + assertInvoke(getSingleMethod(c1, "f2"), "T", "clone") + assertInvoke(getSingleMethod(c1, "f3"), "C1", "clone") + assertInvoke(getSingleMethod(c2, "f1"), "T", "clone") + assertInvoke(getSingleMethod(c2, "f2"), "T", "clone") + assertInvoke(getSingleMethod(c2, "f3"), "C1", "clone") + + val List(c1b, c2b, tb, ub) = compileClasses(compiler)(invocationReceiversTestCode.definitions("String")) + def ms(c: ClassNode, n: String) = c.methods.asScala.toList.filter(_.name == n) + assert(ms(tb, "clone").length == 1) + assert(ms(ub, "clone").isEmpty) + val List(c1Clone) = ms(c1b, "clone") + assertEquals(c1Clone.desc, "()Ljava/lang/Object;") + assert((c1Clone.access | Opcodes.ACC_BRIDGE) != 0) + assertSameCode(convertMethod(c1Clone), List(VarOp(ALOAD, 0), Invoke(INVOKEVIRTUAL, "C1", "clone", "()Ljava/lang/String;", false), Op(ARETURN))) + + def iv(m: Method) = getSingleMethod(c1b, "f1").instructions.collect({case i: Invoke => i}) + assertSameCode(iv(getSingleMethod(c1b, "f1")), List(Invoke(INVOKEINTERFACE, "T", "clone", "()Ljava/lang/String;", true))) + assertSameCode(iv(getSingleMethod(c1b, "f2")), List(Invoke(INVOKEINTERFACE, "T", "clone", "()Ljava/lang/String;", true))) + // invokeinterface T.clone in C1 is OK here because it is not an override of Object.clone (different siganture) + assertSameCode(iv(getSingleMethod(c1b, "f3")), List(Invoke(INVOKEINTERFACE, "T", "clone", "()Ljava/lang/String;", true))) + } + + @Test + def invocationReceiversProtected(): Unit = { + // http://lrytz.github.io/scala-aladdin-bugtracker/displayItem.do%3Fid=455.html / 9954eaf + // also https://issues.scala-lang.org/browse/SI-1430 / 0bea2ab (same but with interfaces) + val aC = + """package a; + |/*package private*/ abstract class A { + | public int f() { return 1; } + | public int t; + |} + """.stripMargin + val bC = + """package a; + |public class B extends A { } + """.stripMargin + val iC = + """package a; + |/*package private*/ interface I { int f(); } + """.stripMargin + val jC = + """package a; + |public interface J extends I { } + """.stripMargin + val cC = + """package b + |class C { + | def f1(b: a.B) = b.f + | def f2(b: a.B) = { b.t = b.t + 1 } + | def f3(j: a.J) = j.f + |} + """.stripMargin + val List(c) = compileClasses(compiler)(cC, javaCode = List((aC, "A.java"), (bC, "B.java"), (iC, "I.java"), (jC, "J.java"))) + assertInvoke(getSingleMethod(c, "f1"), "a/B", "f") // receiver needs to be B (A is not accessible in class C, package b) + println(getSingleMethod(c, "f2").instructions.stringLines) + assertInvoke(getSingleMethod(c, "f3"), "a/J", "f") // receiver needs to be J + } + + @Test + def specialInvocationReceivers(): Unit = { + val code = + """class C { + | def f1(a: Array[String]) = a.clone() + | def f2(a: Array[Int]) = a.hashCode() + | def f3(n: Nothing) = n.hashCode() + | def f4(n: Null) = n.toString() + | + |} + """.stripMargin + val List(c) = compileClasses(compiler)(code) + assertInvoke(getSingleMethod(c, "f1"), "[Ljava/lang/String;", "clone") // array descriptor as receiver + assertInvoke(getSingleMethod(c, "f2"), "java/lang/Object", "hashCode") // object receiver + assertInvoke(getSingleMethod(c, "f3"), "java/lang/Object", "hashCode") + assertInvoke(getSingleMethod(c, "f4"), "java/lang/Object", "toString") + } +} + +object invocationReceiversTestCode { + // if cloneType is more specific than Object (e.g., String), a bridge method is generated. + def definitions(cloneType: String) = + s"""trait T { override def clone(): $cloneType = "hi" } + |trait U extends T + |class C1 extends U with Cloneable { + | // The comments below are true when $cloneType is Object. + | // C1 gets a forwarder for clone that invokes T.clone. this is needed because JVM method + | // resolution always prefers class members, so it would resolve to Object.clone, even if + | // C1 is a subtype of the interface T which has an overriding default method for clone. + | + | // invokeinterface T.clone + | def f1 = (this: T).clone() + | + | // cannot invokeinterface U.clone (NoSuchMethodError). Object.clone would work here, but + | // not in the example in C2 (illegal access to protected). T.clone works in all cases and + | // resolves correctly. + | def f2 = (this: U).clone() + | + | // invokevirtual C1.clone() + | def f3 = (this: C1).clone() + |} + | + |class C2 { + | def f1(t: T) = t.clone() // invokeinterface T.clone + | def f2(t: U) = t.clone() // invokeinterface T.clone -- Object.clone would be illegal (protected, explained in C1) + | def f3(t: C1) = t.clone() // invokevirtual C1.clone -- Object.clone would be illegal + |} + """.stripMargin + + val runCode = + """ + |val r = new StringBuffer() + |val c1 = new C1 + |r.append(c1.f1) + |r.append(c1.f2) + |r.append(c1.f3) + |val t = new T { } + |val u = new U { } + |val c2 = new C2 + |r.append(c2.f1(t)) + |r.append(c2.f1(u)) + |r.append(c2.f1(c1)) + |r.append(c2.f2(u)) + |r.append(c2.f2(c1)) + |r.append(c2.f3(c1)) + |r.toString + """.stripMargin } diff --git a/test/junit/scala/issues/RunTest.scala b/test/junit/scala/issues/RunTest.scala index 2bc8008222..781f2ef343 100644 --- a/test/junit/scala/issues/RunTest.scala +++ b/test/junit/scala/issues/RunTest.scala @@ -140,4 +140,11 @@ class RunTest extends ClearAfterClass { val i = Integer.TYPE assertEquals(run[List[Class[_]]](code), List(i, i)) } + + @Test + def invocationReceivers(): Unit = { + import invocationReceiversTestCode._ + assertEquals(run[String](definitions("Object") + runCode), "hi" * 9) + assertEquals(run[String](definitions("String") + runCode), "hi" * 9) // bridge method for clone generated + } } diff --git a/test/junit/scala/tools/nsc/backend/jvm/analysis/NullnessAnalyzerTest.scala b/test/junit/scala/tools/nsc/backend/jvm/analysis/NullnessAnalyzerTest.scala index 78dbab82f4..ab05c15e85 100644 --- a/test/junit/scala/tools/nsc/backend/jvm/analysis/NullnessAnalyzerTest.scala +++ b/test/junit/scala/tools/nsc/backend/jvm/analysis/NullnessAnalyzerTest.scala @@ -68,12 +68,12 @@ class NullnessAnalyzerTest extends ClearAfterClass { // So in the frame for `ALOAD 0`, the stack is still empty. val res = - """ L0: 0: NotNull - | LINENUMBER 1 L0: 0: NotNull - | ALOAD 0: 0: NotNull - |INVOKEVIRTUAL java/lang/Object.toString ()Ljava/lang/String;: 0: NotNull, 1: NotNull - | ARETURN: 0: NotNull, 1: Unknown1 - | L0: null""".stripMargin + """ L0: 0: NotNull + | LINENUMBER 1 L0: 0: NotNull + | ALOAD 0: 0: NotNull + |INVOKEVIRTUAL C.toString ()Ljava/lang/String;: 0: NotNull, 1: NotNull + | ARETURN: 0: NotNull, 1: Unknown1 + | L0: null""".stripMargin // println(showAllNullnessFrames(newNullnessAnalyzer(m), m)) assertEquals(showAllNullnessFrames(newNullnessAnalyzer(m), m), res) } -- cgit v1.2.3 From 3fca034c51dd159ff077fdde7e3146b1e41cc925 Mon Sep 17 00:00:00 2001 From: Lukas Rytz Date: Tue, 12 Apr 2016 10:59:58 +0200 Subject: Ensure ClassBTypes constructed from symbol and classfile are identical A super call (invokespecial) to a default method T.m is only allowed if the interface T is a direct parent of the class. Super calls are introduced for example in Mixin when generating forwarder methods: trait T { override def clone(): Object = "hi" } trait U extends T class C extends U The class C gets a forwarder that invokes T.clone(). During code generation the interface T is added as direct parent to class C. Note that T is not a (direct) parent in the frontend type of class C. This commit stores interfaces that are added to a class during code generation in the InlineInfo classfile attribute. This allows filtering the interface list when constructing a ClassBType from a classfile. --- .../tools/nsc/backend/jvm/BCodeBodyBuilder.scala | 4 +- .../tools/nsc/backend/jvm/BCodeSkelBuilder.scala | 1 - .../scala/tools/nsc/backend/jvm/BTypes.scala | 30 +++++++++++++-- .../nsc/backend/jvm/opt/InlineInfoAttribute.scala | 44 ++++++++++++++-------- .../backend/jvm/opt/BTypesFromClassfileTest.scala | 14 +------ 5 files changed, 59 insertions(+), 34 deletions(-) diff --git a/src/compiler/scala/tools/nsc/backend/jvm/BCodeBodyBuilder.scala b/src/compiler/scala/tools/nsc/backend/jvm/BCodeBodyBuilder.scala index a429cfa0f2..a2c521eb91 100644 --- a/src/compiler/scala/tools/nsc/backend/jvm/BCodeBodyBuilder.scala +++ b/src/compiler/scala/tools/nsc/backend/jvm/BCodeBodyBuilder.scala @@ -1060,8 +1060,10 @@ abstract class BCodeBodyBuilder extends BCodeSkelBuilder { val receiverName = internalName(receiverClass) // super calls are only allowed to direct parents - if (style.isSuper && receiverClass.isTraitOrInterface && !cnode.interfaces.contains(receiverName)) + if (style.isSuper && receiverClass.isTraitOrInterface && !cnode.interfaces.contains(receiverName)) { + thisBType.info.get.inlineInfo.lateInterfaces += receiverName cnode.interfaces.add(receiverName) + } def needsInterfaceCall(sym: Symbol) = { sym.isTraitOrInterface || diff --git a/src/compiler/scala/tools/nsc/backend/jvm/BCodeSkelBuilder.scala b/src/compiler/scala/tools/nsc/backend/jvm/BCodeSkelBuilder.scala index 47884da560..f190c1f2de 100644 --- a/src/compiler/scala/tools/nsc/backend/jvm/BCodeSkelBuilder.scala +++ b/src/compiler/scala/tools/nsc/backend/jvm/BCodeSkelBuilder.scala @@ -112,7 +112,6 @@ abstract class BCodeSkelBuilder extends BCodeHelpers { gen(cd.impl) - val shouldAddLambdaDeserialize = ( settings.target.value == "jvm-1.8" && settings.Ydelambdafy.value == "method" diff --git a/src/compiler/scala/tools/nsc/backend/jvm/BTypes.scala b/src/compiler/scala/tools/nsc/backend/jvm/BTypes.scala index f6bccca050..49353afef4 100644 --- a/src/compiler/scala/tools/nsc/backend/jvm/BTypes.scala +++ b/src/compiler/scala/tools/nsc/backend/jvm/BTypes.scala @@ -7,7 +7,7 @@ package scala.tools.nsc package backend.jvm import scala.annotation.switch -import scala.collection.{mutable, concurrent} +import scala.collection.{concurrent, mutable} import scala.collection.concurrent.TrieMap import scala.reflect.internal.util.Position import scala.tools.asm @@ -18,6 +18,7 @@ import scala.tools.nsc.backend.jvm.BackendReporting._ import scala.tools.nsc.backend.jvm.analysis.BackendUtils import scala.tools.nsc.backend.jvm.opt._ import scala.collection.convert.decorateAsScala._ +import scala.collection.mutable.ListBuffer import scala.tools.nsc.settings.ScalaSettings /** @@ -180,8 +181,6 @@ abstract class BTypes { Some(classBTypeFromParsedClassfile(superName)) } - val interfaces: List[ClassBType] = classNode.interfaces.asScala.map(classBTypeFromParsedClassfile)(collection.breakOut) - val flags = classNode.access /** @@ -226,6 +225,9 @@ abstract class BTypes { val inlineInfo = inlineInfoFromClassfile(classNode) + val classfileInterfaces: List[ClassBType] = classNode.interfaces.asScala.map(classBTypeFromParsedClassfile)(collection.breakOut) + val interfaces = classfileInterfaces.filterNot(i => inlineInfo.lateInterfaces.contains(i.internalName)) + classBType.info = Right(ClassInfo(superClass, interfaces, flags, nestedClasses, nestedInfo, inlineInfo)) classBType } @@ -1144,7 +1146,27 @@ object BTypes { final case class InlineInfo(isEffectivelyFinal: Boolean, sam: Option[String], methodInfos: Map[String, MethodInlineInfo], - warning: Option[ClassInlineInfoWarning]) + warning: Option[ClassInlineInfoWarning]) { + /** + * A super call (invokespecial) to a default method T.m is only allowed if the interface T is + * a direct parent of the class. Super calls are introduced for example in Mixin when generating + * forwarder methods: + * + * trait T { override def clone(): Object = "hi" } + * trait U extends T + * class C extends U + * + * The class C gets a forwarder that invokes T.clone(). During code generation the interface T + * is added as direct parent to class C. Note that T is not a (direct) parent in the frontend + * type of class C. + * + * All interfaces that are added to a class during code generation are added to this buffer and + * stored in the InlineInfo classfile attribute. This ensures that the ClassBTypes for a + * specific class is the same no matter if it's constructed from a Symbol or from a classfile. + * This is tested in BTypesFromClassfileTest. + */ + val lateInterfaces: ListBuffer[InternalName] = ListBuffer.empty + } val EmptyInlineInfo = InlineInfo(false, None, Map.empty, None) diff --git a/src/compiler/scala/tools/nsc/backend/jvm/opt/InlineInfoAttribute.scala b/src/compiler/scala/tools/nsc/backend/jvm/opt/InlineInfoAttribute.scala index 079a9eec9b..79d26b0b4e 100644 --- a/src/compiler/scala/tools/nsc/backend/jvm/opt/InlineInfoAttribute.scala +++ b/src/compiler/scala/tools/nsc/backend/jvm/opt/InlineInfoAttribute.scala @@ -47,11 +47,12 @@ case class InlineInfoAttribute(inlineInfo: InlineInfo) extends Attribute(InlineI result.putByte(InlineInfoAttribute.VERSION) - var finalSelfSam = 0 - if (inlineInfo.isEffectivelyFinal) finalSelfSam |= 1 - // finalSelfSam |= 2 // no longer written - if (inlineInfo.sam.isDefined) finalSelfSam |= 4 - result.putByte(finalSelfSam) + var flags = 0 + if (inlineInfo.isEffectivelyFinal) flags |= 1 + // flags |= 2 // no longer written + if (inlineInfo.sam.isDefined) flags |= 4 + if (inlineInfo.lateInterfaces.nonEmpty) flags |= 8 + result.putByte(flags) for (samNameDesc <- inlineInfo.sam) { val (name, desc) = samNameDesc.span(_ != '(') @@ -78,6 +79,9 @@ case class InlineInfoAttribute(inlineInfo: InlineInfo) extends Attribute(InlineI result.putByte(inlineInfo) } + result.putShort(inlineInfo.lateInterfaces.length) + for (i <- inlineInfo.lateInterfaces) result.putShort(cw.newUTF8(i)) + result } @@ -97,10 +101,11 @@ case class InlineInfoAttribute(inlineInfo: InlineInfo) extends Attribute(InlineI val version = nextByte() if (version == 1) { - val finalSelfSam = nextByte() - val isFinal = (finalSelfSam & 1) != 0 - val hasSelf = (finalSelfSam & 2) != 0 - val hasSam = (finalSelfSam & 4) != 0 + val flags = nextByte() + val isFinal = (flags & 1) != 0 + val hasSelf = (flags & 2) != 0 + val hasSam = (flags & 4) != 0 + val hasLateInterfaces = (flags & 8) != 0 if (hasSelf) nextUTF8() // no longer used @@ -116,14 +121,21 @@ case class InlineInfoAttribute(inlineInfo: InlineInfo) extends Attribute(InlineI val desc = nextUTF8() val inlineInfo = nextByte() - val isFinal = (inlineInfo & 1) != 0 - // val traitMethodWithStaticImplementation = (inlineInfo & 2) != 0 // no longer used - val isInline = (inlineInfo & 4) != 0 - val isNoInline = (inlineInfo & 8) != 0 + val isFinal = (inlineInfo & 1) != 0 + // = (inlineInfo & 2) != 0 // no longer used + val isInline = (inlineInfo & 4) != 0 + val isNoInline = (inlineInfo & 8) != 0 (name + desc, MethodInlineInfo(isFinal, isInline, isNoInline)) }).toMap - InlineInfoAttribute(InlineInfo(isFinal, sam, infos, None)) + val lateInterfaces = if (!hasLateInterfaces) Nil else { + val numLateInterfaces = nextShort() + (0 until numLateInterfaces).map(_ => nextUTF8()) + } + + val info = InlineInfo(isFinal, sam, infos, None) + info.lateInterfaces ++= lateInterfaces + InlineInfoAttribute(info) } else { val msg = UnknownScalaInlineInfoVersion(cr.getClassName, version) InlineInfoAttribute(BTypes.EmptyInlineInfo.copy(warning = Some(msg))) @@ -141,7 +153,7 @@ object InlineInfoAttribute { * is ignored. * * [u1] version - * [u1] isEffectivelyFinal (<< 0), hasTraitImplClassSelfType (<< 1), hasSam (<< 2) + * [u1] isEffectivelyFinal (<< 0), hasTraitImplClassSelfType (<< 1), hasSam (<< 2), hasLateInterfaces (<< 3) * [u2]? traitImplClassSelfType (reference) * [u2]? samName (reference) * [u2]? samDescriptor (reference) @@ -149,6 +161,8 @@ object InlineInfoAttribute { * [u2] name (reference) * [u2] descriptor (reference) * [u1] isFinal (<< 0), traitMethodWithStaticImplementation (<< 1), hasInlineAnnotation (<< 2), hasNoInlineAnnotation (<< 3) + * [u2]? numLateInterfaces + * [u2] lateInterface (reference) */ final val VERSION: Byte = 1 diff --git a/test/junit/scala/tools/nsc/backend/jvm/opt/BTypesFromClassfileTest.scala b/test/junit/scala/tools/nsc/backend/jvm/opt/BTypesFromClassfileTest.scala index 1ce913006d..9e6a148dba 100644 --- a/test/junit/scala/tools/nsc/backend/jvm/opt/BTypesFromClassfileTest.scala +++ b/test/junit/scala/tools/nsc/backend/jvm/opt/BTypesFromClassfileTest.scala @@ -67,19 +67,7 @@ class BTypesFromClassfileTest { // there's a separate InlineInfoTest. val chk1 = sameBTypes(fromSym.superClass, fromClassfile.superClass, checked) - - // was: - // val chk2 = sameBTypes(fromSym.interfaces, fromClassfile.interfaces, chk1) - - // TODO: The new trait encoding emits redundant parents in the backend to avoid linkage errors in invokespecial - // Need to give this some more thought, maybe do it earlier so it is reflected in the Symbol's info, too. - val fromSymInterfaces = fromSym.interfaces - val fromClassFileInterfaces = fromClassfile.interfaces - val (matching, other) = fromClassFileInterfaces.partition(x => fromSymInterfaces.exists(_.internalName == x.internalName)) - val chk2 = sameBTypes(fromSym.interfaces, matching, chk1) - for (redundant <- other) { - assert(matching.exists(x => x.isSubtypeOf(redundant).orThrow), redundant) - } + val chk2 = sameBTypes(fromSym.interfaces, fromClassfile.interfaces, chk1) // The fromSym info has only member classes, no local or anonymous. The symbol is read from the // Scala pickle data and only member classes are created / entered. -- cgit v1.2.3 From b61c35c38931be3b690bce92d5db9647802b1b34 Mon Sep 17 00:00:00 2001 From: Lukas Rytz Date: Wed, 13 Apr 2016 12:21:23 +0200 Subject: Ensure that lzycompute methods are entered into the scope For some reason this was not the case, leading to spurious inliner warnings (no inline info found for method O$lzycompute). --- src/compiler/scala/tools/nsc/transform/LazyVals.scala | 14 ++++++++------ .../tools/nsc/backend/jvm/opt/ScalaInlineInfoTest.scala | 16 ++++++++++++++-- 2 files changed, 22 insertions(+), 8 deletions(-) diff --git a/src/compiler/scala/tools/nsc/transform/LazyVals.scala b/src/compiler/scala/tools/nsc/transform/LazyVals.scala index 8a0086dbdd..bc9f70679c 100644 --- a/src/compiler/scala/tools/nsc/transform/LazyVals.scala +++ b/src/compiler/scala/tools/nsc/transform/LazyVals.scala @@ -215,14 +215,16 @@ abstract class LazyVals extends Transform with TypingTransformers with ast.TreeD def mkSlowPathDef(clazz: Symbol, lzyVal: Symbol, cond: Tree, syncBody: List[Tree], stats: List[Tree], retVal: Tree): Tree = { - // Q: is there a reason to first set owner to `clazz` (by using clazz.newMethod), and then - // changing it to lzyVal.owner very soon after? Could we just do lzyVal.owner.newMethod? - val defSym = clazz.newMethod(nme.newLazyValSlowComputeName(lzyVal.name.toTermName), lzyVal.pos, STABLE | PRIVATE) + val owner = lzyVal.owner + val defSym = owner.newMethod(nme.newLazyValSlowComputeName(lzyVal.name.toTermName), lzyVal.pos, STABLE | PRIVATE) defSym setInfo MethodType(List(), lzyVal.tpe.resultType) - defSym.owner = lzyVal.owner + if (owner.isClass) owner.info.decls.enter(defSym) debuglog(s"crete slow compute path $defSym with owner ${defSym.owner} for lazy val $lzyVal") - if (bitmaps.contains(lzyVal)) - bitmaps(lzyVal).map(_.owner = defSym) + // this is a hack i don't understand for lazy vals nested in a lazy val, introduced in 3769f4d, + // tested in pos/t3670 (add9be64). class A { val n = { lazy val b = { lazy val dd = 3; dd }; b } } + // bitmaps has an entry bMethodSym -> List(bitmap$0), where bitmap$0.owner == bMethodSym. + // now we set bitmap$0.owner = b$lzycomputeMethodSym. + for (bitmap <- bitmaps(lzyVal)) bitmap.owner = defSym val rhs: Tree = gen.mkSynchronizedCheck(clazz, cond, syncBody, stats).changeOwner(currentOwner -> defSym) DefDef(defSym, addBitmapDefs(lzyVal, BLOCK(rhs, retVal))) diff --git a/test/junit/scala/tools/nsc/backend/jvm/opt/ScalaInlineInfoTest.scala b/test/junit/scala/tools/nsc/backend/jvm/opt/ScalaInlineInfoTest.scala index 46b3ad3f8e..e2d03d8c62 100644 --- a/test/junit/scala/tools/nsc/backend/jvm/opt/ScalaInlineInfoTest.scala +++ b/test/junit/scala/tools/nsc/backend/jvm/opt/ScalaInlineInfoTest.scala @@ -24,8 +24,7 @@ object ScalaInlineInfoTest extends ClearAfterClass.Clearable { @RunWith(classOf[JUnit4]) class ScalaInlineInfoTest extends ClearAfterClass { ClearAfterClass.stateToClear = ScalaInlineInfoTest - - val compiler = newCompiler() + val compiler = ScalaInlineInfoTest.compiler def inlineInfo(c: ClassNode): InlineInfo = c.attrs.asScala.collect({ case a: InlineInfoAttribute => a.inlineInfo }).head @@ -113,6 +112,7 @@ class ScalaInlineInfoTest extends ClearAfterClass { val infoC = inlineInfo(c) val expectC = InlineInfo(false, None, Map( "O()LT$O$;" -> MethodInlineInfo(true ,false,false), + "O$lzycompute()LT$O$;" -> MethodInlineInfo(true, false,false), "f6()I" -> MethodInlineInfo(false,false,false), "x1()I" -> MethodInlineInfo(false,false,false), "T$_setter_$x1_$eq(I)V" -> MethodInlineInfo(false,false,false), @@ -167,4 +167,16 @@ class ScalaInlineInfoTest extends ClearAfterClass { ("U",None))) } + + @Test + def lzyComputeInlineInfo(): Unit = { + val code = "class C { object O }" + val List(c, om) = compileClasses(compiler)(code) + val infoC = inlineInfo(c) + val expected = Map( + "()V" -> MethodInlineInfo(false,false,false), + "O$lzycompute()LC$O$;" -> MethodInlineInfo(true,false,false), + "O()LC$O$;" -> MethodInlineInfo(true,false,false)) + assert(infoC.methodInfos == expected, mapDiff(infoC.methodInfos, expected)) + } } -- cgit v1.2.3