Kittens 2.0 roadmap (resolve inconsistencies)

[joroKr21]

We have discussed this already on other issues and on gitter but it's also good to have a dedicated issue. There are a lot of inconsistencies in kittens 1.0 which cannot be solved in a binary compatible way which dictates a 2.0 release to resolve them.

Here is my attempt to list the issues I noticed and propose solutions:

1. Implicit prioritization scheme: There are a few orthogonal decisions to be taken here:

   1. Traits or abstract classes - I think this one is clear. Abstract classes are less powerful and generate less bytecode.
   2. Public or private[derived] visibility - Implicit prioritization is more or less an implementation detail and should be hidden. But we rely on the fact that private[derived] classes are not publicly visible although their methods are. This might be considered hacky. I still lean towards private[derived].
   3. Should we move more instances to the companion objects? - Usually the companion object only contains an apply summoner and the instances start from the first super class. We could save one class per derivation by moving them one level down. I have no strong feelings either way.

2. Respecting existing instances - I think this one is clear. Hand written code should always take priority. OrElse has proven quite useful here and Functor shows we can do it for HKTs as well.

3. Handling of recursive types - In most cases this should work out of the box. It looks like there are still implicit parameters that need to be wrapped with Lazy but adding tests will show us where. There are however notable exceptions:

   1. Empty - Calling .empty for a recursive type will almost certainly result in SO. I'm talking about direct recursion here, not something that goes through List or Option for which we can also define EmptyK instances. I don't like the possibility of causing SO in user code but also the chances that someone would define a directly recursive case class (not sealed trait) are low. This is also the reason why I think Empty should not be defined for coproducts.
   2. Monoid - Same issue as Empty. Note that users can always define an implicit lazy val with semi derivation.
   3. EmptyK - I think we should remove the coproduct instances for EmptyK for two reasons: recusive ADTs might cause a SO and it's too arbitrary to pick the first coproduct member in alphabetical order.

4. Priority of nested instances vs product and coproduct instances for HKTs. - By nested instances I mean instances defined in terms of Split1 (e.g. Functor[a => List[Option[a]]]). I think nested instances should always have higher priority because they give us a higher chance to hit existing instances instead of falling back to coproduct of products. Also it makes sense to me to try first the outer and then the inner instance when both are possible (e.g. MonoidK over Trivial1 vs MonoidK under Applicative).

5. Naming - There are two competing schemes right now. mkFooHCons (1) vs productDerivedFoo (2). It's not terribly important but I would stick to the majority (1).

I think we should review all derived typeclasses (they are not that many) to make sure the issues are addressed for all of them. I started work on this but would like some opinions on the questions raised.

Tracking progress here:

• [x] Eq - #160
• [x] Hash - #157
• [x] PartialOrder - #152
• [x] Order - #153
• [x] Empty - #135
• [x] Show - #154
• [x] ShowPretty - #155
• [x] Semigroup - #137
• [x] Monoid - #132
• [x] EmptyK - #149
• [x] SemigroupK - #145
• [x] MonoidK - #146
• [x] Pure - #151
• [x] Functor - #140
• [x] Foldable - #139
• [x] Traverse - #138
• [x] ConsK - #136
• [x] Iterable - #163

[joroKr21]

@kailuowang sorry for the wall of text, I hope it wouldn't discourage you to give opinions :smile:

[kailuowang]

Thanks so much @joroKr21 . Overall I think this roadmap is good guidance for 2.0 release.

Traits or abstract classes - I think this one is clear. Abstract classes are less powerful and generate less bytecode.

:+1:

I still lean towards private[derived].

:+1:

Should we move more instances to the companion objects?

I'd say if you already have a bunch of XXXInstancesN class, it feels odd to have some instances in the companion object.

Hand written code should always take priority.

Strong 👍

Empty and EmptyK for coproduct

I learned that users are using these derivations for convenience in test. So the fact that they are not safe for recursive ADT and arbitrary and doesn't bother them. Also if we remove EmptyK derivation for coproduct, do we need to remove it for Pure derivation which relies on EmptyK. For those reasons, I am leaning towards keeping them in.

nested instances should always have higher priority

👍

Naming - There are two competing schemes right now. mkFooHCons (1) vs productDerivedFoo (2). It's not terribly important but I would stick to the majority (1).

