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 bridge from Throwable to E for ApplicativeError #4286

Open
morgen-peschke opened this issue Aug 24, 2022 · 3 comments
Open

Add bridge from Throwable to E for ApplicativeError #4286

morgen-peschke opened this issue Aug 24, 2022 · 3 comments

Comments

@morgen-peschke
Copy link
Contributor

Interacting with code that can throw exceptions can be awkward when working with ApplicativeError if E isn't Throwable, because the ApplicativeError#catch* methods don't really have a way to bridge into F unless you know what F is.

You can absolutely make it work, you just can't really leverage the ApplicativeError#catch* helpers, which is a shame because they're really handy, and I think it would be really straightforward to offer alternatives that are more generally applicable.

I propose adding parallel methods which would allow adapting a Throwable to an E. For example, ApplicativeError#catchNonFatalAs could look like this:

  /**
   * Often E can be created from Throwable. Here we try to call pure or 
   * catch, adapt into E, and raise.
   *
   * Exceptions that cannot be adapted to E will be propagated 
   */
  def catchNonFatalAs[A](adaptIfPossible: Throwable => Option[E])(a: => A): F[A] =
    try pure(a)
    catch {
      case NonFatal(e) => adaptIfPossible(e).map(raise(_)).getOrElse(throw e)
    }

Similar methods could be created for ApplicativeError#catchNonFatalEval and ApplicativeError#catchOnly.

@armanbilge
Copy link
Member

This is really interesting!

Exceptions that cannot be adapted to E will be propagated

This doesn't seem right to me. Exception-throwing code should ideally never be written outside of a catchNonFatal; pure code that uses this method (or any method in Cats) should not throw exceptions.

It seems to me what you actually want is maybe something like F[G[A]], with ApplicativeError[G, E] and ApplicativeThrow[F]. That way non-adaptable exceptions can be safely handled in the F effect.

In which case I think you want something like this?

F.catchNonFatal(doTheThing()).map(G.pure).recover {
  case ex: MyAdaptableException => G.raiseError(whatever)
}

Maybe a dedicated method/syntax could make that a bit neater. I'm not entirely sure where it could live.


While writing this, I noticed catchOnly does the same propagating thing. So maybe there's precedent for this, then, but personally I don't like it.

/**
* Evaluates the specified block, catching exceptions of the specified type. Uncaught exceptions are propagated.
*/
def catchOnly[T >: Null <: Throwable]: CatchOnlyPartiallyApplied[T, F, E] =
new CatchOnlyPartiallyApplied[T, F, E](this)

@morgen-peschke
Copy link
Contributor Author

If I understand the existing catchOnly, the intent is for anything that isn't caught to be considered a fatal error, and not something the application logic should be expected to handle, so I've followed a similar convention to minimize surprises.

I could be missing something though. The local helper this grew out of did originally require the adapting function to be total, so I'm certainly am not opposed to this approach.

@morgen-peschke
Copy link
Contributor Author

@armanbilge I'm still on the fence for catchNonFatalAs, but I've come around on requiring the mapping function for catchOnlyAs to be total, on the grounds that if you're expecting a particular subtype, you really should know how to map it to E

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

No branches or pull requests

2 participants