Skip to content
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

Assertion failed: Trying to move a local VarDef after the super constructor call of a non-native JS class #4929

Closed
OndrejSpanel opened this issue Jan 9, 2024 · 4 comments
Assignees
Labels
bug Confirmed bug. Needs to be fixed.
Milestone

Comments

@OndrejSpanel
Copy link

OndrejSpanel commented Jan 9, 2024

When compiling my project using my own facades for magenta.ts and tone.js, I get following Scala.js assertion:

[error] Trying to move a local VarDef after the super constructor call of a non-native JS class
[error] Error while emitting Main.scala
[error] assertion failed:
[error]   Trying to move a local VarDef after the super constructor call of a non-native JS class at RangePosition(C:\Dev\scalaJsLocalVarDefAssert\src\main\scala\Main.scala, 207, 239, 277)
[error]      while compiling: C:\Dev\scalaJsLocalVarDefAssert\src\main\scala\Main.scala
[error]         during phase: jscode
[error]      library version: version 2.13.12
[error]     compiler version: version 2.13.12
[error]   reconstructed args: -bootclasspath C:\Users\Ondra\AppData\Local\Coursier\Cache\v1\https\repo1.maven.org\maven2\org\scala-lang\scala-library\2.13.12\scala-library-2.13.12.jar -classpath C:\Dev\scalaJsLocalVarDefAssert\target\scala-2.13\classes;C:\Users\Ondra\AppData\Local\Coursier\Cache\v1\https\repo1.maven.org\maven2\org\scala-js\scalajs-library_2.13\1.15.0\scalajs-library_2.13-1.15.0.jar;C:\Users\Ondra\AppData\Local\Coursier\Cache\v1\https\repo1.maven.org\maven2\org\scala-js\scalajs-scalalib_2.13\2.13.12%2B1.15.0\scalajs-scalalib_2.13-2.13.12%2B1.15.0.jar;C:\Users\Ondra\AppData\Local\Coursier\Cache\v1\https\repo1.maven.org\maven2\org\scala-js\scalajs-javalib\1.15.0\scalajs-javalib-1.15.0.jar -Xplugin:C:\Users\Ondra\AppData\Local\Coursier\Cache\v1\https\repo1.maven.org\maven2\org\scala-js\scalajs-compiler_2.13.12\1.15.0\scalajs-compiler_2.13.12-1.15.0.jar
[error]
[error]   last tree to typer: Select(Select(Ident(scala), scalajs), js)
[error]        tree position: line 6 of C:\Dev\scalaJsLocalVarDefAssert\src\main\scala\Main.scala
[error]             tree tpe: scala.scalajs.js.type
[error]               symbol: final package js in package scalajs
[error]    symbol definition: final package js (a ModuleSymbol)
[error]       symbol package: scala.scalajs
[error]        symbol owners: package js
[error]            call site: class Defined in package 
[error]
[error] == Source file context for tree position ==
[error]
[error]      3
[error]      4 @js.native
[error]      5 @JSGlobal("Native")
[error]      6 class Native(output: js.Any = js.native, callbackObject: js.Any = js.native) extends js.Any
[error]      7
[error]      8 class Defined(callbackObject: js.Any) extends Native(callbackObject = callbackObject)
[error]      9
[error] one error found```

The minimal repro code (using Scala 2.13.18 and Scala.js 1.15.0):

Note: I get no error when trying this with Scala 3.2.1 and Scala.js 1.15.

import scala.scalajs.js
import scala.scalajs.js.annotation.JSGlobal

@js.native
@JSGlobal("Native")
class Native(output: js.Any = js.native, callbackObject: js.Any = js.native) extends js.Any

class Defined(callbackObject: js.Any) extends Native(callbackObject = callbackObject)

I guess there is probably some error in my code or in the facade, but the assertion is not very helpful.

Online repro: https://scribble.ninja/u/OndrejSpanel/crxnynclgqgzlfdcltblxjpofdqy

@sjrd sjrd added the bug Confirmed bug. Needs to be fixed. label Jan 9, 2024
@gzm0
Copy link
Contributor

gzm0 commented Jan 28, 2024

I can reproduce this. Thank you for filing this issue.

The issue seems to be that genPrimaryJSClassCtor cannot deal with the following tree:

List({
  <artifact> val x$1: scala.scalajs.js.Any = callbackObject;
  <artifact> val x$2: scala.scalajs.js.Any = helloworld.this.Native.<init>$default$1();
  Defined.super.<init>(x$2, x$1)
})

(it is unwilling to move the defs across the super boundary).

@gzm0
Copy link
Contributor

gzm0 commented Jan 28, 2024

If I disable the assert that moves the VarDefs, it becomes apparent why it is needed (comment mine):

js class helloworld.Defined extends helloworld.Native {
  constructor def constructor(arg: any): any = {
    var callbackObject: any = null;
    val overload: int = {
      callbackObject = arg;
      0
    };
    super((void 0), x$1); // BAD: x$1 is not assigned here!
    val x$1: any = callbackObject;
    /*<skip>*/;
    (void 0)
  }
}

@gzm0
Copy link
Contributor

gzm0 commented Jan 28, 2024

I have a WIP fix in https://github.com/gzm0/scala-js/tree/fix-default-js-ctor-params for this. But I feel it needs to be a bit more principled. I'm not sure how much time I'll have to work on this, hence sharing my results so far.

@sjrd
Copy link
Member

sjrd commented Feb 5, 2024

Other data point: whether the parent class is native or not does not matter. It can be a non-native JS class and the same issue appears.

@sjrd sjrd added this to the v1.15.1 milestone Feb 5, 2024
sjrd added a commit to sjrd/scala-js that referenced this issue Feb 5, 2024
Previously, the logic was based on moving statements after the
constructor by default, but keep selected ones before. The list
of what to keep turned out to be too restrictive, and it was not
clear how to expand it in a principled way.

Instead, we now reverse that logic. We keep statements where they
are by default, and only move `C.this.field = ident;` statements
after the super constructor call.

This requires only a little bit of special-casing for outer pointer
null checks. Fortunately, we already some logic to identify and
decompose those, which we reuse here.
@sjrd sjrd self-assigned this Feb 5, 2024
sjrd added a commit to sjrd/scala-js that referenced this issue Feb 5, 2024
Previously, the logic was based on moving statements after the
constructor by default, but keep selected ones before. The list
of what to keep turned out to be too restrictive, and it was not
clear how to expand it in a principled way.

Instead, we now reverse that logic. We keep statements where they
are by default, and only move `C.this.field = ident;` statements
after the super constructor call.

This requires only a little bit of special-casing for outer pointer
null checks. Fortunately, we already some logic to identify and
decompose those, which we reuse here.
sjrd added a commit to sjrd/scala-js that referenced this issue Feb 5, 2024
Previously, the logic was based on moving statements after the
constructor by default, but keep selected ones before. The list
of what to keep turned out to be too restrictive, and it was not
clear how to expand it in a principled way.

Instead, we now reverse that logic. We keep statements where they
are by default, and only move `C.this.field = ident;` statements
after the super constructor call.

This requires only a little bit of special-casing for outer pointer
null checks. Fortunately, we already some logic to identify and
decompose those, which we reuse here.
sjrd added a commit to sjrd/scala-js that referenced this issue Feb 6, 2024
Previously, we moved all statements in the constructors after the
super constructor call. However, it turns out that there are
statements that must be kept before, notably local `val`s
generated for default arguments to the super constructor.

We now keep statements where they are by default. We only move
statements of the form `C.this.field = ident;`, which are the
only ones that require access to `this`.

This requires only a little bit of special-casing for outer pointer
null checks. Fortunately, we already had some logic to identify and
decompose those, which we reuse here.
@gzm0 gzm0 closed this as completed in 2e4594f Feb 11, 2024
gzm0 added a commit that referenced this issue Feb 11, 2024
Fix #4929: Fix logic for moving early assignements in JS ctors.
sjrd added a commit to sjrd/dotty that referenced this issue Apr 29, 2024
…n JS ctors.

Previously, we moved all statements in the constructors after the
super constructor call. However, it turns out that there are
statements that must be kept before, notably local `val`s
generated for default arguments to the super constructor.

We now keep statements where they are by default. We only move
statements of the form `C.this.field = ident;`, which are the
only ones that require access to `this`.

Forward port of the upstream commit
scala-js/scala-js@2e4594f
sjrd added a commit to dotty-staging/dotty that referenced this issue Apr 29, 2024
…n JS ctors.

Previously, we moved all statements in the constructors after the
super constructor call. However, it turns out that there are
statements that must be kept before, notably local `val`s
generated for default arguments to the super constructor.

We now keep statements where they are by default. We only move
statements of the form `C.this.field = ident;`, which are the
only ones that require access to `this`.

Forward port of the upstream commit
scala-js/scala-js@2e4594f
sjrd added a commit to dotty-staging/dotty that referenced this issue Apr 29, 2024
…n JS ctors.

Previously, we moved all statements in the constructors after the
super constructor call. However, it turns out that there are
statements that must be kept before, notably local `val`s
generated for default arguments to the super constructor.

We now keep statements where they are by default. We only move
statements of the form `C.this.field = ident;`, which are the
only ones that require access to `this`.

Forward port of the upstream commit
scala-js/scala-js@2e4594f
WojciechMazur pushed a commit to WojciechMazur/dotty that referenced this issue May 6, 2024
…n JS ctors.

Previously, we moved all statements in the constructors after the
super constructor call. However, it turns out that there are
statements that must be kept before, notably local `val`s
generated for default arguments to the super constructor.

We now keep statements where they are by default. We only move
statements of the form `C.this.field = ident;`, which are the
only ones that require access to `this`.

Forward port of the upstream commit
scala-js/scala-js@2e4594f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Confirmed bug. Needs to be fixed.
Projects
None yet
Development

No branches or pull requests

3 participants