8000 Fix #1811: Add support for secondary constructors in JS classes by nicolasstucki · Pull Request #1982 · scala-js/scala-js · GitHub
[go: up one dir, main page]

Skip to content

Fix #1811: Add support for secondary constructors in JS classes #1982

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

Conversation

nicolasstucki
Copy link
Contributor

No description provided.

@nicolasstucki
Copy link
Contributor Author

Review by @sjrd.

@scala-jenkins
Copy link

Refer to this link for build results (access rights to CI server needed):

https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/2361/
https://scala-webapps.epfl.ch/jenkins/job/scalajs-matrix-build/3061/
Test FAILed.


// Boldly get rid of everything in infoBuilder, because it's broken
currentClassInfoBuilder.addMethod(
ir.Infos.generateMethodInfo(newConstructor))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why / what is it broken? How hard is it to fix it?

Copy link
Member

Choose a reason for hiding this comment

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

It's the fact that the secondary constructors call other constructors with ApplyStatically. While their tree is built, these calls are registered to the info builder. But these called constructors are afterwards removed, because their bodies are mangled into an only JS constructor, and hence the info builder says that it calls methods that do not, in fact, exist.

I don't know how hard exactly it would be to fix it, but it would be hacky in any case.

I think we should simply get rid of these InfoBuilders altogether, for all of GenJSCode. Instead, we can very simply generate the ClassInfo from the ClassDef.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we get rid of the InfoBuilders first? It seems to be a rather easy change.

Copy link
Member

Choose a reason for hiding this comment

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

I guess we could, yes, sure.

@nicolasstucki nicolasstucki force-pushed the support-secondary-contructors-in-JS-classes branch from 4050518 to a6ab08a Compare October 29, 2015 16:22
@@ -88,18 +88,18 @@
'set scalaJSStage in Global := FullOptStage' \
++$scala testSuite/test &&
sbtretry 'set Seq(testSuite, noIrCheckTest).map(pr => scalaJSOutputMode in pr := org.scalajs.core.tools.javascript.OutputMode.ECMAScript6)' \
'set Seq(testSuite, noIrCheckTest).map(pr => postLinkJSEnv in pr := NodeJSEnv(executable = "/home/jenkins/apps/iojs-2.0/bin/iojs", args = Seq("--harmony-rest-parameters")).value.withSourceMap(false))' \
'set Seq(testSuite, noIrCheckTest).map(pr => postLinkJSEnv in pr := NodeJSEnv(executable = "/home/jenkins/apps/node/bin/node", args = Seq("--harmony-rest-parameters")).value.withSourceMap(false))' \
Copy link
Member

Choose a reason for hiding this comment

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

We should probably first have a PR dedicated to only removing executable = of all the iojs stuff. Then we can rebase this PR on top of that.

@scala-jenkins
Copy link

Refer to this link for build results (access rights to CI server needed):

https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/2368/
https://scala-webapps.epfl.ch/jenkins/job/scalajs-matrix-build/3069/
Test FAILed.

@nicolasstucki nicolasstucki force-pushed the support-secondary-contructors-in-JS-classes branch from a6ab08a to cc6ef16 Compare October 30, 2015 12:28
lazy val isRepeatedUncurry = alt.paramss.flatten.map(isRepeated)

def paramTypesPosterasure = enteringPhase(currentRun.posterasurePhase) {
alt.paramss.flatten.apply(paramIndex).tpe
Copy link
Member

Choose a reason for hiding this comment

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

Instead of alt.paramss.flatten, use alt.params. There are a few like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

alt.params does not seem to exist. I will at least factor it out.

Copy link
Member

Choose a reason for hiding this comment

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

Ah maybe it's alt.tpe.params

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That one exists but makes the compiler crash. I looks like those parameters are not the ones in enteringPhase(currentRun.uncurryPhase).

Copy link
Member

Choose a reason for hiding this comment

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

scalac mystery ... Never mind, then.

@sjrd
Copy link
Member
sjrd commented Jan 8, 2016

That's all, I think.

@nicolasstucki nicolasstucki force-pushed the support-secondary-contructors-in-JS-classes branch from 3599292 to 16e8dc5 Compare January 8, 2016 11:08
8000
if ir.Definitions.isConstructorName(mtd.name) =>
Nil

/* Dispatch to constructor with no arguments
Copy link
Member

Choose a reason for hiding this comment

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

at least one argument?

@sjrd
Copy link
Member
sjrd commented Jan 8, 2016

That's all.

@scala-jenkins
Copy link

Refer to this link for build results (access rights to CI server needed):

https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/2703/
https://scala-webapps.epfl.ch/jenkins/job/scalajs-matrix-build/3490/
Test PASSed.

@nicolasstucki nicolasstucki force-pushed the support-secondary-contructors-in-JS-classes branch from 16e8dc5 to c9d60c7 Compare January 11, 2016 08:47
@scala-jenkins
Copy link

Refer to this link for build results (access rights to CI server needed):

https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/2716/
https://scala-webapps.epfl.ch/jenkins/job/scalajs-matrix-build/3507/
Test PASSed.

@sjrd
Copy link
Member
sjrd commented Jan 11, 2016

LGTM :)

sjrd added a commit that referenced this pull request Jan 11, 2016
…tors-in-JS-classes

Fix #1811: Add support for secondary constructors in JS classes
@sjrd sjrd merged commit 7eda38c into scala-js:master Jan 11, 2016
@nicolasstucki nicolasstucki deleted the support-secondary-contructors-in-JS-classes branch January 11, 2016 15:39
sjrd added a commit to dotty-staging/dotty that referenced this pull request Sep 18, 2020
This commit implements the entire specification of non-native JS
classes, including nested ones, except the behavior of *anonymous*
JS classes. Anonymous JS classes are not supposed to have their own
class/prototype, but currently they still do.

The most important PRs to scala-js/scala-js that define what is
implemented here were:

1. scala-js/scala-js#1809
   Essentials of Scala.js-defined classes (former name of
   non-native JS classes)
2. scala-js/scala-js#1982
   Support for secondary constructors
3. scala-js/scala-js#2186
   Support for default parameters in constructors
4. scala-js/scala-js#2659
   Support for `js.Symbol`s in `@JSName` annotations
5. scala-js/scala-js#3161
   Nested JS classes

However, this commit was written more based on the state of things
at Scala.js v1.2.0 rather than as a port of the abovementioned PRs.

The support is spread over 4 components:

* The `ExplicitJSClasses` phase, which reifies all nested JS
  classes, and has extensive documentation in the code.
* The `JSExportsGen` component of the back-end, which creates
  dispatchers for run-time overloading, default parameters and
  variadic arguments (equivalent to `GenJSExports` in Scala 2).
* The `JSConstructorGen` component, which massages the constructors
  of JS classes and their dispatcher into a unique JS constructor.
* Bits and pieces in `JSCodeGen`, notably to generate the
  `js.ClassDef`s for non-native JS classes, orchestrate the two
  other back-end components, and to adapt calls to the members of
  non-native JS classes.

`JSConstructorGen` in particular is copied quasi-verbatim from
pieces of `GenJSCode` in Scala 2, since it works on `js.IRNode`s,
without knowledge of scalac/dotc data structures.
sjrd added a commit to dotty-staging/dotty that referenced this pull request Sep 18, 2020
This commit implements the entire specification of non-native JS
classes, including nested ones, except the behavior of *anonymous*
JS classes. Anonymous JS classes are not supposed to have their own
class/prototype, but currently they still do.

The most important PRs to scala-js/scala-js that define what is
implemented here were:

1. scala-js/scala-js#1809
   Essentials of Scala.js-defined classes (former name of
   non-native JS classes)
2. scala-js/scala-js#1982
   Support for secondary constructors
3. scala-js/scala-js#2186
   Support for default parameters in constructors
4. scala-js/scala-js#2659
   Support for `js.Symbol`s in `@JSName` annotations
5. scala-js/scala-js#3161
   Nested JS classes

However, this commit was written more based on the state of things
at Scala.js v1.2.0 rather than as a port of the abovementioned PRs.

The support is spread over 4 components:

* The `ExplicitJSClasses` phase, which reifies all nested JS
  classes, and has extensive documentation in the code.
* The `JSExportsGen` component of the back-end, which creates
  dispatchers for run-time overloading, default parameters and
  variadic arguments (equivalent to `GenJSExports` in Scala 2).
* The `JSConstructorGen` component, which massages the constructors
  of JS classes and their dispatcher into a unique JS constructor.
* Bits and pieces in `JSCodeGen`, notably to generate the
  `js.ClassDef`s for non-native JS classes, orchestrate the two
  other back-end components, and to adapt calls to the members of
  non-native JS classes.

`JSConstructorGen` in particular is copied quasi-verbatim from
pieces of `GenJSCode` in Scala 2, since it works on `js.IRNode`s,
without knowledge of scalac/dotc data structures.
sjrd added a commit to dotty-staging/dotty that referenced this pull request Sep 24, 2020
This commit implements the entire specification of non-native JS
classes, including nested ones, except the behavior of *anonymous*
JS classes. Anonymous JS classes are not supposed to have their own
class/prototype, but currently they still do.

The most important PRs to scala-js/scala-js that define what is
implemented here were:

1. scala-js/scala-js#1809
   Essentials of Scala.js-defined classes (former name of
   non-native JS classes)
2. scala-js/scala-js#1982
   Support for secondary constructors
3. scala-js/scala-js#2186
   Support for default parameters in constructors
4. scala-js/scala-js#2659
   Support for `js.Symbol`s in `@JSName` annotations
5. scala-js/scala-js#3161
   Nested JS classes

However, this commit was written more based on the state of things
at Scala.js v1.2.0 rather than as a port of the abovementioned PRs.

The support is spread over 4 components:

* The `ExplicitJSClasses` phase, which reifies all nested JS
  classes, and has extensive documentation in the code.
* The `JSExportsGen` component of the back-end, which creates
  dispatchers for run-time overloading, default parameters and
  variadic arguments (equivalent to `GenJSExports` in Scala 2).
* The `JSConstructorGen` component, which massages the constructors
  of JS classes and their dispatcher into a unique JS constructor.
* Bits and pieces in `JSCodeGen`, notably to generate the
  `js.ClassDef`s for non-native JS classes, orchestrate the two
  other back-end components, and to adapt calls to the members of
  non-native JS classes.

`JSConstructorGen` in particular is copied quasi-verbatim from
pieces of `GenJSCode` in Scala 2, since it works on `js.IRNode`s,
without knowledge of scalac/dotc data structures.
sjrd added a commit to dotty-staging/dotty that referenced this pull request Oct 2, 2020
This commit implements the entire specification of non-native JS
classes, including nested ones, except the behavior of *anonymous*
JS classes. Anonymous JS classes are not supposed to have their own
class/prototype, but currently they still do.

The most important PRs to scala-js/scala-js that define what is
implemented here were:

1. scala-js/scala-js#1809
   Essentials of Scala.js-defined classes (former name of
   non-native JS classes)
2. scala-js/scala-js#1982
   Support for secondary constructors
3. scala-js/scala-js#2186
   Support for default parameters in constructors
4. scala-js/scala-js#2659
   Support for `js.Symbol`s in `@JSName` annotations
5. scala-js/scala-js#3161
   Nested JS classes

However, this commit was written more based on the state of things
at Scala.js v1.2.0 rather than as a port of the abovementioned PRs.

The support is spread over 4 components:

* The `ExplicitJSClasses` phase, which reifies all nested JS
  classes, and has extensive documentation in the code.
* The `JSExportsGen` component of the back-end, which creates
  dispatchers for run-time overloading, default parameters and
  variadic arguments (equivalent to `GenJSExports` in Scala 2).
* The `JSConstructorGen` component, which massages the constructors
  of JS classes and their dispatcher into a unique JS constructor.
* Bits and pieces in `JSCodeGen`, notably to generate the
  `js.ClassDef`s for non-native JS classes, orchestrate the two
  other back-end components, and to adapt calls to the members of
  non-native JS classes.

`JSConstructorGen` in particular is copied quasi-verbatim from
pieces of `GenJSCode` in Scala 2, since it works on `js.IRNode`s,
without knowledge of scalac/dotc data structures.
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.

4 participants
0