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

[Triage] Break out MTL to a separate library #1603

Closed
kailuowang opened this issue Apr 12, 2017 · 30 comments
Closed

[Triage] Break out MTL to a separate library #1603

kailuowang opened this issue Apr 12, 2017 · 30 comments

Comments

@kailuowang
Copy link
Contributor

kailuowang commented Apr 12, 2017

The idea originated from the gitter discussion here
So here is the idea:
The existing MTL is broken (see #1210), instead of letting this problem (and the solution of it) leak out to other cats typeclasses, we move them to a different library (mainly to support "entirely-separate law testing").
We release cats 1.0.0 after this new library is ready to use in a way by and large compatible with the existing MTL usage, i.e. users will have an easy migration path - simply add one more dependency.

Also from @edmundnoble (please feel free to edit/replace what you want to present here)

... this library will have to depend on cats. And... no need for Scato.
... the MTL issue I had with modifying cats disappears when all you do with it is MTL and not rewrite cats.
So the law testing works very differently from cats as is.

@edmundnoble and @adelbertc can you guys list other alternatives (e.g. adopting Scato)?

@typelevel/cats please provide your vote and/or feedback on this solution.

Update:

A summary of the feedbacks expressed so far:

  1. everyone agrees with moving out MonadReader, MonadWriter, MonadState.
  2. For FunctorFilter, TraverseFilter and MonadFilter, it seems everyone is okay for them to go too.
  3. MonadCombine it's mostly either neutral or keeping it in cats, i.e. no strong preference of moving them out
  4. For MonadError we still have some disagreement.

Now that we've done quite some debate, how about we resort to democracy for a resolution?

@kailuowang kailuowang added this to the 1.0.0-MF milestone Apr 12, 2017
@sellout
Copy link
Contributor

sellout commented Apr 12, 2017

So, I know I suggested this (and if it happens, I vote for naming it “Transmogrifier”), but I do have some reservations.

One, as @mpilquist raised, is “where do we draw the line?” I don’t want to end up in a situation where figuring out what comes from here vs Cats is non-obvious.

Another is that I worry about making transformers available without these type classes. Specific transformer stacks already get hardcoded too often. If we make it harder to get the type classes that abstract over them, it’ll happen even more.

On the plus side, being able to change encoding, etc. independently of breaking changes to Cats seems useful. I am pro-modularity in general.

@adelbertc
Copy link
Contributor

My current view on this is "MTL" type classes (this naming is unfortunate, they're really just type classes and shouldn't be privileged in any way but it gives a name to the problem we're referring to sooo...) is still in the "design process" for Scala. We have the usual subtype encoding which doesn't work, we have Scato, and we also have @edmundnoble 's capabilities-based design. In that sense perhaps it should be split out for the same reason https://github.com/effects4s is its own thing.

On the flip side "where do we draw the line" is a very good point. On principle "MTL" ~= "any type class that shares a superclass with any other type class" But people seem to be doing fine with the current encoding up until they hit Monad* classes. Related though Cats also has stuff like FunctorFilter and TraverseFilter which make me uncomfortable.

🤷‍♂️

@edmundnoble
Copy link
Contributor

edmundnoble commented Apr 13, 2017

@adelbertc How about this:

trait MTL[Type[_], Capability[_]] {
  def injectCapability[A](capability: Capability[A]): Type[A]
  def useCapability[A, B](type: Type[Capability[A]]): Type[A]
}

That should at least tell us how to determine what an MTL type class is.
WithError is MTL[F, Either[E, ?]], WithFilter is MTL[F, Option], and for extra points MonadCombine (edited from MonoidK) is MTL[F, Prod[F, F, ?]] (likely a conversion option, not the actual implementation which should stay in cats).

I am of the opinion that modular (WithError, WithFilter, etc.) design is the best option here, considering that even duplicating classes for Applicative is not only a waste of code but practically I think it encourages users to write code that's fixed to Monad.

Also I am of the opinion that if we are to provide laws for these, we should use Scato because otherwise users are locked out of defining new type classes with new laws for an already extant set of operations.

@adelbertc
Copy link
Contributor

Initial thoughts since it's getting late here, but seems like an interesting approach. It's different enough however that if we do go down this route I'd like to see it outside of Cats, which would also open the door to using an alternative encoding.

Now the hard part: what classes in Cats would be removed in light of this?

@edmundnoble
Copy link
Contributor

IMO we have:

  1. Alternative does not work properly in its current state; but if it did, it wouldn't be "MTL" because it isn't "just" the intersection of Applicative and MonoidK; it requires the some and many operators be overloaded for any instance.
  2. MonadCombine, MonadFilter, MonadError, MonadReader, MonadWriter, MonadState are definitely MTL and should be removed for this.
  3. Some classes do not exist, but IMO should in transmogrifier: MonadAsk (MonadReader - local), MonadTell (MonadWriter - pass)

Not sure if I missed any.

@adelbertc
Copy link
Contributor

adelbertc commented Apr 13, 2017

What of FunctorFilter, TraverseFilter, InvariantMonoidal, ContravariantCartesian,

@edmundnoble
Copy link
Contributor

InvariantMonoidal stays, the other two would be subsumed by WithFilter.

@mpilquist
Copy link
Member

👎 to dropping MonadCombine and ApplicativeError/MonadError from cats 1.0. 👍 to dropping MonadReader, MonadWriter, and MonadState. I'm neutral on MonadFilter.

@edmundnoble
Copy link
Contributor

edmundnoble commented Apr 13, 2017

@mpilquist Can you please provide your reasons?

Edit: you mention 1.0. Are you saying these should wait for a later version to be removed or that you have reservations about removing them?

@mpilquist
Copy link
Member

Utility wise, separate and unite are used frequently enough that I think they should be in the main library. Anything put in to a separate library will lose most of the potential user-base. MonadError[Either[Throwable, ?]] is needed for effect capture libraries (analogous to Catchable from fs2, doobie, and scalaz-effect). If MonadError didn't exist in cats-core, then we'd need to create Catchable in cats-effect / schrodinger / whatever.

@edmundnoble
Copy link
Contributor

Very valid concerns. Two points:

a) MonadCombine more provides separate and unite than it defines them. They have no laws, and can be provided regardless via ops without a new type class. Perhaps this is a good reason to not add MonadCombine into the MTL.
b) effects4s requires Catchable regardless (see the effects4s README at https://github.com/effects4s/effects4s), because it's defined in terms of throwing exceptions and lazy evaluation; which is not captured in the laws or methods of MonadError.

@wedens
Copy link
Contributor

wedens commented Apr 14, 2017

IMO MonadError, MonadReader, MonadWriter, MonadState, MonadTell should be in a separate library. I have no strong opinions on other typeclasses like *Filter or MonadCombine.

@djspiewak
Copy link
Member

@edmundnoble

because it's defined in terms of throwing exceptions and lazy evaluation

Only in the effects4s formulation. That is not fundamental. There is a lawful definition of Catchable which is entirely isomorphic to MonadError[?[_], Throwable].

@edmundnoble
Copy link
Contributor

@djspiewak I am fairly sure we are talking about effects4s here. More importantly, MonadError says nothing about a relationship with Evaluable.eval, which is exactly what distinguishes Catchable from MonadError[?[_], Throwable] (and forms the relationship between Catchable and catch). I would rather not have a bunch of users using MonadError and assuming it will catch thrown exceptions, then switch to another MonadError without that behavior and get runtime exceptions.

@djspiewak
Copy link
Member

djspiewak commented Apr 14, 2017

I am fairly sure we are talking about effects4s here.

Are we? If we are, then Catchable says nothing about the relationship between fail and flatMap, an equally-important law.

More importantly, MonadError says nothing about a relationship with Evaluable.eval, which is exactly what distinguishes Catchable from MonadError[?[_], Throwable]

It really isn't (the distinguisher). eval is very clearly a fishy construct, by virtue of being an FFI to capture side-effects. The fact that it has a particularly special relationship with errors shouldn't be surprising.

My point is that Catchable doesn't need to be a separate thing just because eval is weird. eval already needs special laws; there's no reason to say that Catchable needs to be distinct from MonadError any more than there is a reason to say that Evaluable gets its own notion of a Monad.

Edit: What you're saying (regarding Evaluable and Catchable) is analogous to claiming that Functor is invalid because it doesn't say anything about its relationship to pure. Laws for more specialized typeclasses (like Evaluable) are relevant to those specialized typeclasses, even when the laws reference the more general abstractions above. So just like ApplicativeLaws says something about map, so too does EvaluableLaws say something about handleError.

@edmundnoble
Copy link
Contributor

edmundnoble commented Apr 14, 2017

@djspiewak
a)
"MonadError[Either[Throwable, ?]] is needed for effect capture libraries (analogous to Catchable from fs2, doobie, and scalaz-effect). If MonadError didn't exist in cats-core, then we'd need to create Catchable in cats-effect / schrodinger / whatever."

