Closed jvican closed 8 years ago
Also, it's worth mentioning the fact that we reduce considerably the sourcegen code (by 2/3), which is good for future maintainability of the project.
I've discovered some issues with this PR, don't merge (yet) please.
Yeah, the sourgen reduction is nice. I like where we're going. a few notes:
PicklerUnpickler
In any case, great work! Really awesome to see all these cleanups!!
This can already be merged, but there's no way to share/reuse already generated picklers/unpicklers so far and I don't know if it's going to be possible either. Implicits never get out of the scope. The only way to reuse them is this one:
implicit val pu = PicklerUnpickler.generate[T]
Note the implicit
keyword in the definition of pu. This only can be done in concrete cases. Currently, the way the algorithm proceeds is super inefficient, since it's creating (un)picklers for every call site, and repeating the generation of a LOT of picklers/unpicklers (including the nested ones). However, it's not sure if there's another alternative.
My last commit explains how exposing an AnyPickler
speeds up the compilation time by 40% in my computer (the generation of a pickler for any is crazily requested when compiling the test suite). However, there's room for improvement, and we should try something to reuse the generated picklers/unpicklers.
Agree on having AnyPickler be there by default. I think because we DON'T have contravariant picklers and covariant unpicklers (by necessity) this is fine.
Relating to avoiding generating the code, for PicklerUnpickler.generate, the implicit bit is the best we can do.
At some point I'd love to be able to have something like:
sealed trait Bar
@Autopicklers object Bar
@Autopicklers
final case class Foo....
Where we can autogenerate the implicit val
for the type. Right now though, I think having implicit-val in companion obejcts isn't the worst.
If you think you're ready on this PR, it's good t merge. Looks good!
Yes, that's exactly what I thought, the only way AnyPickler
could be chosen is if it we explicitly pickle something which is Any
. And it that's the case, it's the user's fault.
Yes, let's merge this, I'll move this conversation to a ticket so that we can make a serious/formal proposal.
BTW, I was also thinking about that macro annotation. It's a great idea although it changes the design and the API. But we would really get good improvements. Right now, the code size is bloated of unnecessary repetitive picklers/unpicklers.
Pickler[T]
/Unpickler[T]
together instead of creating them separately. This reduces final code size since now there's only one class instead of two and also improves the reuse of already generated picklers (see issue #409).AbstractPicklerUnpickler
, there can be some errors because of ambiguous implicit values (e.g. sealed-trait-static) when (un)picklers are explicitly called viagenerate
. The fix is to only usePicklerUnpickler.generate
and deprecate separate explicit creation withPickler.generate
andUnpickler.generate
.Any
andEither
to generate pickler/unpickler together.