Replace TransLift with MonadTrans #1621

wedens · 2017-04-22T05:48:14Z

Resolves #1613.

Pros:

MonadTrans is lawful (well... almost). There are identity (liftT . pure = pure) and composition (liftT (m >>= f) = liftT m >>= (liftT . f)) laws. I think for MonadTrans to be really lawful it should extend (in current encoding) Monad. But it will make it unusable for the same reason MonadError et al is (diamonds 💎 )
Machinery like TLExtract is not required
Can be used as a typeclass without dealing with varying typeclass (Unable to use TransLift as a typeclass #1613 is about it)

Cons:

Monad constraint is imposed on for any base functor. For some transformers (e.g. Kleisli) liftT can be implemented with weaker constraint

/cc @djspiewak @ceedubs

codecov-io · 2017-04-22T06:30:12Z

Codecov Report

Merging #1621 into master will decrease coverage by 0.03%.
The diff coverage is 90%.

@@            Coverage Diff             @@
##           master    #1621      +/-   ##
==========================================
- Coverage   93.37%   93.33%   -0.04%     
==========================================
  Files         240      241       +1     
  Lines        3937     3948      +11     
  Branches      138      135       -3     
==========================================
+ Hits         3676     3685       +9     
- Misses        261      263       +2

Impacted Files	Coverage Δ
core/src/main/scala/cats/data/EitherT.scala	`96.42% <ø> (ø)`	⬆️
core/src/main/scala/cats/syntax/monadTrans.scala	`0% <0%> (ø)`
core/src/main/scala/cats/data/OptionT.scala	`100% <100%> (ø)`	⬆️
free/src/main/scala/cats/free/FreeT.scala	`93.58% <100%> (ø)`	⬆️
core/src/main/scala/cats/data/WriterT.scala	`93.47% <100%> (ø)`	⬆️
...n/scala/cats/laws/discipline/MonadTransTests.scala	`100% <100%> (ø)`
laws/src/main/scala/cats/laws/MonadTransLaws.scala	`100% <100%> (ø)`
core/src/main/scala/cats/data/StateT.scala	`97.43% <100%> (ø)`	⬆️
core/src/main/scala/cats/data/Kleisli.scala	`92.64% <100%> (ø)`	⬆️
... and 4 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 b6fc4e8...d39d2f9. Read the comment docs.

djspiewak · 2017-04-22T18:23:31Z

This gets a 👍 from me. I think recent efforts really prove that we weren't gaining anything from the flexibility of TransLift, and if anything we were losing a lot due to the insane complexity of getting the types to work out. The laws point is also a kicker. It was a really neat experiment, but I think it's fair to say that it failed. I would rather have MonadTrans.

wedens · 2017-04-24T07:40:26Z

CI on 2.11.8 fails due to "The job exceeded the maximum time limit for jobs, and has been terminated.". Using coursier may chop off some build time.

kailuowang · 2017-04-24T13:38:04Z

core/src/main/scala/cats/MonadTrans.scala

+/**
+  * A type class which abstracts over the ability to lift an M[A] into a
+  * MonadTransformer
+  */


nit here: the scala doc style in cats is

/** * */

i.e. all stars aligned with the first star.

other than that it's a 👍 from me.

kailuowang · 2017-04-24T14:31:18Z

core/src/main/scala/cats/MonadTrans.scala

+trait MonadTrans[MT[_[_], _]] {
+  /**
+    * Lift a value of type M[A] into a monad transformer MT[M, A]
+    */


And this one. also maybe a blank line above?

haven't noticed it ;)

kailuowang

LGTM. Thanks very much!

ceedubs · 2017-04-24T14:53:47Z

Does anyone know of any data structures that require Monad to lift as opposed to just Applicative (or weaker)? I don't see any examples currently inside Cats, which makes me wonder if it should be ApplicativeTrans. However, I guess the downside with that is that you lose the composition law?

wedens · 2017-04-24T15:02:16Z

@ceedubs ContT, I think

djspiewak · 2017-04-24T15:24:59Z

Apparently Codensity does as well. @sellout and I had this conversation last week.

sellout · 2017-04-24T18:26:20Z

@wedens I don’t think ContT, but I’m far from certain.

@ceedubs Is there anything other than Codensity that requires more than Functor? I ask because FunctorK[F[_[_], _]] (or HFunctor, or whatever) is very similar to Scalaz’s Hoist and PointedK (or HPointed) is very similar to MonadTrans, except that the K (or H) variants only allow a Functor constraint (I think), so I thought that perhaps we could have

@typeclass trait PointedK[F[_[_], _]] extends MonadTrans[F] {
  def pointk[G[_]: Functor]: G ~> F[G, ?]

  def liftT[M[_]: Monad, A](ma: M[A]) = pointk[M].apply(ma)
}

(ok, maybe we have to deal with the natural transformation somehow, but you get the gist). Right now, Codensity is the only thing that I’m pretty sure can implement MonadTrans but not PointedK.

And the underlying need for FunctorK, etc. is that with mutually-recursive recursion schemes you end up with a lot of G[_]s that aren’t monads.

djspiewak · 2017-04-24T18:28:36Z

@sellout I'm pretty sure you can't define runCont without a Monad for the constituent effect of ContT (or at the very least, a FlatMap).

sellout · 2017-04-24T18:32:35Z

@djspiewak Maybe (I think – again, not sure – you only need Applicative), but that doesn’t mean you can’t liftT with a lesser constraint.

djspiewak · 2017-04-24T18:35:14Z

@sellout Oh, you're right. The implementation of runCont doesn't even need Applicative. ContT could be done without any constraints at all. I guess that makes sense, since it's in many ways a dual of Kleisli

sellout · 2017-04-24T18:49:13Z

I’m going to frame that comment.

kailuowang · 2017-04-27T18:49:53Z

core/src/main/scala/cats/syntax/monadTrans.scala

+}
+
+final class MonadTransOps[F[_], A](val fa: F[A]) extends AnyVal {
+  def liftT[MT[_[_], _]](implicit F: Monad[F], MT: MonadTrans[MT]): MT[F, A] =


How about a doctest here? It will provide coverage for both here and the syntax line above. Then you should get green light from codecov

There is a test in SyntaxTests.

I pointed out because codecov reported test miss here. Now I see that SyntaxTest is only a compilation check. I think I prefer a doctest in the type class which actually run the code (thus coverage) and provide a concrete example there. But we can certainly address that in a different PR.

hobwekiva · 2017-04-27T23:10:49Z

core/src/main/scala/cats/Trivial.scala

- */
-sealed trait Trivial
-
-object Trivial {


I understand that as a result of this PR Trivial is no longer needed in cats, but it is still a very useful type constraint. Is there a good reason to remove Trivial?

I thought about making the same objection, but my conclusion was that "less is more" in terms of API surface area. Shapeless already has a Trivial, and it is very likely to be on the classpath of anyone who needs this sort of thing. I don't think there's a need for cats to keep it.

We are also going to remove unapply to a separate module. Maybe we should copy trivial there too?

mpilquist · 2017-04-28T03:43:47Z

core/src/main/scala/cats/syntax/all.scala

@@ -39,7 +39,7 @@ trait AllSyntax
    with ShowSyntax
    with SplitSyntax
    with StrongSyntax
-    with TransLiftSyntax
+    with MonadTransSyntax


Keep in alphabetical order?

adelbertc · 2017-04-28T05:33:21Z

core/src/main/scala/cats/MonadTrans.scala

+  /**
+   * Lift a value of type M[A] into a monad transformer MT[M, A]
+   */
+  def liftT[M[_]: Monad, A](ma: M[A]): MT[M, A]


One thing Edward Kmett noted in a talk he gave on monad homomorphisms is that MonadTrans provides no constraint that the resulting MT[M, A] has a Monad instance. I believe in his talk he fixes this by adding a function taking the Monad[M] constraint and providing a Monad[MT[M, ?]] constraint. In Scala it might look something like:

implicit def mtMonad[M[_]: Monad]: Monad[MT[M, ?]]

Would we want something like that here?

scalaz does it. Not sure if it's useful. But it makes sense in theory.

This would communicate the law: For every lawful M[_]: Monad there exists a lawful MT[M, ?]: Monad

Can we check such a law? Would be cool if we could rule out the naïve ListT for instance.

A simple way would be to pick a fixed M, e.g. Option, get the Monad[MT[Option, ?]] from this method, and then law-check that.

A fancier way would be to use existentials or something to be able to vary M and do law-checking across all these :-)

EDIT Hmm I actually don't know what that would look like in terms of code though.

peterneyens · 2017-04-28T21:53:53Z

Should we merge this as is and add an eventual Monad[MT[M, ?]] law in a follow up PR?

kailuowang · 2017-05-02T14:28:23Z

Note: https://github.com/milessabin/kittens/pull/37 to move Trivial to kittens.

wedens · 2017-05-11T08:51:41Z

is there something holding this PR?

kailuowang · 2017-05-11T13:49:01Z

@wedens I just thumbed up @peterneyens's last comment. @adelbertc what do you think? If you have no issue with that, I think this one is good to merge.

edmundnoble

LGTM.

ceedubs · 2017-05-13T16:06:48Z

While it took a while, I'm sold on the fact that TransLift was a failed experiment.

One thing that I didn't realize at first and that I haven't seen come up in comments (other than the original PR description) is that here, ~~MonadError~~ MonadTrans doesn't extend Monad (citing diamonds as the issue). It sounds like there are some plans to provide mechanisms to attach laws to this, but I just wanted to call that out and make sure that people were aware and don't see it being an issue. cc @djspiewak @adelbertc @kailuowang @sellout

kailuowang · 2017-05-17T18:25:25Z

@ceedubs I just created the issue for adding such a law.
#1674
As for this PR, I think we are good to merge?

ceedubs · 2017-05-17T20:09:18Z

@tpolecat @edmundnoble @djspiewak are you still happy with this PR in response to #1621 (comment) ? I'm definitely not opposed to this PR being merged, but I respect others' opinion on this much more than my own, because I only rarely use MonadTrans.

peterneyens · 2017-05-17T21:11:42Z

I think we may need to change the Translift of ReaderWriterStateT to MonadTrans after #1598.

djspiewak · 2017-05-18T03:03:24Z

@ceedubs

One thing that I didn't realize at first and that I haven't seen come up in comments (other than the original PR description) is that here, MonadError doesn't extend Monad (citing diamonds as the issue).

Uh, am I missing something? MonadError isn't touched in this PR.

ceedubs · 2017-05-18T14:09:13Z

@djspiewak oops I had MonadError on the brain from the Eval discussion. I meant MonadTrans.

ceedubs · 2017-05-18T14:11:05Z

Wait that makes sense that MonadTrans wouldn't extend Monad. Ignore my previous comments.

ceedubs · 2017-05-18T14:11:30Z

Okay this has 3 signoffs. I'm going to go ahead and merge it.

ceedubs · 2017-05-18T14:12:35Z

Oops I had forgotten about #1621 (comment) . I can't take care of it at the moment. I should be able to do it this evening if nobody beats me to it.

wedens · 2017-05-18T14:30:50Z

@ceedubs I've fixed it #1680

edmundnoble · 2017-05-18T16:05:13Z

@wedens Just so you know, cats-mtl has its own formulation of MonadTrans which is not compatible with this one. So I will have to remove it when I remove the MTL type classes from cats.

TransLift replaced with MonadTrans

57d8819

wedens changed the title ~~Replaced TransLift with MonadTrans~~ Replace TransLift with MonadTrans Apr 22, 2017

remove Trivial

00a4fd1

kailuowang added Source Breaking Binary Breaking labels Apr 22, 2017

peterneyens mentioned this pull request Apr 22, 2017

Improve test coverage IdT #1623

Merged

MonadTrans syntax test

946f171

kailuowang reviewed Apr 24, 2017

View reviewed changes

fix doc style

35dc00c

wedens force-pushed the monad_trans branch from 2d1a5d0 to 35dc00c Compare April 24, 2017 14:36

kailuowang approved these changes Apr 24, 2017

View reviewed changes

djspiewak approved these changes Apr 27, 2017

View reviewed changes

tpolecat approved these changes Apr 27, 2017

View reviewed changes

kailuowang reviewed Apr 27, 2017

View reviewed changes

hobwekiva reviewed Apr 27, 2017

View reviewed changes

mpilquist reviewed Apr 28, 2017

View reviewed changes

ordered syntax traits

d39d2f9

adelbertc reviewed Apr 28, 2017

View reviewed changes

kailuowang mentioned this pull request Apr 28, 2017

Consider moving Unapply out of cats #1607

Closed

edmundnoble approved these changes May 12, 2017

View reviewed changes

ceedubs merged commit b4995b2 into typelevel:master May 18, 2017

wedens deleted the monad_trans branch May 18, 2017 14:25

ceedubs mentioned this pull request Mar 25, 2018

Follow up on def vs val in #940 #965

Closed

Replace TransLift with MonadTrans #1621

Replace TransLift with MonadTrans #1621

Conversation

wedens commented Apr 22, 2017 • edited Loading

codecov-io commented Apr 22, 2017 • edited Loading

Codecov Report

djspiewak commented Apr 22, 2017

wedens commented Apr 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kailuowang left a comment

Choose a reason for hiding this comment

ceedubs commented Apr 24, 2017

wedens commented Apr 24, 2017

djspiewak commented Apr 24, 2017

sellout commented Apr 24, 2017

djspiewak commented Apr 24, 2017 • edited Loading

sellout commented Apr 24, 2017

djspiewak commented Apr 24, 2017 • edited Loading

sellout commented Apr 24, 2017

kailuowang Apr 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adelbertc Apr 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adelbertc Apr 28, 2017 • edited Loading

Choose a reason for hiding this comment

peterneyens commented Apr 28, 2017

kailuowang commented May 2, 2017

wedens commented May 11, 2017

kailuowang commented May 11, 2017

edmundnoble left a comment

Choose a reason for hiding this comment

ceedubs commented May 13, 2017 • edited Loading

kailuowang commented May 17, 2017

ceedubs commented May 17, 2017

peterneyens commented May 17, 2017

djspiewak commented May 18, 2017

ceedubs commented May 18, 2017

ceedubs commented May 18, 2017

ceedubs commented May 18, 2017

ceedubs commented May 18, 2017

wedens commented May 18, 2017

edmundnoble commented May 18, 2017

wedens commented Apr 22, 2017 •

edited

Loading

codecov-io commented Apr 22, 2017 •

edited

Loading

djspiewak commented Apr 24, 2017 •

edited

Loading

djspiewak commented Apr 24, 2017 •

edited

Loading

kailuowang Apr 27, 2017 •

edited

Loading

adelbertc Apr 28, 2017 •

edited

Loading

adelbertc Apr 28, 2017 •

edited

Loading

ceedubs commented May 13, 2017 •

edited

Loading