Merge pull request #8652 from sjrd/trait-init-like-scala-2

Revert to the Scala 2 encoding for trait initialization and super calls

Author: sjrd

compiler/src/dotty/tools/backend/jvm/BCodeBodyBuilder.scala

@@ -780,6 +780,7 @@ trait BCodeBodyBuilder extends BCodeSkelBuilder {
             val invokeStyle =
               if (sym.isStaticMember) InvokeStyle.Static
               else if (sym.is(Private) || sym.isClassConstructor) InvokeStyle.Special
+              else if (app.hasAttachment(BCodeHelpers.UseInvokeSpecial)) InvokeStyle.Special
               else InvokeStyle.Virtual
 
             if (invokeStyle.hasInstance) genLoadQualifier(fun)
@@ -1165,9 +1166,16 @@ trait BCodeBodyBuilder extends BCodeSkelBuilder {
       val isInterface = isEmittedInterface(receiverClass)
       import InvokeStyle._
       if (style == Super) {
-        // DOTTY: this differ from how super-calls in traits are handled in the scalac backend,
-        // this is intentional but could change in the future, see https://github.com/lampepfl/dotty/issues/5928
-        bc.invokespecial(receiverName, jname, mdescr, isInterface)
+        if (isInterface && !method.is(JavaDefined)) {
+          val args = new Array[BType](bmType.argumentTypes.length + 1)
+          val ownerBType = toTypeKind(method.owner.info)
+          bmType.argumentTypes.copyToArray(args, 1)
+          val staticDesc = MethodBType(ownerBType :: bmType.argumentTypes, bmType.returnType).descriptor
+          val staticName = traitSuperAccessorName(method)
+          bc.invokestatic(receiverName, staticName, staticDesc, isInterface)
+        } else {
+          bc.invokespecial(receiverName, jname, mdescr, isInterface)
+        }
       } else {
         val opc = style match {
           case Static => Opcodes.INVOKESTATIC

compiler/src/dotty/tools/backend/jvm/BCodeHelpers.scala

@@ -82,6 +82,12 @@ trait BCodeHelpers extends BCodeIdiomatic with BytecodeWriters {
     }
   }
 
+  final def traitSuperAccessorName(sym: Symbol): String = {
+    val nameString = sym.javaSimpleName.toString
+    if (sym.name == nme.TRAIT_CONSTRUCTOR) nameString
+    else nameString + "$"
+  }
+
   // -----------------------------------------------------------------------------------------
   // finding the least upper bound in agreement with the bytecode verifier (given two internal names handed by ASM)
   // Background:
@@ -950,4 +956,11 @@ object BCodeHelpers {
     val Super   = new InvokeStyle(3) // InvokeSpecial (super calls)
   }
 
+  /** An attachment on Apply nodes indicating that it should be compiled with
+   *  `invokespecial` instead of `invokevirtual`. This is used for static
+   *  forwarders.
+   *  See BCodeSkelBuilder.makeStaticForwarder for more details.
+   */
+  val UseInvokeSpecial = new dotc.util.Property.Key[Unit]
+
 }

compiler/src/dotty/tools/backend/jvm/BCodeSkelBuilder.scala

@@ -14,9 +14,9 @@ import dotty.tools.dotc.CompilationUnit
 import dotty.tools.dotc.core.Annotations.Annotation
 import dotty.tools.dotc.core.Decorators._
 import dotty.tools.dotc.core.Flags._
-import dotty.tools.dotc.core.StdNames.str
+import dotty.tools.dotc.core.StdNames.{nme, str}
 import dotty.tools.dotc.core.Symbols._
-import dotty.tools.dotc.core.Types.Type
+import dotty.tools.dotc.core.Types.{MethodType, Type}
 import dotty.tools.dotc.util.Spans._
 import dotty.tools.dotc.report
 
@@ -496,7 +496,23 @@ trait BCodeSkelBuilder extends BCodeHelpers {
 
         case ValDef(name, tpt, rhs) => () // fields are added in `genPlainClass()`, via `addClassFields()`
 
-        case dd: DefDef => genDefDef(dd)
+        case dd: DefDef =>
+          /* First generate a static forwarder if this is a non-private trait
+           * trait method. This is required for super calls to this method, which
+           * go through the static forwarder in order to work around limitations
+           * of the JVM.
+           * In theory, this would go in a separate MiniPhase, but it would have to
+           * sit in a MegaPhase of its own between GenSJSIR and GenBCode, so the cost
+           * is not worth it. We directly do it in this back-end instead, which also
+           * kind of makes sense because it is JVM-specific.
+           */
+          val sym = dd.symbol
+          val needsStaticImplMethod =
+            claszSymbol.isInterface && !dd.rhs.isEmpty && !sym.isPrivate && !sym.isStaticMember
+          if needsStaticImplMethod then
+            genStaticForwarderForDefDef(dd)
+
+          genDefDef(dd)
 
         case tree: Template =>
           val body =
@@ -537,6 +553,37 @@ trait BCodeSkelBuilder extends BCodeHelpers {
 
     } // end of method initJMethod
 
+    private def genStaticForwarderForDefDef(dd: DefDef): Unit =
+      val forwarderDef = makeStaticForwarder(dd)
+      genDefDef(forwarderDef)
+
+    /* Generates a synthetic static forwarder for a trait method.
+     * For a method such as
+     *   def foo(...args: Ts): R
+     * in trait X, we generate the following method:
+     *   static def foo$($this: X, ...args: Ts): R =
+     *     invokespecial $this.X::foo(...args)
+     * We force an invokespecial with the attachment UseInvokeSpecial. It is
+     * necessary to make sure that the call will not follow overrides of foo()
+     * in subtraits and subclasses, since the whole point of this forward is to
+     * encode super calls.
+     */
+    private def makeStaticForwarder(dd: DefDef): DefDef =
+      val origSym = dd.symbol.asTerm
+      val name = traitSuperAccessorName(origSym)
+      val info = origSym.info match
+        case mt: MethodType =>
+          MethodType(nme.SELF :: mt.paramNames, origSym.owner.typeRef :: mt.paramInfos, mt.resType)
+      val sym = origSym.copy(
+        name = name.toTermName,
+        flags = Method | JavaStatic,
+        info = info
+      )
+      tpd.DefDef(sym.asTerm, { paramss =>
+        val params = paramss.head
+        tpd.Apply(params.head.select(origSym), params.tail)
+          .withAttachment(BCodeHelpers.UseInvokeSpecial, ())
+      })
 
     def genDefDef(dd: DefDef): Unit = {
       val rhs = dd.rhs

compiler/src/dotty/tools/dotc/Compiler.scala

@@ -86,7 +86,6 @@ class Compiler {
          new LiftTry,                // Put try expressions that might execute on non-empty stacks into their own methods
          new CollectNullableFields,  // Collect fields that can be nulled out after use in lazy initialization
          new ElimOuterSelect,        // Expand outer selections
-         new AugmentScala2Traits,    // Augments Scala2 traits with additional members needed for mixin composition.
          new ResolveSuper,           // Implement super accessors
          new FunctionXXLForwarders,  // Add forwarders for FunctionXXL apply method
          new ParamForwarding,        // Add forwarders for aliases of superclass parameters
@@ -110,8 +109,7 @@ class Compiler {
                                         // Note: constructors changes decls in transformTemplate, no InfoTransformers should be added after it
          new FunctionalInterfaces,   // Rewrites closures to implement @specialized types of Functions.
          new Instrumentation) ::     // Count closure allocations under -Yinstrument-closures
-    List(new LinkScala2Impls,        // Redirect calls to trait methods defined by Scala 2.x, so that they now go to
-         new LambdaLift,             // Lifts out nested functions to class scope, storing free variables in environments
+    List(new LambdaLift,             // Lifts out nested functions to class scope, storing free variables in environments
                                      // Note: in this mini-phase block scopes are incorrect. No phases that rely on scopes should be here
          new ElimStaticThis,         // Replace `this` references to static objects by global identifiers
          new CountOuterAccesses) ::  // Identify outer accessors that can be dropped

compiler/src/dotty/tools/dotc/core/Flags.scala

@@ -378,13 +378,8 @@ object Flags {
   /** Children were queried on this class */
   val (_, _, ChildrenQueried @ _) = newFlags(56, "<children-queried>")
 
-  /** A module variable (Scala 2.x only)
-   *    /
-   *  A Scala 2.x trait that has been partially augmented.
-   *  This is set in `AugmentScala2Trait` and reset in `LinkScala2Impls`
-   *  when the trait is fully augmented.
-   */
-  val (_, Scala2ModuleVar @ _, Scala2xPartiallyAugmented @ _) = newFlags(57, "<modulevar>", "<scala-2.x-partially-augmented>")
+  /** A module variable (Scala 2.x only) */
+  val (_, Scala2ModuleVar @ _, _) = newFlags(57, "<modulevar>")
 
   /** A macro */
   val (Macro @ _, _, _) = newFlags(58, "<macro>")

compiler/src/dotty/tools/dotc/core/SymDenotations.scala

@@ -1153,7 +1153,7 @@ object SymDenotations {
 
     final def scalacLinkedClass(using Context): Symbol =
       if (this.is(ModuleClass)) companionNamed(effectiveName.toTypeName)
-      else if (this.isClass) companionNamed(effectiveName.moduleClassName).sourceModule.moduleClass
+      else if (this.isClass) companionNamed(effectiveName.moduleClassName)
       else NoSymbol
 
     /** Find companion class symbol with given name, or NoSymbol if none exists.

compiler/src/dotty/tools/dotc/core/unpickleScala2/Scala2Unpickler.scala

@@ -454,8 +454,12 @@ class Scala2Unpickler(bytes: Array[Byte], classRoot: ClassDenotation, moduleClas
       flags = flags &~ Scala2ExpandedName
     }
     if (flags.is(Scala2SuperAccessor)) {
-      name = name.asTermName.unmangle(SuperAccessorName)
-      flags = flags &~ Scala2SuperAccessor
+      /* Scala 2 super accessors are pickled as private, but are compiled as public expanded.
+       * Dotty super accessors, however, are already pickled as public expanded.
+       * We bridge the gap right now.
+       */
+      name = name.asTermName.unmangle(SuperAccessorName).expandedName(owner)
+      flags = flags &~ (Scala2SuperAccessor | Private)
     }
     name = name.mapLast(_.decode)
 
@@ -1310,4 +1314,3 @@ class Scala2Unpickler(bytes: Array[Byte], classRoot: ClassDenotation, moduleClas
         errorBadSignature("expected an TypeDef (" + other + ")")
     }
 }
-

compiler/src/dotty/tools/dotc/transform/AugmentScala2Traits.scala

@@ -9,6 +9,7 @@ import dotty.tools.dotc.core.StdNames._
 import ast._
 import Trees._
 import Flags._
+import NameOps._
 import SymUtils._
 import Symbols._
 import Decorators._
@@ -204,7 +205,33 @@ class Constructors extends MiniPhase with IdentityDenotTransformer { thisPhase =
                 initFlags = sym.flags &~ Private,
                 owner = constr.symbol).installAfter(thisPhase)
               constrStats += intoConstr(stat, sym)
-            }
+            } else
+              dropped += sym
+          case stat @ DefDef(name, _, _, tpt, _)
+              if stat.symbol.isGetter && stat.symbol.owner.is(Trait) && !stat.symbol.is(Lazy) =>
+            val sym = stat.symbol
+            assert(isRetained(sym), sym)
+            if (!stat.rhs.isEmpty && !isWildcardArg(stat.rhs))
+              /* !!! Work around #9390
+               * This should really just be `sym.setter`. However, if we do that, we'll miss
+               * setters for mixed in `private var`s. Even though the scope clearly contains the
+               * setter symbol with the correct Name structure (since the `find` finds it),
+               * `.decl(setterName)` used by `.setter` through `.accessorNamed` will *not* find it.
+               * Could it be that the hash table of the `Scope` is corrupted?
+               * We still try `sym.setter` first as an optimization, since it will work for all
+               * public vars in traits and all (public or private) vars in classes.
+               */
+              val symSetter =
+                if sym.setter.exists then
+                  sym.setter
+                else
+                  val setterName = sym.asTerm.name.setterName
+                  sym.owner.info.decls.find(d => d.is(Accessor) && d.name == setterName)
+              val setter =
+                if (symSetter.exists) symSetter
+                else sym.accessorNamed(Mixin.traitSetterName(sym.asTerm))
+              constrStats += Apply(ref(setter), intoConstr(stat.rhs, sym).withSpan(stat.span) :: Nil)
+            clsStats += cpy.DefDef(stat)(rhs = EmptyTree)
           case DefDef(nme.CONSTRUCTOR, _, ((outerParam @ ValDef(nme.OUTER, _, _)) :: _) :: Nil, _, _) =>
             clsStats += mapOuter(outerParam.symbol).transform(stat)
           case _: DefTree =>

compiler/src/dotty/tools/dotc/transform/Constructors.scala

@@ -215,9 +215,9 @@ object ExplicitOuter {
 
   /** Class is always instantiated in the compilation unit where it is defined */
   private def hasLocalInstantiation(cls: ClassSymbol)(using Context): Boolean =
-    // scala2x modules always take an outer pointer(as of 2.11)
-    // dotty modules are always locally instantiated
-    cls.owner.isTerm || cls.is(Private) || cls.is(Module, butNot = Scala2x)
+    // Modules are normally locally instantiated, except if they are declared in a trait,
+    // in which case they will be instantiated in the classes that mix in the trait.
+    cls.owner.isTerm || cls.is(Private, butNot = Module) || (cls.is(Module) && !cls.owner.is(Trait))
 
   /** The outer parameter accessor of cass `cls` */
   private def outerParamAccessor(cls: ClassSymbol)(using Context): TermSymbol =