[Do not merge] Separate Bitraverse hierarchy default implementations

[ceedubs]

This is a first-step for #992. I've started with the Bitraverse type
hierarchy because it's not a very deep one. The idea is that we would
continue this effort for other type class hierarchies.

I am not particularly fond of the DefaultX naming convention, but it was the least bad of the ones I could think of. I'm open to other ideas.

[codecov-io]

Codecov Report

Merging #1194 into master will decrease coverage by 0.06%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##           master    #1194      +/-   ##
==========================================
- Coverage   89.16%   89.09%   -0.07%     
==========================================
  Files         234      234              
  Lines        3137     3164      +27     
  Branches       52       54       +2     
==========================================
+ Hits         2797     2819      +22     
- Misses        340      345       +5

Impacted Files	Coverage Δ
core/src/main/scala/cats/Bitraverse.scala	25% <0%> (-50%)	⬇️
core/src/main/scala/cats/Bifoldable.scala	100% <100%> (ø)	⬆️
core/src/main/scala/cats/data/Validated.scala	95.78% <100%> (ø)	⬆️
core/src/main/scala/cats/data/WriterT.scala	93.93% <100%> (ø)	⬆️
core/src/main/scala/cats/data/Ior.scala	97.43% <100%> (ø)	⬆️
core/src/main/scala/cats/functor/Bifunctor.scala	100% <100%> (ø)	⬆️
core/src/main/scala/cats/instances/tuple.scala	100% <100%> (ø)	⬆️
core/src/main/scala/cats/instances/either.scala	97.36% <100%> (ø)	⬆️
core/src/main/scala/cats/data/Const.scala	92.68% <100%> (ø)	⬆️
core/src/main/scala/cats/data/XorT.scala	93.13% <100%> (ø)	⬆️
... and 6 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 556e97b...4bc34cc. Read the comment docs.

[fthomas]

Thanks for bringing this forward, @ceedubs! One aspect I don't like about this is that it clutters the cats namespace with a lot of DefaultX types. Would it be an option to put the DefaultX traits into the companions of X and just call them Default, e.g. DefaultBitraverse becomse Bitraverse.Default?

[ceedubs]

@fthomas I like that. I'll give it a go.

[kailuowang]

👍 thanks so much @ceedubs

[ceedubs]

@johnynek @non should I make similar changes to the type classes in cats-kernel, or is compatibility already locked in for 1.0 on that module? It seems to me that these changes should be binary compatible but not source compatible, but I would have to check that to be sure.

[peterneyens]

It seems I missed this in #1079, maybe you can change the type projection to λ[(α, β) => F[G[α, β], G[α, β]]] in this PR ?

[johnynek]

I don't think they are binary compatible because the traits may become interface only traits, and that might eliminate the classes that were hanging around to give the implementations.

It sounds like there was already a discussion, but I'll add my thoughts for what they might be worth: I'm excited most about cats when we can use broadly agreed upon best practices for FP in scala (generally learned from a few years of lessons) coupled with well established and valuable abstractions. I'm less excited about more speculative stuff because often those don't work out like we hope, and we want to change it later, which results in churn. I hope cats can (very soon) have a high stability bar so that it is safe to put it as a library that libraries depend on.

I'm personally not super excited about this. I certainly see that a careless implementer might not do the best thing, but I assume that careless person will just use the default types too. I'd rather not make this change to kernel. It seems like it mostly adds friction with the hope of nagging someone into thinking more.

[ceedubs]

Let's hold off on merging this for now. I'm not sure I want to go forward with this if it's going to be a disconnect between kernel and other modules. I think there are some minor benefits from this change, but I'm not sure they are big enough to warrant the discrepancy. However, I probably won't have time to continue the discussion today.

[ceedubs]

@johnynek oh one more note:

I hope cats can (very soon) have a high stability bar so that it is safe to put it as a library that libraries depend on.

Totally agreed. My (personal) goal is a 1.0 release candidate by the end of September. I wasn't sure whether or not there was any wiggle room for kernel compatibility before then, but I think it's acceptable to say that there isn't if it's going to mess up other libraries.

[ceedubs]

@johnynek I'll follow up with some of my thoughts here. I've tried to include some points on both sides of whether or not this should happen. Please feel free to weigh in with any others you may have. Just to put them into perspective, I'm really not set on this change being made. There seemed to be several people who wanted this at the Typelevel Summit in Philly and no real objections, so I've gone forward with it, but I'm okay with it not landing in Cats.

Potential Benefits

Less Arbitrary

Which methods we choose to be abstract and which are derived is somewhat arbitrary in some cases. For example, I tend to think of map2 as the canonical method on Apply and I think we could have chosen for it to be abstract instead of ap. Also pure mathematicians often tend to think of Monads as pure, map, and flatten as opposed to pure and flatMap, though I think in this case the latter is almost certainly a better representation in the scala world (for performance reasons). I like the idea of the type class trait (interface) itself not being opinionated on which methods should be implemented in terms of which others. However, being opinionated can also be convenient, and I think that the Default traits scratch this itch.

Reminds you to Override

I think @non's main goal for this was to be able to opt-in to getting compile errors if you forget to override a method. For example, the default map implementation for Monad in terms of flatMap and pure is probably never the implementation that you want for performance reasons. By extending Monad instead of Monad.Default the compiler could help you in making conscious decisions as to which methods you want to override.

In practice, I think it's fairly rare that you would want to do this, because it's going to result in a lot of boilerplate, and you are probably usually okay if you just remember to override a few common methods.

There is one particular use-case in which I have wanted this. I've wanted to create helpers for creating type class instances for isomorphic types. For example, if you have an IsoFunctor[JsonDecoder, Kleisli[String Xor ?, Json, ?]] then I'd like (likely in another module or project) for there to be an easy way for you to create JsonDecoder instances that simply delegate through to the Kleisli instances, converting between the two at the edges. In that case, I'd probably want traits like IsoMonad that override every single method in the Monad trait and use the conversion, so that I can reap the rewards of any optimization overrides in the Kleisli instance (okay I think Kleisli was probably a bad example here). With the current Cats setup, when a new method is added to the Monad type class trait, you are almost certainly going to forget to add an override to your IsoMonad instance, because the code is in a separate location and you'll inherit the default implementation without the compiler telling you.

Again, in practice this probably wouldn't be that bad as long as you remembered to override several key methods. I think the lack of compiler help is unfortunate, but it's not the end of the world.

Pure Interface

This results in interface-only traits. This could potentially be helpful for people who want to use Cats from Java, but I don't think we've ever had that goal in mind, so I don't really put any weight on this.

Potential Downsides

Compatibility

This is currently not how Cats is set up (including cats-kernel). Introducing it would be both a source- and binary-incompatible change.

Indirection

It's another layer of indirection for users. Now they not only need to know what a Functor is, but also that if they want to create their own they probably want to extend Functor.Default so they don't have to put a ton of boilerplate in their instance.

I think this downside can largely be counteracted by a little bit of effort in the documentation, but it's still there.

Rare use-cases

I think that even within the Cats source itself we will probably end up using the Default implementation 95% of the time. It's just too much boilerplate to not use it on many of these type classes. If we leave open the option of not inheriting default methods but we always direct people to extend Default and we only very rarely take advantage of this feature ourselves, is it providing more hassle than benefit?

[ceedubs]

@non what are your thoughts on this? I know when we met up a while back we had mentioned this being something we wanted in 1.0.

[julien-truffaut]

While I was working on Unfoldable, I experimented with default methods being defined separately in the companion object e.g. https://github.com/typelevel/cats/pull/1132/files#diff-80384f2930d1f67470bf655c8943b977R36

One advantage is that we can reuse them to write consistency laws.

[ceedubs]

@julien-truffaut that looks like a pretty good idea to me.

Though there is still the question of whether the default implementations should exist in the type class trait itself. It sounds like @johnynek votes no. I think that I could be convinced either way. @typelevel/cats does anyone have strong opinions on this? It's a big decision item that needs to be resolved before 1.0.

[edmundnoble]

@ceedubs Do you plan to include default implementations in 1.0, or would this PR be better off closed for now?

[kailuowang]

Closing stale PRs. Feel free to reopen if there is interest to revive the effort.