Lambda impl methods static and more stably named

The body of lambdas is compiled into a synthetic method
in the enclosing class. Previously, this method was a public
virtual method named `fully$qualified$Class$$anonfun$n`.
For lambdas that didn't capture a `this` reference, a static
method was used.

This commit changes two aspects.

Firstly, all lambda impl methods are now emitted static.
An extra parameter is added to those that require a this
reference.

This is an improvement as it:

  - allows, shorter, more readable names for the lambda impl method
  - avoids pollution of the vtable of the class. Note that javac uses
    private instance methods, rather than public static methods. If
    we followed its lead, we would be unable to support important use
    cases in our inliner

Secondly, the name of the enclosing method has been included in
the name of the lambda impl method to improve debuggability and
to improve serialization compatibility. The serialization improvement
comes from the way that fresh names for the impl methods are
allocated: adding or removing lambdas in methods not named "foo" won't
change the numbering of the `anonfun$foo$n` impl methods from methods
named "foo". This is in line with user expectations about anonymous
class and lambda serialization stability. Brian Goetz has described
this tricky area well in:

  http://cr.openjdk.java.net/~briangoetz/eg-attachments/lambda-serialization.html

This commit doesn't go as far a Javac, we don't use the hash of the
lambda type info, param names, etc to map to a lambda impl method name.
As such, we are more prone to the type-1 and -2 failures described there.
However, our Scala 2.11.8 has similar characteristics, so we aren't going
backwards.

Special case in the naming: Use "new" rather than "<init>" for constructor enclosed
lambdas, as javac does.

I have also changed the way that "delambdafy target" methods are identifed.
Rather than relying on the naming convention, I have switched to using a
symbol attachment. The assumption is that we only need to identify them
from within the same compilation unit.

This means we can distinguish impl metbods for expanded functions
(ones called from an `apply` method of an ahead-of-time expanded
anonfun class), from those that truly end up as targets for lambda
metafactory. Only the latter are translated to static methods in
this patch.

Author: retronym

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

