More efficient code for deciding if a mixin forwarder is needed (#5116)

Also adds a warning on junit test methods that compile as default
methods.

8 files changed, 42 insertions, 16 deletions

diff --git a/src/compiler/scala/tools/nsc/settings/Warnings.scala b/src/compiler/scala/tools/nsc/settings/Warnings.scala
index 79795bcc17..310025a336 100644
--- a/src/compiler/scala/tools/nsc/settings/Warnings.scala
+++ b/src/compiler/scala/tools/nsc/settings/Warnings.scala
@@ -25,6 +25,8 @@ trait Warnings {
   // currently considered too noisy for general use
   val warnUnusedImport     = BooleanSetting("-Ywarn-unused-import", "Warn when imports are unused.")
 
+  val nowarnDefaultJunitMethods = BooleanSetting("-Ynowarn-default-junit-methods", "Don't warn when a JUnit @Test method is generated as a default method (not supported in JUnit 4).")
+
   // Experimental lint warnings that are turned off, but which could be turned on programmatically.
   // They are not activated by -Xlint and can't be enabled on the command line because they are not
   // created using the standard factory methods.
diff --git a/src/compiler/scala/tools/nsc/transform/Mixin.scala b/src/compiler/scala/tools/nsc/transform/Mixin.scala
index 03935c3d67..19ba9345fa 100644
--- a/src/compiler/scala/tools/nsc/transform/Mixin.scala
+++ b/src/compiler/scala/tools/nsc/transform/Mixin.scala
@@ -8,6 +8,7 @@ package transform
 
 import symtab._
 import Flags._
+import scala.annotation.tailrec
 import scala.collection.mutable
 
 abstract class Mixin extends InfoTransform with ast.TreeDSL {
@@ -241,22 +242,16 @@ abstract class Mixin extends InfoTransform with ast.TreeDSL {
           case NoSymbol =>
             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
+              // `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:
+              //   - A non-trait base class of `clazz` defines a 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
+              //   - There exists another concrete, matching method in a parent interface `p` of
+              //     `clazz`, and the `mixinClass` does not itself extend `p`. 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
@@ -265,13 +260,25 @@ abstract class Mixin extends InfoTransform with ast.TreeDSL {
               //     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)
+              @tailrec
+              def existsCompetingMethod(baseClasses: List[Symbol]): Boolean = baseClasses match {
+                case baseClass :: rest =>
+                  if (baseClass ne mixinClass) {
+                    val m = member.overriddenSymbol(baseClass)
+                    val isCompeting = m.exists && {
+                      !m.owner.isTraitOrInterface ||
+                        (!m.isDeferred && !mixinClass.isNonBottomSubClass(m.owner))
+                    }
+                    isCompeting || existsCompetingMethod(rest)
+                  } else existsCompetingMethod(rest)
+
+                case _ => false
+              }
+
+              if (existsCompetingMethod(clazz.baseClasses))
                 cloneAndAddMixinMember(mixinClass, member).asInstanceOf[TermSymbol] setAlias member
+              else if (!settings.nowarnDefaultJunitMethods && JUnitTestClass.exists && member.hasAnnotation(JUnitTestClass))
+                warning(member.pos, "JUnit tests in traits that are compiled as default methods are not executed by JUnit 4. JUnit 5 will fix this issue.")
             }
 
           case _        =>
diff --git a/src/reflect/scala/reflect/internal/Definitions.scala b/src/reflect/scala/reflect/internal/Definitions.scala
index d2312440cc..db8ac9b0cb 100644
--- a/src/reflect/scala/reflect/internal/Definitions.scala
+++ b/src/reflect/scala/reflect/internal/Definitions.scala
@@ -1152,6 +1152,7 @@ trait Definitions extends api.StandardDefinitions {
     lazy val ElidableMethodClass        = requiredClass[scala.annotation.elidable]
     lazy val ImplicitNotFoundClass      = requiredClass[scala.annotation.implicitNotFound]
     lazy val ImplicitAmbiguousClass     = getClassIfDefined("scala.annotation.implicitAmbiguous")
+    lazy val JUnitTestClass             = getClassIfDefined("org.junit.Test")
     lazy val MigrationAnnotationClass   = requiredClass[scala.annotation.migration]
     lazy val ScalaStrictFPAttr          = requiredClass[scala.annotation.strictfp]
     lazy val SwitchClass                = requiredClass[scala.annotation.switch]
diff --git a/src/reflect/scala/reflect/runtime/JavaUniverseForce.scala b/src/reflect/scala/reflect/runtime/JavaUniverseForce.scala
index 4630597668..d50debd7ee 100644
--- a/src/reflect/scala/reflect/runtime/JavaUniverseForce.scala
+++ b/src/reflect/scala/reflect/runtime/JavaUniverseForce.scala
@@ -377,6 +377,7 @@ trait JavaUniverseForce { self: runtime.JavaUniverse  =>
     definitions.ElidableMethodClass
     definitions.ImplicitNotFoundClass
     definitions.ImplicitAmbiguousClass
+    definitions.JUnitTestClass
     definitions.MigrationAnnotationClass
     definitions.ScalaStrictFPAttr
     definitions.SwitchClass
diff --git a/test/files/neg/nowarnDefaultJunitMethods.check b/test/files/neg/nowarnDefaultJunitMethods.check
new file mode 100644
index 0000000000..7efdcc299a
--- /dev/null
+++ b/test/files/neg/nowarnDefaultJunitMethods.check
@@ -0,0 +1,6 @@
+C_1.scala:2: warning: JUnit tests in traits that are compiled as default methods are not executed by JUnit 4. JUnit 5 will fix this issue.
+  @org.junit.Test def foo = 0
+                      ^
+error: No warnings can be incurred under -Xfatal-warnings.
+one warning found
+one error found
diff --git a/test/files/neg/nowarnDefaultJunitMethods.flags b/test/files/neg/nowarnDefaultJunitMethods.flags
new file mode 100644
index 0000000000..85d8eb2ba2
--- /dev/null
+++ b/test/files/neg/nowarnDefaultJunitMethods.flags
@@ -0,0 +1 @@
+-Xfatal-warnings
diff --git a/test/files/neg/nowarnDefaultJunitMethods/C_1.scala b/test/files/neg/nowarnDefaultJunitMethods/C_1.scala
new file mode 100644
index 0000000000..e2565a48bc
--- /dev/null
+++ b/test/files/neg/nowarnDefaultJunitMethods/C_1.scala
@@ -0,0 +1,5 @@
+trait T {
+  @org.junit.Test def foo = 0
+}
+
+class C extends T
diff --git a/test/files/neg/nowarnDefaultJunitMethods/Test.java b/test/files/neg/nowarnDefaultJunitMethods/Test.java
new file mode 100644
index 0000000000..e8d64c2cc8
--- /dev/null
+++ b/test/files/neg/nowarnDefaultJunitMethods/Test.java
@@ -0,0 +1,3 @@
+package org.junit;
+
+public @interface Test { }