amnaredo / test

0 stars 0 forks source link

Can't opt-out of default ReadWriters #87

Open amnaredo opened 3 years ago

amnaredo commented 3 years ago

It's awesome that uPickle comes with so many default Readers and Writers but it seems that I can't opt out.

Scala still continues to surprise me when it comes to implicits. I'm not importing any but somehow they're always in-scope. For example upickle.write(1) works without any imports, where as I would've imagined that import upickle._ or import upickle.IntRW would've been necessary.

I'm happy to submit a PR. Are you ok with me changing this behaviour?

ID: 32 Original Author: japgolly

amnaredo commented 3 years ago

You can totally opt out

scala> upickle.write(1)
res0: String = 1

scala> implicit val w = upickle.Writer[Int](_ => ???)
w: upickle.Writer[Int] = upickle.Writer$$anon$2@30a7e71

scala> upickle.write(1)
scala.NotImplementedError: an implementation is missing

I'm actually not sure how the implicit readers/writers are appearing in global scope, but I suspect it has something to do with implicit search order (blargh).

On one hand, it's nice to not- have to import all these things automatically. On the other, it means if you do end up replacing them, your replacing isn't typesafe: forget a replacement in one place and you get silently corrupted data when a default implementation "helpfully" gets filled in. This is probably even worse with the case-class macro-implicits, because those apply to all sorts of things you don't expect (and may have thought you already defined a custom implicit for).

One option is to instead of having upickle inheriting from trait upickle.Implicits, have an object upickle.Implicits inheriting from trait upickle.Implicits, and move the macro implicits macroR/macroW into this trait aswell.

That way people by default would need to import upickle.Implicits._, but if they want to customize it, they can easily define an

object MyImplicits extends upickle.Implicits{ 
  ... custom implicit read/writers
}

and import MyImplicits._ everywhere, never again worrying about it implicitly "helpfully" falling back to a default implementation that is different from what they want.

WDYT?

Original Author: lihaoyi

amnaredo commented 3 years ago

Exactly. If you want very safe control over your codecs, if you can't opt-out of everything you never know when you're missing a typeclass. This is the problem I recently encountered.

I did a little experimenting and learnt something interesting: Scala automatically looks for implicit values of T[_] both

I think your idea works, I would just tweak it slightly. Instead of Implicits I would call it Codecs and only include implicit Readers, Writers, ReadWriters. If there are any other types of implicits around I wouldn't include them in Codecs, I'd probably just leave them in the main package depending on what they are.

Original Author: japgolly

amnaredo commented 3 years ago

I've got everything working as discussed, except for a problem with the implicit prioritization in 2.10:

https://groups.google.com/forum/#!topic/scala-internals/XQmg_IBFbIM

The only way I've managed to get implicit prioritization working is to put the low priority macro Reader/Writer in their companion objects, but that makes it impossible to disable them. Still trying to figure out a solution; feel free to clone the repo and beat at it to see if you can make any headway ^_^ I'm reluctant to give up support for 2.10.x because lots of people still use it.

Original Author: lihaoyi

amnaredo commented 3 years ago

Not great, but for now a solution exists for classes that you control, via putting the apply/unapply or Reader/Writer in the companion object:

https://github.com/lihaoyi/upickle#custom-picklers

Not going to do the more general "modularize implicits to require explicit import" thing for now due to the bug I haven't managed to resolve that is described above. Perhaps we can revisit when I stop supporting 2.10 some time in the future, since switching to 2.11's blackbox macros solves the problem entirely.

Original Author: lihaoyi