Merge pull request #160 from scala/ctor-params-no-getter

Deal with constructor parameters properly

core/src/main/scala/pickling/Macros.scala

@@ -108,29 +108,43 @@ trait PicklerMacros extends Macro {
             )
           """)
         } else (nonLoopyFields ++ loopyFields).flatMap(fir => {
+        // for each field, compute a tree for pickling it
+        // (or empty list, if impossible)
+
+        def pickleLogic(fieldValue: Tree): Tree =
+          if (fir.tpe.typeSymbol.isEffectivelyFinal) q"""
+            b.hintStaticallyElidedType()
+            $fieldValue.pickleInto(b)
+          """ else q"""
+            val subPicklee: ${fir.tpe} = $fieldValue
+            if (subPicklee == null || subPicklee.getClass == classOf[${fir.tpe}]) b.hintDynamicallyElidedType()
+            subPicklee.pickleInto(b)
+          """
+
+        def putField(getterLogic: Tree) =
+          q"builder.putField(${fir.name}, b => ${pickleLogic(getterLogic)})"
+
+        // we assume getterLogic is a tree of type Try[${fir.tpe}]
+        def tryPutField(getterLogic: Tree) = {
+          val tryName = c.fresh(newTermName("tr"))
+          val valName = c.fresh(newTermName("value"))
+          q"""
+            val $tryName = $getterLogic
+            if ($tryName.isSuccess) {
+              val $valName = $tryName.get
+              builder.putField(${fir.name}, b => ${pickleLogic(Ident(valName))})
+            }
+          """
+        }
+
         if (sym.isModuleClass) {
           Nil
         } else if (fir.hasGetter) {
-          def putField(getterLogic: Tree) = {
-            def wrap(pickleLogic: Tree) = q"builder.putField(${fir.name}, b => $pickleLogic)"
-            wrap {
-              if (fir.tpe.typeSymbol.isEffectivelyFinal) q"""
-                b.hintStaticallyElidedType()
-                $getterLogic.pickleInto(b)
-              """ else q"""
-                val subPicklee: ${fir.tpe} = $getterLogic
-                if (subPicklee == null || subPicklee.getClass == classOf[${fir.tpe}]) b.hintDynamicallyElidedType() else ()
-                subPicklee.pickleInto(b)
-              """
-            }
-          }
           if (fir.isPublic) List(putField(q"picklee.${newTermName(fir.name)}"))
           else reflectively("picklee", fir)(fm => putField(q"$fm.get.asInstanceOf[${fir.tpe}]"))
         } else {
-          // NOTE: this means that we've encountered a primary constructor parameter elided in the "constructors" phase
-          // we can do nothing about that, so we don't serialize this field right now leaving everything to the unpickler
-          // when deserializing we'll have to use the Unsafe.allocateInstance strategy
-          Nil
+          reflectivelyWithoutGetter("picklee", fir)(fvalue =>
+            tryPutField(q"$fvalue.asInstanceOf[scala.util.Try[${fir.tpe}]]"))
         }
       })
       val endEntry = q"builder.endEntry()"
@@ -259,11 +273,12 @@ trait UnpicklerMacros extends Macro {
       // nevertheless don't despair and try to prove whether this is or is not the fact
       // i was super scared that string sharing is going to fail due to the same reason, but it did not :)
       // in the worst case we can do the same as the interpreted runtime does - just go for allocateInstance
-      val pendingFields = cir.fields.filter(fir =>
-        fir.isNonParam ||
-        (!canCallCtor && fir.isReifiedParam) ||
-        shouldBotherAboutLooping(fir.tpe)
+
+      // pending fields are fields that are restored after instantiation (e.g., through field assignments)
+      val pendingFields = if (!canCallCtor) cir.fields else cir.fields.filter(fir =>
+        fir.isNonParam || shouldBotherAboutLooping(fir.tpe)
       )
+
       val instantiationLogic = {
         if (sym.isModuleClass) {
           q"${sym.module}"
@@ -296,7 +311,15 @@ trait UnpicklerMacros extends Macro {
           val initPendingFields = pendingFields.flatMap(fir => {
             val readFir = readField(fir.name, fir.tpe)
             if (fir.isPublic && fir.hasSetter) List(q"$instance.${newTermName(fir.name)} = $readFir".asInstanceOf[Tree])
-            else if (fir.accessor.isEmpty) List()
+            else if (fir.accessor.isEmpty) List(q"""
+              try {
+                val javaField = $instance.getClass.getDeclaredField(${fir.name})
+                javaField.setAccessible(true)
+                javaField.set($instance, $readFir)
+              } catch {
+                case e: java.lang.NoSuchFieldException => /* do nothing */
+              }
+            """)
             else reflectively(instance, fir)(fm => q"$fm.set($readFir)".asInstanceOf[Tree])
           })
 

core/src/main/scala/pickling/Runtime.scala

@@ -189,7 +189,11 @@ class InterpretedUnpicklerRuntime(mirror: Mirror, tag: FastTypeTag[_])(implicit
               List[FieldIR]()
             } else {
               val (nonLoopyFields, loopyFields) = cir.fields.partition(fir => !shouldBotherAboutLooping(fir.tpe))
-              (nonLoopyFields ++ loopyFields).filter(fir => fir.isNonParam || fir.isReifiedParam)
+              (nonLoopyFields ++ loopyFields).filter(fir =>
+                fir.hasGetter || {
+                  // exists as Java field
+                  scala.util.Try(clazz.getDeclaredField(fir.name)).isSuccess
+                })
             }
 
           def fieldVals = pendingFields.map(fir => {
@@ -223,10 +227,19 @@ class InterpretedUnpicklerRuntime(mirror: Mirror, tag: FastTypeTag[_])(implicit
           if (shouldBotherAboutSharing(tpe)) registerUnpicklee(inst, preregisterUnpicklee())
           val im = mirror.reflect(inst)
 
+          //debug(s"pendingFields: ${pendingFields.size}")
+          //debug(s"fieldVals: ${fieldVals.size}")
+
           pendingFields.zip(fieldVals) foreach {
             case (fir, fval) =>
-              val fmX = im.reflectField(fir.field.get)
-              fmX.set(fval)
+              if (fir.hasGetter) {
+                val fmX = im.reflectField(fir.field.get)
+                fmX.set(fval)
+              } else {
+                val javaField = clazz.getDeclaredField(fir.name)
+                javaField.setAccessible(true)
+                javaField.set(inst, fval)
+              }
           }
 
           inst

core/src/main/scala/pickling/RuntimePickler.scala

@@ -1,5 +1,7 @@
 package scala.pickling
 
+import java.lang.reflect.Field
+
 import scala.reflect.runtime.{universe => ru}
 import ir._
 
@@ -37,7 +39,7 @@ class RuntimePickler(classLoader: ClassLoader, clazz: Class[_])(implicit pf: Pic
   import ru._
 
   sealed abstract class Logic(fir: irs.FieldIR, isEffFinal: Boolean) {
-    def run(builder: PBuilder, im: ru.InstanceMirror): Unit = {
+    def run(builder: PBuilder, picklee: Any, im: ru.InstanceMirror): Unit = {
       val fldMirror = im.reflectField(fir.field.get)
       val fldValue: Any = fldMirror.get
       val fldClass = if (fldValue != null) fldValue.getClass else null
@@ -83,6 +85,41 @@ class RuntimePickler(classLoader: ClassLoader, clazz: Class[_])(implicit pf: Pic
     }
   }
 
+  sealed class PrivateJavaFieldLogic(fir: irs.FieldIR, field: Field) extends Logic(fir, false) {
+    override def run(builder: PBuilder, picklee: Any, im: ru.InstanceMirror): Unit = {
+      field.setAccessible(true)
+      val fldValue = field.get(picklee)
+      val fldClass = if (fldValue != null) fldValue.getClass else null
+
+      //debug(s"pickling field of type: ${fir.tpe.toString}")
+      //debug(s"isEffFinal: $isEffFinal")
+      //debug(s"field value: $fldValue")
+      //debug(s"field class: ${fldClass.getName}")
+
+      // idea: picklers for all fields could be created and cached once and for all.
+      // however, it depends on whether the type of the field is effectively final or not.
+      // essentially, we have to emulate the behavior of generated picklers, which make
+      // the same decision.
+      val fldPickler = SPickler.genPickler(classLoader, fldClass).asInstanceOf[SPickler[Any]]
+      //debug(s"looked up field pickler: $fldPickler")
+
+      builder.putField(field.getName, b => {
+        pickleLogic(fldClass, fldValue, b, fldPickler)
+      })
+    }
+
+    def pickleLogic(fldClass: Class[_], fldValue: Any, b: PBuilder, fldPickler: SPickler[Any]): Unit = {
+      pickleInto(fldClass, fldValue, b, fldPickler)
+    }
+  }
+
+  final class PrivateEffectivelyFinalJavaFieldLogic(fir: irs.FieldIR, field: Field) extends PrivateJavaFieldLogic(fir, field) {
+    override def pickleLogic(fldClass: Class[_], fldValue: Any, b: PBuilder, fldPickler: SPickler[Any]): Unit = {
+      b.hintStaticallyElidedType()
+      pickleInto(fldClass, fldValue, b, fldPickler)
+    }
+  }
+
   // difference to old runtime pickler: create tag based on fieldClass instead of fir.tpe
   def pickleInto(fieldClass: Class[_], fieldValue: Any, builder: PBuilder, pickler: SPickler[Any]): Unit = {
     val fieldTag = FastTypeTag.mkRaw(fieldClass, mirror)
@@ -95,16 +132,28 @@ class RuntimePickler(classLoader: ClassLoader, clazz: Class[_])(implicit pf: Pic
     new SPickler[Any] {
       val format: PickleFormat = pf
 
-      val fields: List[Logic] =
-        cir.fields.filter(_.hasGetter).map { fir =>
-          if (fir.tpe.typeSymbol.isEffectivelyFinal) new EffectivelyFinalLogic(fir)
-          else if (fir.tpe.typeSymbol.asType.isAbstractType) new AbstractLogic(fir)
-          else new DefaultLogic(fir)
-        }
+      val fields: List[Logic] = cir.fields.flatMap { fir =>
+        if (fir.hasGetter)
+          List(
+            if (fir.tpe.typeSymbol.isEffectivelyFinal) new EffectivelyFinalLogic(fir)
+            else if (fir.tpe.typeSymbol.asType.isAbstractType) new AbstractLogic(fir)
+            else new DefaultLogic(fir)
+          )
+        else
+          try {
+            val javaField = clazz.getDeclaredField(fir.name)
+            List(
+              if (fir.tpe.typeSymbol.isEffectivelyFinal) new PrivateEffectivelyFinalJavaFieldLogic(fir, javaField)
+              else new PrivateJavaFieldLogic(fir, javaField)
+            )
+          } catch {
+            case e: java.lang.NoSuchFieldException => List()
+          }
+      }
 
       def putFields(picklee: Any, builder: PBuilder): Unit = {
         val im = mirror.reflect(picklee)
-        fields.foreach(_.run(builder, im))
+        fields.foreach(_.run(builder, picklee, im))
       }
 
       def pickle(picklee: Any, builder: PBuilder): Unit = {

core/src/main/scala/pickling/Tools.scala

@@ -375,6 +375,18 @@ abstract class Macro { self =>
   private var reflectivePrologueEmitted = false // TODO: come up with something better
   def reflectively(target: String, fir: FieldIR)(body: Tree => Tree): List[Tree] = reflectively(newTermName(target), fir)(body)
 
+  def reflectivelyWithoutGetter(target: String, fir: FieldIR)(body: Tree => Tree): List[Tree] = {
+    val pickleeName = newTermName(target)
+    val getFieldValue = q"""
+      val clazz = $pickleeName.getClass
+      scala.util.Try(clazz.getDeclaredField(${fir.name})).map { javaField =>
+        javaField.setAccessible(true)
+        javaField.get($pickleeName)
+      }
+    """
+    List(body(getFieldValue))
+  }
+
   /**
    *  requires: !fir.accessor.isEmpty
    */

core/src/test/scala/pickling/run/ctor-params.scala

@@ -0,0 +1,38 @@
+package scala.pickling.test.ctorparams
+
+import org.scalatest.FunSuite
+import scala.pickling._
+import json._
+
+class Partitioner(numParts: Int) {
+  def numPartitions = numParts
+  override def toString = s"Partitioner($numParts)"
+}
+
+class Partitioner2(numParts: Int, val other: String) {
+  def numPartitions = numParts
+  override def toString = s"Partitioner2($numParts,$other)"
+}
+
+class CtorParamsTest extends FunSuite {
+  test("runtime pickler") {
+    val par: Any = new Partitioner(8)
+    val p: JSONPickle = par.pickle
+    val up = p.unpickle[Any]
+    assert(par.toString == up.toString)
+  }
+
+  test("static pickler") {
+    val par = new Partitioner(8)
+    val p: JSONPickle = par.pickle
+    val up = p.unpickle[Partitioner]
+    assert(par.toString == up.toString)
+  }
+
+  test("ctor param and public getter") {
+    val par = new Partitioner2(8, "test")
+    val p: JSONPickle = par.pickle
+    val up = p.unpickle[Partitioner2]
+    assert(par.toString == up.toString)
+  }
+}