This is what I'm arguing against. You seem to be arguing about whether or not MonadError is a valid formulation of Catchable; I think this is entirely irrelevant to whether or not Catchable needs to exist in effects4s. And if Evaluable does, then Catchable needs to say something about it.

b)
"Nothing about the relationship between fail and flatMap, an equally-important law"
The law relating fail and flatMap seems to be a given because parametricity guarantees there is no A value in a failed expression; i.e. fail(ex).map(f) <-> fail(ex), and fail(ex).join <-> fail(ex). Thus because flatMap is map(f).join:

fail[X](ex) <-> fail[IO[Y]](ex) // parametricity
fail[X](ex).map(f: X => IO[Y]) <-> fail[IO[Y]](ex)  // map is equal, because there's no value
fail[X](ex).map(f: X => IO[Y]).join <-> fail[IO[Y]].join // add join to both sides
fail(ex).flatMap(f) <-> fail(ex) // join is equal, because there's no inner value to flatten effects from

c)
"eval is very clearly a fishy construct"

Here's my point: Evaluable is necessary for all library writers who do not wish to lock down an IO type. And if you're using Evaluable and capturing side effects, you need to know how they interact with exceptions, and so you need this formulation of Catchable.

If you are not using an effect capture library, you do not care about side effects, and so you're doing MTL; then MonadError is fine. Now if your argument is "keep MonadError in cats because it's too convenient to put anywhere else", I still cannot agree. You cannot keep everything useful out of transmogrifier, or it will... not be useful.

