Add bridges for overridden methods in lambda indy call

If a SAM trait's abstract method overrides a method in a
supertrait while changing the return type, the generated
invokedynamic instruction needs to pass the types of the
overridden methods to `LambdaMetaFactory` so that bridge
methods can be added to the generated lambda.

SAM `Function` trees now have a synthetic `ClassSymbol`,
into which we enter an overriding concrete method symbol
that represents the runtime LMF-generated implementation
of that function. `erasure` then computes bridge methods
for that class, which are added to the `LMF.metafactory`
call in `jvm`.

Java does this instead by generating a default method in
the subinterface overriding the superinterface's method.
Theoretically, we could generate the bridges in the same
way, but that has the downside that the project with the
interfaces would need recompiled, which might not be the
project that creates the lambdas. Generating bridges the
LMF way means that only the classes affected by this bug
need to be recompiled to get the fix.

Honestly I'm surprised that this hasn't come up already.

Fixes scala/bug#10512.

Author: hrhino

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

@@ -626,7 +626,7 @@ abstract class BCodeBodyBuilder extends BCodeSkelBuilder {
         case Apply(fun, args) if app.hasAttachment[delambdafy.LambdaMetaFactoryCapable] =>
           val attachment = app.attachments.get[delambdafy.LambdaMetaFactoryCapable].get
           genLoadArguments(args, paramTKs(app))
-          genInvokeDynamicLambda(attachment.target, attachment.arity, attachment.functionalInterface, attachment.sam, attachment.isSerializable, attachment.addScalaSerializableMarker)
+          genInvokeDynamicLambda(attachment)
           generatedType = methodBTypeFromSymbol(fun.symbol).returnType
 
         case Apply(fun, List(expr)) if currentRun.runDefinitions.isBox(fun.symbol) =>
@@ -1332,7 +1332,9 @@ abstract class BCodeBodyBuilder extends BCodeSkelBuilder {
     def genSynchronized(tree: Apply, expectedType: BType): BType
     def genLoadTry(tree: Try): BType
 
-    def genInvokeDynamicLambda(lambdaTarget: Symbol, arity: Int, functionalInterface: Symbol, sam: Symbol, isSerializable: Boolean, addScalaSerializableMarker: Boolean) {
+    def genInvokeDynamicLambda(canLMF: delambdafy.LambdaMetaFactoryCapable) = {
+      import canLMF._
+
       val isStaticMethod = lambdaTarget.hasFlag(Flags.STATIC)
       def asmType(sym: Symbol) = classBTypeFromSymbol(sym).toASMType
 
@@ -1349,23 +1351,37 @@ abstract class BCodeBodyBuilder extends BCodeSkelBuilder {
       val constrainedType = MethodBType(lambdaParams.map(p => typeToBType(p.tpe)), typeToBType(lambdaTarget.tpe.resultType)).toASMType
       val samMethodType = methodBTypeFromSymbol(sam).toASMType
       val markers = if (addScalaSerializableMarker) classBTypeFromSymbol(definitions.SerializableClass).toASMType :: Nil else Nil
-      visitInvokeDynamicInsnLMF(bc.jmethod, sam.name.toString, invokedType, samMethodType, implMethodHandle, constrainedType, isSerializable, markers)
+      val overriddenMethods =
+        bridges.map(b => methodBTypeFromSymbol(b).toASMType)
+      visitInvokeDynamicInsnLMF(bc.jmethod, sam.name.toString, invokedType, samMethodType, implMethodHandle, constrainedType, overriddenMethods, isSerializable, markers)
       if (isSerializable)
         addIndyLambdaImplMethod(cnode.name, implMethodHandle)
     }
   }
 
   private def visitInvokeDynamicInsnLMF(jmethod: MethodNode, samName: String, invokedType: String, samMethodType: asm.Type,
-                                        implMethodHandle: asm.Handle, instantiatedMethodType: asm.Type,
-                                        serializable: Boolean, markerInterfaces: Seq[asm.Type]) = {
+                                        implMethodHandle: asm.Handle, instantiatedMethodType: asm.Type, overriddenMethodTypes: Seq[asm.Type],
+                                        serializable: Boolean, markerInterfaces: Seq[asm.Type]): Unit = {
     import java.lang.invoke.LambdaMetafactory.{FLAG_BRIDGES, FLAG_MARKERS, FLAG_SERIALIZABLE}
     // scala/bug#10334: make sure that a lambda object for `T => U` has a method `apply(T)U`, not only the `(Object)Object`
     // version. Using the lambda a structural type `{def apply(t: T): U}` causes a reflective lookup for this method.
-    val needsBridge = samMethodType != instantiatedMethodType
-    val bridges = if (needsBridge) Seq(Int.box(1), instantiatedMethodType) else Nil
+    val needsGenericBridge = samMethodType != instantiatedMethodType
+    // scala/bug#10512: any methods which `samMethod` overrides need bridges made for them
+    // this is done automatically during erasure for classes we generate, but LMF needs to have them explicitly mentioned
+    // so we have to compute them at this relatively late point.
+    val bridges = (
+      if (needsGenericBridge)
+        instantiatedMethodType +: overriddenMethodTypes
+      else overriddenMethodTypes
+    ).distinct.filterNot(_ == samMethodType)
+
+    /* We're saving on precious BSM arg slots by not passing 0 as the bridge count */
+    val bridgeArgs = if (bridges.nonEmpty) Int.box(bridges.length) +: bridges else Nil
+
     def flagIf(b: Boolean, flag: Int): Int = if (b) flag else 0
-    val flags = FLAG_MARKERS | flagIf(serializable, FLAG_SERIALIZABLE) | flagIf(needsBridge, FLAG_BRIDGES)
-    val bsmArgs = Seq(samMethodType, implMethodHandle, instantiatedMethodType, Int.box(flags), Int.box(markerInterfaces.length)) ++ markerInterfaces ++ bridges
+    val flags = FLAG_MARKERS | flagIf(serializable, FLAG_SERIALIZABLE) | flagIf(bridges.nonEmpty, FLAG_BRIDGES)
+    val bsmArgs = Seq(samMethodType, implMethodHandle, instantiatedMethodType, Int.box(flags), Int.box(markerInterfaces.length)) ++ markerInterfaces ++ bridgeArgs
+
     jmethod.visitInvokeDynamicInsn(samName, invokedType, lambdaMetaFactoryAltMetafactoryHandle, bsmArgs: _*)
   }
 

src/compiler/scala/tools/nsc/transform/Delambdafy.scala

@@ -28,7 +28,7 @@ abstract class Delambdafy extends Transform with TypingTransformers with ast.Tre
   /** the following two members override abstract members in Transform */
   val phaseName: String = "delambdafy"
 
-  final case class LambdaMetaFactoryCapable(target: Symbol, arity: Int, functionalInterface: Symbol, sam: Symbol, isSerializable: Boolean, addScalaSerializableMarker: Boolean)
+  final case class LambdaMetaFactoryCapable(lambdaTarget: Symbol, arity: Int, functionalInterface: Symbol, sam: Symbol, bridges: List[Symbol], isSerializable: Boolean, addScalaSerializableMarker: Boolean)
 
   /**
     * Get the symbol of the target lifted lambda body method from a function. I.e. if
@@ -59,7 +59,10 @@ abstract class Delambdafy extends Transform with TypingTransformers with ast.Tre
     private[this] lazy val methodReferencesThis: Set[Symbol] =
       (new ThisReferringMethodsTraverser).methodReferencesThisIn(unit.body)
 
-    private def mkLambdaMetaFactoryCall(fun: Function, target: Symbol, functionalInterface: Symbol, samUserDefined: Symbol, isSpecialized: Boolean): Tree = {
+    private def mkLambdaMetaFactoryCall(fun: Function, target: Symbol, functionalInterface: Symbol, samUserDefined: Symbol, userSamCls: Symbol, isSpecialized: Boolean): Tree = {
+      /* user-defined SAM types should have gotten a class symbol made for them in `typer` */
+      assert(isFunctionType(fun.tpe) || (samUserDefined.exists && userSamCls.isClass), s"$fun / ${fun.symbol} / ${fun.tpe}")
+
       val pos = fun.pos
       def isSelfParam(p: Symbol) = p.isSynthetic && p.name == nme.SELF
       val hasSelfParam = isSelfParam(target.firstParam)
@@ -98,13 +101,21 @@ abstract class Delambdafy extends Transform with TypingTransformers with ast.Tre
       val isSerializable = samUserDefined == NoSymbol || samUserDefined.owner.isNonBottomSubClass(definitions.JavaSerializableClass)
       val addScalaSerializableMarker = samUserDefined == NoSymbol
 
+      val samBridges = logResultIf[List[Symbol]](s"will add SAM bridges for $fun", _.nonEmpty) {
+        userSamCls.fold[List[Symbol]](Nil) {
+          _.info.decls.collect {
+            case bridge: MethodSymbol if bridge hasFlag BRIDGE => bridge
+          }.toList
+        }
+      }
+
       // The backend needs to know the target of the lambda and the functional interface in order
       // to emit the invokedynamic instruction. We pass this information as tree attachment.
       //
       // see https://docs.oracle.com/javase/8/docs/api/java/lang/invoke/LambdaMetafactory.html
       //   instantiatedMethodType is derived from lambdaTarget's signature
       //   samMethodType is derived from samOf(functionalInterface)'s signature
-      apply.updateAttachment(LambdaMetaFactoryCapable(lambdaTarget, fun.vparams.length, functionalInterface, sam, isSerializable, addScalaSerializableMarker))
+      apply.updateAttachment(LambdaMetaFactoryCapable(lambdaTarget, fun.vparams.length, functionalInterface, sam, samBridges, isSerializable, addScalaSerializableMarker))
 
       apply
     }
@@ -254,8 +265,11 @@ abstract class Delambdafy extends Transform with TypingTransformers with ast.Tre
           (functionalInterface, isSpecialized)
         }
 
-      val sam = originalFunction.attachments.get[SAMFunction].map(_.sam).getOrElse(NoSymbol)
-      mkLambdaMetaFactoryCall(originalFunction, target, functionalInterface, sam, isSpecialized)
+      val (sam, samCls) = originalFunction.attachments.get[SAMFunction] match {
+        case Some(SAMFunction(_, sam, samCls)) => (sam,      samCls)
+        case None                              => (NoSymbol, NoSymbol)
+      }
+      mkLambdaMetaFactoryCall(originalFunction, target, functionalInterface, sam, samCls, isSpecialized)
     }
 
     // here's the main entry point of the transform

