Skip to content

Commit 33e7106

Browse files
lrytzlrytz
committed
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.
1 parent b932419 commit 33e7106

File tree

12 files changed

+223
-57
lines changed

12 files changed

+223
-57
lines changed

‎src/compiler/scala/tools/nsc/transform/Mixin.scala‎

Lines changed: 35 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,6 @@ abstract class Mixin extends InfoTransform with ast.TreeDSL {
4747
sym.isMethod
4848
&& (!sym.hasFlag(DEFERRED | SUPERACCESSOR) || (sym hasFlag lateDEFERRED))
4949
&& sym.owner.isTrait
50-
&& sym.isMethod
5150
&& (!sym.isModule || sym.hasFlag(PRIVATE | LIFTED))
5251
&& (!(sym hasFlag (ACCESSOR | SUPERACCESSOR)) || sym.isLazy)
5352
&& !sym.isPrivate
@@ -240,8 +239,41 @@ abstract class Mixin extends InfoTransform with ast.TreeDSL {
240239
for (member <- mixinClass.info.decls ; if isImplementedStatically(member)) {
241240
member overridingSymbol clazz match {
242241
case NoSymbol =>
243-
if (clazz.info.findMember(member.name, 0, 0L, stableOnly = false).alternatives contains member)
244-
cloneAndAddMixinMember(mixinClass, member).asInstanceOf[TermSymbol] setAlias member
242+
val isMemberOfClazz = clazz.info.findMember(member.name, 0, 0L, stableOnly = false).alternatives.contains(member)
243+
if (isMemberOfClazz) {
244+
val competingMethods = clazz.baseClasses.iterator
245+
.filter(_ ne mixinClass)
246+
.map(member.overriddenSymbol)
247+
.filter(_.exists)
248+
.toList
249+
250+
// `member` is a concrete `method` defined in `mixinClass`, which is a base class of
251+
// `clazz`, and the method is not overridden in `clazz`. A forwarder is needed if:
252+
//
253+
// - A non-trait base class defines matching method. Example:
254+
// class C {def f: Int}; trait T extends C {def f = 1}; class D extends T
255+
// Even if C.f is abstract, the forwarder in D is needed, otherwise the JVM would
256+
// resolve `D.f` to `C.f`, see jvms-6.5.invokevirtual.
257+
//
258+
// - There exists another concrete, matching method in any of the base classes, and
259+
// the `mixinClass` does not itself extend that base class. In this case the
260+
// forwarder is needed to disambiguate. Example:
261+
// trait T1 {def f = 1}; trait T2 extends T1 {override def f = 2}; class C extends T2
262+
// In C we don't need a forwarder for f because T2 extends T1, so the JVM resolves
263+
// C.f to T2.f non-ambiguously. See jvms-5.4.3.3, "maximally-specific method".
264+
// trait U1 {def f = 1}; trait U2 {self:U1 => override def f = 2}; class D extends U2
265+
// In D the forwarder is needed, the interfaces U1 and U2 are unrelated at the JVM
266+
// level.
267+
268+
lazy val mixinSuperInterfaces = mixinClass.ancestors.filter(_.isTraitOrInterface)
269+
val needsForwarder = competingMethods.exists(m => {
270+
!m.owner.isTraitOrInterface ||
271+
(!m.isDeferred && !mixinSuperInterfaces.contains(m.owner))
272+
})
273+
if (needsForwarder)
274+
cloneAndAddMixinMember(mixinClass, member).asInstanceOf[TermSymbol] setAlias member
275+
}
276+
245277
case _ =>
246278
}
247279
}

‎test/files/neg/t4749.check‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ t4749.scala:26: warning: Fail6 has a main method with parameter type Array[Strin
2626
object Fail6 {
2727
^
2828
t4749.scala:42: warning: Win3 has a main method with parameter type Array[String], but bippy.Win3 will not be a runnable program.
29-
Reason: main method must have exact signature (Array[String])Unit
29+
Reason: main methods cannot refer to type parameters or abstract types.
3030
object Win3 extends WinBippy[Unit] { }
3131
^
3232
error: No warnings can be incurred under -Xfatal-warnings.

‎test/files/neg/t5148.check‎

Lines changed: 0 additions & 16 deletions
This file was deleted.

‎test/files/neg/t5148.scala‎ renamed to ‎test/files/pos/t5148.scala‎

File renamed without changes.

‎test/files/run/mixin-signatures.check‎

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,23 @@
11
class Test$bar1$ {
2-
public java.lang.String Test$bar1$.f(java.lang.Object)
2+
public default java.lang.String Foo1.f(java.lang.Object)
3+
generic: public default java.lang.String Foo1.f(T)
34
public java.lang.Object Test$bar1$.f(java.lang.Object) <bridge> <synthetic>
45
public java.lang.String Test$bar1$.g(java.lang.String)
56
public java.lang.Object Test$bar1$.g(java.lang.Object) <bridge> <synthetic>
67
public java.lang.String Test$bar1$.g(java.lang.Object) <bridge> <synthetic>
7-
public java.lang.Object Test$bar1$.h(java.lang.Object)
8+
public default java.lang.Object Base.h(java.lang.Object)
9+
generic: public default R Base.h(T)
810
}
911

1012
class Test$bar2$ {
11-
public java.lang.Object Test$bar2$.f(java.lang.String)
13+
public default java.lang.Object Foo2.f(java.lang.String)
14+
generic: public default R Foo2.f(java.lang.String)
1215
public java.lang.Object Test$bar2$.f(java.lang.Object) <bridge> <synthetic>
1316
public java.lang.String Test$bar2$.g(java.lang.String)
1417
public java.lang.Object Test$bar2$.g(java.lang.Object) <bridge> <synthetic>
1518
public java.lang.Object Test$bar2$.g(java.lang.String) <bridge> <synthetic>
16-
public java.lang.Object Test$bar2$.h(java.lang.Object)
19+
public default java.lang.Object Base.h(java.lang.Object)
20+
generic: public default R Base.h(T)
1721
}
1822

1923
class Test$bar3$ {
@@ -23,7 +27,8 @@ class Test$bar3$ {
2327
public java.lang.String Test$bar3$.g(java.lang.String)
2428
public java.lang.Object Test$bar3$.g(java.lang.Object) <bridge> <synthetic>
2529
public java.lang.String Test$bar3$.g(java.lang.Object) <bridge> <synthetic>
26-
public java.lang.Object Foo3.h(java.lang.Object)
30+
public default java.lang.Object Base.h(java.lang.Object)
31+
generic: public default R Base.h(T)
2732
}
2833

2934
class Test$bar4$ {
@@ -33,7 +38,8 @@ class Test$bar4$ {
3338
public java.lang.String Test$bar4$.g(java.lang.String)
3439
public java.lang.Object Test$bar4$.g(java.lang.Object) <bridge> <synthetic>
3540
public java.lang.Object Test$bar4$.g(java.lang.String) <bridge> <synthetic>
36-
public java.lang.Object Foo4.h(java.lang.Object)
41+
public default java.lang.Object Base.h(java.lang.Object)
42+
generic: public default R Base.h(T)
3743
}
3844

3945
class Test$bar5$ {
@@ -45,7 +51,8 @@ class Test$bar5$ {
4551
public java.lang.Object Test$bar5$.g(java.lang.Object) <bridge> <synthetic>
4652
public java.lang.Object Test$bar5$.g(java.lang.String) <bridge> <synthetic>
4753
public java.lang.String Test$bar5$.g(java.lang.Object) <bridge> <synthetic>
48-
public java.lang.Object Test$bar5$.h(java.lang.Object)
54+
public default java.lang.Object Base.h(java.lang.Object)
55+
generic: public default R Base.h(T)
4956
}
5057

5158
interface Foo1 {

‎test/files/run/t5652.check‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,5 +4,4 @@ public default void T1.$init$()
44
public final int A1.A1$$g$2()
55
public int A1.f1()
66
public final int A2.A2$$g$1()
7-
public int A2.f0()
87
public int A2.f2()

‎test/files/run/t6554.scala‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,14 @@
1-
trait Foo[A] {
1+
trait T1[A] {
22
def minBy[B](b: B): A = ???
33
}
4-
5-
class Bar extends Foo[Int]
4+
5+
// The second trait is needed to make sure there's a forwarder generated in Bar.
6+
// otherwise Bar.minBy is just the inherited default method from T1.
7+
trait T2[A] { self: T1[A] =>
8+
override def minBy[B](b: B): A = ???
9+
}
10+
11+
class Bar extends T1[Int] with T2[Int]
612

713
object Test extends App {
814
val sigs = classOf[Bar].getDeclaredMethods.map(m => s"${m.toString} / ${m.toGenericString}").sorted

‎test/files/run/t7932.check‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,6 @@
1-
warning: there was one feature warning; re-run with -feature for details
21
public Category<?> C.category()
32
public Category<scala.Tuple2> C.category1()
3+
public default Category<java.lang.Object> M1.category()
4+
public default Category<scala.Tuple2> M1.category1()
5+
public default Category<java.lang.Object> M2.category()
6+
public default Category<scala.Tuple2> M2.category1()

‎test/files/run/t7932.scala‎

Lines changed: 21 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,28 @@
1+
import scala.language.higherKinds
2+
13
class Category[M[_, _]]
2-
trait M[F] {
4+
5+
trait M1[F] {
36
type X[a, b] = F
47
def category: Category[X] = null
58
def category1: Category[Tuple2] = null
69
}
7-
abstract class C extends M[Float]
10+
11+
// The second trait is needed to make sure there's a forwarder generated in C.
12+
// otherwise the trait methods are just the inherited default methods from M1.
13+
trait M2[F] { self: M1[F] =>
14+
override def category: Category[X] = null
15+
override def category1: Category[Tuple2] = null
16+
}
17+
18+
abstract class C extends M1[Float] with M2[Float]
19+
820
object Test extends App {
9-
val ms = classOf[C].getMethods.filter(_.getName.startsWith("category"))
10-
println(ms.map(_.toGenericString).sorted.mkString("\n"))
21+
def t(c: Class[_]) = {
22+
val ms = c.getMethods.filter(_.getName.startsWith("category"))
23+
println(ms.map(_.toGenericString).sorted.mkString("\n"))
24+
}
25+
t(classOf[C])
26+
t(classOf[M1[_]])
27+
t(classOf[M2[_]])
1128
}

‎test/junit/scala/issues/BytecodeTest.scala‎

Lines changed: 133 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,14 @@ package scala.issues
33
import org.junit.runner.RunWith
44
import org.junit.runners.JUnit4
55
import org.junit.Test
6+
67
import scala.tools.asm.Opcodes._
78
import scala.tools.nsc.backend.jvm.AsmUtils
89
import scala.tools.nsc.backend.jvm.CodeGenTools._
910
import org.junit.Assert._
11+
1012
import scala.collection.JavaConverters._
13+
import scala.tools.asm.tree.ClassNode
1114
import scala.tools.partest.ASMConverters._
1215
import scala.tools.testing.ClearAfterClass
1316

@@ -208,4 +211,134 @@ class BytecodeTest extends ClearAfterClass {
208211
Label(14), Op(ICONST_0),
209212
Label(17), Op(IRETURN)))
210213
}
214+
215+
object forwarderTestUtils {
216+
def findMethods(cls: ClassNode, name: String): List[Method] = cls.methods.iterator.asScala.find(_.name == name).map(convertMethod).toList
217+
218+
import language.implicitConversions
219+
implicit def s2c(s: Symbol)(implicit classes: Map[String, ClassNode]): ClassNode = classes(s.name)
220+
221+
def checkForwarder(c: ClassNode, target: String) = {
222+
val List(f) = findMethods(c, "f")
223+
assertSameCode(f, List(VarOp(ALOAD, 0), Invoke(INVOKESPECIAL, target, "f", "()I", false), Op(IRETURN)))
224+
}
225+
}
226+
227+
@Test
228+
def traitMethodForwarders(): Unit = {
229+
import forwarderTestUtils._
230+
val code =
231+
"""trait T1 { def f = 1 }
232+
|trait T2 extends T1 { override def f = 2 }
233+
|trait T3 { self: T1 => override def f = 3 }
234+
|
235+
|abstract class A1 { def f: Int }
236+
|class A2 { def f: Int = 4 }
237+
|
238+
|trait T4 extends A1 { def f = 5 }
239+
|trait T5 extends A2 { override def f = 6 }
240+
|
241+
|trait T6 { def f: Int }
242+
|trait T7 extends T6 { abstract override def f = super.f + 1 }
243+
|
244+
|trait T8 { override def clone() = super.clone() }
245+
|
246+
|class A3 extends T1 { override def f = 7 }
247+
|
248+
|class C1 extends T1
249+
|class C2 extends T2
250+
|class C3 extends T1 with T2
251+
|class C4 extends T2 with T1
252+
|class C5 extends T1 with T3
253+
|
254+
|// traits extending a class that defines f
255+
|class C6 extends T4
256+
|class C7 extends T5
257+
|class C8 extends A1 with T4
258+
|class C9 extends A2 with T5
259+
|
260+
|// T6: abstract f in trait
261+
|class C10 extends T6 with T1
262+
|class C11 extends T6 with T2
263+
|abstract class C12 extends A1 with T6
264+
|class C13 extends A2 with T6
265+
|class C14 extends T4 with T6
266+
|class C15 extends T5 with T6
267+
|
268+
|// superclass overrides a trait method
269+
|class C16 extends A3
270+
|class C17 extends A3 with T1
271+
|
272+
|// abstract override
273+
|class C18 extends T6 { def f = 22 }
274+
|class C19 extends C18 with T7
275+
|
276+
|class C20 extends T8
277+
""".stripMargin
278+
279+
implicit val classes = compileClasses(compiler)(code).map(c => (c.name, c)).toMap
280+
281+
val noForwarder = List('C1, 'C2, 'C3, 'C4, 'C10, 'C11, 'C12, 'C13, 'C16, 'C17)
282+
for (c <- noForwarder) assertEquals(findMethods(c, "f"), Nil)
283+
284+
checkForwarder('C5, "T3")
285+
checkForwarder('C6, "T4")
286+
checkForwarder('C7, "T5")
287+
checkForwarder('C8, "T4")
288+
checkForwarder('C9, "T5")
289+
checkForwarder('C14, "T4")
290+
checkForwarder('C15, "T5")
291+
assertSameSummary(getSingleMethod('C18, "f"), List(BIPUSH, IRETURN))
292+
checkForwarder('C19, "T7")
293+
assertSameCode(getSingleMethod('C19, "T7$$super$f"), List(VarOp(ALOAD, 0), Invoke(INVOKESPECIAL, "C18", "f", "()I", false), Op(IRETURN)))
294+
assertInvoke(getSingleMethod('C20, "clone"), "T8", "clone") // mixin forwarder
295+
}
296+
297+
@Test
298+
def noTraitMethodForwardersForOverloads(): Unit = {
299+
import forwarderTestUtils._
300+
val code =
301+
"""trait T1 { def f(x: Int) = 0 }
302+
|trait T2 { def f(x: String) = 1 }
303+
|class C extends T1 with T2
304+
""".stripMargin
305+
val List(c, t1, t2) = compileClasses(compiler)(code)
306+
assertEquals(findMethods(c, "f"), Nil)
307+
}
308+
309+
@Test
310+
def traitMethodForwardersForJavaDefaultMethods(): Unit = {
311+
import forwarderTestUtils._
312+
val j1 = ("interface J1 { int f(); }", "J1.java")
313+
val j2 = ("interface J2 { default int f() { return 1; } }", "J2.java")
314+
val j3 = ("interface J3 extends J1 { default int f() { return 2; } }", "J3.java")
315+
val j4 = ("interface J4 extends J2 { default int f() { return 3; } }", "J4.java")
316+
val code =
317+
"""trait T1 extends J2 { override def f = 4 }
318+
|trait T2 { self: J2 => override def f = 5 }
319+
|
320+
|class K1 extends J2
321+
|class K2 extends J1 with J2
322+
|class K3 extends J2 with J1
323+
|
324+
|class K4 extends J3
325+
|class K5 extends J3 with J1
326+
|class K6 extends J1 with J3
327+
|
328+
|class K7 extends J4
329+
|class K8 extends J4 with J2
330+
|class K9 extends J2 with J4
331+
|
332+
|class K10 extends T1 with J2
333+
|class K11 extends J2 with T1
334+
|
335+
|class K12 extends J2 with T2
336+
""".stripMargin
337+
implicit val classes = compileClasses(compiler)(code, List(j1, j2, j3, j4)).map(c => (c.name, c)).toMap
338+
339+
val noForwarder = List('K1, 'K2, 'K3, 'K4, 'K5, 'K6, 'K7, 'K8, 'K9, 'K10, 'K11)
340+
for (c <- noForwarder) assertEquals(findMethods(c, "f"), Nil)
341+
342+
checkForwarder('K12, "T2")
343+
}
211344
}

0 commit comments

Comments
 (0)