👍

[joroKr21]

I learned that users are using these derivations for convenience in test. So the fact that they are not safe for recursive ADT and arbitrary and doesn't bother them. Also if we remove EmptyK derivation for coproduct, do we need to remove it for Pure derivation which relies on EmptyK. For those reasons, I am leaning towards keeping them in.

Yeah I think I agree with you. Trying to prevent SO due to misuse is too brittle anyway.

[milessabin]

For the issues with Lazy and SOEs, I strongly recommend you think about how to migrate to by-name implicits in 2.13+. At this point any effort spent on trying to improve Lazy is wasted IMO.

[milessabin]

it's too arbitrary to pick the first coproduct member in alphabetical order

Agreed. However, if there's a unique coproduct member which has an EmptyK instance, then the choice isn't arbitrary.

[milessabin]

Overall: this is good stuff :+1:

[joroKr21]

For the issues with Lazy and SOEs, I strongly recommend you think about how to migrate to by-name implicits in 2.13+

That's a good point. It should be as simple as: replace all uses of Lazy with =>. But this means more or less a completely separate code base for 2.13.+ unless you have a better idea?

At this point any effort spent on trying to improve Lazy is wasted IMO.

I didn't mean changing Lazy in any way but rather tweaking where it's used. However let's drop this idea.

Agreed. However, if there's a unique coproduct member which has an EmptyK instance, then the choice isn't arbitrary.

Hmm, is there a nice way to express this?

[milessabin]

is there a nice way to express this?

It's just the absence of an EmptyK for the tail. Mutatis mutandis for Pure.

[kailuowang]

replace all uses of Lazy with =>. But this means more or less a completely separate code base for 2.13.+

If we don't want a completely separate code base for 2.13+, maybe the following could be possibly an intermediate solution?

Make Lazy a type alias, on 2.12- it just points to shapeless.Lazy on 2.13+ it will be

trait LazyFromByName[A] {
  def  value a: A
}
object LazyFromByName {
   implicit def fromByName[A](implicit ev: => A): LazyFromByName[A] = new LazyFromByName[A] {
     def value: A = ev
  } 
}

[joroKr21]

Good idea, we should try.

We also need to solve the Hash issue for case classes in 2.13.0-RC1. I don't quite understand the way it was solved in Cats though (typelevel/cats#2792).

[kailuowang]

Didn't know hash for case class is different on 2.13 , we might need to copy from the default case class hash code again.

[joroKr21]

is there a nice way to express this?

It's just the absence of an EmptyK for the tail. Mutatis mutandis for Pure.

It doesn't seem to work with IsCCons1 :disappointed: There the ambiguous implicits error pops up:

[error]  both method ambiguousIfPresent in object Refute of type [T](implicit ev: T)shapeless.Refute[T]
[error]  and method refute in object Refute of type [T](implicit dummy: DummyImplicit)shapeless.Refute[T]
[error]  match expected type shapeless.Refute[cats.derived.MkEmptyK[this.T]]

[Michaelt293]

I wonder whether there is room for a ShowQuoted type class where strings within case classes are wrapped in double quotes. This would allow code to copied and pasted straight out of the REPL and into tests which I think would be quite useful.

[kailuowang]

@Michaelt293 I think that can be achieved by writing your own Show or ShowPretty instance for String that wraps string with double quotes. If such an instance is in scope when deriving the Show or ShowPretty instance of a case class, it will be used and give you the result you want.

[Michaelt293]

Yes, I have used that approach. Works quite well (just have to avoid importing the cats Show instance for String).

[kailuowang]

I am not sure if the convenience justifies a new type class and derivation. Maybe a helper method would suffice?

[Michaelt293]

Agree. Seems like a helper method is the way to go. Thanks for the input.

[joroKr21]

I think we are ready to release. We can wait for stable cats 2.0 or do a milestone release as well.

[kailuowang]

do we need a milesstone release? Cats 2.0-RC1 is close (although not 100% sure because it's pending scalatest 3.1.0-RC1, whose last ETA from maintainer was "tonight" a couple of days ago. )

[joroKr21]

We could also wait for Cats 2.0-RC1 and do an RC release, I don't mind that. We probably don't need a milestone release.