SI-8359 Adjust parameter order of accessor method in Delambdafy

Under `-Ydelambdafy:method`, a public, static accessor method is
created to expose the private method containing the body of the
lambda.

Currently this accessor method has its parameters in the same order
structure as those of the lambda body method.

What is this order? There are three categories of parameters:

  1. lambda parameters
  2. captured parameters (added by lambdalift)
  3. self parameters (added to lambda bodies that end up in trait
     impl classes by mixin, and added unconditionally to the static
     accessor method.)

These are currently emitted in order #3, #1, #2.

Here are examples of the current behaviour:

BEFORE (trait):

```
% cat sandbox/test.scala && scalac-hash v2.11.5 -Ydelambdafy:method sandbox/test.scala && javap -private -classpath . 'Test$class'
trait Member; class Capture; trait LambdaParam
trait Test {
  def member: Member
  def foo {
    val local = new Capture
    (arg: LambdaParam) => "" + arg + member + local
  }
}
Compiled from "test.scala"
public abstract class Test$class {
  public static void foo(Test);
  private static final java.lang.String $anonfun$1(Test, LambdaParam, Capture);
  public static void $init$(Test);
  public static final java.lang.String accessor$1(Test, LambdaParam, Capture);
}
```

BEFORE (class):

```
% cat sandbox/test.scala && scalac-hash v2.11.5 -Ydelambdafy:method sandbox/test.scala && javap -private -classpath . Test
trait Member; class Capture; trait LambdaParam
abstract class Test {
  def member: Member
  def foo {
    val local = new Capture
    (arg: LambdaParam) => "" + arg + member + local
  }
}
Compiled from "test.scala"
public abstract class Test {
  public abstract Member member();
  public void foo();
  private final java.lang.String $anonfun$1(LambdaParam, Capture);
  public Test();
  public static final java.lang.String accessor$1(Test, LambdaParam, Capture);
}
```

Contrasting the class case with Java:

```
% cat sandbox/Test.java && javac -d . sandbox/Test.java && javap -private -classpath . Test
public abstract class Test {
  public static class Member {};
  public static class Capture {};
  public static class LambaParam {};
  public static interface I {
    public abstract Object c(LambaParam arg);
  }
  public abstract Member member();
  public void test() {
    Capture local = new Capture();
    I i1 = (LambaParam arg) -> "" + member() + local;
  }
}

Compiled from "Test.java"
public abstract class Test {
  public Test();
  public abstract Test$Member member();
  public void test();
  private java.lang.Object lambda$test$0(Test$Capture, Test$LambaParam);
}
```

We can see that in Java 8 lambda parameters come after captures. If we
want to use Java's LambdaMetafactory to spin up our anoymous FunctionN
subclasses on the fly, our ordering must change.

I can see three options for change:

  1. Adjust `LambdaLift` to always prepend captured parameters,
     rather than appending them. I think we could leave `Mixin` as
     it is, it already prepends the self parameter. This would result
     a parameter ordering, in terms of the list above: #3, #2, #1.
  2. More conservatively, do this just for methods known to hold
     lambda bodies. This might avoid needlessly breaking code that
     has come to depend on our binary encoding.
  3. Adjust the parameters of the accessor method only. The body
     of this method can permute params before calling the lambda
     body method.

This commit implements option #2.

In also prototyped #1, and found it worked so long as I limited it to
non-constructors, to sidestep the need to make corresponding
changes elsewhere in the compiler to avoid the crasher shown
in the enclosed test case, which was minimized from a bootstrap
failure from an earlier a version of this patch.

We would need to defer option #1 to 2.12 in any case, as some of
these lifted methods are publicied by the optimizer, and we must
leave the signatures alone to comply with MiMa.

I've included a test that shows this in all in action. However, that
is currently disabled, as we don't have a partest category for tests
that require Java 8.

Author: retronym

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

@@ -367,7 +367,7 @@ abstract class LambdaLift extends InfoTransform {
 
     private def addFreeArgs(pos: Position, sym: Symbol, args: List[Tree]) = {
       free get sym match {
-        case Some(fvs) => args ++ (fvs.toList map (fv => atPos(pos)(proxyRef(fv))))
+        case Some(fvs) => addFree(sym, free = fvs.toList map (fv => atPos(pos)(proxyRef(fv))), original = args)
         case _         => args
       }
     }
@@ -379,9 +379,9 @@ abstract class LambdaLift extends InfoTransform {
           case DefDef(_, _, _, vparams :: _, _, _) =>
             val addParams = cloneSymbols(ps).map(_.setFlag(PARAM))
             sym.updateInfo(
-              lifted(MethodType(sym.info.params ::: addParams, sym.info.resultType)))
+              lifted(MethodType(addFree(sym, free = addParams, original = sym.info.params), sym.info.resultType)))
 
-            copyDefDef(tree)(vparamss = List(vparams ++ freeParams))
+            copyDefDef(tree)(vparamss = List(addFree(sym, free = freeParams, original = vparams)))
           case ClassDef(_, _, _, _) =>
             // SI-6231
             // Disabled attempt to to add getters to freeParams
@@ -562,4 +562,12 @@ abstract class LambdaLift extends InfoTransform {
     }
   } // class LambdaLifter
 
