OptionIdOps.some should handle nulls correctly

[Ivan091]

Considering #871, I’ve been really surprised to find that OptionIdOps.some actually allows constructing Some(null) with no issues. You can write:

val s: String = null
val o = s.some // is Some(null)

However, should we allow users to do that? It’s a really bad practice to construct Some(null), as this defeats the purpose of Option and breaks a lot of code. People use Cats as an example for their own projects, and, I think, we should not promote allowing the construction of Some(null).

I propose that we deprecate OptionIdOps.some and introduce OptionIdOps.option as a safe alternative:

def option: Option[A] = Option(a)

Alternatively, we could roll back OptionIdOps.some to its original null-safe state.

[satorg]

The primary purpose of x.some is to improve type inferrence in certain cases:

import cats.syntax.option.*

def fc[A](a0: A, as: A*)(f: A => Boolean)(c: (A, A) => A): Option[A] =
  as.foldLeft(Some(a0): Option[A]): // `Some(a0)` requires type ascription
    case (Some(aa), a) => if f(a) then Some(c(aa, a)) else None
    case (None, _)     => None

def fc2[A](a0: A, as: A*)(f: A => Boolean)(c: (A, A) => A): Option[A] =
  as.foldLeft(a0.some): // no type ascription is necessary
    case (Some(aa), a) => if f(a) then Some(c(aa, a)) else None
    case (None, _)     => None

Otherwise x.some is supposed to be equivalent to Some(x).

Perhaps, it wouldn't hurt if Cats had the x.option syntax too, but it would be a sytnax for the sake of syntax only, since Option(x) does the type inferrence just as well.

[Ivan091]

Hmm, applying the same logic .some is syntax for syntax as well, since one could rewrite you first example like the following:

  def fc[A](a0: A, as: A*)(f: A => Boolean)(c: (A, A) => A): Option[A] = {
    as.foldLeft(Option(a0)) { // `Option.apply()` fixes the issue
      case (Some(aa), a) => if (f(a)) Some(c(aa, a)) else None
      case (None, _) => None
    }
  }

Sorry, I'm used to the old syntax :).

But, it was not my point. As for me, to be really pragmatic you should not use Some.apply() at all:

  def fc[A](a0: A, as: A*)(f: A => Boolean)(c: (A, A) => A): Option[A] = {
    as.foldLeft(Option(a0)) {
      case (Some(aa), a) => if (f(a)) Option(c(aa, a)) else Option.empty
      case (None, _) => Option.empty
    }
  }

If someone uses .some instead of Option.apply() they lose null safety and might get Some(null) in rare cases. Due to that, the .some seems not safe and rather useful in tests or for some constants than in prod ready code.

[satorg]

x.some is as safe as Some(x) is but provides better type inference in certain scenarios – that's its only purpose. Option(x) works too, of course, but contains additional check for null inside, which is not required in many cases.

If you are concerned about null safety, I'd rather suggest to stick to Option(x). If someone uses either Some(x) or x.some, they get what they asked – x value wrapped into Some, even if x happens to be null.

The principle of least surprise mentioned in #871, seems making sense to me here – there's nothing in x.some syntax that would suggest that it should treat nulls differently from Some(x).

[ritschwumm]

to be honest, i'd find it very surprising if .some did not generically create a Some out of absolutely everything i throw at it, but for some reason behaves differently for Null. i don't even want to know how this interacts with explicit and implicit nulls...

it might be a good idea to add another method to OptionIdOps which mirrors the behaviour of Option.apply (which, frankly, i'd very much prefer to have a proper, greppable name).