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

Implemented submarine error propagation for Handle #489

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 36 additions & 0 deletions core/src/main/scala/cats/mtl/Handle.scala
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ package mtl
import cats.data._

import scala.annotation.implicitNotFound
import scala.util.control.NoStackTrace

@implicitNotFound(
"Could not find an implicit instance of Handle[${F}, ${E}]. If you\nhave a good way of handling errors of type ${E} at this location, you may want\nto construct a value of type EitherT for this call-site, rather than ${F}.\nAn example type:\n\n EitherT[${F}, ${E}, *]\n\nThis is analogous to writing try/catch around this call. The EitherT will\n\"catch\" the errors of type ${E}.\n\nIf you do not wish to handle errors of type ${E} at this location, you should\nadd an implicit parameter of this type to your function. For example:\n\n (implicit fhandle: Handle[${F}, ${E}}])\n")
Expand Down Expand Up @@ -218,5 +219,40 @@ private[mtl] trait HandleInstances extends HandleLowPriorityInstances {
}

object Handle extends HandleInstances {

def apply[F[_], E](implicit ev: Handle[F, E]): Handle[F, E] = ev

def ensure[F[_], E]: AdHocSyntax[F, E] =
Copy link
Member

Choose a reason for hiding this comment

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

Should we ask for the ApplicativeThrow constraint here, and then pass it into AdHocSyntax?

Copy link
Member Author

Choose a reason for hiding this comment

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

If we do, we lose the syntax and have to insert extra parens at the call site.

new AdHocSyntax[F, E]

final class AdHocSyntax[F[_], E] {

def apply[A](body: Handle[F, E] => F[A])(implicit F: ApplicativeThrow[F]): Inner[A] =
Copy link
Member

Choose a reason for hiding this comment

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

Could we implement this with Handle[F, Throwable] instead of ApplicativeThrow[F]?

new Inner(body)

final class Inner[A](body: Handle[F, E] => F[A])(implicit F: ApplicativeThrow[F]) {
def recover(h: E => F[A]): F[A] = {
val Marker = new AnyRef

def inner[B](fb: F[B])(f: E => F[B]): F[B] =
ApplicativeThrow[F].handleErrorWith(fb) {
case Submarine(e, Marker) => f(e.asInstanceOf[E])
case t => ApplicativeThrow[F].raiseError(t)
}

val fa = body(new Handle[F, E] {
def applicative = Applicative[F]
def raise[E2 <: E, B](e: E2): F[B] =
ApplicativeThrow[F].raiseError(Submarine(e, Marker))
def handleWith[B](fb: F[B])(f: E => F[B]): F[B] = inner(fb)(f)
})

inner(fa)(h)
}
}
}

private final case class Submarine[E](e: E, marker: AnyRef)
extends RuntimeException
Copy link
Member

@armanbilge armanbilge Apr 30, 2023

Choose a reason for hiding this comment

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

Should this just extend Throwable? (Or even ControlThrowable, but I think that one is considered fatal by NonFatal.) It's not really an ordinary exception, and even if it leaked, without a stacktrace it would be hard to do something about it 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

I went back and forth. Technically RuntimeException is accurate and appropriate, but it's all sort of dependent on perspective. Certainly anything that would be considered fatal is off the table since it just instantly torpedoes the whole runtime.

with NoStackTrace
}
85 changes: 84 additions & 1 deletion tests/shared/src/test/scala/cats/mtl/tests/HandleTests.scala
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,16 @@ package cats
package mtl
package tests

import cats.data.{Kleisli, WriterT}
import cats.data.{EitherT, Kleisli, WriterT}
import cats.laws.discipline.arbitrary._
import cats.mtl.syntax.all._
import cats.syntax.all._

import org.scalacheck.{Arbitrary, Cogen}, Arbitrary.arbitrary

class HandleTests extends BaseSuite {
type F[A] = EitherT[Eval, Throwable, A]

test("handleForApplicativeError") {
case class Foo[A](bar: A)

Expand All @@ -39,4 +46,80 @@ class HandleTests extends BaseSuite {
Handle[Kleisli[Foo, Unit, *], String]
Handle[WriterT[Foo, String, *], String]
}

test("submerge custom errors") {
sealed trait Error extends Product with Serializable

object Error {
case object First extends Error
case object Second extends Error
case object Third extends Error
}

val test =
Handle.ensure[F, Error](implicit h => Error.Second.raise.as("nope")) recover {
case Error.First => "0".pure[F]
case Error.Second => "1".pure[F]
case Error.Third => "2".pure[F]
}

assertEquals(test.value.value.toOption, Some("1"))
}

test("submerge two independent errors") {
sealed trait Error1 extends Product with Serializable

object Error1 {
case object First extends Error1
case object Second extends Error1
case object Third extends Error1
}

sealed trait Error2 extends Product with Serializable

val test = Handle.ensure[F, Error1] { implicit h1 =>
Handle.ensure[F, Error2] { implicit h2 =>
// it's helpful to test the raise syntax infers even when multiple handles are present
val _ = h2
Error1.Third.raise.as("nope")
} recover { e => e.toString.pure[F] }
} recover {
case Error1.First => "first1".pure[F]
case Error1.Second => "second1".pure[F]
case Error1.Third => "third1".pure[F]
}

assertEquals(test.value.value.toOption, Some("third1"))
}

{
Copy link
Member

Choose a reason for hiding this comment

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

Should this be a test("...") { ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Can it be? I have no idea how that works in MUnit.

Copy link
Member

Choose a reason for hiding this comment

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

I'm confused :) is this a test or not haha, that is really what I am asking 😂

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a lexical block which contains Discipline stuff, which is many tests.

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see now, sorry. I was confused by the end of the block, it looked like an ordinary test ... but it's because the discipline tests have to exist inside of the Eval since that's how the Handle is constructed. Thanks!

final case class Error(value: Int)

object Error {
implicit val arbError: Arbitrary[Error] =
Arbitrary(arbitrary[Int].flatMap(Error(_)))

implicit val cogenError: Cogen[Error] =
Cogen((_: Error).value.toLong)

implicit val eqError: Eq[Error] =
Eq.by((_: Error).value)
}

implicit val eqThrowable: Eq[Throwable] =
Eq.fromUniversalEquals[Throwable]

val test = Handle.ensure[F, Error] { implicit h =>
EitherT liftF {
Eval later {
checkAll(
"Handle.ensure[F, Error]",
cats.mtl.laws.discipline.HandleTests[F, Error].handle[Int])
}
}
} recover { case Error(_) => ().pure[F] }

test.value.value
()
}
}