Consider removing attemptNarrow

[travisbrown]

Somehow I'd never paid attention to attemptNarrow, which we added in 2.1.0. It has some problems. For example it's trivially easy to make it crash at runtime:

scala> val e: Either[Any, Unit] = Left(List("abc"))
e: Either[Any,Unit] = Left(List(abc))

scala> import cats.implicits._
import cats.implicits._

scala> def x = e.attemptNarrow[List[Int]] match {
     |   case Right(Left(List(x))) => x
     |   case _ => 0
     | }
x: Int

scala> x
java.lang.ClassCastException: java.lang.String cannot be cast to java.lang.Integer
  at scala.runtime.BoxesRunTime.unboxToInt(BoxesRunTime.java:99)
  at .x(<console>:2)
  ... 36 elided

It can also be used to launder a runtime type check:

scala> import cats.implicits._, scala.reflect.ClassTag
import cats.implicits._
import scala.reflect.ClassTag

scala> def isIt[A: ClassTag](any: Any): Boolean =
     |   (Left(any): Either[Any, Unit]).attemptNarrow[A].isRight
isIt: [A](any: Any)(implicit evidence$1: scala.reflect.ClassTag[A])Boolean

scala> val x = if (true) 1 else "abc"
x: Any = 1

scala> isIt[String](x)
res0: Boolean = false

scala> isIt[Int](x)
res1: Boolean = true

Maybe that's fine, and you should know that if you've got a ClassTag around you've given up any hope of parametricity, but that seems pretty far from the spirit of Cats.

@tpolecat requested tests for the erasure issues in the original PR, but unfortunately the tests that were added don't actually check for the relevant problems.

I guess the reasoning is that E is usually an exception type, and generally won't be generic? In any case I think we should make both the type class and syntax methods package-private in future 2.x releases and remove them in 3.0, or at least put an explicit <: Throwable constraint on them (which doesn't eliminate the possibility of runtime crashes, but does make them much less likely).

[travisbrown]

This is partially addressed by #3361. In the longer run we should consider what to do with attemptNarrow and catchOnly (probably we should just add documentation about the risks, but we could also consider deprecation).

[bbjubjub2494]

I can't find any mentions of this on GitHub so, on the topic of catchOnly, it's possible to catch fatal exceptions in the sense of scala.util.control.NonFatal. To me that doesn't feel safe at all.

import cats.data.Validated

def boom = throw new OutOfMemoryError
Validated.catchOnly[Error] { boom } // => Invalid(java.lang.OutOfMemoryError): Validated[Error,Nothing]

Looking at the hierarchy, it looks like the list of problematic type parameters would be limited to fatal exceptions themselves, which is obvious, but also Exception (due toInterruptedException), Error and Throwable. Anything more specific (including RuntimeException) wouldn't be a problem afaict.

[lukoyanov]

@travisbrown please consider keeping attemptNarrow with properly documented risks.

I found this method pretty convenient when you need to temporary put your business error from F[Either[E, A]] into F[A] with rethrow and then later extract it back with attemptNarrow.

[travisbrown]

@lukoyanov The current plan I think is to keep it but with a <: Throwable constraint on E. Does that affect your use cases?

[lukoyanov]

Does that affect your use cases?

@travisbrown <: Throwable constraint on E looks reasonable, perfectly works for me.

[taig]

I just got bitten by this method in conjunction with Scala 3's union types:

import cats.effect.IOApp
import cats.effect.IO
import cats.syntax.all.*

final case class A() extends Throwable
final case class B() extends Throwable
final case class C() extends Throwable

object Reproducer extends IOApp.Simple:
  override def run: IO[Unit] =
    IO.raiseError(C())
      .attemptNarrow[A | B]
      .map:
        case Right(_)  => IO.unit
        case Left(A()) => IO.unit
        case Left(B()) => IO.unit
      .void

scala.MatchError: Left(app.C) (of class scala.util.Left)