Skip to content
This repository was archived by the owner on Feb 20, 2019. It is now read-only.

Handle sealed classes in check for non-cyclic types #285

Open
wants to merge 1 commit into
base: 0.10.x
Choose a base branch
from
Open
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
43 changes: 35 additions & 8 deletions core/src/main/scala/pickling/Tools.scala
Original file line number Diff line number Diff line change
Expand Up @@ -258,14 +258,41 @@ abstract class ShareAnalyzer[U <: Universe](val u: U) extends RichTypes {
if (visited(currTpe)) {
if (tpe <:< currTpe) true // TODO: make sure this sanely works for polymorphic types
else loop(rest, visited)
} else if (currTpe.isNotNullable || currTpe.isEffectivelyPrimitive || currSym == StringClass || currSym.isModuleClass) loop(rest, visited)
// TODO: extend the traversal logic to support sealed classes
// when doing that don't forget:
// 1) sealeds can themselves be extended, so we need to recur
// 2) the entire sealed hierarchy should be added to todo
else if (!currSym.isFinal) true // NOTE: returning true here is important for soundness!
else {
val more = newClassIR(currTpe).fields.map(_.tpe)
} else if (currTpe.isNotNullable || currTpe.isEffectivelyPrimitive || currSym == StringClass || currSym.isModuleClass) {
loop(rest, visited)
} else if (currSym.isClass && currSym.asClass.isSealed) {
val currTargs: List[Type] =
currTpe.asInstanceOf[scala.reflect.internal.SymbolTable#Type]
.dealias
.typeArgs
.map(_.asInstanceOf[Type])

// field types have to be OK
val more = flattenedClassIR(currTpe).fields.map(_.tpe)

// all known subclasses have to be OK, too
val ksc = currSym.asClass.knownDirectSubclasses
val subclasses = ksc.map(sym => sym.asClass.toTypeIn(currTpe))

// collect field types of all subclasses
val fieldTypes = subclasses.flatMap { subclassTpe =>
flattenedClassIR(subclassTpe).fields.map(_.tpe)
}

// for field types that are type parameters,
// use type arguments of currTpe (currTargs) instead
val fieldTypesToCheck = fieldTypes.flatMap { tp =>
if (tp.typeSymbol.isParameter) currTargs
Copy link
Contributor

Choose a reason for hiding this comment

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

So, if the type is a symbol we add ALL the type arguments to be checked? I guess there's no way to limit this to the specific one we want to check? How does this handle with any non-trivial type-parameter usage?

else List(tp)
}

val allTodo = rest ++ more ++ fieldTypesToCheck
loop(allTodo, visited + currTpe)
}
else if (!currSym.isFinal) {
true // NOTE: returning true here is important for soundness!
} else {
val more = flattenedClassIR(currTpe).fields.map(_.tpe)
loop(rest ++ more, visited + currTpe)
}
case _ => false
Expand Down
22 changes: 22 additions & 0 deletions core/src/test/scala/pickling/run/issue128.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
package scala.pickling.test.issue128

import scala.pickling._, scala.pickling.Defaults._
import scala.pickling.json._

import org.scalatest.FunSuite

case class A(intOpt: Option[Int]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

e.g. here we should try this with super dirty type parameters that are also "ok to pickle".

intOpt match {
case Some(int) =>
case None =>
}
}

class Issue128Test extends FunSuite {
test("Issue #128") {
val opt = Some(5)
val a = A(opt)
val pickle = a.pickle
assert(pickle.unpickle[A] === a)
}
}