+  private def addFree[A](sym: Symbol, free: List[A], original: List[A]): List[A] = {
+    val prependFree = (
+         !sym.isConstructor // this condition is redundant for now. It will be needed if we remove the second condition in 2.12.x
+      && (settings.Ydelambdafy.value == "method" && sym.isDelambdafyTarget) // SI-8359 Makes the lambda body a viable as the target MethodHandle for a call to LambdaMetafactory
+    )
+    if (prependFree) free ::: original
+    else             original ::: free
+  }
 }

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

@@ -789,6 +789,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  = isSynthetic && isMethod && (name containsName tpnme.ANON_FUN_NAME)
     final def isDefinedInPackage  = effectiveOwner.isPackageClass
     final def needsFlatClasses    = phase.flatClasses && rawowner != NoSymbol && !rawowner.isPackageClass
 

test/files/pos/t8359-closelim-crash.flags

@@ -0,0 +1 @@
+-optimize

test/files/pos/t8359-closelim-crash.scala

@@ -0,0 +1,23 @@
+package test
+
+// This is a minimization of code that crashed the compiler during bootstrapping
+// in the first iteration of https://github.com/scala/scala/pull/4373, the PR
+// that adjusted the order of free and declared params in LambdaLift.
+
+// Was:
+//  java.lang.AssertionError: assertion failed:
+//  Record Record(<$anon: Function1>,Map(value a$1 -> Deref(LocalVar(value b)))) does not contain a field value b$1
+// at scala.tools.nsc.Global.assert(Global.scala:262)
+// at scala.tools.nsc.backend.icode.analysis.CopyPropagation$copyLattice$State.getFieldNonRecordValue(CopyPropagation.scala:113)
+// at scala.tools.nsc.backend.icode.analysis.CopyPropagation$copyLattice$State.getFieldNonRecordValue(CopyPropagation.scala:122)
+// at scala.tools.nsc.backend.opt.ClosureElimination$ClosureElim$$anonfun$analyzeMethod$1$$anonfun$apply$2.replaceFieldAccess$1(ClosureElimination.scala:124)
+class Typer {
+  def bar(a: Boolean, b: Boolean): Unit = {
+    @inline
+    def baz(): Unit = {
+      ((_: Any) => (Typer.this, a, b)).apply("")
+    }
+    ((_: Any) => baz()).apply("")
+  }
+}
+

test/pending/run/delambdafy-lambdametafactory.scala

@@ -0,0 +1,50 @@
+//
+// Tests that the static accessor method for lambda bodies
+// (generated under -Ydelambdafy:method) are compatible with
+// Java 8's LambdaMetafactory.
+//
+import java.lang.invoke._
+
+class C {
+  def test1: Unit = {
+    (x: String) => x.reverse
+  }
+  def test2: Unit = {
+    val capture1 = "capture1"
+    (x: String) => capture1 + " " + x.reverse
+  }
+  def test3: Unit = {
+    (x: String) => C.this + " " + x.reverse
+  }
+}
+trait T {
+  def test4: Unit = {
+    (x: String) => x.reverse
+  }
+}
+
+// A functional interface. Function1 contains abstract methods that are filled in by mixin 
+trait Function1ish[A, B] {
+  def apply(a: A): B
+}
+
+object Test {
+  def lambdaFactory[A, B](hostClass: Class[_], instantiatedParam: Class[A], instantiatedRet: Class[B], accessorName: String,
+                          capturedParams: Array[(Class[_], AnyRef)] = Array()) = {
+    val caller = MethodHandles.lookup
+    val methodType = MethodType.methodType(classOf[AnyRef], Array[Class[_]](classOf[AnyRef]))
+    val instantiatedMethodType = MethodType.methodType(instantiatedRet, Array[Class[_]](instantiatedParam))
+    val (capturedParamTypes, captured) = capturedParams.unzip
+    val targetMethodType = MethodType.methodType(instantiatedRet, capturedParamTypes :+ instantiatedParam)
+    val invokedType = MethodType.methodType(classOf[Function1ish[_, _]], capturedParamTypes)
+    val target = caller.findStatic(hostClass, accessorName, targetMethodType)
+    val site = LambdaMetafactory.metafactory(caller, "apply", invokedType, methodType, target, instantiatedMethodType)
+    site.getTarget.invokeWithArguments(captured: _*).asInstanceOf[Function1ish[A, B]]
+  }
+  def main(args: Array[String]) {
+    println(lambdaFactory(classOf[C], classOf[String], classOf[String], "accessor$1").apply("abc"))
+    println(lambdaFactory(classOf[C], classOf[String], classOf[String], "accessor$2", Array(classOf[String] -> "capture1")).apply("abc"))
+    println(lambdaFactory(classOf[C], classOf[String], classOf[String], "accessor$3", Array(classOf[C] -> new C)).apply("abc"))
+    println(lambdaFactory(Class.forName("T$class"), classOf[String], classOf[String], "accessor$4", Array(classOf[T] -> new T{})).apply("abc"))
+  }
+}