@@ -238,7 +238,8 @@ abstract class TreeGen extends scala.reflect.internal.TreeGen with TreeDSL {
    *  (outside the synchronized block).
    *
    *  The idiom works only if the condition is using a volatile field.
-   *  @see http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html
+    *
+    *  @see http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html
    */
   def mkSynchronizedCheck(clazz: Symbol, cond: Tree, syncBody: List[Tree], stats: List[Tree]): Tree =
     mkSynchronizedCheck(mkAttributedThis(clazz), cond, syncBody, stats)
@@ -274,8 +275,19 @@ abstract class TreeGen extends scala.reflect.internal.TreeGen with TreeDSL {
   }
 
   // used to create the lifted method that holds a function's body
-  def mkLiftedFunctionBodyMethod(localTyper: analyzer.Typer)(owner: Symbol, fun: Function) =
-    mkMethodForFunctionBody(localTyper)(owner, fun, nme.ANON_FUN_NAME)(additionalFlags = ARTIFACT)
+  def mkLiftedFunctionBodyMethod(localTyper: global.analyzer.Typer)(owner: global.Symbol, fun: global.Function) = {
+    def nonLocalEnclosingMember(sym: Symbol): Symbol = {
+      if (sym.isLocalDummy) sym.enclClass.primaryConstructor
+      else if (sym.isLocalToBlock) nonLocalEnclosingMember(sym.originalOwner)
+      else sym
+    }
+    val ownerName = nonLocalEnclosingMember(fun.symbol.originalOwner).name match {
+      case nme.CONSTRUCTOR => nme.NEWkw // do as javac does for the suffix, prefer "new" to "$lessinit$greater$1"
+      case x => x
+    }
+    val newName = nme.ANON_FUN_NAME.append(nme.NAME_JOIN_STRING).append(ownerName)
+    mkMethodForFunctionBody(localTyper)(owner, fun, newName)(additionalFlags = ARTIFACT)
+  }
 
 
   /**
@@ -310,6 +322,38 @@ abstract class TreeGen extends scala.reflect.internal.TreeGen with TreeDSL {
     newDefDef(methSym, moveToMethod(useMethodParams(fun.body)))(tpt = TypeTree(resTp))
   }
 
+  /**
+    * Create a new `DefDef` based on `orig` with an explicit self parameter.
+    *
+    * Details:
+    *   - Must by run after erasure
+    *   - If `maybeClone` is the identity function, this runs "in place"
+    *     and mutates the symbol of `orig`. `orig` should be discarded
+    *   - Symbol owners and returns are substituted, as are parameter symbols
+    *   - Recursive calls are not rewritten. This is correct if we assume
+    *     that we either:
+    *       - are in "in-place" mode, but can guarantee that no recursive calls exists
+    *       - are associating the RHS with a cloned symbol, but intend for the original
+    *         method to remain and for recursive calls to target it.
+    */
+  final def mkStatic(orig: DefDef, maybeClone: Symbol => Symbol): DefDef = {
+    assert(phase.erasedTypes, phase)
+    assert(!orig.symbol.hasFlag(SYNCHRONIZED), orig.symbol.defString)
+    val origSym = orig.symbol
+    val origParams = orig.symbol.info.params
+    val newSym = maybeClone(orig.symbol)
+    newSym.setFlag(STATIC)
+    // Add an explicit self parameter
+    val selfParamSym = newSym.newSyntheticValueParam(newSym.owner.typeConstructor, nme.SELF)
+    newSym.updateInfo(newSym.info match {
+      case mt @ MethodType(params, res) => copyMethodType(mt, selfParamSym :: params, res)
+    })
+    val selfParam = ValDef(selfParamSym)
+    val rhs = orig.rhs.substituteThis(newSym.owner, atPos(newSym.pos)(gen.mkAttributedIdent(selfParamSym)))
+      .substituteSymbols(origParams, newSym.info.params.drop(1)).changeOwner(origSym -> newSym)
+    treeCopy.DefDef(orig, orig.mods, orig.name, orig.tparams, (selfParam :: orig.vparamss.head) :: Nil, orig.tpt, rhs).setSymbol(newSym)
+  }
+
   // TODO: the rewrite to AbstractFunction is superfluous once we compile FunctionN to a SAM type (aka functional interface)
   def functionClassType(fun: Function): Type =
     if (isFunctionType(fun.tpe)) abstractFunctionType(fun.vparams.map(_.symbol.tpe), fun.body.tpe.deconst)

src/compiler/scala/tools/nsc/backend/jvm/analysis/BackendUtils.scala

@@ -121,7 +121,7 @@ class BackendUtils[BT <: BTypes](val btypes: BT) {
 
   def getBoxedUnit: FieldInsnNode = new FieldInsnNode(GETSTATIC, srBoxedUnitRef.internalName, "UNIT", srBoxedUnitRef.descriptor)
 
-  private val anonfunAdaptedName = """.*\$anonfun\$\d+\$adapted""".r
+  private val anonfunAdaptedName = """.*\$anonfun\$.*\$\d+\$adapted""".r
   def hasAdaptedImplMethod(closureInit: ClosureInstantiation): Boolean = {
     isBuiltinFunctionType(Type.getReturnType(closureInit.lambdaMetaFactoryCall.indy.desc).getInternalName) &&
     anonfunAdaptedName.pattern.matcher(closureInit.lambdaMetaFactoryCall.implMethod.getName).matches

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

@@ -61,14 +61,18 @@ abstract class Delambdafy extends Transform with TypingTransformers with ast.Tre
 
     private def mkLambdaMetaFactoryCall(fun: Function, target: Symbol, functionalInterface: Symbol, samUserDefined: Symbol, isSpecialized: Boolean): Tree = {
       val pos = fun.pos
+      def isSelfParam(p: Symbol) = p.isSynthetic && p.name == nme.SELF
+      val hasSelfParam = isSelfParam(target.firstParam)
+
       val allCapturedArgRefs = {
         // find which variables are free in the lambda because those are captures that need to be
         // passed into the constructor of the anonymous function class
         val captureArgs = FreeVarTraverser.freeVarsOf(fun).iterator.map(capture =>
           gen.mkAttributedRef(capture) setPos pos
         ).toList
 
-        if (target hasFlag STATIC) captureArgs  // no `this` reference needed
+        if (!hasSelfParam) captureArgs.filterNot(arg => isSelfParam(arg.symbol))
+        else if (currentMethod.hasFlag(Flags.STATIC)) captureArgs
         else (gen.mkAttributedThis(fun.symbol.enclClass) setPos pos) :: captureArgs
       }
 
@@ -179,7 +183,7 @@ abstract class Delambdafy extends Transform with TypingTransformers with ast.Tre
         val numCaptures  = targetParams.length - functionParamTypes.length
         val (targetCapturedParams, targetFunctionParams) = targetParams.splitAt(numCaptures)
 
-        val methSym = oldClass.newMethod(target.name.append("$adapted").toTermName, target.pos, target.flags | FINAL | ARTIFACT)
+        val methSym = oldClass.newMethod(target.name.append("$adapted").toTermName, target.pos, target.flags | FINAL | ARTIFACT | STATIC)
         val bridgeCapturedParams = targetCapturedParams.map(param => methSym.newSyntheticValueParam(param.tpe, param.name.toTermName))
         val bridgeFunctionParams =
           map2(targetFunctionParams, bridgeParamTypes)((param, tp) => methSym.newSyntheticValueParam(tp, param.name.toTermName))
@@ -223,10 +227,8 @@ abstract class Delambdafy extends Transform with TypingTransformers with ast.Tre
 
     private def transformFunction(originalFunction: Function): Tree = {
       val target = targetMethod(originalFunction)
-      target.makeNotPrivate(target.owner)
-
-      // must be done before calling createBoxingBridgeMethod and mkLambdaMetaFactoryCall
-      if (!(target hasFlag STATIC) && !methodReferencesThis(target)) target setFlag STATIC
+      assert(target.hasFlag(Flags.STATIC))
+      target.setFlag(notPRIVATE)
 
       val funSym = originalFunction.tpe.typeSymbolDirect
       // The functional interface that can be used to adapt the lambda target method `target` to the given function type.
@@ -252,11 +254,22 @@ abstract class Delambdafy extends Transform with TypingTransformers with ast.Tre
     // here's the main entry point of the transform
     override def transform(tree: Tree): Tree = tree match {
       // the main thing we care about is lambdas
-      case fun: Function => super.transform(transformFunction(fun))
+      case fun: Function =>
+        super.transform(transformFunction(fun))
       case Template(_, _, _) =>
+        def pretransform(tree: Tree): Tree = tree match {
+          case dd: DefDef if dd.symbol.isDelambdafyTarget =>
+            if (!dd.symbol.hasFlag(STATIC) && methodReferencesThis(dd.symbol)) {
+              gen.mkStatic(dd, sym => sym)
+            } else {
+              dd.symbol.setFlag(STATIC)
+              dd
+            }
+          case t => t
+        }
         try {
           // during this call boxingBridgeMethods will be populated from the Function case
-          val Template(parents, self, body) = super.transform(tree)
+          val Template(parents, self, body) = super.transform(deriveTemplate(tree)(_.mapConserve(pretransform)))
           Template(parents, self, body ++ boxingBridgeMethods)
         } finally boxingBridgeMethods.clear()
       case _ => super.transform(tree)

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

@@ -1329,6 +1329,7 @@ abstract class SpecializeTypes extends InfoTransform with TypingTransformers {
   class SpecializationDuplicator(casts: Map[Symbol, Type]) extends Duplicator(casts) {
     override def retyped(context: Context, tree: Tree, oldThis: Symbol, newThis: Symbol, env: scala.collection.Map[Symbol, Type]): Tree =
       enteringSpecialize(super.retyped(context, tree, oldThis, newThis, env))
+
   }
 
   /** A tree symbol substituter that substitutes on type skolems.

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

@@ -213,6 +213,7 @@ abstract class UnCurry extends InfoTransform
         // Expand the function body into an anonymous class
         gen.expandFunction(localTyper)(fun, inConstructorFlag)
       } else {
+        val mustExpand = mustExpandFunction(fun)
         // method definition with the same arguments, return type, and body as the original lambda
         val liftedMethod = gen.mkLiftedFunctionBodyMethod(localTyper)(fun.symbol.owner, fun)
 
@@ -221,11 +222,18 @@ abstract class UnCurry extends InfoTransform
           gen.mkForwarder(gen.mkAttributedRef(liftedMethod.symbol), (fun.vparams map (_.symbol)) :: Nil)
         ))
 
+        if (!mustExpand) {
+          liftedMethod.symbol.updateAttachment(DelambdafyTarget)
+          liftedMethod.updateAttachment(DelambdafyTarget)
+        }
+
         val typedNewFun = localTyper.typedPos(fun.pos)(Block(liftedMethod, super.transform(newFun)))
-        if (mustExpandFunction(fun)) {
+        if (mustExpand) {
           val Block(stats, expr : Function) = typedNewFun
           treeCopy.Block(typedNewFun, stats, gen.expandFunction(localTyper)(expr, inConstructorFlag))
-        } else typedNewFun
+        } else {
+          typedNewFun
+        }
       }
 
     def transformArgs(pos: Position, fun: Symbol, args: List[Tree], formals: List[Type]) = {
@@ -341,13 +349,18 @@ abstract class UnCurry extends InfoTransform
 
     private def isSelfSynchronized(ddef: DefDef) = ddef.rhs match {
       case Apply(fn @ TypeApply(Select(sel, _), _), _) =>
-        fn.symbol == Object_synchronized && sel.symbol == ddef.symbol.enclClass && !ddef.symbol.enclClass.isTrait
+        fn.symbol == Object_synchronized && sel.symbol == ddef.symbol.enclClass && !ddef.symbol.enclClass.isTrait &&
+          !ddef.symbol.isDelambdafyTarget /* these become static later, unsuitable for ACC_SYNCHRONIZED */
       case _ => false
     }
 
     /** If an eligible method is entirely wrapped in a call to synchronized
      *  locked on the same instance, remove the synchronized scaffolding and
      *  mark the method symbol SYNCHRONIZED for bytecode generation.
+     *
+     *  Delambdafy targets are deemed ineligible as the Delambdafy phase will
+     *  replace `this.synchronized` with `$this.synchronzed` now that it emits
+     *  all lambda impl methods as static.
      */
     private def translateSynchronized(tree: Tree) = tree match {
       case dd @ DefDef(_, _, _, _, _, Apply(fn, body :: Nil)) if isSelfSynchronized(dd) =>

src/compiler/scala/tools/nsc/typechecker/Duplicators.scala

@@ -229,7 +229,12 @@ abstract class Duplicators extends Analyzer {
 
         case ddef @ DefDef(_, _, _, _, tpt, rhs) =>
           ddef.tpt modifyType fixType
-          super.typed(ddef.clearType(), mode, pt)
+          val result = super.typed(ddef.clearType(), mode, pt)
+          // TODO this is a hack, we really need a cleaner way to transport symbol attachments to duplicated methods
+          // bodies in specialized subclasses.
+          if (ddef.hasAttachment[DelambdafyTarget.type])
+            result.symbol.updateAttachment(DelambdafyTarget)
+          result
 
         case fun: Function =>
           debuglog("Clearing the type and retyping Function: " + fun)

src/reflect/scala/reflect/internal/StdAttachments.scala

@@ -51,6 +51,8 @@ trait StdAttachments {
     */
   case class SAMFunction(samTp: Type, sam: Symbol) extends PlainAttachment
 
+  case object DelambdafyTarget extends PlainAttachment
+
   /** When present, indicates that the host `Ident` has been created from a backquoted identifier.
    */
   case object BackquotedIdentifierAttachment extends PlainAttachment

src/reflect/scala/reflect/internal/Symbols.scala

@@ -807,7 +807,7 @@ trait Symbols extends api.Symbols { self: SymbolTable =>
 
     final def isAnonymousFunction = isSynthetic && (name containsName tpnme.ANON_FUN_NAME)
     final def isDelambdafyFunction = isSynthetic && (name containsName tpnme.DELAMBDAFY_LAMBDA_CLASS_NAME)
-    final def isDelambdafyTarget  = isArtifact && isMethod && (name containsName tpnme.ANON_FUN_NAME)
+    final def isDelambdafyTarget  = isArtifact && isMethod && hasAttachment[DelambdafyTarget.type]
     final def isDefinedInPackage  = effectiveOwner.isPackageClass
     final def needsFlatClasses    = phase.flatClasses && rawowner != NoSymbol && !rawowner.isPackageClass
 

src/reflect/scala/reflect/runtime/JavaUniverseForce.scala

@@ -38,6 +38,7 @@ trait JavaUniverseForce { self: runtime.JavaUniverse  =>
     this.FixedMirrorTypeCreator
     this.CompoundTypeTreeOriginalAttachment
     this.SAMFunction
+    this.DelambdafyTarget
     this.BackquotedIdentifierAttachment
     this.ForAttachment
     this.SyntheticUnitAttachment

test/files/run/delambdafy_t6028.check

@@ -11,7 +11,7 @@ package <empty> {
     def foo(methodParam: String): Function0 = {
       val methodLocal: String = "";
       {
-        (() => T.this.$anonfun$1(methodParam, methodLocal))
+        (() => T.this.$anonfun$foo$1(methodParam, methodLocal))
       }
     };
     def bar(barParam: String): Object = {
@@ -21,10 +21,10 @@ package <empty> {
     def tryy(tryyParam: String): Function0 = {
       var tryyLocal: runtime.ObjectRef = scala.runtime.ObjectRef.create("");
       {
-        (() => T.this.$anonfun$2(tryyParam, tryyLocal))
+        (() => T.this.$anonfun$tryy$1(tryyParam, tryyLocal))
       }
     };
-    final <artifact> private[this] def $anonfun$1(methodParam$1: String, methodLocal$1: String): String = T.this.classParam.+(T.this.field()).+(methodParam$1).+(methodLocal$1);
+    final <artifact> private[this] def $anonfun$foo$1(methodParam$1: String, methodLocal$1: String): String = T.this.classParam.+(T.this.field()).+(methodParam$1).+(methodLocal$1);
     abstract trait MethodLocalTrait$1 extends Object {
       def /*MethodLocalTrait$1*/$init$(barParam$1: String): Unit = {
         ()
@@ -54,7 +54,7 @@ package <empty> {
       T.this.MethodLocalObject$lzycompute$1(barParam$1, MethodLocalObject$module$1)
     else
       MethodLocalObject$module$1.elem.$asInstanceOf[T#MethodLocalObject$2.type]();
-    final <artifact> private[this] def $anonfun$2(tryyParam$1: String, tryyLocal$1: runtime.ObjectRef): Unit = try {
+    final <artifact> private[this] def $anonfun$tryy$1(tryyParam$1: String, tryyLocal$1: runtime.ObjectRef): Unit = try {
       tryyLocal$1.elem = tryyParam$1
     } finally ()
   }

test/files/run/delambdafy_t6555.check

@@ -6,8 +6,8 @@ package <empty> {
       ()
     };
     private[this] val f: String => String = {
-      final <artifact> def $anonfun(param: String): String = param;
-      ((param: String) => $anonfun(param))
+      final <artifact> def $anonfun$f(param: String): String = param;
+      ((param: String) => $anonfun$f(param))
     };
     <stable> <accessor> def f(): String => String = Foo.this.f
   }

test/files/run/delambdafy_uncurry_byname_method.check

@@ -7,8 +7,8 @@ package <empty> {
     };
     def bar(x: () => String): String = x.apply();
     def foo(): String = Foo.this.bar({
-      final <artifact> def $anonfun(): String = "";
-      (() => $anonfun())
+      final <artifact> def $anonfun$foo(): String = "";
+      (() => $anonfun$foo())
     })
   }
 }

test/files/run/delambdafy_uncurry_method.check

@@ -7,8 +7,8 @@ package <empty> {
     };
     def bar(): Unit = {
       val f: Int => Int = {
-        final <artifact> def $anonfun(x: Int): Int = x.+(1);
-        ((x: Int) => $anonfun(x))
+        final <artifact> def $anonfun|(x: Int): Int = x.+(1);
+        ((x: Int) => $anonfun|(x))
       };
       ()
     }

test/files/run/t9097.scala

@@ -28,6 +28,6 @@ object Test extends StoreReporterDirectTest {
     assert(!storeReporter.hasErrors, message = filteredInfos map (_.msg) mkString "; ")
     val out = baos.toString("UTF-8")
     // was 2 before the fix, the two PackageDefs for a would both contain the ClassDef for the closure
-    assert(out.lines.count(_ contains "def $anonfun$1(x$1: Int): String") == 1, out)
+    assert(out.lines.count(_ contains "def $anonfun$hihi$1(x$1: Int): String") == 1, out)
   }
 }

test/junit/scala/tools/nsc/backend/jvm/IndySammyTest.scala

@@ -43,7 +43,7 @@ class IndySammyTest extends BytecodeTesting {
     val c = compileClass(s"class C { ${lamDef(from, to, body)}; ${appDef(arg)} }", allowMessage = allowMessage)
 
     val applySig = getAsmMethod(funClass, "apply").desc
-    val anonfun = getMethod(c, "C$$$anonfun$1")
+    val anonfun = getMethod(c, "$anonfun$lam$1")
     val lamInsn = getInstructions(c, "lam").dropNonOp
     val applyInvoke = getMethod(c, "app")
 

test/junit/scala/tools/nsc/backend/jvm/OptimizedBytecodeTest.scala

@@ -43,9 +43,9 @@ class OptimizedBytecodeTest extends BytecodeTesting {
     val c = compileClass(code)
 
     assertSameSummary(getMethod(c, "t"), List(
-      LDC, ASTORE, ALOAD /*0*/, ALOAD /*1*/, "C$$$anonfun$1", IRETURN))
-    assertSameSummary(getMethod(c, "C$$$anonfun$1"), List(LDC, "C$$$anonfun$2", IRETURN))
-    assertSameSummary(getMethod(c, "C$$$anonfun$2"), List(-1 /*A*/, GOTO /*A*/))
+      LDC, ASTORE, ALOAD /*0*/, ALOAD /*1*/, "$anonfun$t$1", IRETURN))
+    assertSameSummary(getMethod(c, "$anonfun$t$1"), List(ALOAD, IFNONNULL, ACONST_NULL, ATHROW, -1, LDC, "$anonfun$t$2", IRETURN))
+    assertSameSummary(getMethod(c, "$anonfun$t$2"), List(-1 /*A*/, GOTO /*A*/))
   }
 
   @Test
@@ -295,9 +295,9 @@ class OptimizedBytecodeTest extends BytecodeTesting {
         |}
       """.stripMargin
     val c = compileClass(code, allowMessage = _.msg.contains("exception handler declared in the inlined method"))
-    assertInvoke(getMethod(c, "f1a"), "C", "C$$$anonfun$1")
+    assertInvoke(getMethod(c, "f1a"), "C", "$anonfun$f1a$1")
     assertInvoke(getMethod(c, "f1b"), "C", "wrapper1")
-    assertInvoke(getMethod(c, "f2a"), "C", "C$$$anonfun$3")
+    assertInvoke(getMethod(c, "f2a"), "C", "$anonfun$f2a$1")
     assertInvoke(getMethod(c, "f2b"), "C", "wrapper2")
   }
 
@@ -331,7 +331,7 @@ class OptimizedBytecodeTest extends BytecodeTesting {
         |class Listt
       """.stripMargin
     val List(c, nil, nilMod, listt) = compileClasses(code)
-    assertInvoke(getMethod(c, "t"), "C", "C$$$anonfun$1")
+    assertInvoke(getMethod(c, "t"), "C", "$anonfun$t$1")
   }
 
   @Test
@@ -357,6 +357,6 @@ class OptimizedBytecodeTest extends BytecodeTesting {
   def optimiseEnablesNewOpt(): Unit = {
     val code = """class C { def t = (1 to 10) foreach println }"""
     val List(c) = readAsmClasses(newCompiler(extraArgs = "-optimise -deprecation").compileToBytes(code, allowMessage = _.msg.contains("is deprecated")))
-    assertInvoke(getMethod(c, "t"), "C", "C$$$anonfun$1") // range-foreach inlined from classpath
+    assertInvoke(getMethod(c, "t"), "C", "$anonfun$t$1") // range-foreach inlined from classpath
   }
 }