8000 Rearrange the init sequence of PrimRefs and PrimTypeWithRefs. by sjrd · Pull Request #5139 · scala-js/scala-js · GitHub
[go: up one dir, main page]

Skip to content

Rearrange the init sequence of PrimRefs and PrimTypeWithRefs. #5139

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

Merged
merged 1 commit into from
Mar 15, 2025

Conversation

sjrd
Copy link
Member
@sjrd sjrd commented Mar 10, 2025

So that it is more obvious that it is not circular.


The second commit is meant to address the comments that were originally reported in #5136.

@sjrd sjrd requested a review from gzm0 March 10, 2025 10:53
@sjrd sjrd force-pushed the rearrange-init-sequence-of-primref branch from 2afe56d to d6f6725 Compare March 14, 2025 19:54
@sjrd sjrd requested a review from gzm0 March 14, 2025 19:55
@gzm0
Copy link
Contributor
gzm0 commented Mar 15, 2025

There seem to also be these mima problems:

[error]  * method productElementNames()scala.collection.Iterator in class org.scalajs.ir.Types#PrimRef does not have a correspondent in current version

[error]    filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("org.scalajs.ir.Types#PrimRef.productElementNames")

[error]  * method productElementName(Int)java.lang.String in class org.scalajs.ir.Types#PrimRef does not have a correspondent in current version

[error]    filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("org.scalajs.ir.Types#PrimRef.productElementName")

Also, the stability check fails. Did we rely on equals from the case class?

Copy link
Contributor
@gzm0 gzm0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this much more.

However, it seems the CI is failing:

  • Some additional mima issues on 2.13.x (not a problem)
  • Stability test seems to fail 😨

@sjrd
Copy link
Member Author
sjrd commented Mar 15, 2025

FTR this was the stability diff:

$ diff -u test-suite/target/test-suite-stability.js test-suite/js/.2.12/target/scala-2.12/scalajs-test-suite-test-fastopt/main.js
--- test-suite/target/test-suite-stability.js   2025-03-15 08:54:16.533595600 +0100
+++ test-suite/js/.2.12/target/scala-2.12/scalajs-test-suite-test-fastopt/main.js       2025-03-15 08:54:33.831557800 +0100
@@ -402525,9 +402525,9 @@
 $c_scm_ArrayOps$ofBoolean.prototype.repr__O = (function() {
   return this.scm_ArrayOps$ofBoolean__f_repr;
 });
-$c_scm_ArrayOps$ofBoolean.prototype.apply__I__ = (function(index) {
+$c_scm_ArrayOps$ofBoolean.prototype.apply__I__ = (function(idx) {
   var this$ = this.scm_ArrayOps$ofBoolean__f_repr;
-  return $n(this$).get(index);
+  return $n(this$).get(idx);
 });
 $c_scm_ArrayOps$ofBoolean.prototype.clone__ = (function() {
   if ($is_jl_Cloneable(this)) {
@@ -402758,9 +402758,9 @@
 $c_scm_ArrayOps$ofByte.prototype.repr__O = (function() {
   return this.scm_ArrayOps$ofByte__f_repr;
 });
-$c_scm_ArrayOps$ofByte.prototype.apply__I__ = (function(index) {
+$c_scm_ArrayOps$ofByte.prototype.apply__I__ = (function(idx) {
   var this$ = this.scm_ArrayOps$ofByte__f_repr;
-  return $n(this$).get(index);
+  return $n(this$).get(idx);
 });
 $c_scm_ArrayOps$ofByte.prototype.clone__ = (function() {
   if ($is_jl_Cloneable(this)) {
@@ -403224,9 +403224,9 @@
 $c_scm_ArrayOps$ofDouble.prototype.repr__O = (function() {
   return this.scm_ArrayOps$ofDouble__f_repr;
 });
-$c_scm_ArrayOps$ofDouble.prototype.apply__I__ = (function(index) {
+$c_scm_ArrayOps$ofDouble.prototype.apply__I__ = (function(idx) {
   var this$ = this.scm_ArrayOps$ofDouble__f_repr;
-  return $n(this$).get(index);
+  return $n(this$).get(idx);
 });
 $c_scm_ArrayOps$ofDouble.prototype.clone__ = (function() {
   if ($is_jl_Cloneable(this)) {
@@ -403457,9 +403457,9 @@
 $c_scm_ArrayOps$ofFloat.prototype.repr__O = (function() {
   return this.scm_ArrayOps$ofFloat__f_repr;
 });
-$c_scm_ArrayOps$ofFloat.prototype.apply__I__ = (function(index) {
+$c_scm_ArrayOps$ofFloat.prototype.apply__I__ = (function(idx) {
   var this$ = this.scm_ArrayOps$ofFloat__f_repr;
-  return $n(this$).get(index);
+  return $n(this$).get(idx);
 });
 $c_scm_ArrayOps$ofFloat.prototype.clone__ = (function() {
   if ($is_jl_Cloneable(this)) {
@@ -403690,9 +403690,9 @@
 $c_scm_ArrayOps$ofInt.prototype.repr__O = (function() {
   return this.scm_ArrayOps$ofInt__f_repr;
 });
-$c_scm_ArrayOps$ofInt.prototype.apply__I__ = (function(index) {
+$c_scm_ArrayOps$ofInt.prototype.apply__I__ = (function(idx) {
   var this$ = this.scm_ArrayOps$ofInt__f_repr;
-  return $n(this$).get(index);
+  return $n(this$).get(idx);
 });
 $c_scm_ArrayOps$ofInt.prototype.clone__ = (function() {
   if ($is_jl_Cloneable(this)) {
@@ -403926,9 +403926,9 @@
 $c_scm_ArrayOps$ofLong.prototype.repr__O = (function() {
   return this.scm_ArrayOps$ofLong__f_repr;
 });
-$c_scm_ArrayOps$ofLong.prototype.apply__I__ = (function(index) {
+$c_scm_ArrayOps$ofLong.prototype.apply__I__ = (function(idx) {
   var this$ = this.scm_ArrayOps$ofLong__f_repr;
-  return $n(this$).get(index);
+  return $n(this$).get(idx);
 });
 $c_scm_ArrayOps$ofLong.prototype.clone__ = (function() {
   if ($is_jl_Cloneable(this)) {
@@ -404629,9 +404629,9 @@
 $c_scm_ArrayOps$ofUnit.prototype.repr__O = (function() {
   return this.scm_ArrayOps$ofUnit__f_repr;
 });
-$c_scm_ArrayOps$ofUnit.prototype.apply__I__ = (function(index) {
+$c_scm_ArrayOps$ofUnit.prototype.apply__I__ = (function(idx) {
   var this$ = this.scm_ArrayOps$ofUnit__f_repr;
-  $n(this$).get(index);
+  $n(this$).get(idx);
 });
 $c_scm_ArrayOps$ofUnit.prototype.clone__ = (function() {
   if ($is_jl_Cloneable(this)) {

So it looks to be confined into reflective proxies. It seems they don't have a stable way to choose the parameter names. Giving a stable hash code to PrimRef "fixes" the issue.


Edit, more info: in the IR of ArrayOps$ofByte, we find:

  def apply;I;B(index: int): byte = {
    mod:scala.collection.mutable.ArrayOps$ofByte$.apply$extension;[B;I;B(this.repr;[B(), index)
  }
  def apply;I;Ljava.lang.Object(idx: int): any = {
    mod:scala.collection.mutable.ArrayOps$ofByte$.apply$extension;[B;I;B(this.repr;[B(), idx)
  }

The second one is a bridge. Normally we use a "more specific" test to deterministical 8000 ly choose an overload for the reflective proxy target. Here there is a primitive, and our specificity test does not consider either more specific than the other (this is probably wrong?). So it falls back to the "pick any" criteria of the spec:

/* From the JavaDoc of java.lang.Class.getMethod:
*
* If more than one [candidate] method is found in C, and one of these
* methods has a return type that is more specific than any of the
* others, that method is reflected; otherwise one of the methods is
* chosen arbitrarily.
*/

I guess our "choose arbitrarily" is also "non-deterministically" (unless TypeRefs and therefore MethodNames have stable hash codes)

@sjrd sjrd force-pushed the rearrange-init-sequence-of-primref branch from d6f6725 to 5ab874d Compare March 15, 2025 08:04
@sjrd sjrd requested a review from gzm0 March 15, 2025 08:11
So that it is more obvious that it is not circular.

`PrimRef` is not a case class anymore.
@sjrd sjrd force-pushed the rearrange-init-sequence-of-primref branch from 5ab874d to b1eabac Compare March 15, 2025 09:42
@sjrd
Copy link
Member Author
sjrd commented Mar 15, 2025

Squashed.

@gzm0 gzm0 merged commit e7b4f69 into scala-js:main Mar 15, 2025
3 checks passed
@sjrd sjrd deleted the rearrange-init-sequence-of-primref branch March 15, 2025 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants
0