Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

-Yexplicit-nulls does not play nicely with Option.apply #22195

Open
j-mie6 opened this issue Dec 11, 2024 · 8 comments
Open

-Yexplicit-nulls does not play nicely with Option.apply #22195

j-mie6 opened this issue Dec 11, 2024 · 8 comments

Comments

@j-mie6
Copy link

j-mie6 commented Dec 11, 2024

Compiler version

3.5.2

Minimized example

//> using options -Yexplicit-nulls

class Foo
Option[Foo](null)

Output

Gives a type error that expected Foo but got Null. Fine, except the point of Option.apply is that it is a null-safe wrapper into Option.

Expectation

Realistically, its type needs to be def apply[A](x: A | Null): Option[A] when that flag is turned on, otherwise it's now useless.

@j-mie6 j-mie6 added the stat:needs triage Every issue needs to have an "area" and "itype" label label Dec 11, 2024
@Gedochao Gedochao added itype:enhancement area:nullability and removed stat:needs triage Every issue needs to have an "area" and "itype" label labels Dec 11, 2024
@Gedochao
Copy link
Contributor

cc @noti0na1

@noti0na1
Copy link
Member

Since we couldn't update the std library right now, there is a "patch" function for Option defined at library/src/scala/runtime/stdLibPatches/Predef.scala

  extension (opt: Option.type)
    @experimental
    inline def fromNullable[T](t: T | Null): Option[T] = Option(t).asInstanceOf[Option[T]]

@Gedochao
Copy link
Contributor

@noti0na1 potentially could be improved after #22043

@olhotak
Copy link
Contributor

olhotak commented Dec 12, 2024

There was a discussion somewhere where @sjrd provided convincing reasons why Option.apply is the wrong name for a method that converts a value of type T|Null to a value of type Option[T], and that any potential future update to the library should add Option.fromNullable instead of changing the type of Option.apply. I can't find that discussion now, but if anyone remembers where it was, it would be good to link it here so we can find it in the future.

@sjrd
Copy link
Member

sjrd commented Dec 12, 2024

The gist is: List(null) creates a 1-element list where the element is null. It doesn't return Nil. The same is true for every other apply method of "collection-like" things. Option(null) is the odd one. It should return Some(null), not None.

@j-mie6
Copy link
Author

j-mie6 commented Dec 12, 2024

Well, yes, though Option's job is to subsume null, so it's not unexpected by any means. At least there is a fromNullable, I suppose

@jducoeur
Copy link
Contributor

I can see the reasoning, although this one's a big deal due to interop. So long as there's a Scalafix to do the rewrite (which seems doable) it's likely okay, but this change needs to be shouted from the rooftops: the use of Option(...) to wrap null-returning Java functions is one of those things that often gets taught early (certainly I do so when teaching folks doing any sort of interop), so it's going to take a lot of habit-changing, and I think it's going to touch a lot of code.

@sjrd
Copy link
Member

sjrd commented Dec 12, 2024

I mean we're never actually going to change the behavior of Option(null). At worst it's going to get deprecated in favor of fromNullable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants