Skip to content
This repository has been 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

Conversation

eed3si9n
Copy link
Member

@eed3si9n eed3si9n commented Feb 5, 2015

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.

@jsuereth
Copy link
Contributor

jsuereth commented Feb 6, 2015

un mergeable...

@jsuereth
Copy link
Contributor

jsuereth commented Feb 6, 2015

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

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
Copy link
Member Author

eed3si9n commented Feb 8, 2015

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

@jsuereth
Copy link
Contributor

jsuereth commented Feb 9, 2015

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

// 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?

@eed3si9n
Copy link
Member Author

eed3si9n commented Feb 9, 2015

@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.

@jvican
Copy link
Member

jvican commented May 6, 2016

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants