Skip to content
This repository has been archived by the owner on Feb 20, 2019. It is now read-only.

Add tests for AnyPickler/AnyUnpickler #403

Closed
wants to merge 5 commits into from

Conversation

jvican
Copy link
Member

@jvican jvican commented Apr 11, 2016

jsuereth and others added 5 commits April 11, 2016 09:52
- FastTypeTag now retains type cosntructor info
- FastTypeTag mkRaw changes to only use java.lang.Class
- Added flag to disable will robinson warnings
- moved FastTypeTag to tags package
- Added tests for stability of FastTypeTag makeRaw vs. apply
- Update tests/picklers for new methods
- FastTypeTag replaces AppliedType
- Update pickler registry accordingly
- Do not completely fix previously registered "odd" type tags yet.
- note how to limit tpe -> tag conversion to one location
- Update some documentation
- remove/cleanup todos
* AnyUnpickler works if `HybridRuntime` is not used. But as it's in the
  context, it doesn't. AnyPickler doesn't work in any case.

* This could be used to overcome the limitations pointed out in scala#401.
@jvican
Copy link
Member Author

jvican commented Apr 11, 2016

These tests show how the current state of AnyPickler and AnyUnpickler doesn't work. As this looks like something easy to solve, do I have your permission to look into it @jsuereth?
/cc @phaller @heathermiller

@jvican
Copy link
Member Author

jvican commented Apr 11, 2016

BTW, just for the record, AnyPickler fails with a different error when HybridRuntime is not used: it actually doesn't serialize correctly Box and outputs a () and some more garbage.

@phaller
Copy link
Contributor

phaller commented Apr 11, 2016

@jvican How about creating a branch? F-P could then build on this branch until it's merged into 0.11.x.

@jvican
Copy link
Member Author

jvican commented Apr 11, 2016

That's actually my plan @phaller 😄 I'm first working on the fix for this since I've spotted the error. I'll open an issue and create a PR later! Afterwards, we'll use it in a branch of fp, although we'll also need to touch spores-pickling's PR.

@phaller
Copy link
Contributor

phaller commented Apr 11, 2016

@jvican sounds good! 👍

@jsuereth
Copy link
Contributor

@jvican please do! I'd love to see what you come up with. I'll be working on finishing up the FastTypeTag cleanup work, including starting to handle the "tricky" types that are commented out in the test. Hopefully we don't have too many merge conflicts. Ping me if you find issues with the FastTypeTag cleanups.


val b = Box(List(1,2,3,4,5))
val containerPickler = implicitly[Pickler[List[Int]]]
val pickled = b.pickle.value
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note, you'll need to actually register the pickler with the hybrid runtime (currently) so this is expected to fail.

Specifically:

scala.pickling.internal.currentRuntime.picklerRegistry.registerPickler(containerPickler)

Then, AnyPickler should be able to work with hybrid mode.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay of my reply. Yes, I realised that, but even registering the pickler I believe there's no possible way AnyPickler would work. The major problem is that AnyPickler deals with generic types that are erased as if they were Any and therefore the key that uses for looking up in the map is scala.pickling.runtime.Box[scala.Any] which does not match scala.pickling.runtime.Box[scala.collection.immutable.List[scala.Int]].

Even if we register the tag with the Any keyword, the value mapped to that key would not work for other generic types of Box. The only way to solve this problem would be have access to the actual types behind Any, but again, the class name will only return the erased type of Box.

I've also tried other workarounds that give no success.

@jsuereth
Copy link
Contributor

@jvican Just a warning, I'm going to "squash" my commits in the FastTypeTag CL before committing, as I have a lot of loose/extraneous commits in there.

@jvican
Copy link
Member Author

jvican commented Apr 12, 2016

Go ahead @jsuereth 👍 Thanks for the heads up.

@jvican jvican closed this May 6, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants