pickling icon indicating copy to clipboard operation
pickling copied to clipboard

Handle sealed classes in check for non-cyclic types

Open eed3si9n opened this issue 9 years ago • 6 comments

This is retargeted pull request for #134. Fixes #128

original case by @phaller

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.

eed3si9n avatar Feb 05 '15 20:02 eed3si9n

un mergeable...

jsuereth avatar Feb 06 '15 13:02 jsuereth

Sorry, i saw this wasn't mergeable, but I think that was from before the rewrite of history anyway.

jsuereth avatar Feb 06 '15 13:02 jsuereth

Saved the original branch to issue-128-original, and rebased this one against 0.10.x.

eed3si9n avatar Feb 08 '15 20:02 eed3si9n

I think this LGTM, but would have to have someone outline more how this was being triggered.

jsuereth avatar Feb 09 '15 13:02 jsuereth

@jsuereth The commit message says:

When deciding to delay the initialization of fields due to possible cycles,...

If we flip this statement, we can derive: When there are possible cycles, we delay the initialization.

#128 is a bug in the delay causing uninitialized field. Here's how unpickler for A looks like after macro expansion:

    def unpickle(tagKey: String, reader: scala.pickling.PReader): Any = if (tagKey.$eq$eq(scala.pickling.FastTypeTag.Null.key))
      null
    else
      if (tagKey.$eq$eq(scala.pickling.FastTypeTag.Ref.key))
        implicitly[Unpickler[refs.Ref]].unpickle(tagKey, reader)
      else
        {
          val oid = scala.pickling.internal.`package`.preregisterUnpicklee();
          val AInstance = new A(null);
          scala.pickling.internal.`package`.registerUnpicklee(AInstance, oid);
          try {
            val javaField = AInstance.getClass.getDeclaredField("intOpt");
            javaField.setAccessible(true);
            javaField.set(AInstance, {
              val reader$macro$17 = reader.readField("intOpt");
              {
                var unpickler$unpickle$macro$18: scala.pickling.Unpickler[Option[Int]] = null;
                unpickler$unpickle$macro$18 = implicitly[scala.pickling.Unpickler[Option[Int]]];
                scala.pickling.internal.GRL.lock();
                try {
                  reader$macro$17.hintTag(unpickler$unpickle$macro$18.tag);
                  <empty>;
                  val typeString = reader$macro$17.beginEntry();
                  val result = unpickler$unpickle$macro$18.unpickle(typeString, reader$macro$17);
                  reader$macro$17.endEntry();
                  <empty>;
                  result.asInstanceOf[Option[Int]]
                } finally scala.pickling.internal.GRL.unlock()
              }
            })
          } catch {
            case (e @ (_: java.lang.NoSuchFieldException)) => ()
          };
          AInstance
        };

I am guessing all that reflection stuff is to handle cycles (your own type appearing in parameter position like Node(child: Option[Node])). Catching NoSuchFieldException is a bit concerning.

eed3si9n avatar Feb 09 '15 18:02 eed3si9n

Since 0.11.x is going to take over 0.10.x, is is safe to close this?

jvican avatar May 06 '16 15:05 jvican