Better support for nullability in Option.apply and drop Option.fromNullable

[hamzaremmal]

Yes, this is not the most ideal signature for Option.apply but it allows at least to drop Option.fromNullable and an easier migration to explicit nulls (no need to add fromNullable everywhere).

Superseed #24231 in the sense where we focus here on the nullability issue rather than the more global issue.

Reverts #24230
Relate to #24206

[hamzaremmal]

This was weird because it shouldn't have an error when we migrated the stdlib in #23566 and it should have become an error again in #24230 but none of the diffs mention a change here.

/cc @noti0na1

[hamzaremmal]

I removed it because it is useless now that we have Option.apply(A | Null)

[noti0na1]

I'm not sure about the conclusion in yesterday's core meeting. We need a clear answer now:

1. Suppose we fix the inference issue, can we keep Option.apply nullable in 3.8? even if we know user code with macro will break? If not, when can we introduce the change and "break" the compatibility?
2. Can we remove fromNullable and update Option.apply at the same time in 3.8? If not, when can we remove an experimental definition?

@Gedochao @hamzaremmal @sjrd @olhotak

[hamzaremmal]

So from the notes of yesterday's core meeting:

This should be reverted in 3.8.0 and we can add | Null` in the future, when the inference is fixed

As far as I'm concerned, this PR fixes inference with | Null and covers the issue for Option.apply.

[noti0na1]

Indeed, but I'm still confused by the conclusion. I want a clear answer whether breaking the macro code is considered breaking compatibility.

[tgodzik]

As far as I'm concerned, this PR fixes inference with | Null and covers the issue for Option.apply

If we indeed have the inference fixed then there is probably no need to revert it

Indeed, but I'm still confused by the conclusion. I want a clear answer whether breaking the macro code is considered breaking compatibility.

We haven't discussed issues with macros, but breaking macros is something that should be avoided.

[hamzaremmal]

Indeed, but I'm still confused by the conclusion. I want a clear answer whether breaking the macro code is considered breaking compatibility.

In this case, we can never add | Null anywhere because I can always build a macro that destructs an expression where we have T | Null. It just turned out that one project destructed Option.apply. If we were to keep the revert only because of the macro, then we should revert all the | Null that were added in a public method.

[WojciechMazur]

Regarding the macro case the issue is source code was trivial to fix and it becomes backward compatible with 3.7/2.13 stdlib - kitlangton/neotype#355

The only thing I haven't tested was usage in existing code, if it will still work then it should be fine to include these changes.

[noti0na1]

I'm not against keeping the apply nullable, as long as everyone agrees on the changes. The meeting document was not clear about the two issues.

[WojciechMazur]

The meeting document was not clear about the two issues.

Neither was the discussion.
One main point was regarding to TASTy and making it easier to adjust for nullability in the future, then remove it if needed. For the concussion was searching for best approach that we can get in short amount of time and that won't bite us in the future again. In that case having Option nullable makes sense to me as it won't clash with remaining parts of the stdlib, otherwise it would be confusing to me. Revert was the safest option, ensuring a full compatibility.
As long as non-macro code works fine without any changes I'd vote to make Option.apply nullable.

[sjrd]

I have no idea what the typer change does (and what it doesn't do).

For the rest, seems worth it if it works.

[hamzaremmal]

I have no idea what the typer change does (and what it doesn't do)

It behaves exactly as it behaved before, it just removes now | Null as if we only have T.

The PR that improves this into a more general design is #24231 but that one needs more decisions and design of rules.