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-9408 Avoid capturing outer class in local classes. #4652

Merged
merged 3 commits into from Jul 23, 2015

Conversation

Projects
None yet
4 participants
@retronym
Member

retronym commented Jul 22, 2015

Review by @lrytz

@scala-jenkins scala-jenkins added this to the 2.12.0-M3 milestone Jul 22, 2015

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Jul 22, 2015

Member

Commit message error:

-class NoSerializable$C(outer$: C)
+class NoSerializable$C(outer$: NoSerializable)
Member

retronym commented Jul 22, 2015

Commit message error:

-class NoSerializable$C(outer$: C)
+class NoSerializable$C(outer$: NoSerializable)

retronym added some commits Jul 22, 2015

SI-9408 Record known subclasses of local classes
Using the same facility that we use to record subclasses of
sealed classes, record the subclasses of term-owned ("local")
classes.

I have changed existing callers of `children` to use `sealedChildren`
so we don't start using this new information in pattern matching
and type pattern checkability analysis.

The following commit will build on this to infer finality of local
classes in the context of outer pointer elision in the constructors
phase.
Refactor to avoid duplicated work in Constructors
 - Check if the clazz that owns all the decls we're filtering is
effectively final once, rather than for each decl.
 - Query the full set of decls once.
SI-9408 Avoid capturing outer class in local classes.
Previously, only local classes declared final would be candidates
for outer pointer elision in the constructor phase.

This commit infers finality of local classes to expand the scope
of this optimization.

== Background ==

This was brought to our attention when shapeless enabled
indylambda and found that a hitherto serializable
data structure started to capture the enclosing class and hence
lost its serializability.

    class NotSerializable {
      def test = () => {
        class C; assertSerializable(new C)
      }
    }

Under `-Ydelambdafy:inline`, it used to capture the enclosing anon
function class as its outer, which, being final, didn't in turn
capture the enclosing class.

    class NotSerializable {
      def test = new anonFun$1
    }
    class anonFun$1 {
      def apply = assertSerializable(new C(this))
    }
    class ...$C(outer$: anonFun)

indylambda perturbs the enclosing structure of the function body.

    class NotSerializable {
      def anonFun$1 = {class C; assertSerializable(new C()))
      def test = lambdaMetaFactory(<<anonFun$1>>)
    }

Which leads to:

    class NotSerializable$C(outer$: NotSerializable)
@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Jul 23, 2015

Member

@lrytz Ready for re-review.

Member

retronym commented Jul 23, 2015

@lrytz Ready for re-review.

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Jul 23, 2015

Member

Previous review comments:
retronym@a4dbd57
retronym@9c89d20

Member

retronym commented Jul 23, 2015

Previous review comments:
retronym@a4dbd57
retronym@9c89d20

@lrytz

This comment has been minimized.

Show comment
Hide comment
@lrytz

lrytz Jul 23, 2015

Member

there are two more things we could do - both of them unrelated to this bugfix

  • a sealed class with no children is also effectively final (though this probably never happens in practice)
  • we could extend isNotOverridden (above) to do the same for local classes as for sealed classes (check if the member is overridden in any of the children).
Member

lrytz commented on src/reflect/scala/reflect/internal/Symbols.scala in 93bee55 Jul 23, 2015

there are two more things we could do - both of them unrelated to this bugfix

  • a sealed class with no children is also effectively final (though this probably never happens in practice)
  • we could extend isNotOverridden (above) to do the same for local classes as for sealed classes (check if the member is overridden in any of the children).
@lrytz

This comment has been minimized.

Show comment
Hide comment
@lrytz

lrytz Jul 23, 2015

Member

LGTM!

Member

lrytz commented Jul 23, 2015

LGTM!

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Jul 23, 2015

Member

I've spun out a new ticket for the suggested improvements: https://issues.scala-lang.org/browse/SI-9414

Member

retronym commented Jul 23, 2015

I've spun out a new ticket for the suggested improvements: https://issues.scala-lang.org/browse/SI-9414

lrytz added a commit that referenced this pull request Jul 23, 2015

Merge pull request #4652 from retronym/ticket/9408
SI-9408 Avoid capturing outer class in local classes.

@lrytz lrytz merged commit 823fb0f into scala:2.12.x Jul 23, 2015

6 checks passed

cla @retronym signed the Scala CLA. Thanks!
Details
combined All previous commits successful.
integrate-ide [255] SUCCESS. Took 3 s.
Details
validate-main [330] SUCCESS. Took 134 min.
Details
validate-publish-core [322] SUCCESS. Took 14 min.
Details
validate-test [256] SUCCESS. Took 119 min.
Details

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment