typelevel / cats

Lightweight, modular, and extensible library for functional programming.
https://typelevel.org/cats/
Other
5.25k stars 1.21k forks source link

Built-in test suites for typeclass instances #168

Open ceedubs opened 9 years ago

ceedubs commented 9 years ago

In #160 I almost suggested a test that lmap f is consistent with dimap f idenity and that rmap g is consistent with dimap identity g but I realized that would just be testing against the default implementations.

I think it could be handy to easily be able to test that all methods for an instance are consistent with the default implementation. For example if you override foldMap in your Foldable instance for performance reasons, you want to make sure that your implementation is consistent with the default.

There are some other tests unrelated to default implementations that could be handy. For example, we may want to test that Foldable.exists (which I don't believe actually currently...exists) is lazy.

I don't think these things really qualify as laws, but we probably want to make sure they hold for all Cats implementations, and other people may want to use them for their custom structures.

This would probably lead to quite a bit of boilerplate somewhere inside cats that would probably be difficult to keep up to date, but I think it could pay for itself in bug prevention.

If people are interested, I could put together a code example of one possible approach to this sort of thing. Let me know what you think.

fthomas commented 9 years ago

While putting #160 together I also thought briefly about checking lmap and rmap in terms of dimap but decided against doing it because I'd have copy&pasted the default implementations into the Laws class. I agree that it could be useful to have consistency checks for methods for which we provide default implementations.

That said, such a consistency check already slipped into FlatMapLaws: https://github.com/non/cats/blob/0bb7353d5de672edc599a96d76912d861a614752/laws/src/main/scala/cats/laws/FlatMapLaws.scala#L16 This one checks that apply is consistent wit an apply derived from flatMap and map. Guess I should submit a PR that removes that check.

non commented 9 years ago

I actually like the idea of a consistency check there -- it helps someone who overrides everything for performance avoid accidentally breaking an "implicit" invariant.

Why don't you leave the other check in for now and we can think about how best to support this situation?

woparry commented 9 years ago

Proposal: Remove all default implementations from cats' typeclasses. It's almost always less performant to use them rather than the methods on the concrete types. This means that Monad would have abstract map, flatMap, etc. Provide additionally a DefaultMonad that has these defaults and requires only flatMap and pure. Add tests to the laws that verify consistency of your implementation with the default ie:

implicit override def F: Monad[F]
val default: Monad[F] = new DefaultMonad[F] {
  def flatMap[A, B](fa: F[A])(f: A => F[B]): F[B] = F.flatMap(fa)(f)
  def pure[A](x: A): F[A] = F.pure(x)
}
def mapConsistent[A,B](fa: F[A], f: A => B): IsEq[F[B]] =
  F.map(fa)(f) <-> default.map(fa)(f)
non commented 9 years ago

That's a really nice design, especially because it means we don't have to "double-encode" the default implementations in both the type class and the test.

I'm trying to think of a better name than DefaultMonad but other than that I like this proposal.

woparry commented 9 years ago

I'll start of a sketch of this for one typeclass and if we like it we can go further.

non commented 9 years ago

Sounds like a plan. Thanks! :8ball:

woparry commented 9 years ago

370