src/compiler/scala/tools/nsc/transform/Erasure.scala

@@ -467,7 +467,7 @@ abstract class Erasure extends InfoTransform
 
   override def newTyper(context: Context) = new Eraser(context)
 
-  class ComputeBridges(unit: CompilationUnit, root: Symbol) {
+  class EnterBridges(unit: CompilationUnit, root: Symbol) {
 
     class BridgesCursor(root: Symbol) extends overridingPairs.Cursor(root) {
       override def parents              = root.info.firstParent :: Nil
@@ -478,22 +478,19 @@ abstract class Erasure extends InfoTransform
       override def exclude(sym: Symbol) = !sym.isMethod || super.exclude(sym)
     }
 
-    var toBeRemoved  = immutable.Set[Symbol]()
     val site         = root.thisType
     val bridgesScope = newScope
     val bridgeTarget = mutable.HashMap[Symbol, Symbol]()
-    var bridges      = List[Tree]()
 
     val opc = enteringExplicitOuter { new BridgesCursor(root) }
 
-    def compute(): (List[Tree], immutable.Set[Symbol]) = {
+    def computeAndEnter(includeDeferred: Boolean): Unit = {
       while (opc.hasNext) {
-        if (enteringExplicitOuter(!opc.low.isDeferred))
-          checkPair(opc.currentPair)
+        if (includeDeferred || enteringExplicitOuter(!opc.low.isDeferred))
+          checkPair(opc. currentPair)
 
         opc.next()
       }
-      (bridges, toBeRemoved)
     }
 
     /** Check that a bridge only overrides members that are also overridden by the original member.
@@ -597,17 +594,35 @@ abstract class Erasure extends InfoTransform
 
       if (shouldAdd) {
         exitingErasure(root.info.decls enter bridge)
-        if (other.owner == root) {
-          exitingErasure(root.info.decls.unlink(other))
-          toBeRemoved += other
-        }
 
         bridgesScope enter bridge
-        bridges ::= makeBridgeDefDef(bridge, member, other)
+        addBridge(bridge, member, other)
+        //bridges ::= makeBridgeDefDef(bridge, member, other)
       }
     }
 
-    def makeBridgeDefDef(bridge: Symbol, member: Symbol, other: Symbol) = exitingErasure {
+    protected def addBridge(bridge: Symbol, member: Symbol, other: Symbol) {} // hook for GenerateBridges
+  }
+
+  class GenerateBridges(unit: CompilationUnit, root: Symbol) extends EnterBridges(unit, root) {
+
+    var bridges      = List.empty[Tree]
+    var toBeRemoved  = immutable.Set.empty[Symbol]
+
+    def generate(includeDeferred: Boolean): (List[Tree], immutable.Set[Symbol]) = {
+      super.computeAndEnter(includeDeferred)
+      (bridges, toBeRemoved)
+    }
+
+    override def addBridge(bridge: Symbol, member: Symbol, other: Symbol): Unit = {
+      if (other.owner == root) {
+        exitingErasure(root.info.decls.unlink(other))
+        toBeRemoved += other
+      }
+      bridges ::= makeBridgeDefDef(bridge, member, other)
+    }
+
+    final def makeBridgeDefDef(bridge: Symbol, member: Symbol, other: Symbol) = exitingErasure {
       // type checking ensures we can safely call `other`, but unless `member.tpe <:< other.tpe`,
       // calling `member` is not guaranteed to succeed in general, there's
       // nothing we can do about this, except for an unapply: when this subtype test fails,
@@ -617,8 +632,8 @@ abstract class Erasure extends InfoTransform
       // does the first argument list have exactly one argument -- for user-defined unapplies we can't be sure
       def maybeWrap(bridgingCall: Tree): Tree = {
         val guardExtractor = ( // can't statically know which member is going to be selected, so don't let this depend on member.isSynthetic
-             (member.name == nme.unapply || member.name == nme.unapplySeq)
-          && !exitingErasure((member.tpe <:< other.tpe))) // no static guarantees (TODO: is the subtype test ever true?)
+          (member.name == nme.unapply || member.name == nme.unapplySeq)
+            && !exitingErasure((member.tpe <:< other.tpe))) // no static guarantees (TODO: is the subtype test ever true?)
 
         import CODE._
         val _false    = FALSE
@@ -643,6 +658,7 @@ abstract class Erasure extends InfoTransform
       }
       DefDef(bridge, rhs)
     }
+
   }
 
   /** The modifier typer which retypes with erased types. */
@@ -806,7 +822,7 @@ abstract class Erasure extends InfoTransform
       tree1 match {
         case fun: Function =>
           fun.attachments.get[SAMFunction] match {
-            case Some(SAMFunction(samTp, _)) => fun setType specialScalaErasure(samTp)
+            case Some(SAMFunction(samTp, _, _)) => fun setType specialScalaErasure(samTp)
             case _ => fun
           }
 
@@ -934,17 +950,24 @@ abstract class Erasure extends InfoTransform
      */
     private def bridgeDefs(owner: Symbol): (List[Tree], immutable.Set[Symbol]) = {
       assert(phase == currentRun.erasurePhase, phase)
-      new ComputeBridges(unit, owner) compute()
+      new GenerateBridges(unit, owner).generate(includeDeferred = false)
     }
 
-    def addBridges(stats: List[Tree], base: Symbol): List[Tree] =
+    def addBridgesToTemplate(stats: List[Tree], base: Symbol): List[Tree] =
       if (base.isTrait) stats
       else {
         val (bridges, toBeRemoved) = bridgeDefs(base)
         if (bridges.isEmpty) stats
         else (stats filterNot (stat => toBeRemoved contains stat.symbol)) ::: bridges
       }
 
+    def addBridgesToLambda(lambdaClass: Symbol): Unit = {
+      assert(phase == currentRun.erasurePhase, phase)
+      assert(lambdaClass.isClass, lambdaClass)
+      val eb = new EnterBridges(unit, lambdaClass)
+      eb.computeAndEnter(includeDeferred = true)
+    }
+
     /**  Transform tree at phase erasure before retyping it.
      *   This entails the following:
      *
@@ -1221,7 +1244,7 @@ abstract class Erasure extends InfoTransform
         case Template(parents, self, body) =>
           //Console.println("checking no dble defs " + tree)//DEBUG
           checkNoDoubleDefs(tree.symbol.owner)
-          treeCopy.Template(tree, parents, noSelfType, addBridges(body, currentOwner))
+          treeCopy.Template(tree, parents, noSelfType, addBridgesToTemplate(body, currentOwner))
 
         case Match(selector, cases) =>
           Match(Typed(selector, TypeTree(selector.tpe)), cases)
@@ -1242,6 +1265,12 @@ abstract class Erasure extends InfoTransform
         case TypeDef(_, _, _, _) =>
           EmptyTree
 
+        case fun: Function =>
+          fun.attachments.get[SAMFunction] foreach {
+            samf => addBridgesToLambda(samf.samCls)
+          }
+          fun
+
         case _ =>
           tree
       }

src/compiler/scala/tools/nsc/transform/UnCurry.scala

@@ -81,7 +81,7 @@ abstract class UnCurry extends InfoTransform
     private def mustExpandFunction(fun: Function) = {
       // (TODO: Can't use isInterface, yet, as it hasn't been updated for the new trait encoding)
       val canUseLambdaMetaFactory = (fun.attachments.get[SAMFunction] match {
-        case Some(SAMFunction(userDefinedSamTp, sam)) =>
+        case Some(SAMFunction(userDefinedSamTp, sam, _)) =>
           // LambdaMetaFactory cannot mix in trait members for us, or instantiate classes -- only pure interfaces need apply
           erasure.compilesToPureInterface(erasure.javaErasure(userDefinedSamTp).typeSymbol) &&
           // impl restriction -- we currently use the boxed apply, so not really useful to allow specialized sam types (https://github.com/scala/scala/pull/4971#issuecomment-198119167)