New trait encoding: use default methods, jettison impl classes

Until now, concrete methods in traits were encoded with
"trait implementation classes".

  - Such a trait would compile to two class files
    - the trait interface, a Java interface, and
    - the implementation class, containing "trait implementation methods"
  - trait implementation methods are static methods has an explicit self
    parameter.
  - some methods don't require addition of an interface method, such as
    private methods. Calls to these directly call the implementation method
  - classes that mixin a trait install "trait forwarders", which implement
    the abstract method in the interface by forwarding to the trait
    implementation method.

The new encoding:
  - no longer emits trait implementation classes or trait implementation
    methods.
  - instead, concrete methods are simply retained in the interface, as JVM 8
    default interface methods (the JVM spec changes in
    [JSR-335](http://download.oracle.com/otndocs/jcp/lambda-0_9_3-fr-eval-spec/index.html)
    pave the way)
  - use `invokespecial` to call private or particular super implementations
    of a method (rather `invokestatic`)
    - in cases when we `invokespecial` to a method in an indirect ancestor, we add
    that ancestor redundantly as a direct parent. We are investigating alternatives
    approaches here.
  - we still emit trait fowrarders, although we are
    [investigating](scala/scala-dev#98) ways to only do
    this when the JVM would be unable to resolve the correct method using its rules
    for default method resolution.

Here's an example:

```
trait T {
  println("T")
  def m1 = m2
  private def m2 = "m2"
}
trait U extends T {
  println("T")
  override def m1 = super[T].m1
}
class C extends U {
  println("C")
  def test = m1
}
```

The old and new encodings are displayed and diffed here: https://gist.github.com/retronym/f174d23f859f0e053580

Some notes in the implementation:

  - No need to filter members from class decls at all in AddInterfaces
    (although we do have to trigger side effecting info transformers)
  - We can now emit an EnclosingMethod attribute for classes nested
    in private trait methods
  - Created a factory method for an AST shape that is used in
    a number of places to symbolically bind to a particular
    super method without needed to specify the qualifier of
    the `Super` tree (which is too limiting, as it only allows
    you to refer to direct parents.)
    - I also found a similar tree shape created in Delambdafy,
      that is better expressed with an existing tree creation
      factory method, mkSuperInit.

Author: retronym

src/compiler/scala/tools/nsc/ast/TreeGen.scala

@@ -145,6 +145,7 @@ abstract class TreeGen extends scala.reflect.internal.TreeGen with TreeDSL {
   override def mkCast(tree: Tree, pt: Type): Tree = {
     debuglog("casting " + tree + ":" + tree.tpe + " to " + pt + " at phase: " + phase)
     assert(!tree.tpe.isInstanceOf[MethodType], tree)
+    assert(!pt.isInstanceOf[MethodType], tree)
     assert(pt eq pt.normalize, tree +" : "+ debugString(pt) +" ~>"+ debugString(pt.normalize))
     atPos(tree.pos) {
       mkAsInstanceOf(tree, pt, any = !phase.next.erasedTypes, wrapInApply = isAtPhaseAfter(currentRun.uncurryPhase))

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

@@ -535,6 +535,22 @@ abstract class BCodeBodyBuilder extends BCodeSkelBuilder {
     private def genApply(app: Apply, expectedType: BType): BType = {
       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), _) =>
@@ -582,19 +598,19 @@ abstract class BCodeBodyBuilder extends BCodeSkelBuilder {
 
           generatedType = genTypeApply()
 
-        // '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).
-        case Apply(fun @ Select(Super(_, _), _), args) =>
-          val invokeStyle = InvokeStyle.Super
-          // if (fun.symbol.isConstructor) Static(true) else SuperCall(mix);
-          mnode.visitVarInsn(asm.Opcodes.ALOAD, 0)
-          genLoadArguments(args, paramTKs(app))
-          genCallMethod(fun.symbol, invokeStyle, app.pos)
-          generatedType = methodBTypeFromSymbol(fun.symbol).returnType
+        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
+          }
+          genSuperApply(hostClass, fun.symbol, args)
 
         // 'new' constructor call: Note: since constructors are
         // thought to return an instance of what they construct,
@@ -1050,19 +1066,26 @@ abstract class BCodeBodyBuilder extends BCodeSkelBuilder {
       hostSymbol.info ; methodOwner.info
 
       def needsInterfaceCall(sym: Symbol) = (
-           sym.isInterface
+           sym.isTraitOrInterface
         || sym.isJavaDefined && sym.isNonBottomSubClass(definitions.ClassfileAnnotationClass)
       )
 
+      val isTraitCallToObjectMethod =
+        hostSymbol != methodOwner && methodOwner.isTraitOrInterface && ObjectTpe.decl(method.name) != NoSymbol && method.overrideChain.last.owner == ObjectClass
+
       // whether to reference the type of the receiver or
       // the type of the method owner
-      val useMethodOwner = (
+      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)
+
+      if (style.isSuper && (isTraitCallToObjectMethod || receiver.isTraitOrInterface) && !cnode.interfaces.contains(jowner))
+        cnode.interfaces.add(jowner)
+
       val jname    = method.javaSimpleName.toString
       val bmType   = methodBTypeFromSymbol(method)
       val mdescr   = bmType.descriptor
@@ -1342,7 +1365,7 @@ abstract class BCodeBodyBuilder extends BCodeSkelBuilder {
       def asmType(sym: Symbol) = classBTypeFromSymbol(sym).toASMType
 
       val implMethodHandle =
-        new asm.Handle(if (lambdaTarget.hasFlag(Flags.STATIC)) asm.Opcodes.H_INVOKESTATIC else asm.Opcodes.H_INVOKEVIRTUAL,
+        new asm.Handle(if (lambdaTarget.hasFlag(Flags.STATIC)) asm.Opcodes.H_INVOKESTATIC else if (lambdaTarget.owner.isTrait) asm.Opcodes.H_INVOKEINTERFACE else asm.Opcodes.H_INVOKEVIRTUAL,
           classBTypeFromSymbol(lambdaTarget.owner).internalName,
           lambdaTarget.name.toString,
           methodBTypeFromSymbol(lambdaTarget).descriptor)

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

@@ -32,7 +32,7 @@ abstract class BCodeHelpers extends BCodeIdiomatic with BytecodeWriters {
    * the InnerClass / EnclosingMethod classfile attributes. See comment in BTypes.
    */
   def considerAsTopLevelImplementationArtifact(classSym: Symbol) =
-    classSym.isImplClass || classSym.isSpecialized
+    classSym.isSpecialized
 
   /**
    * Cache the value of delambdafy == "inline" for each run. We need to query this value many
@@ -145,15 +145,12 @@ abstract class BCodeHelpers extends BCodeIdiomatic with BytecodeWriters {
     assert(classSym.isClass, classSym)
 
     def doesNotExist(method: Symbol) = {
-      // (1) SI-9124, some trait methods don't exist in the generated interface. see comment in BTypes.
-      // (2) Value classes. Member methods of value classes exist in the generated box class. However,
+      //     Value classes. Member methods of value classes exist in the generated box class. However,
       //     nested methods lifted into a value class are moved to the companion object and don't exist
       //     in the value class itself. We can identify such nested methods: the initial enclosing class
       //     is a value class, but the current owner is some other class (the module class).
-      method.owner.isTrait && method.isImplOnly || { // (1)
-        val enclCls = nextEnclosingClass(method)
-        exitingPickler(enclCls.isDerivedValueClass) && method.owner != enclCls // (2)
-      }
+      val enclCls = nextEnclosingClass(method)
+      exitingPickler(enclCls.isDerivedValueClass) && method.owner != enclCls
     }
 
     def enclosingMethod(sym: Symbol): Option[Symbol] = {
@@ -248,7 +245,7 @@ abstract class BCodeHelpers extends BCodeIdiomatic with BytecodeWriters {
    * Build the [[InlineInfo]] for a class symbol.
    */
   def buildInlineInfoFromClassSymbol(classSym: Symbol, classSymToInternalName: Symbol => InternalName, methodSymToDescriptor: Symbol => String): InlineInfo = {
-    val traitSelfType = if (classSym.isTrait && !classSym.isImplClass) {
+    val traitSelfType = if (classSym.isTrait) {
       // The mixin phase uses typeOfThis for the self parameter in implementation class methods.
       val selfSym = classSym.typeOfThis.typeSymbol
       if (selfSym != classSym) Some(classSymToInternalName(selfSym)) else None
@@ -259,7 +256,7 @@ abstract class BCodeHelpers extends BCodeIdiomatic with BytecodeWriters {
     val isEffectivelyFinal = classSym.isEffectivelyFinal
 
     val sam = {
-      if (classSym.isImplClass || classSym.isEffectivelyFinal) None
+      if (classSym.isEffectivelyFinal) None
       else {
         // Phase travel necessary. For example, nullary methods (getter of an abstract val) get an
         // empty parameter list in later phases and would therefore be picked as SAM.
@@ -284,41 +281,15 @@ abstract class BCodeHelpers extends BCodeIdiomatic with BytecodeWriters {
           val name      = methodSym.javaSimpleName.toString // same as in genDefDef
           val signature = name + methodSymToDescriptor(methodSym)
 
-          // Some detours are required here because of changing flags (lateDEFERRED, lateMODULE):
+          // Some detours are required here because of changing flags (lateDEFERRED):
           // 1. Why the phase travel? Concrete trait methods obtain the lateDEFERRED flag in Mixin.
           //    This makes isEffectivelyFinalOrNotOverridden false, which would prevent non-final
           //    but non-overridden methods of sealed traits from being inlined.
-          // 2. Why the special case for `classSym.isImplClass`? Impl class symbols obtain the
-          //    lateMODULE flag during Mixin. During the phase travel to exitingPickler, the late
-          //    flag is ignored. The members are therefore not isEffectivelyFinal (their owner
-          //    is not a module). Since we know that all impl class members are static, we can
-          //    just take the shortcut.
-          val effectivelyFinal = classSym.isImplClass || exitingPickler(methodSym.isEffectivelyFinalOrNotOverridden)
-
-          // Identify trait interface methods that have a static implementation in the implementation
-          // class. Invocations of these methods can be re-wrired directly to the static implementation
-          // if they are final or the receiver is known.
-          //
-          // Using `erasure.needsImplMethod` is not enough: it keeps field accessors, module getters
-          // and super accessors. When AddInterfaces creates the impl class, these methods are
-          // initially added to it.
-          //
-          // The mixin phase later on filters out most of these members from the impl class (see
-          // Mixin.isImplementedStatically). However, accessors for concrete lazy vals remain in the
-          // impl class after mixin. So the filter in mixin is not exactly what we need here (we
-          // want to identify concrete trait methods, not any accessors). So we check some symbol
-          // properties manually.
-          val traitMethodWithStaticImplementation = {
-            import symtab.Flags._
-            classSym.isTrait && !classSym.isImplClass &&
-              erasure.needsImplMethod(methodSym) &&
-              !methodSym.isModule &&
-              !(methodSym hasFlag (ACCESSOR | SUPERACCESSOR))
-          }
+          val effectivelyFinal = exitingPickler(methodSym.isEffectivelyFinalOrNotOverridden) && !(methodSym.owner.isTrait && methodSym.isModule)
 
           val info = MethodInlineInfo(
             effectivelyFinal                    = effectivelyFinal,
-            traitMethodWithStaticImplementation = traitMethodWithStaticImplementation,
+            traitMethodWithStaticImplementation = false,
             annotatedInline                     = methodSym.hasAnnotation(ScalaInlineClass),
             annotatedNoInline                   = methodSym.hasAnnotation(ScalaNoInlineClass)
           )
@@ -866,7 +837,6 @@ abstract class BCodeHelpers extends BCodeIdiomatic with BytecodeWriters {
       || sym.isArtifact
       || sym.isLiftedMethod
       || sym.isBridge
-      || (sym.ownerChain exists (_.isImplClass))
     )
 
     /* @return

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

@@ -174,7 +174,7 @@ abstract class BCodeSkelBuilder extends BCodeHelpers {
           if (lmoc != NoSymbol) {
             // it must be a top level class (name contains no $s)
             val isCandidateForForwarders = {
-              exitingPickler { !(lmoc.name.toString contains '$') && lmoc.hasModuleFlag && !lmoc.isImplClass && !lmoc.isNestedClass }
+              exitingPickler { !(lmoc.name.toString contains '$') && lmoc.hasModuleFlag && !lmoc.isNestedClass }
             }
             if (isCandidateForForwarders) {
               log(s"Adding static forwarders from '$claszSymbol' to implementations in '$lmoc'")
@@ -563,7 +563,7 @@ abstract class BCodeSkelBuilder extends BCodeHelpers {
       }
 
       val isNative         = methSymbol.hasAnnotation(definitions.NativeAttr)
-      val isAbstractMethod = (methSymbol.isDeferred || methSymbol.owner.isInterface) && !methSymbol.hasFlag(Flags.JAVA_DEFAULTMETHOD)
+      val isAbstractMethod = rhs == EmptyTree
       val flags = GenBCode.mkFlags(
         javaFlags(methSymbol),
         if (isAbstractMethod)        asm.Opcodes.ACC_ABSTRACT   else 0,

src/compiler/scala/tools/nsc/backend/jvm/BTypesFromSymbols.scala

@@ -172,7 +172,6 @@ class BTypesFromSymbols[G <: Global](val global: G) extends BTypes {
      */
     def primitiveOrClassToBType(sym: Symbol): BType = {
       assertClassNotArray(sym)
-      assert(!sym.isImplClass, sym)
       primitiveTypeToBType.getOrElse(sym, classBTypeFromSymbol(sym))
     }
 
@@ -337,7 +336,7 @@ class BTypesFromSymbols[G <: Global](val global: G) extends BTypes {
     // Check for hasAnnotationFlag for SI-9393: the classfile / java source parsers add
     // scala.annotation.Annotation as superclass to java annotations. In reality, java
     // annotation classfiles have superclass Object (like any interface classfile).
-    val superClassSym = if (classSym.isImplClass || classSym.hasJavaAnnotationFlag) ObjectClass else {
+    val superClassSym = if (classSym.hasJavaAnnotationFlag) ObjectClass else {
       val sc = classSym.superClass
       // SI-9393: Java annotation classes don't have the ABSTRACT/INTERFACE flag, so they appear
       // (wrongly) as superclasses. Fix this for BTypes: the java annotation will appear as interface
@@ -603,11 +602,7 @@ class BTypesFromSymbols[G <: Global](val global: G) extends BTypes {
    */
   final def isTopLevelModuleClass(sym: Symbol): Boolean = exitingPickler {
     // phase travel to pickler required for isNestedClass (looks at owner)
-    val r = sym.isModuleClass && !sym.isNestedClass
-    // The mixin phase adds the `lateMODULE` flag to trait implementation classes. Since the flag
-    // is late, it should not be visible here inside the time travel. We check this.
-    if (r) assert(!sym.isImplClass, s"isModuleClass should be false for impl class $sym")
-    r
+    sym.isModuleClass && !sym.isNestedClass
   }
 
   /**
@@ -684,7 +679,7 @@ class BTypesFromSymbols[G <: Global](val global: G) extends BTypes {
 
     val finalFlag = (
       (((sym.rawflags & symtab.Flags.FINAL) != 0) || isTopLevelModuleClass(sym))
-        && !sym.enclClass.isInterface
+        && !sym.enclClass.isTrait
         && !sym.isClassConstructor
         && !sym.isMutable // lazy vals and vars both
       )
@@ -697,12 +692,12 @@ class BTypesFromSymbols[G <: Global](val global: G) extends BTypes {
     GenBCode.mkFlags(
       if (privateFlag) ACC_PRIVATE else ACC_PUBLIC,
       if ((sym.isDeferred && !sym.hasFlag(symtab.Flags.JAVA_DEFAULTMETHOD))|| sym.hasAbstractFlag) ACC_ABSTRACT else 0,
-      if (sym.isInterface) ACC_INTERFACE else 0,
+      if (sym.isTraitOrInterface) ACC_INTERFACE else 0,
       if (finalFlag && !sym.hasAbstractFlag) ACC_FINAL else 0,
       if (sym.isStaticMember) ACC_STATIC else 0,
       if (sym.isBridge) ACC_BRIDGE | ACC_SYNTHETIC else 0,
       if (sym.isArtifact) ACC_SYNTHETIC else 0,
-      if (sym.isClass && !sym.isInterface) ACC_SUPER else 0,
+      if (sym.isClass && !sym.isTraitOrInterface) ACC_SUPER else 0,
       if (sym.hasJavaEnumFlag) ACC_ENUM else 0,
       if (sym.isVarargsMethod) ACC_VARARGS else 0,
       if (sym.hasFlag(symtab.Flags.SYNCHRONIZED)) ACC_SYNCHRONIZED else 0,

src/compiler/scala/tools/nsc/backend/jvm/opt/CallGraph.scala

@@ -137,7 +137,7 @@ class CallGraph[BT <: BTypes](val btypes: BT) {
                   callee = method,
                   calleeDeclarationClass = declarationClassBType,
                   safeToInline = safeToInline,
-                  safeToRewrite = safeToRewrite,
+                  safeToRewrite = false,
                   canInlineFromSource = canInlineFromSource,
                   annotatedInline = annotatedInline,
                   annotatedNoInline = annotatedNoInline,
@@ -299,7 +299,7 @@ class CallGraph[BT <: BTypes](val btypes: BT) {
               receiverType.info.orThrow.inlineInfo.isEffectivelyFinal // (1)
           }
 
-          val isRewritableTraitCall = isStaticallyResolved && methodInlineInfo.traitMethodWithStaticImplementation
+          val isRewritableTraitCall = false
 
           val warning = calleeDeclarationClassBType.info.orThrow.inlineInfo.warning.map(
             MethodInlineInfoIncomplete(calleeDeclarationClassBType.internalName, calleeMethodNode.name, calleeMethodNode.desc, _))

src/compiler/scala/tools/nsc/backend/jvm/opt/Inliner.scala

@@ -27,7 +27,7 @@ class Inliner[BT <: BTypes](val btypes: BT) {
   import backendUtils._
 
   def runInliner(): Unit = {
-    rewriteFinalTraitMethodInvocations()
+//    rewriteFinalTraitMethodInvocations()
 
     for (request <- collectAndOrderInlineRequests) {
       val Right(callee) = request.callsite.callee // collectAndOrderInlineRequests returns callsites with a known callee