Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SI-8359 Adjust parameter order of accessor method in Delambdafy #4373

Merged
merged 1 commit into from
Apr 10, 2015

Conversation

retronym
Copy link
Member

@retronym retronym commented Mar 4, 2015

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.

@retronym
Copy link
Member Author

retronym commented Mar 6, 2015

Here'a minimization of the code that crashes during bootstrap:

package test

abstract class Typer {
  def foo(a: Boolean): AnyRef

  def bar(a: Boolean, b: Boolean): AnyRef = {
    def baz(n : => Any): Unit = {
      ((_: Any) => foo(a || b)).apply("")
      ()
    }
    ((_: Any) => baz("")).apply("")
    ???
  }
}

I suspect that the backend has some assumptions about parameter order that are now violated.

e.g. in CopyPropagation:

      // this relies on having the same order in paramAccessors and
      // the arguments on the stack. It should be the same!
      for ((p, i) <- paramAccessors.zipWithIndex) {
//        assert(p.tpe == paramTypes(i), "In: " + ctor.fullName
//               + " having acc: " + (paramAccessors map (_.tpe))+ " vs. params" + paramTypes
//               + "\n\t failed at pos " + i + " with " + p.tpe + " == " + paramTypes(i))
        if (p.tpe == paramTypes(i))
          bindings += (p -> values.head)
        values = values.tail
      }

@retronym retronym force-pushed the topic/indylambda-permutations-2 branch from fde4585 to 35eeffd Compare March 6, 2015 06:40
@retronym
Copy link
Member Author

retronym commented Mar 6, 2015

I've tweaked the first commit to (hopefully) fix bootstrapping, and added a second commit to limit this change to Option 2.

@gkossakowski
Copy link
Contributor

/rebuild

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.
@retronym retronym force-pushed the topic/indylambda-permutations-2 branch from 35eeffd to 1e7a990 Compare March 24, 2015 02:01
@retronym
Copy link
Member Author

I've squashed the first commit (which affected this change for all methods) out of existence. It failed MiMa.

@retronym
Copy link
Member Author

@gkossakowski The build is green, this is ready for review.

@retronym
Copy link
Member Author

/ping @gkossakowski or @lrytz for review.

@lrytz
Copy link
Member

lrytz commented Apr 10, 2015

@retronym could you update the PR description, it still says "this commit implements option 1" - i guess we can just use the commit message?

@retronym
Copy link
Member Author

@lrytz done

@lrytz
Copy link
Member

lrytz commented Apr 10, 2015

LGTM! In 2.12 and GenBCode, maybe we can enable the new order also for constructors - to be seen.

As a side-note, for delambdafy:method, we should re-consider the current state of having both a lambda body method and an accessor.

lrytz added a commit that referenced this pull request Apr 10, 2015
SI-8359 Adjust parameter order of accessor method in Delambdafy
@lrytz lrytz merged commit 6e7b326 into scala:2.11.x Apr 10, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants