From 95501ffe7f8e19442e5cfc75e4cfbf621e3c098f Mon Sep 17 00:00:00 2001
From: Philipp Haller <hallerp@gmail.com>
Date: Tue, 20 May 2014 13:03:19 +0200
Subject: [PATCH] Handle sealed classes in check for non-cyclic types

When deciding to delay the initialization of fields due to
possible cycles, sealed classes were not handled. This could
cause issues, such as #128, where the initialization of
a constructor parameter was delayed and thus the parameter
was null during the execution of the constructor.
---
 core/src/main/scala/pickling/Tools.scala    | 41 +++++++++++++++++----
 core/src/test/scala/pickling/issue128.scala | 22 +++++++++++
 2 files changed, 56 insertions(+), 7 deletions(-)
 create mode 100644 core/src/test/scala/pickling/issue128.scala

diff --git a/core/src/main/scala/pickling/Tools.scala b/core/src/main/scala/pickling/Tools.scala
index a933941ed1..778e62454c 100644
--- a/core/src/main/scala/pickling/Tools.scala
+++ b/core/src/main/scala/pickling/Tools.scala
@@ -206,13 +206,40 @@ abstract class ShareAnalyzer[U <: Universe](val u: U) {
           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 {
+          } 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
+              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)
           }
diff --git a/core/src/test/scala/pickling/issue128.scala b/core/src/test/scala/pickling/issue128.scala
new file mode 100644
index 0000000000..6eba327ce2
--- /dev/null
+++ b/core/src/test/scala/pickling/issue128.scala
@@ -0,0 +1,22 @@
+package scala.pickling.test.issue128
+
+import scala.pickling._
+import scala.pickling.json._
+
+import org.scalatest.FunSuite
+
+case class A(intOpt: Option[Int]) {
+  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)
+  }
+}