scala / pickling

Fast, customizable, boilerplate-free pickling support for Scala
lampwww.epfl.ch/~hmiller/pickling
BSD 3-Clause "New" or "Revised" License
831 stars 79 forks source link

Incorrect handling of Option[xxx] if None #453

Open jcrowley66 opened 7 years ago

jcrowley66 commented 7 years ago

Apologies if this is a duplicate, but several searches did not turn up this problem. [Using JSON format]

With this definition: case class PickleNone(test:Option[String])

this sequence works OK: val test = PickleNone(None) val roundTrip = test.pickle.value.unpickle[PickleNone]

gives test == roundTrip

but this sequence: val test = PickleNone(None) val roundTrip = test.pickle.value.unpickle[AnyRef]

results in test != roundTrip

It appears in the second case that the unpickle step sees the scala.None.type and instantiates a new instance for None. Unfortunately, Scala expects None to be a singleton, and any None == None test is reduced to object identity, which now fails.

Scala 2.11.8 Pickling 0.10.1

Full test case follows

package pickletests

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

case class PickleNone(testOption:Option[String])

object PickleNoneTest extends App { val test = PickleNone(None) val testPickled = test.pickle.value val testUnpickled = testPickled.unpickle[PickleNone] val testAnyRef = testPickled.unpickle[AnyRef] val testAnyRefAsPickleNone = testPickled.unpickle[AnyRef].asInstanceOf[PickleNone]

Console.println("test == testUnpickled is: " + (test == testUnpickled)) Console.println(" test == testAnyRef is: " + (test == testAnyRef)) Console.println(" test == asInstanceOf is: " + (test == testAnyRefAsPickleNone))

Console.println("") Console.println("") Console.println(" test.testOption is: " + test.testOption.toString + " @%8X".format(System.identityHashCode(test.testOption))) Console.println(" testUnpickled.testOption is: " + testUnpickled.testOption.toString + " @%8X".format(System.identityHashCode(testUnpickled.testOption))) Console.println(" testAnyRef.testOption is: " + testAnyRef.asInstanceOf[PickleNone].testOption.toString + " @%8X".format(System.identityHashCode(testAnyRef.asInstanceOf[PickleNone].testOption))) Console.println("testAnyRefAsPickleNone.testOption is: " + testAnyRefAsPickleNone.testOption.toString + " @%8X".format(System.identityHashCode(testAnyRefAsPickleNone.testOption))) }

michielboekhoff commented 7 years ago

Just tested this, this seems to have been fixed in 0.11.x. Can you confirm this, @jcrowley66?

jcrowley66 commented 7 years ago

Ran under both 0.10.1 and 0.11.0-M2. JDK 1.8.0_121 in both cases. Both failed, and 0.11.0-M2 seems to have regressed, since each of the None instances now has a separate object ID. (Sorry about the formatting - can't quite figure it out!)

"C:\Program Files\Java\jdk1.8.0_121\bin\java"

Using scala pickling 0.10.1

test == testUnpickled is: true test == testAnyRef is: false test == asInstanceOf is: false

                  test.testOption is: None @4940809C
         testUnpickled.testOption is: None @4940809C
            testAnyRef.testOption is: None @7A138FC5
testAnyRefAsPickleNone.testOption is: None @379AB47B

Process finished with exit code 0

Using scala pickling 0.11.0-M2

test == testUnpickled is: false test == testAnyRef is: false test == asInstanceOf is: false

                  test.testOption is: None @6928F576
         testUnpickled.testOption is: None @ 9CD25FF
            testAnyRef.testOption is: None @27E0F2F5
testAnyRefAsPickleNone.testOption is: None @3574E198

Process finished with exit code 0

jcrowley66 commented 7 years ago

Sorry re the Close - hit the wrong button. Both of the previous tests run under Windows 7.

jcrowley66 commented 7 years ago

Set up a completely clean project (using IntelliJ) with PickleNoneTest as the only source code present. Ran with 0.10.1 as the only Jar included, then deleted that and ran with 0.11.0-M2 as the only Jar. Same results.

michielboekhoff commented 7 years ago

I'll have a look at this later today.

michielboekhoff commented 7 years ago

I've been able to replicate this issue. Interesting that I hadn't been able to replicate this. I'll have a look to see if I can find a fix.

jcrowley66 commented 7 years ago

Great, thanks.

John Crowley Westport, CT 203-856-2396

On Jun 7, 2017, at 10:43 AM, Michiel notifications@github.com wrote:

I've been able to replicate this issue. Interesting that I hadn't been able to replicate this. I'll have a look to see if I can find a fix.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/scala/pickling/issues/453#issuecomment-306816891, or mute the thread https://github.com/notifications/unsubscribe-auth/Aamnt3zCA47raIIRujGfD3W1SKbhU6n1ks5sBrcbgaJpZM4NrqYh.

michielboekhoff commented 7 years ago

Right, so this issue is intermittently showing. I can't figure out exactly what it is. Anyone available to provide some assistance?

michielboekhoff commented 7 years ago

Right, I've done some more investigation and I'm going to share my findings:

Sometimes, the test I've made passes, seemingly randomly. I think what's happening is that a generated pickler for None is creating a new instance, which gets assigned a different ID. As this is not intended, I would think, it's best to write a manual pickler for Option, which I can have a go at tomorrow.

jcrowley66 commented 7 years ago

Agree, when scala.None.type is detected while unpickling it needs to be handled separately so that the unique scala.None instance is returned. Sorry that I have no experience with the innards of the pickling library, so cannot offer any advice on the implementation.

michielboekhoff commented 7 years ago

I don't think that'd be too hard, I'll set it up. Just wondering why the test passes intermittently... maybe force either runtime pickling or static pickling, see if that makes it better?

jcrowley66 commented 7 years ago

Now you are definitely beyond my expertise!

jcrowley66 commented 7 years ago

This might be a hint - slightly different problem, but in the same ballpark. This is 0.10.1 - will try to run with 0.11.x tomorrow & report if any difference.

This source: responseTo: Option[UUIDCarrier] = None, // None if this is original message gets pickled as: "responseTo": { "$type": "scala.None$" },

which then throws an exception while trying to unpickle:

scala.pickling.PicklingException: Tag scala.None$ not recognized, looking for one of: scala.None.type, scala.Some[realtime.edt.common.UUIDCarrier]