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

Add Cats Async, ApplicativeAsk and ArrowChoice instances #13

Closed
wants to merge 6 commits into from

Conversation

LukaJCB
Copy link

@LukaJCB LukaJCB commented Aug 24, 2018

Should fix #2


sealed abstract class ArrowInstances extends ArrowInstances1 {

implicit def catsMonadForArrow[E]: Async[Arrow[E, ?]] = new Async[Arrow[E, ?]] with StackSafeMonad[Arrow[E, ?]] {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this use Task instead of Arrow?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, that would constraint it unnecessary, An Arrow[E, A] is just as much valid as a Task[A]. Though for Effect it does need to be Task, as you can't implement runAsync otherwise. For Async we can do it for any Arrow.

Copy link
Collaborator

@fwbrasil fwbrasil Aug 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arrow is more general than monad, so I don't think it's a good fit to a monad typeclass. Also, the fixed Unit input is important for performance. This has much better performance:

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

than

    def flatMap[A, B](fa: Arrow[E, A])(f: A => Arrow[E, B]): Arrow[E, B] =
      Arrow[E].flatMap(e => fa(e).flatMap(a => f(a)(e)))

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right yeah, that's why I need to write the Effect type for Task, but we should still provide all the instances that make sense :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example, we also have Async instances for Kleisli[F, R, A] where F has an Async instance itself. Since Arrow is roughly equivalent to that, it stands to reason that we should provide that too :)


def pure[A](x: A): Arrow[E, A] = Arrow.successful(x)

def suspend[A](thunk: => Arrow[E, A]): Arrow[E, A] =
Copy link
Collaborator

@fwbrasil fwbrasil Aug 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arrows aren't strict so I'm not sure I understand why suspend would be necessary

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the contract for Sync :)

def pure[A](x: A): Arrow[E, A] = Arrow.successful(x)

def suspend[A](thunk: => Arrow[E, A]): Arrow[E, A] =
Arrow[E].flatMap(e => try { thunk(e) } catch { case NonFatal(t) => Arrow.failed[A](t)(e) })
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you don't need to catch here, Arrow will do it

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, cool thanks! :)

@LukaJCB
Copy link
Author

LukaJCB commented Aug 25, 2018

I think I might need your help, what would be the best way to provide something akin to a () => Unit that runs the effects of a Task on a given ExecutionContext without blocking. It's needed to implement the runAsync method of cats.effect.Effect: https://github.com/typelevel/cats-effect/blob/master/core/shared/src/main/scala/cats/effect/Effect.scala#L55

@alexandru
Copy link

For Effect you don’t need the ExecutionContext. Whatever the implementation does is fine.

The question is, how to execute a task and get a result back, ideally in a non-blocking way.

@codecov-io
Copy link

codecov-io commented Aug 26, 2018

Codecov Report

Merging #13 into master will decrease coverage by 3.5%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #13      +/-   ##
=========================================
- Coverage   78.41%   74.9%   -3.51%     
=========================================
  Files          19      20       +1     
  Lines         797     833      +36     
  Branches       90     103      +13     
=========================================
- Hits          625     624       -1     
- Misses        172     209      +37
Impacted Files Coverage Δ
...tdlib-cats/src/main/scala/arrows/stdlib/cats.scala 0% <0%> (ø)
...itter/src/main/scala/arrows/twitter/ArrowRun.scala 97.4% <0%> (-1.3%) ⬇️

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 0cc0937...dd74123. Read the comment docs.

@LukaJCB
Copy link
Author

LukaJCB commented Aug 26, 2018

I've now implemented it by creating the future and discarding it inside a SyncIO, not sure if it's the best way to do that, but it seems to work.

Copy link

@alexandru alexandru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good thus far.

@@ -50,6 +55,29 @@ lazy val `arrows-stdlib` =
lazy val `arrows-stdlib-jvm` = `arrows-stdlib`.jvm
lazy val `arrows-stdlib-js` = `arrows-stdlib`.js.settings(test := {})

lazy val `arrows-stdlib-cats` =

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the convention for these sub-projects? What is stdlib referring to? Couldn't it be just arrows-cats?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stdlib refers to the use of stdlib Future wheras arrows-twitter uses twitter Future.