Edit: If this makes my position any clearer: I do not think that everyone should stop using MonadError and swap it out for Catchable. I apologize if I gave that impression.

@djspiewak
Copy link
Member

djspiewak commented Apr 14, 2017

@edmundnoble

This is what I'm arguing against. You seem to be arguing about whether or not MonadError is a valid formulation of Catchable; I think this is entirely irrelevant to whether or not Catchable needs to exist in effects4s. And if Evaluable does, then Catchable needs to say something about it.

And yet, oddly, Applicative exists and Functor doesn't say anything about it. This is precisely an analogous situation to what we're talking about with Catchable/MonadError and Evaluable, but you seem to have come to the opposite conclusion about those two?

MonadError is needed for effect libraries unless we want to have a proliferation of precisely isomorphic typeclasses with identical interfaces and laws. I see no reason that this sort of proliferation would be desirable in any way.

Here's my point: Evaluable is necessary for all library writers who do not wish to lock down an IO type. And if you're using Evaluable and capturing side effects, you need to know how they interact with exceptions, and so you need this formulation of Catchable.

No, you need a formulation of Catchable. type Catchable[F[_]] = MonadError[F, Throwable] is a perfectly reasonable formulation of Catchable, and all of the laws can still be defined in exactly the way that you would define them relative to Catchable. Having a separate nominal type buys you nothing. The laws for how exceptions are captured are defined with the laws about how effects are captured, because… ✨ exceptions are effects! ✨ There's no need to have a separate nominal type which defines its own laws in terms of some derived typeclasses. Again, coming back to Functor and Applicative. The Functor laws say nothing about pure, because Functor doesn't have pure; but the Applicative laws most definitely say something about map. Similarly, the Evaluable laws say something about attempt, but the MonadError laws don't need to say anything about eval.

If you are not using an effect capture library, you do not care about side effects, and so you're doing MTL; then MonadError is fine.

Of course this is true. But the converse is not. MonadError is just fine. Period. There's no extra qualifications there.

You cannot keep everything useful out of transmogrifier, or it will... not be useful.

And this is the crux of the matter: I don't think transmogrifier is useful anyway. MTL is broken in cats. Fine. MTL has been broken in scalaz for years and people lived with it. MTL could be a lot better, and that would be awesome. Trying to move everything MTL-ish out of cats removes things from cats that are useful outside the landscape of MTL, and trying to have this argument now while we're (ostensibly) attempting to nail down cats 1.0 is just really strange timing.

Edit: Maybe I'm just being too negative. But I think the problem with this whole thing is trying to draw some arbitrary line between typeclasses which are only useful for MTL and typeclasses which are more broadly applicable. Everyone is going to have a slightly different answer to that question.

