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-9390 Emit local defs that don't capture this as static #5099

Merged
merged 5 commits into from Jun 6, 2016

Conversation

Projects
None yet
5 participants
@retronym
Member

retronym commented Apr 14, 2016

This avoids unnecessary memory retention, and allows lambdas
that call the local methods to be serializable, regardless of
whether or not the enclosing class is serializable.

The second point is especially pressing, given that the enclosing
class for local methods defined in a used to be the (serializable)
anonymous function class, but as of Scala 2.12 will be the enclosing
class of the lambda.

This change is similar in spirit to SI-9408 / 93bee55.

@scala-jenkins scala-jenkins added this to the 2.12.0-M5 milestone Apr 14, 2016

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Apr 14, 2016

Member

See https://gist.github.com/JoshRosen/8aacdee0162da430868e7f73247d45d8 for an analysis of this in the context of Spark. /cc @JoshRosen

Member

retronym commented Apr 14, 2016

See https://gist.github.com/JoshRosen/8aacdee0162da430868e7f73247d45d8 for an analysis of this in the context of Spark. /cc @JoshRosen

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Apr 14, 2016

Member

Review, additional tests by @adriaanm | @lrytz, please.

Member

retronym commented Apr 14, 2016

Review, additional tests by @adriaanm | @lrytz, please.

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Apr 14, 2016

Member

Bytecode diff from 2.12.0-M4 for a simple example:

 scala> :paste -raw
 // Entering paste mode (ctrl-D to finish)

 class StaticLift { def foo = { def bar(x: Any) = true; bar _ } }

 // Exiting paste mode, now interpreting.


 scala> :javap -c -private StaticLift
 Compiled from "<pastie>"
 public class StaticLift {
   public scala.Function1<java.lang.Object, java.lang.Object> foo();
     Code:
-       0: aload_0
-       1: invokedynamic #37,  0             // InvokeDynamic #0:apply:(LStaticLift;)Lscala/Function1;
-       6: areturn
+       0: invokedynamic #36,  0             // InvokeDynamic #0:apply:()Lscala/Function1;
+       5: areturn

-  private final boolean bar$1(java.lang.Object);
+  private static final boolean bar$1(java.lang.Object);
     Code:
        0: iconst_1
        1: ireturn

-  public final boolean StaticLift$$$anonfun$1(java.lang.Object);
+  public static final boolean StaticLift$$$anonfun$1(java.lang.Object);
     Code:
        0: aload_0
-       1: aload_1
-       2: invokespecial #46                 // Method bar$1:(Ljava/lang/Object;)Z
-       5: ireturn
+       1: invokestatic  #45                 // Method bar$1:(Ljava/lang/Object;)Z
+       4: ireturn

   public StaticLift();
     Code:
        0: aload_0
-       1: invokespecial #50                 // Method java/lang/Object."<init>":()V
+       1: invokespecial #49                 // Method java/lang/Object."<init>":()V
        4: return

-  public final java.lang.Object StaticLift$$$anonfun$1$adapted(java.lang.Object);
+  public static final java.lang.Object StaticLift$$$anonfun$1$adapted(java.lang.Object);
     Code:
        0: aload_0
-       1: aload_1
-       2: invokevirtual #52                 // Method StaticLift$$$anonfun$1:(Ljava/lang/Object;)Z
-       5: invokestatic  #58                 // Method scala/runtime/BoxesRunTime.boxToBoolean:(Z)Ljava/lang/Boolean;
-       8: areturn
+       1: invokestatic  #51                 // Method StaticLift$$$anonfun$1:(Ljava/lang/Object;)Z
+       4: invokestatic  #57                 // Method scala/runtime/BoxesRunTime.boxToBoolean:(Z)Ljava/lang/Boolean;
+       7: areturn

   private static java.lang.Object $deserializeLambda$(java.lang.invoke.SerializedLambda);
     Code:
        0: aload_0
-       1: invokedynamic #70,  0             // InvokeDynamic #1:lambdaDeserialize:(Ljava/lang/invoke/SerializedLambda;)Ljava/lang/Object;
+       1: invokedynamic #69,  0             // InvokeDynamic #1:lambdaDeserialize:(Ljava/lang/invoke/SerializedLambda;)Ljava/lang/Object;
        6: areturn
-}
+}
\ No newline at end of file
Member

retronym commented Apr 14, 2016

Bytecode diff from 2.12.0-M4 for a simple example:

 scala> :paste -raw
 // Entering paste mode (ctrl-D to finish)

 class StaticLift { def foo = { def bar(x: Any) = true; bar _ } }

 // Exiting paste mode, now interpreting.


 scala> :javap -c -private StaticLift
 Compiled from "<pastie>"
 public class StaticLift {
   public scala.Function1<java.lang.Object, java.lang.Object> foo();
     Code:
-       0: aload_0
-       1: invokedynamic #37,  0             // InvokeDynamic #0:apply:(LStaticLift;)Lscala/Function1;
-       6: areturn
+       0: invokedynamic #36,  0             // InvokeDynamic #0:apply:()Lscala/Function1;
+       5: areturn

-  private final boolean bar$1(java.lang.Object);
+  private static final boolean bar$1(java.lang.Object);
     Code:
        0: iconst_1
        1: ireturn

-  public final boolean StaticLift$$$anonfun$1(java.lang.Object);
+  public static final boolean StaticLift$$$anonfun$1(java.lang.Object);
     Code:
        0: aload_0
-       1: aload_1
-       2: invokespecial #46                 // Method bar$1:(Ljava/lang/Object;)Z
-       5: ireturn
+       1: invokestatic  #45                 // Method bar$1:(Ljava/lang/Object;)Z
+       4: ireturn

   public StaticLift();
     Code:
        0: aload_0
-       1: invokespecial #50                 // Method java/lang/Object."<init>":()V
+       1: invokespecial #49                 // Method java/lang/Object."<init>":()V
        4: return

-  public final java.lang.Object StaticLift$$$anonfun$1$adapted(java.lang.Object);
+  public static final java.lang.Object StaticLift$$$anonfun$1$adapted(java.lang.Object);
     Code:
        0: aload_0
-       1: aload_1
-       2: invokevirtual #52                 // Method StaticLift$$$anonfun$1:(Ljava/lang/Object;)Z
-       5: invokestatic  #58                 // Method scala/runtime/BoxesRunTime.boxToBoolean:(Z)Ljava/lang/Boolean;
-       8: areturn
+       1: invokestatic  #51                 // Method StaticLift$$$anonfun$1:(Ljava/lang/Object;)Z
+       4: invokestatic  #57                 // Method scala/runtime/BoxesRunTime.boxToBoolean:(Z)Ljava/lang/Boolean;
+       7: areturn

   private static java.lang.Object $deserializeLambda$(java.lang.invoke.SerializedLambda);
     Code:
        0: aload_0
-       1: invokedynamic #70,  0             // InvokeDynamic #1:lambdaDeserialize:(Ljava/lang/invoke/SerializedLambda;)Ljava/lang/Object;
+       1: invokedynamic #69,  0             // InvokeDynamic #1:lambdaDeserialize:(Ljava/lang/invoke/SerializedLambda;)Ljava/lang/Object;
        6: areturn
-}
+}
\ No newline at end of file
@@ -141,7 +141,7 @@ class InlineWarningTest extends ClearAfterClass {
val warn =
"""M::f()I is annotated @inline but could not be inlined:
|The callee M::f()I contains the instruction INVOKESPECIAL M.nested$1 ()I
|The callee M::f()I contains the instruction INVOKESTATIC M.nested$1 ()I

This comment has been minimized.

@lrytz

lrytz Apr 14, 2016

Member

a nice thing about static methods is that we can in principle make them public for the purpose of the optimizer and don't need to name-mangle

@lrytz

lrytz Apr 14, 2016

Member

a nice thing about static methods is that we can in principle make them public for the purpose of the optimizer and don't need to name-mangle

@JoshRosen

This comment has been minimized.

Show comment
Hide comment
@JoshRosen

JoshRosen Apr 15, 2016

Good news: I tested out this PR as of e6a566e and it appears to have fixed the serializability issues with local defs that I found in Spark.

Bad news: in more local testing, I discovered a related problem affecting local classes. For example:

class MyClass {
  Seq().map { x: Any => new Object {} }
}

In this example, the map's closure will become an instance method of MyClass (rather than a static method) because the anonymous class created by new Object {} takes an instance of MyClass in its constructor.

This sounds like SI-1419; would it be possible to target that issue for 2.12?

JoshRosen commented Apr 15, 2016

Good news: I tested out this PR as of e6a566e and it appears to have fixed the serializability issues with local defs that I found in Spark.

Bad news: in more local testing, I discovered a related problem affecting local classes. For example:

class MyClass {
  Seq().map { x: Any => new Object {} }
}

In this example, the map's closure will become an instance method of MyClass (rather than a static method) because the anonymous class created by new Object {} takes an instance of MyClass in its constructor.

This sounds like SI-1419; would it be possible to target that issue for 2.12?

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Apr 15, 2016

Member

@JoshRosen That's a tricky one. As of SI-9408 / 93bee55, the constructor of the anonymous class no longer stores retains a reference to the enclosing class, but its constructor does still include this:

class MyClass {
  def test = {
    { (x: Any) => new Object {} }
  }
}
  final class anon$1 extends Object {
    def <init>($outer: MyClass): <$anon: Object> = {
      anon$1.super.<init>();
      // $outer is not stored in a field
      ()
    }
  }

The transform responsible for this keeps the constructor signature intact because in general, another compilation unit might be calling the constructor of an inner class, and we need to have an agreed constructor signature given that client and class might be separately compiled.

However, for local classes like in your example, we could in fact totally remove $outer from the constructor signature (we'd need to rewrite calls to the constructor in the same method, of course). This won't be trivial to implement, but might be worthwhile.

Member

retronym commented Apr 15, 2016

@JoshRosen That's a tricky one. As of SI-9408 / 93bee55, the constructor of the anonymous class no longer stores retains a reference to the enclosing class, but its constructor does still include this:

class MyClass {
  def test = {
    { (x: Any) => new Object {} }
  }
}
  final class anon$1 extends Object {
    def <init>($outer: MyClass): <$anon: Object> = {
      anon$1.super.<init>();
      // $outer is not stored in a field
      ()
    }
  }

The transform responsible for this keeps the constructor signature intact because in general, another compilation unit might be calling the constructor of an inner class, and we need to have an agreed constructor signature given that client and class might be separately compiled.

However, for local classes like in your example, we could in fact totally remove $outer from the constructor signature (we'd need to rewrite calls to the constructor in the same method, of course). This won't be trivial to implement, but might be worthwhile.

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Apr 15, 2016

Member

However, for local classes like in your example, we could in fact totally remove $outer from the constructor signature (we'd need to rewrite calls to the constructor in the same method, of course). This won't be trivial to implement, but might be worthwhile.

Here's a first cut at this change: retronym/scala@ticket/9390...retronym:ticket/9390-2

Member

retronym commented Apr 15, 2016

However, for local classes like in your example, we could in fact totally remove $outer from the constructor signature (we'd need to rewrite calls to the constructor in the same method, of course). This won't be trivial to implement, but might be worthwhile.

Here's a first cut at this change: retronym/scala@ticket/9390...retronym:ticket/9390-2

@lrytz

This comment has been minimized.

Show comment
Hide comment
@lrytz

lrytz Apr 15, 2016

Member

LGTM!

Member

lrytz commented Apr 15, 2016

LGTM!

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Apr 16, 2016

Member

Let's run this one by @adriaanm next week before merging.

Member

retronym commented Apr 16, 2016

Let's run this one by @adriaanm next week before merging.

@retronym retronym added the on-hold label Apr 16, 2016

@adriaanm adriaanm self-assigned this Apr 26, 2016

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Apr 28, 2016

Member

LGTM -- should we look into unmangling names of members that end up being static, to keep them shorter? (If I understood Lukas's comment correctly, the name mangling we're talking about emulates static binding, i.e., these members should not be late bound/overridable, but they need not be private.)

Member

adriaanm commented Apr 28, 2016

LGTM -- should we look into unmangling names of members that end up being static, to keep them shorter? (If I understood Lukas's comment correctly, the name mangling we're talking about emulates static binding, i.e., these members should not be late bound/overridable, but they need not be private.)

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Apr 28, 2016

Member

Sorry about the delay here -- unfortunately this will need a rebase.

Member

adriaanm commented Apr 28, 2016

Sorry about the delay here -- unfortunately this will need a rebase.

@adriaanm adriaanm added WIP and removed on-hold labels May 3, 2016

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym May 19, 2016

Member

@lrytz @adriaanm I've rebased this on top of another open PR that also touches Delambdafy.

Additionally, I have added support for avoiding capturing the enclosing class for lambdas that only need to instantiate local classes elided outer fields.

Member

retronym commented May 19, 2016

@lrytz @adriaanm I've rebased this on top of another open PR that also touches Delambdafy.

Additionally, I have added support for avoiding capturing the enclosing class for lambdas that only need to instantiate local classes elided outer fields.

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym May 19, 2016

Member

/rebuild

Member

retronym commented May 19, 2016

/rebuild

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym May 19, 2016

Member

Looks like that null is getting used somewhere.

[quick.partest-extras] Exception in thread "main" java.lang.NullPointerException
[quick.partest-extras]  at scala.collection.LinearSeqLike$$anon$1.hasNext(LinearSeqLike.scala:43)
[quick.partest-extras]  at scala.collection.TraversableOnce.collectFirst(TraversableOnce.scala:144)
[quick.partest-extras]  at scala.tools.util.PathResolver$Environment$.searchForBootClasspath(PathRes
Member

retronym commented May 19, 2016

Looks like that null is getting used somewhere.

[quick.partest-extras] Exception in thread "main" java.lang.NullPointerException
[quick.partest-extras]  at scala.collection.LinearSeqLike$$anon$1.hasNext(LinearSeqLike.scala:43)
[quick.partest-extras]  at scala.collection.TraversableOnce.collectFirst(TraversableOnce.scala:144)
[quick.partest-extras]  at scala.tools.util.PathResolver$Environment$.searchForBootClasspath(PathRes
@lrytz

This comment has been minimized.

Show comment
Hide comment
@lrytz

lrytz May 19, 2016

Member
trait LinearSeqLike .. { self =>
  def iterator: Iterator = new AbstractIterator {
    var these = self
    def hasNext = !these.isEmpty
  }
}

The anonymous class constructor has a parameter $outer: LinearSeqLike which is only used in the constructor (stored in field these), so no field for $outer is needed; but its value is still required..

Member

lrytz commented May 19, 2016

trait LinearSeqLike .. { self =>
  def iterator: Iterator = new AbstractIterator {
    var these = self
    def hasNext = !these.isEmpty
  }
}

The anonymous class constructor has a parameter $outer: LinearSeqLike which is only used in the constructor (stored in field these), so no field for $outer is needed; but its value is still required..

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym May 20, 2016

Member

@lrytz Thanks for investigating. Hopefully it will be as easy as adding a check to Constructors that the outer param is not referenced in any of the constructors, other than to forward to the primary constructor.

Member

retronym commented May 20, 2016

@lrytz Thanks for investigating. Hopefully it will be as easy as adding a check to Constructors that the outer param is not referenced in any of the constructors, other than to forward to the primary constructor.

@adriaanm adriaanm added the WIP label May 24, 2016

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm May 24, 2016

Member

Adding the WIP label since scabot will insist on adding "reviewed" based on earlier comments.

Member

adriaanm commented May 24, 2016

Adding the WIP label since scabot will insist on adding "reviewed" based on earlier comments.

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym May 25, 2016

Member

Still trying:

[quick.library] Exception in thread "main" java.lang.NullPointerException
[quick.library]     at scala.tools.nsc.transform.Constructors$TemplateTransformer.scala$tools$nsc$transform$Constructors$TemplateTransformer$$$anonfun$34(Constructors.scala:718)
[quick.library]     at 
Member

retronym commented May 25, 2016

Still trying:

[quick.library] Exception in thread "main" java.lang.NullPointerException
[quick.library]     at scala.tools.nsc.transform.Constructors$TemplateTransformer.scala$tools$nsc$transform$Constructors$TemplateTransformer$$$anonfun$34(Constructors.scala:718)
[quick.library]     at 
Avoid tree sharing with substituteThis
The underlying transformer has a by-name parameter for the
to provide the `to` tree, but this was strict in the layers
of API above.

Tree sharing is frowned upon in general as it leads to cross
talk when, e.g., the erasure typechecker mutates the
`tpe` field of the shared tree in different context.

@retronym retronym referenced this pull request Jun 1, 2016

Closed

Add/elim static this #624

retronym and others added some commits May 4, 2016

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.
SI-9390 Emit local defs that don't capture this as static
This avoids unnecessary memory retention, and allows lambdas
that call the local methods to be serializable, regardless of
whether or not the enclosing class is serializable.

The second point is especially pressing, given that the enclosing
class for local methods defined in a used to be the (serializable)
anonymous function class, but as of Scala 2.12 will be the enclosing
class of the lambda.

This change is similar in spirit to SI-9408 / 93bee55.
@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Jun 1, 2016

Member

@adriaanm @lrytz The last commit here needs review now.

Member

retronym commented Jun 1, 2016

@adriaanm @lrytz The last commit here needs review now.

@retronym retronym removed the WIP label Jun 1, 2016

@lrytz

This comment has been minimized.

Show comment
Hide comment
@lrytz

lrytz Jun 1, 2016

Member

@adriaanm fyi, the first three commits here are the same as in #5157

Member

lrytz commented Jun 1, 2016

@adriaanm fyi, the first three commits here are the same as in #5157

@lrytz

This comment has been minimized.

Show comment
Hide comment
@lrytz

lrytz Jun 1, 2016

Member

the last commit message seems incomplete:

However, the status quo means that a lambda that
This in turn makes lambdas that refer to such classes serializable
even when the outer class is not.

I'll updating this to explicitly pass the implicit phrase "itself serializable" :)

Member

lrytz commented Jun 1, 2016

the last commit message seems incomplete:

However, the status quo means that a lambda that
This in turn makes lambdas that refer to such classes serializable
even when the outer class is not.

I'll updating this to explicitly pass the implicit phrase "itself serializable" :)

@lrytz

This comment has been minimized.

Show comment
Hide comment
@lrytz

lrytz Jun 1, 2016

Member

looks good otherwise!

Member

lrytz commented Jun 1, 2016

looks good otherwise!

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Jun 2, 2016

Member

ping @retronym -- lets apply a quick round of polish based on Lukas's feedback and get this merged today

Member

adriaanm commented Jun 2, 2016

ping @retronym -- lets apply a quick round of polish based on Lukas's feedback and get this merged today

@lrytz

This comment has been minimized.

Show comment
Hide comment
@lrytz

lrytz Jun 2, 2016

Member

note that merging this will also merge the commits it shares with #5157

Member

lrytz commented Jun 2, 2016

note that merging this will also merge the commits it shares with #5157

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Jun 3, 2016

Member

Ok, I'm going to let you coordinate these two then.

Member

adriaanm commented Jun 3, 2016

Ok, I'm going to let you coordinate these two then.

@adriaanm adriaanm assigned lrytz and unassigned adriaanm Jun 3, 2016

SI-9390 Avoid needless outer capture with local classes
An existing optimization in `Constructors` elides the outer
field in member and local classes, if the class doesn't use
the outer reference. (Member classes also need to be final,
which is a secret handshake to say we're also happy to weaken
prefix matching in the pattern matcher.)

That optimization leaves the constructor signature as is: the
constructor still accepts the outer instance, but does not store
it. For member classes, this means that we can separately compile
code that calls the constructor.

Local classes need not be hampered by this constraint, we could
remove the outer instance from the constructor call too.

Why would we want to do this?

Let's look at the case before and after this commit.

Before:
```
class C extends Object {
  def foo(): Function1 = $anonfun();
  final <static> <artifact> def $anonfun$foo$1($this: C, x: Object): Object = new <$anon: Object>($this);
  def <init>(): C = {
    C.super.<init>();
    ()
  }
};
final class anon$1 extends Object {
  def <init>($outer: C): <$anon: Object> = {
    anon$1.super.<init>();
    ()
  }
}
```

After:

```
class C extends Object {
  def foo(): Function1 = $anonfun();
  final <static> <artifact> def $anonfun$foo$1(x: Object): Object = new <$anon: Object>(null);
  def <init>(): C = {
    C.super.<init>();
    ()
  }
};
final class anon$1 extends Object {
  def <init>($outer: C): <$anon: Object> = {
    anon$1.super.<init>();
    ()
  }
}

```

However, the status quo means that a lambda that
This in turn makes lambdas that refer to such classes serializable
even when the outer class is not itself serialiable.

I have not attempted to extend this to calls to secondary constructors.
@lrytz

This comment has been minimized.

Show comment
Hide comment
@lrytz

lrytz Jun 3, 2016

Member

LGTM

Member

lrytz commented Jun 3, 2016

LGTM

@retronym retronym referenced this pull request Jun 6, 2016

Merged

Lambda impl methods static and more stably named #5157

1 of 1 task complete

@retronym retronym merged commit 3bb7358 into scala:2.12.x Jun 6, 2016

6 checks passed

cla @retronym signed the Scala CLA. Thanks!
Details
combined All previous commits successful.
integrate-ide [2264] SUCCESS. Took 3 s.
Details
validate-main [2538] SUCCESS. Took 99 min.
Details
validate-publish-core [2505] SUCCESS. Took 4 min.
Details
validate-test [2224] SUCCESS. Took 95 min.
Details

@adriaanm adriaanm added 2.12 and removed 2.12 labels Oct 29, 2016

retronym added a commit to retronym/scala that referenced this pull request Jul 24, 2017

Don't assume final member class in another comp. unit will stay final
The optimization in scala#5099 that avoided needless capture of the enclosing
class, and hence improved serializability of local classes and functions,
went too far. It also caused constructor calls of member classes to
use `null` rather than the actual outer reference as the `$outer`
constructor argument. The enclosed test case exhibits this problem
by witnessing an null `Outer.this`.

This commit limits the strategy to use `null`-s to the outer refererences
of anonymous and local classes, which rules out cross compilation unit
effects (in absence of `-opt`.)

milessabin added a commit to milessabin/scala that referenced this pull request Aug 1, 2017

Don't assume final member class in another comp. unit will stay final
The optimization in scala#5099 that avoided needless capture of the enclosing
class, and hence improved serializability of local classes and functions,
went too far. It also caused constructor calls of member classes to
use `null` rather than the actual outer reference as the `$outer`
constructor argument. The enclosed test case exhibits this problem
by witnessing an null `Outer.this`.

This commit limits the strategy to use `null`-s to the outer refererences
of anonymous and local classes, which rules out cross compilation unit
effects (in absence of `-opt`.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment