cats icon indicating copy to clipboard operation
cats copied to clipboard

`Either.catchOnly` and `-Yexplicit-nulls` are not playing well together

Open joan38 opened this issue 1 year ago • 8 comments

Hi,

Either.catchOnly does not seem usable with -Yexplicit-nulls enabled on Scala 3.5.2:

[error] -- [E057] Type Mismatch Error: /.../CaseAppParsers.scala:18:17 
[error] 18 |      .catchOnly[NumberFormatException](Duration(s))
[error]    |                 ^
[error]    |Type argument NumberFormatException does not conform to lower bound Null
[error]    |
[error]    | longer explanation available when compiling with `-explain`
[error] two errors found

Is there something that can be done at the library level?

Thanks

joan38 avatar Nov 23 '24 23:11 joan38

looks like the type signature requires it?

https://github.com/typelevel/cats/blob/1cc04eca9f2bc934c869a7c5054b15f6702866fb/core/src/main/scala/cats/syntax/either.scala#L398

I don't know why that >: Null was put in there. Maybe someone else knows.

Here is the full code:

  final private[syntax] class CatchOnlyPartiallyApplied[T](private val dummy: Boolean = true) extends AnyVal {
    def apply[A](f: => A)(implicit CT: ClassTag[T], NT: NotNull[T]): Either[T, A] =
      try {
        Right(f)
      } catch {
        case t if CT.runtimeClass.isInstance(t) =>
          Left(t.asInstanceOf[T])
      }
  }
}

I have no idea why the NotNull[T] was added, but also we need >: Null. Seems like all we should require is that T <: AnyRef to be able to use runtimeClass.isInstance but that's true of any exception...

johnynek avatar Nov 23 '24 23:11 johnynek

@johnynek

I have no idea why the NotNull[T] was added, but also we need >: Null

I was wondering once too and asked pretty much the same question in Discord's scala-users.

@s5bug helped me to figure it out (thank you!).

I still feel it is not obvious from the code at all and perhaps there should be an explanation in scaladocs for that method.

satorg avatar Nov 24 '24 00:11 satorg

For posterity & archival, the >: Null bound is to prevent unhelpful inference of Nothing (i.e. in the case of catchOnly { 123 }). It ensures that a type argument is explicitly applied.

s5bug avatar Nov 24 '24 04:11 s5bug

That’s unfortunate that it breaks explicit null though.

Not sure it’s the right trade if we could get explicit null.

johnynek avatar Nov 24 '24 22:11 johnynek

There is a discussion about the catchOnly method itself in #4616. It seems this and similar methods do not exactly meet the Cats convention for methods to be "safe" and "pure" by default. Otherwise, such methods should be marked as "unsafe".

So perhaps(!) the right way to address it would be to create a new bunch of methods like catchOnlyUnsafe and such and make sure they work on both Scala2 and Scala3 with/without explicit nulls syntax. Then we could gradually deprecate and decommission the old impure ones.

satorg avatar Nov 24 '24 22:11 satorg

There is a discussion about the catchOnly method itself in #4616.

Ah yes, thanks for the reminder. I think my opinion is that catchOnly should be deprecated without replacement.

armanbilge avatar Nov 24 '24 22:11 armanbilge

Arman decided to raise the stakes 😄

satorg avatar Nov 24 '24 22:11 satorg

Interestingly I found this Scala 3 example that does the same bounds:

def apply[T >: Null](x: T): Option[T]

joan38 avatar Dec 02 '24 05:12 joan38