@edmundnoble
Copy link
Contributor

"MonadError is needed for effect libraries unless we want to have a proliferation of precisely isomorphic typeclasses with identical interfaces and laws. I see no reason that this sort of proliferation would be desirable in any way."
You are simply mistaken here, and this seems to be the root of the confusion. There are more laws added with Catchable than MonadError. That's what my entire point here has been; there are laws dictating its interaction with Evaluable, which is a superclass of Catchable. MonadError does not cut it in this case. Catchable[F] cannot be MonadError[F, Throwable] because there is no Evaluable superclass nor laws.

Not that anything is wrong with trait Catchable[F] extends Evaluable[F] with MonadError[F, Throwable].

To address your point on the subject of this thread (MTL being removed from cats in 1.0.0):
"Trying to move everything MTL-ish out of cats removes things from cats that are useful outside the landscape of MTL"
Can you please be more specific? Using MonadError does amount to MTL. Using MonadCombine amounts to using some lawless ops on other type classes. You saw the list above, of exactly which type classes we are talking about when we say MTL: what there can we not remove without damaging the utility of cats outside of MTL?

For the record, I do not blame you for being negative; I haven't talked to a single person other than @sellout and @wedens who thinks this is worth doing. Nearly everyone else is of the opinion that "cats just needs to be 'as-good' as Scalaz". Personally, I think that's absolute shit. We are not Scalaz, we never have been and never will be. The reason this library even exists is (ostensibly) to make experiments beyond Scalaz, and try to find the best way FP features can be encoded in Scala. More importantly, by resigning ourselves to this "oh managers can only approve x libraries per month", "oh adding a dependency is hard" negativity and keeping more and more in cats we are giving up on having modular FP libraries, which in the long term is crucial to the prospects of FP in Scala.

@djspiewak
Copy link
Member

which is a superclass of Catchable

Oh… I didn't realize @alexandru had made Evaluable a supertype of Catchable, rather than a subtype. That's… Well, I don't agree with that, but it's a discussion for the effects4s issue list. Clearly if Evaluable is a supertype of Catchable in Alex's formulation, then you are quite correct that MonadError does not in any way suffice. Sorry for my confusion on that point.

I prefer to formulate things in the opposite direction, with effect suspension specializing Catchable, rather than vice versa, so that we don't need to deal with the awkward question of what happens when a throw is suspended in a non-Catchable effect.

Can you please be more specific? Using MonadError does amount to MTL. Using MonadCombine amounts to using some lawless ops on other type classes. You saw the list above, of exactly which type classes we are talking about when we say MTL: what there can we not remove without damaging the utility of cats outside of MTL?

There's basically two different ways you can think of MonadError: either as characterizing a set of subclasses (like scalaz.Catchable), or as carrying a set of MTL operations. It comes to the same thing, so it's sort of a subjective design question. My concern is for a hypothetical "cats-effect" library (or libraries like it), which don't particularly care about MTL and probably wouldn't want to depend on transmogrifier, but which need MonadError specifically. An alternative option would be for such a library to depend on effects4s and use its Catchable, but that only works if the effects4s hierarchy is both stable and agreeable.

I'm fine with moving out MonadReader, MonadWriter, etc. I'm neutral on MonadCombine.

Nearly everyone else is of the opinion that "cats just needs to be 'as-good' as Scalaz". Personally, I think that's absolute shit. We are not Scalaz, we never have been and never will be.

Agreed. I think it's important that the broad cats ecosystem supports the functionality that people have come to expect from the scalaz ecosystem as a whole, but the shape taken by that functionality and whether that functionality is in cats-core or in something like transmogrifier is not something that should be set in stone. Scalaz is exceptionally monolithic; a design decision which I believe was a mistake.

More importantly, by resigning ourselves to this "oh managers can only approve x libraries per month", "oh adding a dependency is hard" negativity and keeping more and more in cats we are giving up on having modular FP libraries, which in the long term is crucial to the prospects of FP in Scala.

It's not so much about "we can only approve x libraries per month" and more about "the JVM has no solution to the version diamond problem". The more dependencies you have (transitively), the higher the odds that someone has been lazy and not updated to the latest incompatible (but not in a way which affects them) version of a particular artifact, resulting in a dependency clash which cannot be resolved downstream without risking runtime link errors. Jigsaw had a chance to address this problem, and they… didn't. Despite people begging them to. :-/

