change lower bound of Option.orNull to nullable#25733
Conversation
MiMa says the change is binary compatible. Do you see any source compatibility concerns here? We're not forbidden from breaking source compatibility, especially under a |
|
I have taken the liberty of milestoning this for 3.9. My reasoning is that offhand, this looks to me like a pretty basic usability issue for |
|
I don't see any source compatibility concerns here. If any source instantiates On the contrary, this fixes source compatibility for several of the Open Community Build projects with #25461 (which would turn |
|
I just realized this is not a backward TASTy compatible change. I thought the def foo[A, B >: A](x: Option[A])(using Null <:< B): B =
x.orNullthe call was previously well typed as |
|
I think an alternative would be to hide this one as @inline final def orNull: A | Null = this.getOrElse(null)
// binary and TASTy compat
@inline protected final def orNull[A1 >: A](implicit ev: Null <:< A1): A1 = this getOrElse ev(null) |
Why does adding a |
Because it's public in bytecode, and TASTy does not check accessibility. |
|
|
The existing I've changed it to non-final protected and added an extra explicit @sjrd could you please confirm that this is OK and review the PR in general? |
| def get: Nothing = throw new NoSuchElementException("None.get") | ||
|
|
||
| // for binary and TASTy backwards compatibility | ||
| @deprecated @inline override def orNull[A1](implicit ev: Null <:< A1): A1 = null.asInstanceOf[A1] |
There was a problem hiding this comment.
This is not good. It has at least two issues:
- It creates two visible overloads of
orNullonNone, which are only distinguished by an implicit parameter list. - It turns dispatch on
Option.orNullinto a bimorphic call, where it was monomorphic before.
I don't think this is an acceptable change.
The static forwarder for None.orNull has approximately 0% chance of being used in the wild. It would have to be called from Java, explicitly passing the evidence of <:<, all to get null. I think we can destroy it.
There was a problem hiding this comment.
I went back to the version that adds the backwards-compatibility method only to Option, not to None. I also put in an override for the Mima error on None to get the CI to pass. Is that OK?
sjrd
left a comment
There was a problem hiding this comment.
LGTM from me, other than the misleading placement of the MiMa filter.
I will put it up for discussion at the core meeting tomorrow, to be sure.
|
No objections at core meeting today. (I can imagine that in the future, some similar case might motivate us to build a mechanism to emit a legacy static forwarder when one is needed. But this case doesn't seem nearly important enough to motivate us to build that mechanism now.) |
This PR changes the signature of `Option.orNull` from ```scala @inline final def orNull[A1 >: A](implicit ev: Null <:< A1): A1 = this getOrElse ev(null) ``` to ```scala @inline final def orNull: A | Null = this getOrElse ev(null) ``` Under `-Yexplicit-nulls`, type inference sometimes infers `A1` to be a non-null type, leading to a failure to find the implicit `ev`. The new result type is more straightforward for type inference. Without `-Yexplicit-nulls`, `A | Null` is also correct. When `A` is a nullable type, `A | Null` is equivalent to `A`.
|
FYI, this causes a regression for ~10 projects in the OpenCB: #26026 |
This PR changes the signature of
Option.orNullfromto
Under
-Yexplicit-nulls, type inference sometimes infersA1to be a non-null type, leading to a failure to find the implicitev. The new result type is more straightforward for type inference.Without
-Yexplicit-nulls,A | Nullis also correct. WhenAis a nullable type,A | Nullis equivalent toA.How much have you relied on LLM-based tools in this contribution?
No.
How was the solution tested?
Added test
option-ornullminimized from akka-management.Additional notes
This may have to wait until a version when we can start making changes to standard library signatures.