Merge pull request #5369 from lrytz/sd210

Fixes to mixin forwarders

9 files changed, 122 insertions, 65 deletions

diff --git a/src/compiler/scala/tools/nsc/CompilerCommand.scala b/src/compiler/scala/tools/nsc/CompilerCommand.scala
index 9b8e9fa330..3879d7b425 100644
--- a/src/compiler/scala/tools/nsc/CompilerCommand.scala
+++ b/src/compiler/scala/tools/nsc/CompilerCommand.scala
@@ -107,7 +107,7 @@ class CompilerCommand(arguments: List[String], val settings: Settings) {
     else {
       val sb = new StringBuilder
       allSettings foreach {
-        case s: MultiChoiceSetting[_] if s.isHelping => sb append s.help
+        case s if s.isHelping => sb append s.help
         case _ =>
       }
       sb.toString
diff --git a/src/compiler/scala/tools/nsc/settings/AbsScalaSettings.scala b/src/compiler/scala/tools/nsc/settings/AbsScalaSettings.scala
index 8386722b63..9d643825f6 100644
--- a/src/compiler/scala/tools/nsc/settings/AbsScalaSettings.scala
+++ b/src/compiler/scala/tools/nsc/settings/AbsScalaSettings.scala
@@ -30,8 +30,8 @@ trait AbsScalaSettings {
   type OutputSetting <: Setting
 
   def BooleanSetting(name: String, descr: String): BooleanSetting
-  def ChoiceSetting(name: String, helpArg: String, descr: String, choices: List[String], default: String): ChoiceSetting
-  def ChoiceSettingForcedDefault(name: String, helpArg: String, descr: String, choices: List[String], default: String): ChoiceSetting
+  def ChoiceSetting(name: String, helpArg: String, descr: String, choices: List[String], default: String, choicesHelp: List[String] = Nil): ChoiceSetting
+  def ChoiceSettingForcedDefault(name: String, helpArg: String, descr: String, choices: List[String], default: String, choicesHelp: List[String] = Nil): ChoiceSetting
   def IntSetting(name: String, descr: String, default: Int, range: Option[(Int, Int)], parser: String => Option[Int]): IntSetting
   def MultiStringSetting(name: String, helpArg: String, descr: String): MultiStringSetting
   def MultiChoiceSetting[E <: MultiChoiceEnumeration](name: String, helpArg: String, descr: String, domain: E, default: Option[List[String]]): MultiChoiceSetting[E]
diff --git a/src/compiler/scala/tools/nsc/settings/AbsSettings.scala b/src/compiler/scala/tools/nsc/settings/AbsSettings.scala
index 060a24d8d4..08fa56d8e9 100644
--- a/src/compiler/scala/tools/nsc/settings/AbsSettings.scala
+++ b/src/compiler/scala/tools/nsc/settings/AbsSettings.scala
@@ -88,6 +88,12 @@ trait AbsSettings extends scala.reflect.internal.settings.AbsSettings {
     /** Issue error and return */
     def errorAndValue[T](msg: String, x: T): T = { errorFn(msg) ; x }
 
+    /** If this method returns true, print the [[help]] message and exit. */
+    def isHelping: Boolean = false
+
+    /** The help message to be printed if [[isHelping]]. */
+    def help: String = ""
+
     /** After correct Setting has been selected, tryToSet is called with the
      *  remainder of the command line.  It consumes any applicable arguments and
      *  returns the unconsumed ones.
diff --git a/src/compiler/scala/tools/nsc/settings/MutableSettings.scala b/src/compiler/scala/tools/nsc/settings/MutableSettings.scala
index 9cc8faf8c2..7b4c55c2af 100644
--- a/src/compiler/scala/tools/nsc/settings/MutableSettings.scala
+++ b/src/compiler/scala/tools/nsc/settings/MutableSettings.scala
@@ -219,10 +219,10 @@ class MutableSettings(val errorFn: String => Unit)
   }
 
   def BooleanSetting(name: String, descr: String) = add(new BooleanSetting(name, descr))
-  def ChoiceSetting(name: String, helpArg: String, descr: String, choices: List[String], default: String) =
-    add(new ChoiceSetting(name, helpArg, descr, choices, default))
-  def ChoiceSettingForcedDefault(name: String, helpArg: String, descr: String, choices: List[String], default: String) =
-    ChoiceSetting(name, helpArg, descr, choices, default).withPostSetHook(sett =>
+  def ChoiceSetting(name: String, helpArg: String, descr: String, choices: List[String], default: String, choicesHelp: List[String]) =
+    add(new ChoiceSetting(name, helpArg, descr, choices, default, choicesHelp))
+  def ChoiceSettingForcedDefault(name: String, helpArg: String, descr: String, choices: List[String], default: String, choicesHelp: List[String]) =
+    ChoiceSetting(name, helpArg, descr, choices, default, choicesHelp).withPostSetHook(sett =>
       if (sett.value != default) {
         sett.withDeprecationMessage(s"${name}:${sett.value} is deprecated, forcing use of $default")
         sett.value = default
@@ -627,7 +627,7 @@ class MutableSettings(val errorFn: String => Unit)
     descr: String,
     val domain: E,
     val default: Option[List[String]]
-  ) extends Setting(name, s"$descr: `_' for all, `$name:help' to list") with Clearable {
+  ) extends Setting(name, s"$descr: `_' for all, `$name:help' to list choices.") with Clearable {
 
     withHelpSyntax(s"$name:<_,$helpArg,-$helpArg>")
 
@@ -748,9 +748,9 @@ class MutableSettings(val errorFn: String => Unit)
 
     def contains(choice: domain.Value): Boolean = value contains choice
 
-    def isHelping: Boolean = sawHelp
+    override def isHelping: Boolean = sawHelp
 
-    def help: String = {
+    override def help: String = {
       val choiceLength = choices.map(_.length).max + 1
       val formatStr = s"  %-${choiceLength}s %s"
       choices.zipAll(descriptions, "", "").map {
@@ -808,18 +808,33 @@ class MutableSettings(val errorFn: String => Unit)
     helpArg: String,
     descr: String,
     override val choices: List[String],
-    val default: String)
-  extends Setting(name, descr + choices.mkString(" (", ",", ") default:" + default)) {
+    val default: String,
+    val choicesHelp: List[String])
+  extends Setting(name,
+    if (choicesHelp.isEmpty) s"$descr Choices: ${choices.mkString("(", ",", ")")}, default: $default."
+    else s"$descr Default: `$default', `help' to list choices.") {
     type T = String
     protected var v: T = default
     def indexOfChoice: Int = choices indexOf value
 
-    private def usageErrorMessage = f"Usage: $name:<$helpArg>%n where <$helpArg> choices are ${choices mkString ", "} (default: $default)%n"
+    private def choicesHelpMessage = if (choicesHelp.isEmpty) "" else {
+      val choiceLength = choices.map(_.length).max + 1
+      val formatStr = s"  %-${choiceLength}s %s%n"
+      choices.zipAll(choicesHelp, "", "").map({
+        case (choice, desc) => formatStr.format(choice, desc)
+      }).mkString("")
+    }
+    private def usageErrorMessage = f"Usage: $name:<$helpArg> where <$helpArg> choices are ${choices mkString ", "} (default: $default).%n$choicesHelpMessage"
+
+    private var sawHelp = false
+    override def isHelping = sawHelp
+    override def help = usageErrorMessage
 
     def tryToSet(args: List[String]) = errorAndValue(usageErrorMessage, None)
 
     override def tryToSetColon(args: List[String]) = args match {
       case Nil                            => errorAndValue(usageErrorMessage, None)
+      case List("help")                   => sawHelp = true; Some(Nil)
       case List(x) if choices contains x  => value = x ; Some(Nil)
       case List(x)                        => errorAndValue("'" + x + "' is not a valid choice for '" + name + "'", None)
       case xs                             => errorAndValue("'" + name + "' does not accept multiple arguments.", None)
diff --git a/src/compiler/scala/tools/nsc/settings/ScalaSettings.scala b/src/compiler/scala/tools/nsc/settings/ScalaSettings.scala
index dae8539c66..e10fa3a114 100644
--- a/src/compiler/scala/tools/nsc/settings/ScalaSettings.scala
+++ b/src/compiler/scala/tools/nsc/settings/ScalaSettings.scala
@@ -38,8 +38,8 @@ trait ScalaSettings extends AbsScalaSettings
   /** If any of these settings is enabled, the compiler should print a message and exit.  */
   def infoSettings = List[Setting](version, help, Xhelp, Yhelp, showPlugins, showPhases, genPhaseGraph)
 
-  /** Any -multichoice:help? Nicer if any option could report that it had help to offer. */
-  private def multihelp = allSettings exists { case s: MultiChoiceSetting[_] => s.isHelping case _ => false }
+  /** Any -option:help? */
+  private def multihelp = allSettings exists { case s => s.isHelping case _ => false }
 
   /** Is an info setting set? */
   def isInfo = (infoSettings exists (_.isSetByUser)) || multihelp
@@ -133,7 +133,22 @@ trait ScalaSettings extends AbsScalaSettings
 
   val XnoPatmatAnalysis = BooleanSetting ("-Xno-patmat-analysis", "Don't perform exhaustivity/unreachability analysis. Also, ignore @switch annotation.")
   val XfullLubs         = BooleanSetting ("-Xfull-lubs", "Retains pre 2.10 behavior of less aggressive truncation of least upper bounds.")
-  val XgenMixinForwarders = BooleanSetting("-Xgen-mixin-forwarders", "Generate forwarder methods in classes inhering concrete methods from traits.")
+
+  val XmixinForceForwarders = ChoiceSetting(
+    name    = "-Xmixin-force-forwarders",
+    helpArg = "mode",
+    descr   = "Generate forwarder methods in classes inhering concrete methods from traits.",
+    choices = List("true", "junit", "false"),
+    default = "junit",
+    choicesHelp = List(
+      "Always generate mixin forwarders.",
+      "Generate mixin forwarders for JUnit-annotated methods (JUnit 4 does not support default methods).",
+      "Only generate mixin forwarders required for program correctness."))
+
+  object mixinForwarderChoices {
+    def isTruthy = XmixinForceForwarders.value == "true"
+    def isJunit = isTruthy || XmixinForceForwarders.value == "junit"
+  }
 
   // XML parsing options
   object XxmlSettings extends MultiChoiceEnumeration {
@@ -143,7 +158,7 @@ trait ScalaSettings extends AbsScalaSettings
   val Xxml = MultiChoiceSetting(
     name    = "-Xxml",
     helpArg = "property",
-    descr   = "Configure XML parsing",
+    descr   = "Configure XML parsing.",
     domain  = XxmlSettings
   )
 
@@ -169,7 +184,7 @@ trait ScalaSettings extends AbsScalaSettings
   val Ycompacttrees   = BooleanSetting    ("-Ycompact-trees", "Use compact tree printer when displaying trees.")
   val noCompletion    = BooleanSetting    ("-Yno-completion", "Disable tab-completion in the REPL.")
   val debug           = BooleanSetting    ("-Ydebug", "Increase the quantity of debugging output.")
-  val termConflict    = ChoiceSetting     ("-Yresolve-term-conflict", "strategy", "Resolve term conflicts", List("package", "object", "error"), "error")
+  val termConflict    = ChoiceSetting     ("-Yresolve-term-conflict", "strategy", "Resolve term conflicts.", List("package", "object", "error"), "error")
   val log             = PhasesSetting     ("-Ylog", "Log operations during")
   val Ylogcp          = BooleanSetting    ("-Ylog-classpath", "Output information about what classpath is being applied.")
   val Ynogenericsig   = BooleanSetting    ("-Yno-generic-signatures", "Suppress generation of generic signatures for Java.")
@@ -193,7 +208,7 @@ trait ScalaSettings extends AbsScalaSettings
   val Yrangepos       = BooleanSetting    ("-Yrangepos", "Use range positions for syntax trees.")
   val Ymemberpos      = StringSetting     ("-Yshow-member-pos", "output style", "Show start and end positions of members", "") withPostSetHook (_ => Yrangepos.value = true)
   val Yreifycopypaste = BooleanSetting    ("-Yreify-copypaste", "Dump the reified trees in copypasteable representation.")
-  val Ymacroexpand    = ChoiceSetting     ("-Ymacro-expand", "policy", "Control expansion of macros, useful for scaladoc and presentation compiler", List(MacroExpand.Normal, MacroExpand.None, MacroExpand.Discard), MacroExpand.Normal)
+  val Ymacroexpand    = ChoiceSetting     ("-Ymacro-expand", "policy", "Control expansion of macros, useful for scaladoc and presentation compiler.", List(MacroExpand.Normal, MacroExpand.None, MacroExpand.Discard), MacroExpand.Normal)
   val Ymacronoexpand  = BooleanSetting    ("-Ymacro-no-expand", "Don't expand macros. Might be useful for scaladoc and presentation compiler, but will crash anything which uses macros and gets past typer.") withDeprecationMessage(s"Use ${Ymacroexpand.name}:${MacroExpand.None}") withPostSetHook(_ => Ymacroexpand.value = MacroExpand.None)
   val Yreplsync       = BooleanSetting    ("-Yrepl-sync", "Do not use asynchronous code for repl startup")
   val Yreplclassbased = BooleanSetting    ("-Yrepl-class-based", "Use classes to wrap REPL snippets instead of objects")
diff --git a/src/compiler/scala/tools/nsc/settings/Warnings.scala b/src/compiler/scala/tools/nsc/settings/Warnings.scala
index 7ef606b6ef..839e734abc 100644
--- a/src/compiler/scala/tools/nsc/settings/Warnings.scala
+++ b/src/compiler/scala/tools/nsc/settings/Warnings.scala
@@ -25,8 +25,6 @@ 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 d462b00261..582c51b90d 100644
--- a/src/compiler/scala/tools/nsc/transform/Mixin.scala
+++ b/src/compiler/scala/tools/nsc/transform/Mixin.scala
@@ -215,51 +215,59 @@ abstract class Mixin extends InfoTransform with ast.TreeDSL with AccessorSynthes
           case NoSymbol =>
             val isMemberOfClazz = clazz.info.findMember(member.name, 0, 0L, stableOnly = false).alternatives.contains(member)
             if (isMemberOfClazz) {
-              def genForwarder(): Unit = {
-                cloneAndAddMixinMember(mixinClass, member).asInstanceOf[TermSymbol] setAlias member
+              def genForwarder(required: Boolean): Unit = {
+                val owner = member.owner
+                if (owner.isJavaDefined && owner.isInterface && !clazz.parentSymbols.contains(owner)) {
+                  val text = s"Unable to implement a mixin forwarder for $member in $clazz unless interface ${owner.name} is directly extended by $clazz."
+                  if (required) reporter.error(clazz.pos, text)
+                  else warning(clazz.pos, text)
+                } else
+                  cloneAndAddMixinMember(mixinClass, member).asInstanceOf[TermSymbol] setAlias member
               }
 
-              if (settings.XgenMixinForwarders) genForwarder()
-              else {
-
-                // `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 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 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
-                //     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.
-
-                @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))
-                  genForwarder()
-                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.")
+              // `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 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 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
+              //     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.
+
+              @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
               }
+
+              def generateJUnitForwarder: Boolean = {
+                settings.mixinForwarderChoices.isJunit &&
+                  member.annotations.nonEmpty &&
+                  JUnitAnnotations.exists(annot => annot.exists && member.hasAnnotation(annot))
+              }
+
+              if (existsCompetingMethod(clazz.baseClasses) || generateJUnitForwarder)
+                genForwarder(required = true)
+              else if (settings.mixinForwarderChoices.isTruthy)
+                genForwarder(required = false)
             }
 
           case _        =>
diff --git a/src/compiler/scala/tools/nsc/typechecker/SuperAccessors.scala b/src/compiler/scala/tools/nsc/typechecker/SuperAccessors.scala
index 49d892e04f..a38d21fe10 100644
--- a/src/compiler/scala/tools/nsc/typechecker/SuperAccessors.scala
+++ b/src/compiler/scala/tools/nsc/typechecker/SuperAccessors.scala
@@ -146,7 +146,22 @@ abstract class SuperAccessors extends transform.Transform with transform.TypingT
         val intermediateClasses = clazz.info.baseClasses.tail.takeWhile(_ != sym.owner)
         intermediateClasses.map(sym.overridingSymbol).find(s => s.isDeferred && !s.isAbstractOverride && !s.owner.isTrait).foreach {
           absSym =>
-            reporter.error(sel.pos, s"${sym.fullLocationString} cannot be directly accessed from ${clazz} because ${absSym.owner} redeclares it as abstract")
+            reporter.error(sel.pos, s"${sym.fullLocationString} cannot be directly accessed from $clazz because ${absSym.owner} redeclares it as abstract")
+        }
+      } else if (mix != tpnme.EMPTY) {
+        // SD-143: a call super[T].m that resolves to A.m cannot be translated to correct bytecode if
+        //   - A is a class (not a trait / interface), but not the direct superclass. Invokespecial
+        //     would select an overriding method in the direct superclass, rather than A.m.
+        //     https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-6.html#jvms-6.5.invokespecial
+        //   - A is a java-defined interface and not listed as direct parent of the class. In this
+        //     case, `invokespecial A.m` would be invalid.
+        val owner = sym.owner
+        if (!owner.isTrait && owner != clazz.superClass) {
+          reporter.error(sel.pos,
+            s"cannot emit super call: the selected $sym is declared in $owner, which is not the direct superclass of $clazz.\n" +
+            s"An unqualified super call (super.${sym.name}) would be allowed.")
+        } else if (owner.isInterface && owner.isJavaDefined && !clazz.parentSymbols.contains(owner)) {
+          reporter.error(sel.pos, s"unable to emit super call unless interface ${owner.name} (which declares $sym) is directly extended by $clazz.")
         }
       }
 
diff --git a/src/compiler/scala/tools/nsc/typechecker/Typers.scala b/src/compiler/scala/tools/nsc/typechecker/Typers.scala
index 3360599c1b..d8183ea8df 100644
--- a/src/compiler/scala/tools/nsc/typechecker/Typers.scala
+++ b/src/compiler/scala/tools/nsc/typechecker/Typers.scala
@@ -2040,7 +2040,7 @@ trait Typers extends Adaptations with Tags with TypersTracking with PatternTyper
               override def matches(sym: Symbol, sym1: Symbol) =
                 if (sym.isSkolem) matches(sym.deSkolemize, sym1)
                 else if (sym1.isSkolem) matches(sym, sym1.deSkolemize)
-                else super[SubstTypeMap].matches(sym, sym1)
+                else super.matches(sym, sym1)
             }
             // allow defaults on by-name parameters
             if (sym hasFlag BYNAMEPARAM)