Now the flip side of this is that if you shove everything into a single monolith, your surface area for incompatibility becomes quite vast, meaning that minor and mostly-irrelevant changes require versioning as a breaking change, despite the majority of the surface area being fully compatible. This is precisely why the ecosystem of a versions exist in the scalaz universe.

So in summary, versioning is terrible. Blow it all up. Become a hermit. Like Rob.

There's no right answer here. But when in doubt, I agree that we should err on the side of modularity. Even though we don't have real modules, it at least gives us more flexibility in the long run.

@wedens
Copy link
Contributor

wedens commented Apr 15, 2017

If MonadError is not moving out to the mtl library, it will need to be duplicated in the library to be useful. Probably it's not the worst solution in current situation, but it may lead to confusion and name collision (cats.MonadError/xxx.MonadError)

@djspiewak
Copy link
Member

I'm going to ask a stupid question. Please tell me if I'm being absolutely crazy here. Why does the MTL encoding have to use subtyping? I don't actually remember why exactly this is. The main typeclass hierarchy uses subtyping because implication produces ambiguity in the presence of diamonds, but MTL doesn't have that problem (since it does have diamonds).

The quasar codebase has a number of "underscore variants" of the major MTL typeclasses. MonadListen_, MonadTell_, etc. They're literally the same as their non-underscore variants, except they don't extend Monad, and this seems to work out very well. Is there a reason we don't use this encoding?

Using this encoding would provide a very natural separation between "MTL MonadError" and "cats MonadError", since the latter would extend Monad while the former would not.

@wedens
Copy link
Contributor

wedens commented Apr 15, 2017

@djspiewak I think it was already discussed here.

I do the same thing as quasar. Duplicate mtl typeclasses without subtyping. Works for me just fine.

@djspiewak
Copy link
Member

@wedens That discussion seems to hang up on "but I want to get a Monad out of my MonadReader". If we give up that constraint, I'm not sure I see what the problem is.

@edmundnoble
Copy link
Contributor

edmundnoble commented Apr 15, 2017

@djspiewak The MTL type class encoding I have been advocating is Scato, with the Monad constraints removed. E.g., WithError, WithFilter. IMO the biggest question there is aliases: we need a way to say something like type MyCapabilities[F[_]] = WithError[MyError, ?] :+: WithFilter[F] :+: CapabilityNil so that it's actually usable.

This discussion seems to be edging toward design choices for transmogrifier, rather than cats; perhaps I should make an empty repo where these discussions would be more able to thrive?

Edit: This is my inspiration, for the moment: https://ro-che.info/articles/extensible-effects

@edmundnoble
Copy link
Contributor

@kailuowang The evils of democracy ;) I personally am in favor of taking all of the aforementioned type classes out of cats, with MonadCombine's operations moved to MonoidK or Alternative. But I do not see an issue with keeping in MonadError seeing as it's so ubiquitous; it'll just have a WithError counterpart.

@sellout
Copy link
Contributor

sellout commented Apr 17, 2017

I discount my own opinion here, because I am going to use these type classes no matter what I have to do. They could each be in a separately-versioned dependency, and I’d add them all.

So, my own inclination is to have MonadError in Transmogrifier. I am neutral on MonadCombine, but lean toward it being in Cats. I can’t put my finger on why, but I don’t really see it as an MTL class. I do think any operations that can be moved up to MonoidK and Alternative should be, but I think that’s orthogonal to this issue.

@edmundnoble
Copy link
Contributor

@sellout Having MonadError in cats does not mean not having it in transmogrifier. MonadCombine doesn't have any operations that can't be moved up to MonoidK and Alternative, and MonadCombine has no laws.

@kailuowang
Copy link
Contributor Author

taking all of the aforementioned type classes out of cats, with MonadCombine's operations moved to MonoidK or Alternative. But I do not see an issue with keeping in MonadError seeing as it's so ubiquitous; it'll just have a WithError counterpart (in Transmogrifier)

@edmundnoble I think this plan has a good chance to be acceptable for most people (also a 👍 from me). Do you want to create a new issue specifying this plan so that we can have a simple thumbs up /down vote on it?

@edmundnoble
Copy link
Contributor

@kailuowang Done.

@kailuowang
Copy link
Contributor Author

Closing this thanks to decision made at #1616

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

7 participants