-
Notifications
You must be signed in to change notification settings - Fork 1.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
Align erasure of Array[Nothing]
and Array[Null]
with Scala 2
#22517
Conversation
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.
Instead of doing a new scripted test, can you include the test code in https://github.com/scala/scala3/tree/main/sbt-test/scala2-compat/erasure ?
(also the test could include |
def objectARRAY_93(x: Array[_ <: Nothing]): Unit = {} | ||
def objectARRAY_94(x: Array[_ <: Null]): Unit = {} |
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.
This should actually be:
def objectARRAY_93(x: Array[_ <: Nothing]): Unit = {} | |
def objectARRAY_94(x: Array[_ <: Null]): Unit = {} | |
def nothing$ARRAY_93(x: Array[_ <: Nothing]): Unit = {} | |
def null$ARRAY_94(x: Array[_ <: Null]): Unit = {} |
but for some reason, it passes the test locally, I will have to test it better, but my machine keeps crashing... so checking with the CI :)
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.
So this is weird, the CI passes while Array[_ <: Nothing]
erasure is wrong. I've tested with the following:
// In Scala 2
class Foo {
def a: Array[Nothing] = null
def b: Array[Null] = null
def c: Array[_ <: Nothing] = null
def s: Array[_ <: Null] = null
}
// In Scala 3
@main def Test =
(new Foo).a
(new Foo).b
(new Foo).c
(new Foo).s
The generated bytecode from Scala 2 is the following:
Compiled from "s2.scala"
public class Foo {
public java.lang.Object[] a();
public java.lang.Object[] b();
public scala.runtime.Nothing$[] c();
public scala.runtime.Null$[] s();
public Foo();
}
We can clearly see Scala 3 (this PR) emitting a call to the wrong method for Foo.c
:
public final class s3$package$ implements java.io.Serializable {
public static final s3$package$ MODULE$;
public static {};
Code:
0: new #2 // class s3$package$
3: dup
4: invokespecial #16 // Method "<init>":()V
7: putstatic #18 // Field MODULE$:Ls3$package$;
10: return
public scala.runtime.Null$[] Test();
Code:
0: new #29 // class Foo
3: dup
4: invokespecial #30 // Method Foo."<init>":()V
7: invokevirtual #34 // Method Foo.a:()[Ljava/lang/Object;
10: pop
11: new #29 // class Foo
14: dup
15: invokespecial #30 // Method Foo."<init>":()V
18: invokevirtual #37 // Method Foo.b:()[Ljava/lang/Object;
21: pop
22: new #29 // class Foo
25: dup
26: invokespecial #30 // Method Foo."<init>":()V
29: invokevirtual #40 // Method Foo.c:()Ljava/lang/Object;
32: pop
33: new #29 // class Foo
36: dup
37: invokespecial #30 // Method Foo."<init>":()V
40: invokevirtual #43 // Method Foo.s:()[Lscala/runtime/Null$;
43: areturn
}
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've checked a bit and throwing a LinkageError
will not be considered a failure... but the problem is that there is a linkage error before the code I've added (I still need to find which one).
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.
There is already an issue opened on sbt (sbt/sbt#6852). The proposed workaround (fork := true
) worked locally
@@ -53,10 +53,10 @@ object Main { | |||
z.c_40(dummy) | |||
z.c_41(dummy) | |||
z.c_42(dummy) | |||
z.b_43(dummy) | |||
//z.b_43(dummy) |
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.
TODO: Inspect why both these methods' erasures are wrong, or why were they considered correct.
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.
@@ -1,3 +1,5 @@ | |||
ThisBuild / fork := true |
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.
Workaround until #22524 is resolved
I forgot it was approved... I have the reflex to enable auto-merge when I'm done with a PR (Sorry...) |
No worries, seems fine to me as is. |
I think this should require minor @hamzaremmal ? Just for the sake of backporting to LTS since we are at 3.7.0 anyway |
Not necessarily. It's a bug fix to be able to call certain Scala 2 methods that would previously throw |
It's a bug fix, it shouldn't require a minor release |
That's what I thought, but wanted to be sure. Thanks! |
Closes #22515