Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Separate default implementations from type class definitions #992

Open
ceedubs opened this issue Apr 23, 2016 · 7 comments
Open

Separate default implementations from type class definitions #992

ceedubs opened this issue Apr 23, 2016 · 7 comments

Comments

@ceedubs
Copy link
Contributor

ceedubs commented Apr 23, 2016

There has been quite a bit of discussion about this going on for a long time, in #168, #370, on Gitter, and at the Typelevel Summit in Philly. But while #168 and #370 are highly relevant, there hasn't been an issue tracking this specifically.

At the Typelevel Summit in Philly, people seemed to come to a consensus that it would be good to not have type classes automatically provide default implementations of methods. For example, someone might want to define their own Monad instance without inheriting a default implementation of map in terms of flatMap and pure (that is likely less efficient than the version of map that they would otherwise define) .
#368 is one approach to solving this problem. At the TL Summit, people seemed to be leaning toward a "minimum viable product" approach that is (arguably) a bit simpler. A type class trait (Monad for example) would contain only abstract def signatures. Someone could extend Monad without inheriting any default implementations. Each type class trait would have a corresponding trait that included default implementations (for example map in terms of flatMap and pure on the trait corresponding to monad). So far all of the name suggestions that we have come up with for these traits with default implementations haven't seemed quite right. DerivedMonad seems misleading, because it could be interpreted as a Monad that is completely derived in a kittens-like fashion. MonadImpl sounds a bit silly, but I guess at least it's concise.

A major motivator for this is that currently it's really easy to not realize that you are inheriting a default implementation whose performance is really bad. I came to want this when I started toying with adding an IsoFunctor (basically a natural transformation in both directions) and I wanted to provide helpers that could create type class instances for your structure as long as you provided an IsoFunctor between your structure and one that has the relevant type class instance. For example, if your JsonDecoder[A] is isomorphic to Kleisli[Xor[Err, ?], Json, A], then you could define an IsoFunctor[JsonDecoder, Kleisli[Xor[Err, ?], Json, ?] and get a derived Monad instance that delegates through to the Monad instance for Kleisli. Defining these instances is very boiler-platey because you have to define each Monad method and have it delegate through to the same method in the instance of the isomorphic type. It would be really easy for a new method to be added to Monad without being added to the isoFunctorMonad, meaning you would pick up the default Monad implementation of that method and not benefit from any performance optimizations that had been added to the type class instance you are delegating through to. Removing the default implementations from the Monad trait would mean you would be forced to resolve a compile error if you forgot to add the new method.

This approach is going to add a fair amount of boilerplate, but that boilerplate will only exist within cats and other code bases that want to go out of their way to make sure they aren't default implementations with poor performance. I think it's worth it. Are people still on board with going forward with this?

@ceedubs ceedubs added this to the cats-1.0.0 milestone Apr 23, 2016
@adelbertc
Copy link
Contributor

adelbertc commented Apr 23, 2016

There is also the approach taken by scato, tracked in this issue : #963 which certainly looks interesting to me.

I am for this, but whatever mechanism we end up choosing I'd also like the ability to still get the defaults, if only to get the ball rolling while experimenting. Maybe through a mix-in or something.

@adelbertc
Copy link
Contributor

adelbertc commented May 2, 2016

One issue with the current approach is if you want to do MTL style programming, you end up having duplication. A short, contrived, example:

trait MonadMagic[F[_]] extends Monad[F] {
  def magic: F[Int]
}

Now I want to define MonadMagic[Option].. but that means I also need to re-implement the Monad flatMap and pure methods, which leads to duplication and makes me uncomfortable. I suppose it could be solved by putting the Monad[Option] definition in a trait and exposing it for people to mix-in.. but then we'd have to do it for every data type. Which maybe isn't a big deal? But perhaps a deficiency of the current approach.

I suppose you could also do something like

trait MonadMagic[F[_]] {
  def monad: Monad[F]
}

and provide an implicit conversion [F]MonadMagic[F] => Monad[F] but that departs from how we are currently encoding things.

I'm hoping a solution to this ticket might also solve this problem.

@yilinwei
Copy link
Contributor

I have a few observations I'd like to make/points I would like to clarify.

  1. There exist some combinators pureEval, replicateA on Applicative which essentially act as utility functions defined on top of the base combinators. I don't think there's much value in not having the methods defined on the trait and forcing anyone who extends from the trait to override those.

  2. I almost feel as though this should be some form of macro solution for the way cats is coded. Consider I have a class,

case class Foo[A](a: A) {
    def map[AA <: A, B](f: A => B): Foo[B] = ???
    def flatMap[AA <: A, B](f: A => Foo[B]): Foo[B] = ???
}

and an associated Monad typeclass. I feel that there should be an annotation like,

@delegate
trait FooMonad extends Monad[Foo]

which reifies any methods on Foo which I've overriden which fits the signature, such as in this case map and flatMap.

wdyt?

ceedubs added a commit to ceedubs/cats that referenced this issue Jul 14, 2016
This is a first-step for typelevel#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.
@kailuowang
Copy link
Contributor

at this point, do we still want to change type class encoding prior to 1.0.0 release? cc @ceedubs

@djspiewak
Copy link
Member

@kailuowang Strong 👎 from me on changing the encoding prior to 1.0. IMO, if we want to change the encoding, that should be a cats 2.0 thing at this point.

@edmundnoble
Copy link
Contributor

Yeah I'm gonna have to agree. Otherwise cats-mtl would be in cats.

@kailuowang
Copy link
Contributor

According to consensus, moving it out of 1.0.0

@kailuowang kailuowang removed this from the 1.0.0-MF milestone May 31, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants