Cats Effect: STM & TRef #791 #796

VladKopanev · 2019-04-25T11:19:21Z

Cats Effect: STM & TRef #791

jdegoes · 2019-04-25T12:45:13Z

Exactly what I was hoping for! I'll comment more when you're ready, but this is looking great.

VladKopanev · 2019-04-26T11:08:16Z

@jdegoes A couple of question has raised in my head while doing this PR:

Is it okey to put the whole thing in interop-cats module? Should it be somewhere else?
What should I do with documentation on STM and Ref? should I reference origin docs or just fix it regarding that we don't use error type and we operate in abstract F[_] context?
I have implemented FunctionK instance for translation from ZIO to cats IO, I guess we need additional instances for types like monix.Task, Future, so users of those don't need to provide instances by themselves. Is this right?
I also have doubts about implementation of specific methods like STM#collectAll I suspect a possibly big amount of boxing/unboxing there, we could implement it like new STM(ZSTM.collectAll(i.map(_.underlying))) this should decrease number of boxings to 1, but adds 1 iteration through iterable, don't know which implementation is better.

I would appreciate if you could elaborate on this questions and of course leave comments in code if you spot other issues.

jdegoes · 2019-04-26T12:11:37Z

Is it okey to put the whole thing in interop-cats module? Should it be somewhere else?

Yes, it's ok they are put there (in the stm sub package).

What should I do with documentation on STM and Ref? should I reference origin docs or just fix it regarding that we don't use error type and we operate in abstract F[_] context?

I would reference origin docs (definitely not copy them along).

I have implemented FunctionK instance for translation from ZIO to cats IO, I guess we need additional instances for types like monix.Task, Future, so users of those don't need to provide instances by themselves. Is this right?

This makes sense to me. Of course you can probably do that generically for any F[_] that has cats effect instances, since you can go from/to F[_].

I also have doubts about implementation of specific methods like STM#collectAll I suspect a possibly big amount of boxing/unboxing there, we could implement it like new STM(ZSTM.collectAll(i.map(_.underlying))) this should decrease number of boxings to 1, but adds 1 iteration through iterable, don't know which implementation is better.

One iteration through the iterable is not that significant. It will be much faster doing it this way. ^^^

Thanks for your work on this, will do a PR review later today!

Update master

jdegoes · 2019-04-26T22:25:30Z

interop-cats/jvm/src/main/scala/scalaz/zio/interop/catsjvm.scala

@@ -73,8 +74,8 @@ abstract class CatsInstances extends CatsInstances1 {

  implicit def taskEffectInstances[R](
    implicit runtime: Runtime[R]
-  ): effect.ConcurrentEffect[TaskR[R, ?]] with SemigroupK[TaskR[R, ?]] =
-    new CatsConcurrentEffect[R](runtime)
+  ): effect.ConcurrentEffect[TaskR[R, ?]] with SemigroupK[TaskR[R, ?]] with FunctionK[TaskR[R, ?], effect.IO] =


Is FunctionK really intended to be used as an implicit? This would not be true for, e.g., Scalaz's NaturalTransformation.

jdegoes · 2019-04-26T22:26:26Z

interop-cats/jvm/src/main/scala/scalaz/zio/interop/stm/STM.scala

+/**
+ * See [[scalaz.zio.stm.STM]]
+ */
+final class STM[F[+ _], +A] private[stm] (private[stm] val underlying: ZSTM[Throwable, A])(


I'd have this class extend AnyVal, which you will be able to do when removing the implicit.

I would like to make it AnyVal too, but scala compiler says value class may not wrap another user-defined value class, because ZSTM is also an AnyVal. I don't see how to overcome this restriction for now.

jdegoes · 2019-04-26T22:29:59Z

interop-cats/jvm/src/main/scala/scalaz/zio/interop/stm/STM.scala

+ * See [[scalaz.zio.stm.STM]]
+ */
+final class STM[F[+ _], +A] private[stm] (private[stm] val underlying: ZSTM[Throwable, A])(
+  implicit liftIO: ZIO[Any, Throwable, ?] ~> F


I don't think we need this implicit. We should be able to go directly from ZIO to any F[_] that supports Async (or maybe Effect or ConcurrentEffect), given an implicit Runtime. And we can push that down to atomically since that's the only one that needs it.

I found a way to implement atomically using Async and Runtime[Any], and removed all useless FunctionK implicits, please check if it's done right.

jdegoes · 2019-04-26T22:30:51Z

interop-cats/jvm/src/main/scala/scalaz/zio/interop/stm/STM.scala

+    (self map (Left[A, B](_))) orElse (that map (Right[A, B](_)))
+
+  /**
+   * See scalaz.zio.stm.STM.unit


No Scaladoc link?

I did this because in this case sbt doc command fails with err message like Could not find any member to link for "scalaz.zio.stm.STM.unit". I suspect this is because it sees unit in the STM class and also in companion object, and fails to resolve which unit to link.

No worries!

jdegoes · 2019-04-26T22:30:58Z

interop-cats/jvm/src/main/scala/scalaz/zio/interop/stm/STM.scala

+  final def unit: STM[F, Unit] = const(())
+
+  /**
+   * See scalaz.zio.stm.STM.unit


No Scaladoc link?

same problem here.

jdegoes · 2019-04-26T22:31:26Z

interop-cats/jvm/src/main/scala/scalaz/zio/interop/stm/STM.scala

+
+object STM {
+
+  final def succeed[F[+ _], A](a: A)(implicit liftIO: ZIO[Any, Throwable, ?] ~> F): STM[F, A] = new STM(ZSTM.succeed(a))


This doesn't need an implicit.

jdegoes · 2019-04-26T22:31:48Z

interop-cats/jvm/src/main/scala/scalaz/zio/interop/stm/STM.scala

+
+  final def succeed[F[+ _], A](a: A)(implicit liftIO: ZIO[Any, Throwable, ?] ~> F): STM[F, A] = new STM(ZSTM.succeed(a))
+
+  final def atomically[F[+ _], A](stm: STM[F, A])(implicit liftIO: ZIO[Any, Throwable, ?] ~> F): F[A] =


This can make do with an implicit Runtime[Any], then no need for implicit FunctionK.

jdegoes · 2019-04-26T22:31:58Z

interop-cats/jvm/src/main/scala/scalaz/zio/interop/stm/STM.scala

+  final def atomically[F[+ _], A](stm: STM[F, A])(implicit liftIO: ZIO[Any, Throwable, ?] ~> F): F[A] =
+    liftIO(ZSTM.atomically(stm.underlying))
+
+  final def check[F[+ _]](p: Boolean)(implicit liftIO: ZIO[Any, Throwable, ?] ~> F): STM[F, Unit] =


This doesn't need an implicit.

jdegoes · 2019-04-26T22:32:08Z

interop-cats/jvm/src/main/scala/scalaz/zio/interop/stm/STM.scala

+
+  final def collectAll[F[+ _], A](
+    i: Iterable[STM[F, A]]
+  )(implicit liftIO: ZIO[Any, Throwable, ?] ~> F): STM[F, List[A]] =


This doesn't need an implicit.

jdegoes · 2019-04-26T22:32:15Z

interop-cats/jvm/src/main/scala/scalaz/zio/interop/stm/STM.scala

+  )(implicit liftIO: ZIO[Any, Throwable, ?] ~> F): STM[F, List[A]] =
+    new STM(ZSTM.collectAll(i.map(_.underlying)))
+
+  final def die[F[+ _]](t: Throwable)(implicit liftIO: ZIO[Any, Throwable, ?] ~> F): STM[F, Nothing] =


This doesn't need an implicit.

jdegoes · 2019-04-26T22:32:22Z

interop-cats/jvm/src/main/scala/scalaz/zio/interop/stm/STM.scala

+  final def die[F[+ _]](t: Throwable)(implicit liftIO: ZIO[Any, Throwable, ?] ~> F): STM[F, Nothing] =
+    succeedLazy(throw t)
+
+  final def dieMessage[F[+ _]](m: String)(implicit liftIO: ZIO[Any, Throwable, ?] ~> F): STM[F, Nothing] =


This doesn't need an implicit.

jdegoes · 2019-04-26T22:32:27Z

interop-cats/jvm/src/main/scala/scalaz/zio/interop/stm/STM.scala

+  final def dieMessage[F[+ _]](m: String)(implicit liftIO: ZIO[Any, Throwable, ?] ~> F): STM[F, Nothing] =
+    die(new RuntimeException(m))
+
+  final def fail[F[+ _]](e: Throwable)(implicit liftIO: ZIO[Any, Throwable, ?] ~> F): STM[F, Nothing] =


This doesn't need an implicit.

jdegoes · 2019-04-26T22:32:51Z

interop-cats/jvm/src/main/scala/scalaz/zio/interop/stm/STM.scala

+  final def fail[F[+ _]](e: Throwable)(implicit liftIO: ZIO[Any, Throwable, ?] ~> F): STM[F, Nothing] =
+    new STM(ZSTM.fail(e))
+
+  final def foreach[F[+ _], A, B](


This doesn't need an implicit.

jdegoes · 2019-04-26T22:32:56Z

interop-cats/jvm/src/main/scala/scalaz/zio/interop/stm/STM.scala

+  )(f: A => STM[F, B])(implicit liftIO: ZIO[Any, Throwable, ?] ~> F): STM[F, List[B]] =
+    collectAll(as.map(f))
+
+  final def fromEither[F[+ _], A](e: Either[Throwable, A])(implicit liftIO: ZIO[Any, Throwable, ?] ~> F): STM[F, A] =


This doesn't need an implicit.

jdegoes · 2019-04-26T22:33:00Z

interop-cats/jvm/src/main/scala/scalaz/zio/interop/stm/STM.scala

+  final def fromEither[F[+ _], A](e: Either[Throwable, A])(implicit liftIO: ZIO[Any, Throwable, ?] ~> F): STM[F, A] =
+    new STM(ZSTM.fromEither(e))
+
+  final def fromTry[F[+ _], A](a: => Try[A])(implicit liftIO: ZIO[Any, Throwable, ?] ~> F): STM[F, A] =


This doesn't need an implicit.

jdegoes · 2019-04-26T22:33:06Z

interop-cats/jvm/src/main/scala/scalaz/zio/interop/stm/STM.scala

+  final def fromTry[F[+ _], A](a: => Try[A])(implicit liftIO: ZIO[Any, Throwable, ?] ~> F): STM[F, A] =
+    new STM(ZSTM.fromTry(a))
+
+  final def partial[F[+ _], A](a: => A)(implicit liftIO: ZIO[Any, Throwable, ?] ~> F): STM[F, A] =


This doesn't need an implicit.

jdegoes · 2019-04-26T22:33:10Z

interop-cats/jvm/src/main/scala/scalaz/zio/interop/stm/STM.scala

+  final def partial[F[+ _], A](a: => A)(implicit liftIO: ZIO[Any, Throwable, ?] ~> F): STM[F, A] =
+    fromTry(Try(a))
+
+  final def retry[F[+ _]](implicit liftIO: ZIO[Any, Throwable, ?] ~> F): STM[F, Nothing] = new STM(ZSTM.retry)


This doesn't need an implicit.

jdegoes · 2019-04-26T22:33:16Z

interop-cats/jvm/src/main/scala/scalaz/zio/interop/stm/STM.scala

+
+  final def retry[F[+ _]](implicit liftIO: ZIO[Any, Throwable, ?] ~> F): STM[F, Nothing] = new STM(ZSTM.retry)
+
+  final def succeedLazy[F[+ _], A](a: => A)(implicit liftIO: ZIO[Any, Throwable, ?] ~> F): STM[F, A] =


This doesn't need an implicit.

jdegoes · 2019-04-26T22:33:20Z

interop-cats/jvm/src/main/scala/scalaz/zio/interop/stm/STM.scala

+  final def succeedLazy[F[+ _], A](a: => A)(implicit liftIO: ZIO[Any, Throwable, ?] ~> F): STM[F, A] =
+    new STM(ZSTM.succeedLazy(a))
+
+  final def unit[F[+ _]](implicit liftIO: ZIO[Any, Throwable, ?] ~> F): STM[F, Unit] = succeed(())


This doesn't need an implicit.

jdegoes · 2019-04-26T22:34:03Z

interop-cats/jvm/src/main/scala/scalaz/zio/interop/stm/TRef.scala

+import scalaz.zio.ZIO
+import scalaz.zio.stm.{ TRef => ZTRef }
+
+class TRef[F[+ _], A] private (underlying: ZTRef[A])(implicit liftIO: ZIO[Any, Throwable, ?] ~> F) {


Same comment here for the implicit FunctionK. I'd delete that, make TRef extend AnyVal, and push the implicit Runtime[Any] to the commit method. I won't comment individually on these methods.

jdegoes · 2019-04-26T22:34:17Z

interop-cats/jvm/src/main/scala/scalaz/zio/interop/stm/TRef.scala

+
+object TRef {
+
+  final def make[F[+ _], A](a: => A)(implicit liftIO: ZIO[Any, Throwable, ?] ~> F): STM[F, TRef[F, A]] =


This won't won't need an implicit.

jdegoes · 2019-04-26T22:34:28Z

interop-cats/jvm/src/main/scala/scalaz/zio/interop/stm/TRef.scala

+  final def make[F[+ _], A](a: => A)(implicit liftIO: ZIO[Any, Throwable, ?] ~> F): STM[F, TRef[F, A]] =
+    new STM(ZTRef.make(a).map(new TRef(_)))
+
+  final def makeCommit[F[+ _], A](a: => A)(implicit liftIO: ZIO[Any, Throwable, ?] ~> F): F[TRef[F, A]] =


This one will just need the implicit runtime.

jdegoes

This is looking great! Just a minor comment on how we lift ZIO to some F[_], and once those changes are in, we should be good to merge. 🎉

…37d17db)

jdegoes · 2019-04-30T15:26:47Z

interop-cats/jvm/src/main/scala/scalaz/zio/interop/stm/STM.scala

+/**
+ * See [[scalaz.zio.stm.STM]]
+ */
+final class STM[F[+ _], +A] private[stm] (private[stm] val underlying: ZSTM[Throwable, A]) {


Looks super clean. I think you can extend AnyVal here, too, now.

I would like to, but scala compiler has issue with nested AnyVal classes, actually it does not support this feature yet. I didn't find any workarounds at this point. Anyways I think AnyVal has a bunch of restrictions that could make all workarounds meaningless (e.g. passing underlying ZSTM as a trait or other type leads to allocation).

jdegoes · 2019-04-30T15:28:36Z

interop-cats/jvm/src/main/scala/scalaz/zio/interop/stm/STM.scala

+
+  final def succeed[F[+ _], A](a: A): STM[F, A] = new STM(ZSTM.succeed(a))
+
+  final def atomically[F[+ _], A](stm: STM[F, A])(implicit R: Runtime[Any], A: Async[F]): F[A] =


Beautiful! Just this single method needs the implicit.

jdegoes · 2019-04-30T16:09:07Z

interop-cats/jvm/src/main/scala/scalaz/zio/interop/stm/TRef.scala

+
+object TRef {
+
+  final def make[F[+ _], A](a: => A): STM[F, TRef[F, A]] =


This all looks good to me! I'd just sort the methods alphabetically, then it looks good to merge! 👍

jdegoes · 2019-04-30T20:00:18Z

Awesome work! Clean code and super fast revisions. Thank you for your contribution!

VladKopanev · 2019-05-01T00:14:20Z

Thanks @jdegoes! Please let me know if we need to add TPromise, TQueue and TSemaphore in the same fashion into cats interop module, I can help help with that.

jdegoes · 2019-05-01T00:42:42Z

@VladKopanev That would be phenomenal!

* upstream/master: Cats Effect: STM & TRef zio#791 (zio#796) increase RS TCK timeouts in interopRS tests (zio#806) zio#804 Move ArbitraryCause.scala and GenIO.scala to shared (zio#809) docfix: Minor scaladoc fix

* Implement effect-polymorphic class atop STM zio#791 * Implement effect-polymorphic TRef zio#791 * remove unnecessary type parameters zio#791 * Add 'FunctionK' instance for converting TaskR to cats.IO zio#791 * Link to original documentation zio#791 * More efficient `collectAll` zio#791 * Make every method final zio#791 * Revert: Add 'FunctionK' instance for converting TaskR to cats.IO zio#791 (37d17db) * Push implicits down to atomically zio#791 * Reorder methods alphabetically zio#791 * Fix formatting zio#791

VladKopanev marked this pull request as ready for review April 26, 2019 19:49

VladKopanev added 8 commits April 26, 2019 22:58

Merge pull request #1 from scalaz/master

a510755

Update master

Implement effect-polymorphic class atop STM #791

3c8b37f

Implement effect-polymorphic TRef #791

4d58e73

remove unnecessary type parameters #791

1a49bef

Add 'FunctionK' instance for converting TaskR to cats.IO #791

37d17db

Link to original documentation #791

652c70d

More efficient collectAll #791

b14bd6e

Make every method final #791

52dad8b

jdegoes reviewed Apr 26, 2019

View reviewed changes

jdegoes requested changes Apr 26, 2019

View reviewed changes

VladKopanev added 2 commits April 29, 2019 20:45

Revert: Add 'FunctionK' instance for converting TaskR to cats.IO #791 (…

768aa79

…37d17db)

Push implicits down to atomically #791

3fed4f5

jdegoes reviewed Apr 30, 2019

View reviewed changes

VladKopanev added 2 commits April 30, 2019 19:25

Reorder methods alphabetically #791

aad5019

Fix formatting #791

b951023

jdegoes approved these changes Apr 30, 2019

View reviewed changes

jdegoes merged commit fb7b0f1 into zio:master Apr 30, 2019

VladKopanev deleted the feature/791-stm-interop branch May 1, 2019 00:11

VladKopanev mentioned this pull request May 1, 2019

Cats Effect: TPromise, TQueue, TSemaphore #791 #814

Merged

ghostdogpr mentioned this pull request Jun 10, 2019

Cats Effect: STM & TRef #791

Closed


		object STM {

		final def succeed[F[+ _], A](a: A)(implicit liftIO: ZIO[Any, Throwable, ?] ~> F): STM[F, A] = new STM(ZSTM.succeed(a))


		final def succeed[F[+ _], A](a: A)(implicit liftIO: ZIO[Any, Throwable, ?] ~> F): STM[F, A] = new STM(ZSTM.succeed(a))

		final def atomically[F[+ _], A](stm: STM[F, A])(implicit liftIO: ZIO[Any, Throwable, ?] ~> F): F[A] =


		final def retry[F[+ _]](implicit liftIO: ZIO[Any, Throwable, ?] ~> F): STM[F, Nothing] = new STM(ZSTM.retry)

		final def succeedLazy[F[+ _], A](a: => A)(implicit liftIO: ZIO[Any, Throwable, ?] ~> F): STM[F, A] =


		object TRef {

		final def make[F[+ _], A](a: => A)(implicit liftIO: ZIO[Any, Throwable, ?] ~> F): STM[F, TRef[F, A]] =


		final def succeed[F[+ _], A](a: A): STM[F, A] = new STM(ZSTM.succeed(a))

		final def atomically[F[+ _], A](stm: STM[F, A])(implicit R: Runtime[Any], A: Async[F]): F[A] =


		object TRef {

		final def make[F[+ _], A](a: => A): STM[F, TRef[F, A]] =

Cats Effect: STM & TRef #791 #796

Cats Effect: STM & TRef #791 #796

Conversation

VladKopanev commented Apr 25, 2019

jdegoes commented Apr 25, 2019

VladKopanev commented Apr 26, 2019 • edited

jdegoes commented Apr 26, 2019

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

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

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

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

jdegoes left a 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

jdegoes commented Apr 30, 2019

VladKopanev commented May 1, 2019

jdegoes commented May 1, 2019

VladKopanev commented Apr 26, 2019 •

edited