Dubious optionalFromWrite VS fromPutOption

[satorg]

I noticed that in the current version of Doobie fromPutOption and fromGetOption have lower priority than optionalFromWrite and fromReadOption correspondingly:

doobie/modules/core/src/main/scala/doobie/util/write.scala

Lines 175 to 187 in 273a4cb

	trait LowerPriority1Write extends LowerPriority2Write {
	implicit def optionalFromWrite[A](implicit write: Write[A]): Write[Option[A]] =
	write.toOpt
	}
	trait LowerPriority2Write extends WritePlatform {
	implicit def fromPut[A](implicit put: Put[A]): Write[A] =
	new Write.Single(put)
	implicit def fromPutOption[A](implicit put: Put[A]): Write[Option[A]] =
	new Write.SingleOpt(put)
	}

doobie/modules/core/src/main/scala/doobie/util/read.scala

Lines 175 to 187 in 273a4cb

	trait LowerPriority1Read extends LowerPriority2Read {
	implicit def fromReadOption[A](implicit read: Read[A]): Read[Option[A]] = read.toOpt
	}
	trait LowerPriority2Read extends ReadPlatform {
	implicit def fromGet[A](implicit get: Get[A]): Read[A] = new Read.Single(get)
	implicit def fromGetOption[A](implicit get: Get[A]): Read[Option[A]] = new Read.SingleOpt(get)
	}

Out of curiosity, I commended out lines with fromPutOption and fromGetOption and re-built the project (for all Scala versions). No build errors appeared. Then I ran all the tests for all Scala versions. No errors whatsoever.

Taking into account that there are also fromPut/fromGet derivers in the scope, it looks like fromPutOption/fromGetOption are not really working and have no effect on the derivation process – they just don't seem getting any chance to be picked by the compiler. So I wonder shouldn't these methods be rather prioritized over optionalFromWrite and fromReadOption?

On the other hand, optionalFromWrite and fromReadOption look a bit dubious themselves. Having them, now it is possible to constuct something like this:

Write[Option[Option[Option[Int]]]]
Read[Option[Option[Option[Int]]]]

which doesn't seem making any sense. Apparently, that was not possible with fromPutOption and fromGetOption only.

To be honest, I'm not sure if optionalFromWrite and fromReadOption are really necessary – I couldn't come up with any practical purpose for them. But I might be missing something out. But it looks like there something can be improved in regards to these two pairs of derivation methods.

[satorg]

cc @jatcwang , wdyt?

[jatcwang]

The intention of fromPutOption / fromGetOption is to skip the nested call of fromReadOption(fromGet(..)). They should be semantically equivalent so no test failures that you saw is expected. But you're probably right that perhaps they're never actually used, because I think we're going to hitfromReadOption and it will go on to find fromGet to form Read[Option[A]].
We can remove them to be honest.

optionalFromWrite, fromReadOption are necessary because we want to generically obtain a Read[Option[A]] from a Read[A] (for any A). Read[Option[Option[A]]] is valid and required if we want to make thin - it'll always result in Some(None) or Some(Some(..)).
It is what we used to fix left join behaviour from RC5. There's a test for this but the example can probably be improved to make it easier to understand..

(This wasn't the case in RC5 - you had to derive Read[Option[A]] from scratch which require acess to the concrete type of A itself.)

[satorg]

In a case of doubt, as a trade-off approach it could be switching to semi-auto derivation instead of fully-auto one.
I.e. if one has case class Foo(...) and wants to use it for outer joins, then Read[Option[Foo]] instance could be created on demand, by calling it out explicitly

given Read[Option[Foo]] = fromReadOption[Foo]

assuming that fromReadOption is no longer implicit.

It would be somewhat similar to creating Put/Get for JSON serialization from postgres-circe module. Usually, we don't want to get those instances generated automatically. So we have to call it out explicitly for specific type, if we need it.

[jatcwang]

If this is a fresh project then yeah I think what you're suggesting is fine, but automatic provisioning of Option instances are used pervasively in existing codebases I think this change will break too many existing code.

I'll document this more thoroughly in code and FAQ

[satorg]

Just to clarify, automatic lifting of Read[A] => Read[Option[A]] was introduced just recently, somewhen after RC5, wasn't it?

Lifting of Get[A] into Read[Option[A]] does not cause any ambiguity, so it is absolutely safe to keep it.
Liifting of Read[A] => Read[Option[A]] does cause ambiguities, so it may be a good idea to reconsider its implicity.
But since it is a very recent addition within RCx versions, it probably should not cause a lot of turmoil.

[satorg]

Let me elaborate a bit why I think that it could be be a good idea to make Read[A] => Read[Option[A]] and likewise Write conversions non-implicit.

• If we keep those conversions implicit, then it leads to the following implications (as described above):

  • conversions fromGetOption and fromPutOptions never work anymore;
  • moreover, conversions fromGet and fromPut along with Get and Put themselves do not make a lot of sense either (❖);
  • and even if we refactor that out, still we'll be getting inconsistent results with Read[A] => Read[Option[A]] depending on how many Option fields the A type has.

• If we simply make fromReadOption and fromWriteOption non-implicit and ask users to call them in a semi-auto way, then:

  • conversions fromGetOption and fromPutOptions work as usual;
  • Get and Put and corresponsing conversions totally make sense too;
  • and no other ambiquity introduced.

---

❖ – this part is probably the most concerning one because Get and Put become unnecessary, but if we decide to refactor them out, it may affect a lot of downstream libraries like circe-postgress as well as user-side code. However, if we keep them, then it makes the API "bloated" with unnecessary stuff for no apparent reason.

[jatcwang]

Yes it was introduced after RC5, but the breakages I'm talking about is how a query like this:

fr"...".query[(Animal, Option[Person])]

used to work in RC5, but won't compile if we make fromReadOption and fromWriteOption non-implicit.
I try to make it relatively easy to upgrade doobie (especially if you use it directly). To me this change will require too much work from the users to migrate to be worth doing now.

Similarly, getting rid of Put/Get will be just as (if not more) painful for the user and isn't worth it IMO for 1.0