WIP: Support for fs2.Task dropping own Catchable #323

guersam · 2016-08-12T07:25:48Z

An attempt to fix #311.

Although I didn't mean to touch all these stuff at first, eventually I ended up with dropping doobie.util.catchable.Catchable in favor of fs.util.Catchable fighting a lot of ambiguous implicit errors. But probably there would be a much simpler way...

Changes made (which can be reverted) or things to discuss:

Replace most cats compat utils with fs2-cats
Replace cats.data.Xor with scala.util.Either to support fs2.util.Catchable (related discussion: data.Xor and instances/either.scala: Either is now right-biased cats#1192)
Use fs2.util.Suspendable instead of Capture[fs2.Task]
~~Or revive doobie.util.catchable.Catchable and directly support only for fs2.Task?~~
~~Drop IOLite?~~
Move fs2.util.Suspendable instance for cats.data.Kleisli into fs2-cats
Decide which typeclasses (especially Monad) to support, of cats, or of fs2?

TODO:

Align yax directives in a consistent way
Update docs

guersam · 2016-08-12T07:28:44Z

project/FreeGen.scala

    |  implicit val Catchable${sname}IO: Catchable[${sname}IO] =
    |    new Catchable[${sname}IO] {
    |      def attempt[A](f: ${sname}IO[A]): ${sname}IO[Throwable \\/ A] = ${sname.toLowerCase}.attempt(f)
    |      def fail[A](err: Throwable): ${sname}IO[A] = ${sname.toLowerCase}.delay(throw err)
    |    }
+    |#-scalaz
+    |#+cats
+    |#+fs2


I think cats directive is unnecessary here. Right?

guersam · 2016-08-12T09:04:34Z

The second commit is slightly out of scope. I just found unnecessary diffs caused by varying method order with environments annoying.

tpolecat · 2016-08-13T03:16:15Z

Yeah the ever-changing codegen is irritating. I will look at this more closely this weekend. It seems like a big change but it certainly makes sense to get rid of our own Catchable if one is provided by fs2.

guersam · 2016-08-15T14:52:20Z

I have been struggling to combine f.u.Catchable and d.u.Capture into f.u.Effect, but got no clue to implement def unsafeRunAsync[A](fa: F[A])(cb: Attempt[A] => Unit): Unit for Free algebras and Kleisli[M[_]: Effect, E, A]. Is it possible at all?

mpilquist · 2016-08-15T14:54:47Z

FWIW, I think we could introduce the following to FS2:

  trait Suspendable[F[_]] extends Monad[F] {
    def suspend[A](fa: => F[A]): F[A]
    def delay[A](a: => A): F[A]
  }

  trait Effect[F] extends Catchable[F] with Suspendable[F] {
    def unsafeRunAsync[A](fa: F[A])(cb: Attempt[A] => Unit): Unit
  }

guersam · 2016-08-15T14:58:39Z

@mpilquist That's perfect, I managed to write a modified version having every unsafeRunAsync to be ???, which somehow passes all tests. Suspendable must work with it out of the box.

tpolecat · 2016-08-15T16:36:22Z

@guersam thanks for your continued hacking on this. I didn't have any time over the weekend to get into it but I will catch up soon.

guersam · 2016-08-15T16:41:06Z

@tpolecat No problem, and sorry about some inessential diffs which will make the review harder. As noted above I didn't expect the changes to be this huge 😨

This PR moves `suspend` and `delay` from the `Effect` type class to a new `Suspendable` type class. There are two motivations for doing this: - Exposing `unsafeRunAsync` to any function that requires `suspend` or `delay` gives up a lot of parametricity. - Some type constructors have `Suspendable` instances but do not support value extraction. This came up in the [port of Doobie to FS2](typelevel/doobie#323 (comment)). This [came up on Gitter a while back as well](http://www.gitterforum.com/discussion/scalaz-scalaz-stream?page=143).

tpolecat · 2016-08-20T02:55:16Z

Ok, so here are a few thoughts.

Drop IOLite?

This is really irritating because scalaz Task has .unsafePerformSync and fs2 has .unsafeRunSync which means every exec in the book (and there are a lot) needs a yax directive.

Move fs2.util.Suspendable instance for cats.data.Kleisli into fs2-cats

👍

Decide which typeclasses (especially Monad) to support, of cats, or of fs2?

My instinct is to support only cats typeclasses, with the exception of fs2's Suspendable and Catchable which have no equivalent in cats (yet ... they're talking about it).

So I'm wondering if this is too much to do for 0.3.1 … I was kind of thinking of just providing instances for fs2 Task and calling it good for now.

guersam · 2016-08-20T09:16:33Z

Sounds reasonable. A little thought about cats/fs2 typeclass support:

From usability perspective, when dealing with ConnectionIO direct support for cats typeclasses would usually be the better bet.
Regarding maintainability, fs2 typeclasses will give us more consistency if we eventually will replace scalaz-stream with fs2 (compile for scalaz+fs2? #310).
I'm not sure about performance, because that whichever we choose to support for, it involves typeclass conversion anyway. I guess (and hope) the effect will be negligible though.

Breaking the changes smaller for 0.3.1 will also be fine. Although there's no official release for cats/fs2 yet, there's good reason for keep migration steps small enough. In my case, there are actually no big differences since I'm not using the APIs lower than ConnectionIO directly.

guersam · 2016-08-20T09:21:39Z

BTW, is there a reason for the lack of automatic Free generation for postgres classes and jdbc drivermanager? I found it a bit tiring modifying all of them by hand, even for a small change.

tpolecat · 2016-08-21T03:47:46Z

is there a reason for the lack of automatic Free generation for postgres classes and jdbc drivermanager?

I think there was a reason but I don't remember. Opened #329 to follow up.

So for 0.3.1 maybe do the following and push off generalizing support for fs2 typeclasses

Update to latest cats/fs2 (new cats out later tonight)
Add doobie Catchable and Capture instances for fs2 Task

I really doubt we will get any actual cats+fs2 users since both libraries are in so much flux at the moment, so I think we have time to generalize later. I'd just like to get a release out that people can play with.

What do you think?

guersam · 2016-08-21T12:59:45Z

Agreed. I'm willing to port to my scalaz-stream codebase to cats/fs2 to get rid of mixed imports from scalaz and cats. I'll handle #329 first and then add missing instances for fs2.Task.

guersam · 2016-08-22T04:20:43Z

After second trial, I still find it harder than expected to add a few fs2.Task instances, because that fs2.util.Catchable is already a monad so that it always ends up with dropping own doobie.util.Catchable and touch here and there to avoid ambiguous/diverging implicit caused by the compatibility utils.

In this nature, it's getting more and more natural to me to fully support fs2.util.Suspendable from the initial cats/fs2 release, but if it feels like a too big change for 0.3.1 I'm okay to wait for 0.4.0-M1.

tpolecat · 2016-08-22T04:22:28Z

Ok, I will take a look tomorrow and see if I run into the same issue.

- Sort ctors using the arguments as well as the name - Convert `java.lang.Object` to `scala.AnyRef`

guersam · 2016-08-23T05:43:09Z

Latest build failures seem like the cache problem anticipated in #318. @tpolecat Could you flush the cache for this PR once?

tpolecat · 2016-08-23T16:30:39Z

I don't know how to flush the cache but I restarted that build.

guersam · 2016-08-24T00:01:43Z

Cache settings will appear in the travis repo page: https://docs.travis-ci.com/user/caching/#Clearing-Caches

Tried to do this by myself but I don't have the permission. Wish that travis allows PR openers to clear their PR specific cache.

tpolecat · 2016-08-24T00:03:57Z

Ah ok, thanks. Cleared and restarted the build.

tpolecat · 2016-08-24T00:14:46Z

Looks like it worked.

guersam · 2016-08-24T00:24:46Z

Thanks :)

On Wed, Aug 24, 2016, 9:14 AM Rob Norris notifications@github.com wrote:

Looks like it worked.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#323 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AA7JoECgWif7cLoY3eBI9SMaMvUJ722uks5qi4z3gaJpZM4Ji1m1
.

tpolecat · 2016-09-05T17:05:26Z

project/FreeGen.scala

@@ -152,17 +154,17 @@ class FreeGen(managed: List[Class[_]], log: Logger) {
  // Ctor values for all methods in of A plus superclasses, interfaces, etc.
  def ctors[A](implicit ev: ClassTag[A]): List[Ctor] =
    methods(ev.runtimeClass).groupBy(_.getName).toList.flatMap { case (n, ms) =>
-      ms.toList.zipWithIndex.map {
+      ms.toList.sortBy(_.getGenericParameterTypes.map(toScalaType).mkString(",")).zipWithIndex.map {


Thanks for this change, I should have done this a long time ago. Stuff was getting shuffled around every time I ran freegen.

tpolecat · 2016-09-05T17:14:40Z

Ok, I think this looks great. Huge amount of work. I'm fine with foldMapUnsafe for now; we can add the other constraint later if we want to.

Is there anything else that needs to be done in this PR before merging? I'd prefer to get this in and then follow up as needed rather than keep this open longer. What do you think?

adelbertc · 2016-09-05T18:36:51Z

I would hold off on adding the RecursiveTailRecM stuff, I'm becoming convinced that maybe we should just have it on Monad proper. I don't really like the marker trait and reading about the experience here made me reopen this typelevel/cats#1278

tpolecat · 2016-09-05T18:40:58Z

@adelbertc thanks, that was my instinct too. Given the volatility we should do the simplest thing that works.

guersam · 2016-09-05T23:40:40Z

Although some docs (especially the book) should be updated to catch up these changes, I'd like merging this for now too.

tpolecat · 2016-09-06T00:03:54Z

Alright let's do it.

macalinao · 2017-03-11T21:43:46Z

How does this work with monix.Task? The docs mention this briefly but I get the following error:

[error] could not find implicit value for evidence parameter of type fs2.util.Catchable[monix.eval.Task]
[error]     DriverManagerTransactor[Task](

adelbertc · 2017-03-11T23:02:36Z

@macalinao You will need to define an instance of Catchable[monix.eval.Task]. Catchable looks like:

trait Catchable[F[_]] extends Monad[F] {
  /** Lifts a pure exception in to the error mode of the `F` effect. */
  def fail[A](err: Throwable): F[A]
  /** Provides access to the error in `fa`, if present, by wrapping it in a `Left`. */
  def attempt[A](fa: F[A]): F[Attempt[A]]
}

tpolecat · 2017-03-12T02:38:54Z

You'll need instancs of fs2.util.Catchable and fs2.util.Suspendable (or scalaz.Catchable and doobie.util.Capture if you're using the scalaz version of doobie).

macalinao · 2017-03-12T20:55:32Z

I see, thanks. For future readers, here's my typeclass:

  implicit object taskCatchable extends Catchable[Task] with Suspendable[Task] {
    def pure[A](a: A): Task[A] = Task.pure(a)

    def flatMap[A, B](a: Task[A])(f: A => Task[B]): Task[B] =
      a.flatMap(f)

    def fail[A](err: Throwable): Task[A] =
      Task.raiseError(err)

    def attempt[A](fa: Task[A]): Task[Attempt[A]] =
      fa.materialize.map(_.toEither)

    def suspend[A](fa: => Task[A]): Task[A] =
      Task.suspend(fa)
  }

guersam reviewed Aug 12, 2016
View reviewed changes

guersam force-pushed the tmp/311-fs2-task branch from c31ad0b to 531df0e Compare August 12, 2016 08:36

guersam changed the title ~~WIP: Support fs2.util.{Catchable, Task} dropping own Catchable~~ WIP: Support for fs2.Task dropping own Catchable Aug 12, 2016

mpilquist mentioned this pull request Aug 16, 2016

Introduced Suspendable type class typelevel/fs2#710

Merged

guersam added 6 commits August 22, 2016 13:23

Support fs2.util.{Catchable, Task} dropping own Catchable

849142f

Make FreeGen emits ctors in more consistent order

f517b8f

- Sort ctors using the arguments as well as the name - Convert `java.lang.Object` to `scala.AnyRef`

WIP: Effect everywhere

4ffb5b9

Remove unnecessary Monad import in free

db4cb4e

Replace cats constraints with fs2-cats conversions

91d9f93

Some cleanup

85b0a27

guersam force-pushed the tmp/311-fs2-task branch from a684516 to 85b0a27 Compare August 22, 2016 04:24

guersam mentioned this pull request Aug 23, 2016

Add Suspendable and Catchable instances for Kleisli functional-streams-for-scala/fs2-cats#10

Merged

guersam added 2 commits August 23, 2016 13:33

Update to fs2 0.9.0-RC2 and cats 0.7.0

19e77c7

Rename generated low-priority typeclass holders

f030e43

Drop unnecessary fs2 Catchable constraints

fb8d5ad

guersam mentioned this pull request Sep 2, 2016

WIP: cats 0.7 #333

Closed

Update to cats 0.7.2

fc6b016

tpolecat reviewed Sep 5, 2016
View reviewed changes

tpolecat merged commit f7bcbce into typelevel:series/0.3.x Sep 6, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Support for fs2.Task dropping own Catchable #323

WIP: Support for fs2.Task dropping own Catchable #323

guersam commented Aug 12, 2016 •

edited

Loading

guersam Aug 12, 2016

guersam commented Aug 12, 2016

tpolecat commented Aug 13, 2016

guersam commented Aug 15, 2016

mpilquist commented Aug 15, 2016 •

edited

Loading

guersam commented Aug 15, 2016

tpolecat commented Aug 15, 2016

guersam commented Aug 15, 2016 •

edited

Loading

tpolecat commented Aug 20, 2016

guersam commented Aug 20, 2016

guersam commented Aug 20, 2016

tpolecat commented Aug 21, 2016

guersam commented Aug 21, 2016

guersam commented Aug 22, 2016

tpolecat commented Aug 22, 2016

guersam commented Aug 23, 2016

tpolecat commented Aug 23, 2016

guersam commented Aug 24, 2016

tpolecat commented Aug 24, 2016

tpolecat commented Aug 24, 2016

guersam commented Aug 24, 2016

tpolecat Sep 5, 2016

tpolecat commented Sep 5, 2016

adelbertc commented Sep 5, 2016

tpolecat commented Sep 5, 2016

guersam commented Sep 5, 2016

tpolecat commented Sep 6, 2016

macalinao commented Mar 11, 2017

adelbertc commented Mar 11, 2017

tpolecat commented Mar 12, 2017 •

edited

Loading

macalinao commented Mar 12, 2017 •

edited

Loading

WIP: Support for fs2.Task dropping own Catchable #323

WIP: Support for fs2.Task dropping own Catchable #323

Conversation

guersam commented Aug 12, 2016 • edited Loading

guersam Aug 12, 2016

Choose a reason for hiding this comment

guersam commented Aug 12, 2016

tpolecat commented Aug 13, 2016

guersam commented Aug 15, 2016

mpilquist commented Aug 15, 2016 • edited Loading

guersam commented Aug 15, 2016

tpolecat commented Aug 15, 2016

guersam commented Aug 15, 2016 • edited Loading

tpolecat commented Aug 20, 2016

guersam commented Aug 20, 2016

guersam commented Aug 20, 2016

tpolecat commented Aug 21, 2016

guersam commented Aug 21, 2016

guersam commented Aug 22, 2016

tpolecat commented Aug 22, 2016

guersam commented Aug 23, 2016

tpolecat commented Aug 23, 2016

guersam commented Aug 24, 2016

tpolecat commented Aug 24, 2016

tpolecat commented Aug 24, 2016

guersam commented Aug 24, 2016

tpolecat Sep 5, 2016

Choose a reason for hiding this comment

tpolecat commented Sep 5, 2016

adelbertc commented Sep 5, 2016

tpolecat commented Sep 5, 2016

guersam commented Sep 5, 2016

tpolecat commented Sep 6, 2016

macalinao commented Mar 11, 2017

adelbertc commented Mar 11, 2017

tpolecat commented Mar 12, 2017 • edited Loading

macalinao commented Mar 12, 2017 • edited Loading

guersam commented Aug 12, 2016 •

edited

Loading

mpilquist commented Aug 15, 2016 •

edited

Loading

guersam commented Aug 15, 2016 •

edited

Loading

tpolecat commented Mar 12, 2017 •

edited

Loading

macalinao commented Mar 12, 2017 •

edited

Loading