typelevel / cats

Lightweight, modular, and extensible library for functional programming.
https://typelevel.org/cats/
Other
5.27k stars 1.21k forks source link

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

Open joan38 opened 1 week ago

joan38 commented 1 week ago

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

johnynek commented 1 week ago

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...

satorg commented 1 week ago

@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.

s5bug commented 1 week ago

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.

johnynek commented 1 week ago

That’s unfortunate that it breaks explicit null though.

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

satorg commented 1 week ago

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.

armanbilge commented 1 week ago

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.

satorg commented 1 week ago

Arman decided to raise the stakes 😄

joan38 commented 1 day ago

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

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