build.sbt Outdated
crossProject.crossType(superPure)
.settings(commonSettings)
.settings(
crossScalaVersions := Seq("2.12.5"),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we are at 2.12.6.

build.sbt Outdated
name := "arrows-stdlib-cats",
libraryDependencies ++= List(
"org.typelevel" %%% "cats-effect" % "1.0.0-RC3",
"org.typelevel" %%% "cats-effect-laws" % "1.0.0-RC3",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only need cats-effect-laws for testing purposes.

build.sbt Outdated
"org.typelevel" %%% "cats-effect" % "1.0.0-RC3",
"org.typelevel" %%% "cats-effect-laws" % "1.0.0-RC3",
"org.typelevel" %%% "cats-mtl-core" % "0.2.3",
"org.typelevel" %%% "cats-mtl-laws" % "0.2.3",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here too, I think we only need cats-mtl-laws for testing purposes, no reason to depend on it in any other context.

def compose[A, B, C](f: Arrow[B, C], g: Arrow[A, B]): Arrow[A, C] = g.andThen(f)
}

implicit def catsApplicativeAskForArrow[E]: ApplicativeAsk[Arrow[E, ?], E] = new ApplicativeAsk[Arrow[E, ?], E] {
Copy link
Collaborator

@fwbrasil fwbrasil Aug 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could it be a val?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can't be a val because then you can't define the E type parameter :)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could cache it and cast:

private final val catsApplicativeAskForArrowInstance: ApplicativeAsk[Arrow[Any, ?], Any] ...
implicit def catsApplicativeAskForArrow[E] = catsApplicativeAskForArrowInstance.asInstanceOf[ApplicativeAsk[Arrow[E, ?], E]]


implicit def catsEffectForTask(implicit ec: ExecutionContext): Effect[Task] = new ArrowAsync[Unit] with Effect[Task] {
def runAsync[A](fa: Task[A])(cb: Either[Throwable, A] => IO[Unit]): SyncIO[Unit] =
SyncIO { fa.run(())(ec); () }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to call cb here?

implicit val catsArrowChoiceForArrow: ArrowChoice[Arrow] = new ArrowChoice[Arrow] {
def choose[A, B, C, D](f: Arrow[A, C])(g: Arrow[B, D]): Arrow[Either[A, B], Either[C, D]] =
Arrow[Either[A, B]].flatMap {
case Left(a) => f(a).map(Left(_))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bytecode for this transformation will be large. What do you think about optimizing it? Something like

// In a stable scope
private final object ToLeft {
   private final val instance: Any => Left[Any] = Left(_)
   def apply[T]() = instance.asInstanceOf[T => Left[T]]
}

private final object ToRight {
   private final val instance: Any => Right[Any] = Right(_)
   def apply[T]() = instance.asInstanceOf[T => Right[T]]
}

and then

Arrow[Either[A, B]].flatMap { a =>
  if(a.isLeft) f(a).map(toLeft)
  else f(a).map(toRight)
}


trait ArrowAsync[E] extends StackSafeMonad[Arrow[E, ?]] with Async[Arrow[E, ?]] {
def flatMap[A, B](fa: Arrow[E, A])(f: A => Arrow[E, B]): Arrow[E, B] =
Arrow[E].flatMap(e => fa(e).flatMap(a => f(a)(e)))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I'm still not sure this is a good approach. There's the performance issue, and I don't see why passing the same input to both arrows is useful other than when the arrow is a Task so the input is fixed. Could you give an example of what you couldn't express if this class used Task instead of Arrow?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, together with something like ApplicativeAsk, you can define cool ReaderT style code with Arrow:

def foo[F[_]: Monad: ApplicativeAsk[?[_], Config]] = for {
  config <- ApplicativeAsk[F, Config].ask
  response <- serviceCall(config)
} yield response

Just a small example, but this is a pretty neat feature IMO and one could easily use this with e.g. http4s which only needs Sync to define the routes :)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LukaJCB what's the type of serviceCall(config)? @alexandru could you weight in on this?

def pure[A](x: A): Arrow[E, A] = Arrow.successful(x)

def suspend[A](thunk: => Arrow[E, A]): Arrow[E, A] =
Arrow[E].flatMap(e => try { thunk(e) } catch { case NonFatal(t) => Arrow.failed[A](t)(e) })
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's no need to catch here

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure? If I change this line, it evaluates the line eagerly:

scala> def y(): Arrow[Int, String] = { throw new Exception(); }
y: ()arrows.stdlib.Arrow[Int,String]

scala> Arrow[Int].flatMap(y())
java.lang.Exception
  at .y(<console>:14)
  ... 36 elided

If I use my implementation instead:

scala> Arrow[Int].flatMap(i => try { y()(i) } catch { case t => Arrow.failed[String](t)(i) })
res6: arrows.stdlib.Arrow[Int,String] = <function1>

Sync#suspend requires to suspend any side-effects that occur when acquring the Arrow.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, you can't pass the arrow directly since it'll evaluate the by-name param. This should do it:

Arrow[E].flatMap(e => thunk(e))

Arrow[E].flatMap(e => try { thunk(e) } catch { case NonFatal(t) => Arrow.failed[A](t)(e) })

override def delay[A](thunk: => A): Arrow[E, A] =
Arrow[E].flatMap(e => try { Arrow.successful(thunk) } catch { case NonFatal(t) => Arrow.failed[A](t)(e) })
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

@LukaJCB
Copy link
Author

LukaJCB commented Aug 27, 2018

Some of the laws are failing, some related to Bracket some related to memoization and other to ArrowChoice, will need to revise.


implicit def catsEffectForTask(implicit ec: ExecutionContext): Effect[Task] = new ArrowAsync[Unit] with Effect[Task] {
def runAsync[A](fa: Task[A])(cb: Either[Throwable, A] => IO[Unit]): SyncIO[Unit] =
SyncIO(fa.run(())(ec).onComplete(t => cb(t.toEither).unsafeRunAsync(_ => ())))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's cache _ => ():

private final val toUnit: Any => Unit = _ => ()

.unsafeRunAsync(toUnit)


sealed abstract class ArrowInstances extends ArrowInstances1 {

private final object ToLeft {
Copy link
Collaborator

@fwbrasil fwbrasil Aug 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took a look at the bytecode and it doesn't seem that final object will allow constant folding by the JIT compiler. Sorry for the back and forth, but could you change it to:

private final val toLeftInstance: Any => Left[Any, Nothing] = Left(_)
private final def toLeftI[T] = toLeftInstance.asInstanceOf[T => Left[T, Nothing]]

Arrow[E].flatMap(e => try { thunk(e) } catch { case NonFatal(t) => Arrow.failed[A](t)(e) })

override def delay[A](thunk: => A): Arrow[E, A] =
Arrow[E].flatMap(e => Arrow.successful(thunk))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you use map here instead? it's less expensive

Arrow[E].map(_ => thunk)

@fwbrasil
Copy link
Collaborator

@LukaJCB thank you for pushing this forward :) Let me know if I can help debugging the failing tests

@LukaJCB
Copy link
Author

LukaJCB commented Aug 28, 2018

These are the tests that fail:

- effect.repeated sync evaluation not memoized
- effect.repeated async evaluation not memoized 
- effect.repeated callback ignored

- bracket.bracket release is called on Completed or Error

- arrowChoice.left and compose commute 
- arrowChoice.arrow functor

@LukaJCB
Copy link
Author

LukaJCB commented Sep 6, 2018

@alexandru maybe you can help me with the effect memoized related failures, they don't seem to make sense to me, with this simple test, it doesn't seem to memoize anything:

scala> Sync[Task].delay{ println("Heyo!"); 234 }
res1: arrows.stdlib.Task[Int] = <function1>

scala> res1.run
Heyo!
res2: scala.concurrent.Future[Int] = Future(Success(234))

scala> res1.run
Heyo!
res3: scala.concurrent.Future[Int] = Future(Success(234))

@vkostyukov
Copy link
Collaborator

Sorry for the noise but I was wondering what's the status on this?

@LukaJCB Huge thanks for driving this by the way!

@LukaJCB
Copy link
Author

LukaJCB commented Oct 24, 2018

There seems to be a few issues with the law failures I mentioned further up, I still don't really understand them and I'm not sure how to move forward.

@LukaJCB LukaJCB closed this Oct 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide cats-effect integration
5 participants