Fix #1811: Add support for secondary constructors in JS classes #1982
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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.
Why / what is it broken? How hard is it to fix it?
There was a problem hiding this comment.
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.
Could we get rid of the InfoBuilders first? It seems to be a rather easy change.
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.
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.
Instead of alt.paramss.flatten, use alt.params. There are a few like that.
There was a problem hiding this comment.
alt.params does not seem to exist. I will at least factor it out.
There was a problem hiding this comment.
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.
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 |
|
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 :) |
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
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.