-
Notifications
You must be signed in to change notification settings - Fork 396
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
Fix #1811: Add support for secondary constructors in JS classes #1982
Conversation
Review by @sjrd. |
Refer to this link for build results (access rights to CI server needed): https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/2361/ |
|
||
// Boldly get rid of everything in infoBuilder, because it's broken | ||
currentClassInfoBuilder.addMethod( | ||
ir.Infos.generateMethodInfo(newConstructor)) |
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.
Why / what is it broken? How hard is it to fix it?
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.
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
.
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.
Could we get rid of the InfoBuilders first? It seems to be a rather easy change.
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 guess we could, yes, sure.
4050518
to
a6ab08a
Compare
@@ -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))' \ |
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.
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.
Refer to this link for build results (access rights to CI server needed): https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/2368/ |
a6ab08a
to
cc6ef16
Compare
lazy val isRepeatedUncurry = alt.paramss.flatten.map(isRepeated) | ||
|
||
def paramTypesPosterasure = enteringPhase(currentRun.posterasurePhase) { | ||
alt.paramss.flatten.apply(paramIndex).tpe |
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 alt.paramss.flatten
, use alt.params
. There are a few like that.
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.
alt.params
does not seem to exist. I will at least factor it out.
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.
Ah maybe it's alt.tpe.params
There was a problem hiding this comment.
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)
.
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.
scalac mystery ... Never mind, then.
That's all, I think. |
3599292
to
16e8dc5
Compare
if ir.Definitions.isConstructorName(mtd.name) => | ||
Nil | 8000||
|
||
/* Dispatch to constructor with no arguments |
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.
at least one argument?
That's all. |
Refer to this link for build results (access rights to CI server needed): https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/2703/ |
16e8dc5
to
c9d60c7
Compare
Refer to this link for build results (access rights to CI server needed): https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/2716/ |
LGTM :) |
…tors-in-JS-classes Fix #1811: Add support for secondary constructors in JS classes
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.
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.
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.
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.
No description provided.