pickling icon indicating copy to clipboard operation
pickling copied to clipboard

Incorrect handling of Option[xxx] if None

Open jcrowley66 opened this issue 7 years ago • 13 comments

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))) }

jcrowley66 avatar May 31 '17 13:05 jcrowley66

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

michielboekhoff avatar Jun 05 '17 08:06 michielboekhoff

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 avatar Jun 05 '17 20:06 jcrowley66

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

jcrowley66 avatar Jun 05 '17 22:06 jcrowley66

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.

jcrowley66 avatar Jun 05 '17 23:06 jcrowley66

I'll have a look at this later today.

michielboekhoff avatar Jun 06 '17 08:06 michielboekhoff

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.

michielboekhoff avatar Jun 07 '17 14:06 michielboekhoff

Great, thanks.

John Crowley Westport, CT 203-856-2396

On Jun 7, 2017, at 10:43 AM, Michiel [email protected] 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.

jcrowley66 avatar Jun 07 '17 14:06 jcrowley66

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

michielboekhoff avatar Jun 08 '17 11:06 michielboekhoff

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.

michielboekhoff avatar Jun 08 '17 13:06 michielboekhoff

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.

jcrowley66 avatar Jun 08 '17 13:06 jcrowley66

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?

michielboekhoff avatar Jun 08 '17 13:06 michielboekhoff

Now you are definitely beyond my expertise!

jcrowley66 avatar Jun 08 '17 14:06 jcrowley66

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]

jcrowley66 avatar Jun 13 '17 19:06 jcrowley66