Add bridge between ApplicativeError and ApplicativeThrow

[morgen-peschke]

Implements #4286

[satorg]

I would probably prefer to deal with PartialFunction in such a case. Not only it allows to avoid an extra allocation, but also simpler to write a handler, e.g:

def catchNonFatalAs[A](adaptIfPossible: PartialFunction[Throwable, E])...

[satorg]

Not mentioning that the handler would look more like a regular catch in that case.

[danicheg]

Agreed, using PFs for error handling is a common practice 👍🏻

[morgen-peschke]

I would expect most times adaptIfPossible will be a method reference, so it would look like this:

ApplicativeError[Either[E, *], E].catchNonFatalAs(E.fromThrowable) { 
  // block
}

I'm open to being wrong about this, I just expect this sort of transformation won't generally written at the call sites because, if you need to do this, you probably need to do it more than once.

[satorg]

This line concerns me quite a lot, particularly throw e.
It means that the exception will be re-thrown out of the F context, which is not what I would expect from any ApplicativeError instance.

[morgen-peschke]

Unless we require the adaptation function to be total, we're going to need to rethrow these, because there isn't a way to raise a Throwable into an arbitrary ApplicativeError[F, E]

[armanbilge]

Yes, exactly. This is why we must not use a PartialFunction as you suggested in #4616 (comment). See my comments in #4286 (comment).

[satorg]

@armanbilge , PartialFunction[A, B] and A => Option[B] are isomorphic. Do I understand correctly your point that we must not use either of them?

[armanbilge]

Repeating my comments from #4286 (comment):

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.

[morgen-peschke]

Adding the extra effect layer would make these incongruent with catchOnly, which also throws exceptions that aren't expected, rather than attempting to raise them in F.

If there were a less clunky way to do the .map(G.pure).recover { ... }, that would improve the ergonomics enough for the usecase to basically disappear into Sync[F].delay + whatever the improvement is.

[satorg]

Honestly, I personally have never used catchOnly before, and never paid attention to it. And now I'm a bit surprised that such a function even exists in here.

On the other hand though, there are several examples in Cats when a function can throw an exception, e.g. NonEmptyList.fromListUnsafe and alike. It is a slightly different case though, because happens not in a type class but rather in a particular data type. But still close.

From that point of view, if we want to have functions in Cats like catchOnly or similar that can (re)throw exceptions, then perhaps it makes sense to mark them "unsafe", because they really are, e.g.: unsafeCatchOnly or catchOnlyUnsafe. And also explain in the docs, why they are unsafe.

[morgen-peschke]

Would the path forward then be to rename/deprecate the existing catch* methods?

[satorg]

I think this PR would be better off staying focused on its initial goal.
Perhaps, it makes sense to provide a safe version of catchNonFatalAs as @armanbilge suggested along with its unsafe counterpart.

The effort on renaming of the existing unsafe methods could be addressed in a separate PR later on. Actually, not all the existing catch* methods look unsafe to me, only catchOnly does. On the other hand, not only ApplicativeError contains catchOnly but also Validated and EitherSyntax have their specialized versions that can be considered unsafe in the same way. So the renaming is likely worth a separate PR.

[morgen-peschke]

I figured the easiest way to advance would be to implement *Unsafe versions of the proposed methods, and a catchOnlySafe version of catchOnly.

This way we have concrete implementations to look at and figure out which add value.

[satorg]

Personally, I find such a naming quite confusing. I feel in Cats everything is supposed to be safe, unless stated and marked otherwise.

[morgen-peschke]

No argument, I just needed to name it something so I could put a safe variant of catchOnly in there so we can figure out if it's something worth adding or not, as renaming the existing method to catchOnlyUnsafe would break bincompat