Skip to content

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

Merged
merged 5 commits into from
Sep 29, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 20 additions & 7 deletions compiler/src/dotty/tools/dotc/core/Annotations.scala
Original file line number Diff line number Diff line change
Expand Up @@ -142,15 +142,28 @@ object Annotations {
apply(defn.AliasAnnot, List(
ref(TermRef(sym.owner.thisType, sym.name, sym))))

def makeChild(delayedSym: Context => Symbol)(implicit ctx: Context): Annotation = {
def makeChildLater(implicit ctx: Context) = {
val sym = delayedSym(ctx)
New(defn.ChildAnnotType.appliedTo(sym.owner.thisType.select(sym.name, sym)), Nil)
/** Extractor for child annotations */
object Child {

/** A deferred annotation to the result of a given child computation */
def apply(delayedSym: Context => Symbol)(implicit ctx: Context): Annotation = {
def makeChildLater(implicit ctx: Context) = {
val sym = delayedSym(ctx)
New(defn.ChildAnnotType.appliedTo(sym.owner.thisType.select(sym.name, sym)), Nil)
}
deferred(defn.ChildAnnot, implicit ctx => makeChildLater(ctx))
}
deferred(defn.ChildAnnot, implicit ctx => makeChildLater(ctx))
}

def makeChild(sym: Symbol)(implicit ctx: Context): Annotation = makeChild(_ => sym)
/** A regular, non-deferred Child annotation */
def apply(sym: Symbol)(implicit ctx: Context): Annotation = apply(_ => sym)

def unapply(ann: Annotation)(implicit ctx: Context): Option[Symbol] =
if (ann.symbol == defn.ChildAnnot) {
val AppliedType(tycon, (arg: NamedType) :: Nil) = ann.tree.tpe
Some(arg.symbol)
}
else None
}

def makeSourceFile(path: String)(implicit ctx: Context) =
apply(defn.SourceFileAnnot, Literal(Constant(path)))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,7 @@ class ClassfileParser(
ctx.warning(s"no linked class for java enum $sym in ${sym.owner}. A referencing class file might be missing an InnerClasses entry.")
else {
if (!(enumClass is Flags.Sealed)) enumClass.setFlag(Flags.AbstractSealed)
enumClass.addAnnotation(Annotation.makeChild(sym))
enumClass.addAnnotation(Annotation.Child(sym))
}
}
} finally {
Expand Down
16 changes: 13 additions & 3 deletions compiler/src/dotty/tools/dotc/core/tasty/TreePickler.scala
Original file line number Diff line number Diff line change
Expand Up @@ -570,11 +570,21 @@ class TreePickler(pickler: TastyPickler) {
if (flags is Covariant) writeByte(COVARIANT)
if (flags is Contravariant) writeByte(CONTRAVARIANT)
}
sym.annotations.foreach(pickleAnnotation)
sym.annotations.foreach(pickleAnnotation(sym, _))
}

def pickleAnnotation(ann: Annotation)(implicit ctx: Context) =
if (ann.symbol != defn.BodyAnnot) { // inline bodies are reconstituted automatically when unpickling
private def isUnpicklable(owner: Symbol, ann: Annotation)(implicit ctx: Context) = ann match {
case Annotation.Child(sym) => sym.isInaccessibleChildOf(owner)
// If child annotation refers to a local class or enum value under
// a different toplevel class, it is impossible to pickle a reference to it.
// Such annotations will be reconstituted when unpickling the child class.
// See tests/pickling/i3149.scala
case _ => ann.symbol == defn.BodyAnnot
// inline bodies are reconstituted automatically when unpickling
}

def pickleAnnotation(owner: Symbol, ann: Annotation)(implicit ctx: Context) =
if (!isUnpicklable(owner, ann)) {
writeByte(ANNOTATION)
withLength { pickleType(ann.symbol.typeRef); pickleTree(ann.tree) }
}
Expand Down
6 changes: 5 additions & 1 deletion compiler/src/dotty/tools/dotc/core/tasty/TreeUnpickler.scala
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import util.Positions._
import ast.{tpd, Trees, untpd}
import Trees._
import Decorators._
import transform.SymUtils._
import TastyUnpickler._, TastyBuffer._
import scala.annotation.{tailrec, switch}
import scala.collection.mutable.ListBuffer
Expand Down Expand Up @@ -663,7 +664,6 @@ class TreeUnpickler(reader: TastyReader, nameAtRef: NameRef => TermName, posUnpi
def isCodefined =
roots.contains(companion.denot) == seenRoots.contains(companion)
if (companion.exists && isCodefined) {
import transform.SymUtils._
if (sym is Flags.ModuleClass) sym.registerCompanionMethod(nme.COMPANION_CLASS_METHOD, companion)
else sym.registerCompanionMethod(nme.COMPANION_MODULE_METHOD, companion)
}
Expand Down Expand Up @@ -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)
Copy link
Contributor

@liufengyun liufengyun Sep 28, 2017

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?

Copy link
Contributor Author

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

// Child annotations for local classes and enum values are not pickled, so
// need to be re-established here.
sym.registerIfChild(late = true)
tree
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -838,7 +838,7 @@ class Scala2Unpickler(bytes: Array[Byte], classRoot: ClassDenotation, moduleClas
val start = readIndex
readNat() // skip reference for now
target.addAnnotation(
Annotation.makeChild(implicit ctx =>
Annotation.Child(implicit ctx =>
atReadPos(start, () => readSymbolRef())))
}
}
Expand Down
17 changes: 2 additions & 15 deletions compiler/src/dotty/tools/dotc/transform/PostTyper.scala
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import SymDenotations._, Symbols._, StdNames._, Annotations._, Trees._, Scopes._
import util.Positions._
import Decorators._
import config.Printers.typr
import Symbols._, TypeUtils._
import Symbols._, TypeUtils._, SymUtils._
import reporting.diagnostic.messages.SuperCallsNotAllowedInline

/** A macro transform that runs immediately after typer and that performs the following functions:
Expand Down Expand Up @@ -124,14 +124,9 @@ class PostTyper extends MacroTransform with SymTransformer { thisTransformer =>
private def transformAnnot(annot: Annotation)(implicit ctx: Context): Annotation =
annot.derivedAnnotation(transformAnnot(annot.tree))

private def registerChild(sym: Symbol, tp: Type)(implicit ctx: Context) = {
val cls = tp.classSymbol
if (cls.is(Sealed)) cls.addAnnotation(Annotation.makeChild(sym))
}

private def transformMemberDef(tree: MemberDef)(implicit ctx: Context): Unit = {
val sym = tree.symbol
if (sym.is(CaseVal, butNot = Method | Module)) registerChild(sym, sym.info)
sym.registerIfChild()
sym.transformAnnotations(transformAnnot)
}

Expand Down Expand Up @@ -257,14 +252,6 @@ class PostTyper extends MacroTransform with SymTransformer { thisTransformer =>
ctx.compilationUnit.source.exists &&
sym != defn.SourceFileAnnot)
sym.addAnnotation(Annotation.makeSourceFile(ctx.compilationUnit.source.file.path))

// Add Child annotation to sealed parents unless current class is anonymous
if (!sym.isAnonymousClass) // ignore anonymous class
sym.asClass.classParents.foreach { parent =>
val sym2 = if (sym.is(Module)) sym.sourceModule else sym
registerChild(sym2, parent)
}

tree
}
super.transform(tree)
Expand Down
41 changes: 41 additions & 0 deletions compiler/src/dotty/tools/dotc/transform/SymUtils.scala
Original file line number Diff line number Diff line change
Expand Up @@ -134,4 +134,45 @@ class SymUtils(val self: Symbol) extends AnyVal {
}
}
}

/** If this symbol is an enum value or a named class, register it as a child
* in all direct parent classes which are sealed.
* @param @late If true, register only inaccessible children (all others are already
* entered at this point).
*/
def registerIfChild(late: Boolean = false)(implicit ctx: Context): Unit = {
def register(child: Symbol, parent: Type) = {
val cls = parent.classSymbol
if (cls.is(Sealed) && (!late || child.isInaccessibleChildOf(cls)))
cls.addAnnotation(Annotation.Child(child))
}
if (self.isClass && !self.isAnonymousClass)
self.asClass.classParents.foreach { parent =>
val child = if (self.is(Module)) self.sourceModule else self
register(child, parent)
}
else if (self.is(CaseVal, butNot = Method | Module))
register(self, self.info)
}

/** Is this symbol defined locally (i.e. at some level owned by a term) and
* defined in a different toplevel class than its supposed parent class `cls`?
* Such children are not pickled, and have to be reconstituted manually.
*/
def isInaccessibleChildOf(cls: Symbol)(implicit ctx: Context) =
self.isLocal && !cls.topLevelClass.isLinkedWith(self.topLevelClass)

/** If this is a sealed class, its known children */
def children(implicit ctx: Context): List[Symbol] =
self.annotations.collect {
case Annotation.Child(child) => child
}

/** Is symbol directly or indirectly owned by a term symbol? */
@tailrec def isLocal(implicit ctx: Context): Boolean = {
val owner = self.owner
if (owner.isTerm) true
else if (owner.is(Package)) false
else owner.isLocal
}
}
7 changes: 1 addition & 6 deletions compiler/src/dotty/tools/dotc/transform/patmat/Space.scala
Original file line number Diff line number Diff line change
Expand Up @@ -514,12 +514,7 @@ class SpaceEngine(implicit ctx: Context) extends SpaceLogic {

/** Decompose a type into subspaces -- assume the type can be decomposed */
def decompose(tp: Type): List[Space] = {
val children = tp.classSymbol.annotations.filter(_.symbol == ctx.definitions.ChildAnnot).map { annot =>
// refer to definition of Annotation.makeChild
annot.tree match {
case Apply(TypeApply(_, List(tpTree)), _) => tpTree.symbol
}
}
val children = tp.classSymbol.children
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice refactoring!


debug.println(s"candidates for ${tp.show} : [${children.map(_.show).mkString(", ")}]")

Expand Down
2 changes: 1 addition & 1 deletion compiler/src/dotty/tools/dotc/typer/Namer.scala
Original file line number Diff line number Diff line change
Expand Up @@ -470,7 +470,7 @@ class Namer { typer: Typer =>
case vdef: ValDef if (isEnumConstant(vdef)) =>
val enumClass = sym.owner.linkedClass
if (!(enumClass is Flags.Sealed)) enumClass.setFlag(Flags.AbstractSealed)
enumClass.addAnnotation(Annotation.makeChild(sym))
enumClass.addAnnotation(Annotation.Child(sym))
case _ =>
}

Expand Down
7 changes: 7 additions & 0 deletions tests/pickling/i3149.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
sealed class Foo

class Test {
def f = {
class Bar extends Foo
}
}
14 changes: 14 additions & 0 deletions tests/pos/i3149.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
sealed class Foo

class Test {
def f = {
class Bar extends Foo
}
class C {
class Bar extends Foo
}
object O {
class Bar extends Foo
}
}

14 changes: 14 additions & 0 deletions tests/pos/i3149a.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
sealed class Foo

object Foo {
def f = {
class Bar extends Foo
}
class C {
class Bar extends Foo
}
object O {
class Bar extends Foo
}
}