-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Erase Int | Nothing
signatures types into Int
#14971
Erase Int | Nothing
signatures types into Int
#14971
Conversation
As far as I can see, the I would argue that this erasure is a bug and should be fixed. If a library accidentally used this and cares about binary compatibility, they can add the bridge themselves. |
I don't think that's possible. How would they do that? It would result in an ambiguous overload, wouldn't it? |
Indeed, it is not possible 😞 |
You can avoid ambiguous overload errors by relying on import scala.annotation.targetName
class A:
def foo: Int = 1
@targetName("foo") def fooBinCompat: Any = 1 |
... but that's not good enough for library authors in general as a workaround to binary compatibility breakages, because their methods might be overrridden in user code as @sjrd pointed out last time I wanted to break binary compatibility 😬 |
Then the real question is if there are libraries that use these types in their signatures. |
@@ -365,10 +365,18 @@ object TypeErasure { | |||
def erasedLub(tp1: Type, tp2: Type)(using Context): Type = { | |||
// After erasure, C | {Null, Nothing} is just C, if C is a reference type. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this logic has gotten a bit messy and can be simplified. Why not:
if tp1.isRef(defn.NothingClass)
|| tp1.isRef(defn.NullClass) && tp2.derivesFrom(defn.ObjectClass))
then return tp2
And the same to return tp1
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would hope that A | Nothing would not normally appear in library code. The current behavior looks wrong since it violates the property that erasure(A | B) = erasure(B)
if A <: B
.
So I'd tend to go ahead with merging this after the logic is made clearer.
182847c
to
49cef31
Compare
By the way, even if this works around (some) binary-compatibility issues, it can also break tasty-compatibility since we refer to methods by their original names there. |
I believe it is not violated. With the current behavior,
Similarly for |
Apart from signatures and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Based on #14998, the only |
Breaking binary compatbility likes this sets a bad precedent. Is the current behavior really fundamentally broken or merely inconvenient? |
As far as I can see it is only fundamentally broken for the We could do an alternative fix for #14964 that would make the erased lub behave differently if we are computing the erased type for a |
Actually you don't even need ClassTag to run into issues: object Test {
def main(args: Array[String]): Unit = {
val x: Array[Int | Nothing] = Array()
val y: Array[Int] = x
}
} Exception in thread "main" java.lang.ClassCastException: [Ljava.lang.Object; cannot be cast to [I
at Test$.main(arri.scala:3)
at Test.main(arri.scala) So yeah, I guess the erasure of union types is just fundamentally broken which would justify changing it :/. If we do change it in 3.2, we could warn in 3.1.3 about any code that relies on it to ease migration. |
| Nothing
with & Any
and Nothing |
with Any &
for value typesInt | Nothing
signatures types into Int
769dc77
to
c4c4d00
Compare
This make the erasure different for value types as they now correctly erase to the primitive types instead of `Object`. Aligns | Nothing with & Any and Nothing | with Any & for value types. This also fixes `summon[ClassTag[Int | Nothing]]` as it is now equivalent to `summon[ClassTag[Int]]`. Fixes scala#14970 Fixes scala#14964 >⚠️ This is a binary breaking change. > the Previous version generated the `def f: Int` and `def f: Object` variants of the method. > In the new one, we only generate the `def f: Int` version. > Unfortunately, the previous version called the boxed version of the interface that does not exist anymore. > > Is this something that we encounter in practice?
c4c4d00
to
9cd8ce8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
this will also break backwards tasty compatibility of resolving methods by their signature |
Aligns
| Nothing
with& Any
andNothing |
withAny &
for value types.This also fixes
summon[ClassTag[Int | Nothing]]
as it is now equivalent tosummon[ClassTag[Int]]
.This applies to all value types.
Fixes #14970
Fixes #14964