-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix #3149: Fix pickling of child annotations to local classes #3202
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
Conversation
That one was really tricky to get right. |
We could also disallow local classes directly extending a sealed trait since it kinds of defeat the purpose of sealed traits :) |
Actually, here are use cases. PatternMatcher.scala in Dotty is one. Also, Allan told me it was a minimized test case from some community project. |
Actually the test case came to me when I was playing with #3145. |
Where is this used in PatternMatcher.scala ? |
We fail in several PRs with the following. Is this a bug in the buld script? [info] Resolving key references (15718 settings) ... |
Should go away if you rebase. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
It seems we can handle anonymous class using this same trick. See #3145 .
case Apply(TypeApply(_, List(tpTree)), _) => tpTree.symbol | ||
} | ||
} | ||
val children = tp.classSymbol.children |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice refactoring!
@@ -702,6 +702,10 @@ class TreeUnpickler(reader: TastyReader, nameAtRef: NameRef => TermName, posUnpi | |||
if (!sym.isType) { // Only terms might have leaky aliases, see the documentation of `checkNoPrivateLeaks` | |||
sym.info = ta.avoidPrivateLeaks(sym, tree.pos) | |||
} | |||
if ((sym.isClass || sym.is(CaseVal)) && sym.isLocal) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why use isLocal
instead of isInaccessibleChildOf
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because we don't have a parent yet against which to test with inAccessibleChildOf
i3149.scala is an interesting test case: A sealed top-level class `Foo` has a child which is a local (i.e., term-owned) definition in a different top-level class. It is then impossible to establish a symbolic reference to the child class from `Foo`, and it is also impossible to refer to `Foo` using a path. We deal with this by avoiding pickling such child annotations and re-establishing the annotation when the body of the child class is unpickled.
It turns out we have to know a priori whether we should register a local class as a child and we need to be precise about it.
Now that we have defined in centrally, we can avoid the duplication in Space.
i3149.scala is an interesting test case: A sealed top-level class
Foo
has a child which is a local (i.e., term-owned) definition in a different top-level class.
It is then impossible to establish a symbolic reference to the child class
from
Foo
, and it is also impossible to refer toFoo
using a path. We deal with thisby avoiding pickling such child annotations and re-establishing the annotation
when the body of the child class is unpickled.