Fix #1795: Essentials of Scala.js-defined JS classes. #1809
Conversation
|
CI only for now. Or for the curious, you can dive in. One interesting thing is the tests :) |
|
Also, suggestions for more pos tests are welcome :) |
| val genInitialThis = genThis() // outside of withScopedVars | ||
| val thisSym = thisDef.symbol | ||
| withScopedVars( | ||
| thisLocalVarIdent := Some(encodeLocalSym(thisSym)) |
There was a problem hiding this comment.
Consider making a val out of encodeLocalSyms result.
There was a problem hiding this comment.
(will allow you to not write .get.get below).
|
What about overload of normal methods? I didn't see any tests for it. |
https://github.com/scala-js/scala-js/pull/1809/files#diff-6e8c360252085a4ec6f673609bd75631R395 ("simple overloaded methods" + "renamed overloaded methods") I sort of trust the tests in |
ffd799a to
34470e0
Compare
|
And there we go! :D More pos tests still welcome, I have this feeling that I must have missed some. |
|
Self-review: I forgot to forbid overloading of private methods. |
|
The compiler test |
34470e0 to
96a6911
Compare
|
Updated with the error messages to prevent overloading of private methods. And also to prevent over_riding_ of private methods, actually. |
f9822c7 to
552d972
Compare
| if (methodTailJumpThisSym.get != NoSymbol) { | ||
| js.VarRef(encodeLocalSym(methodTailJumpThisSym))(currentClassType) | ||
| if (thisLocalVarIdent.get.isDefined) { | ||
| js.VarRef(thisLocalVarIdent.get.get)(currentClassType) |
|
LGTM unti bca957a |
|
That's all until 5dc9d6b |
| val result = body | ||
| enclosingOwner = oldEnclosingOwner | ||
| allEnclosingOwners = oldAllEnclosingOwners | ||
| result |
There was a problem hiding this comment.
Hmmm.. maybe we should protect this by a finally block.
|
Would you mind separating the state cleanup in |
| for (methodInfo <- methodInfos.values) { | ||
| if (methodInfo.isExported) | ||
| methodInfo.reach(this)(FromExports) | ||
| } |
There was a problem hiding this comment.
Why is this not done in the same place than for normal classes? IIUC, the export reachability for normal classes is actually too aggressive. It reaches even if the class is not instantiated...
There was a problem hiding this comment.
Exports on normal classes take overriding into account. With:
class Foo {
@JSExport def foobar(): Int = 42
}
class Bar extends Foo {
@JSExport override def foobar(): Int = 3
}
new Baryou'll have that Foo["foobar"] can be dce'ed away, because Foo is not instantiated. Only Bar["foobar"] is reachable. This is implemented through callMethod which actually delays the actual call until the class is instantiated(). We are allowed to do this because the semantics of Scala classes and exports do not guarantee anything about what will be present in the prototypes.
For JS classes, however, it is necessary to expose everything as soon as the constructor is reachable, because the classes could be extended by JS classes, or otherwise reflected upon using the JS prototype.
There was a problem hiding this comment.
Ah, makes sense. I was confused by callMethod and delayedCallMethod...
|
That's all until eda1c38 |
| | def foo(x: Int): Int = x + 1 | ||
| | ^ | ||
| """ | ||
| } |
There was a problem hiding this comment.
Please also add a neg test for private members in traits
There was a problem hiding this comment.
Ah, yes, I saw it. The error message is a bit convoluted, but that's fine. No need to complicate compiler code. Who puts private methods in traits you cannot put implementations in anyways...
|
That's all. |
This commit implements most of the specification for Scala.js-defined JavaScript classes, as discussed in scala-js#1795. The following features are not yet supported, and are reported as implementation restrictions: * Secondary constructors * Exporting a Scala.js-defined class * Calling the super constructor with :_* to a varargs param. This commit does not implement the deprecations that would lead to nicer definitions either. These aspects will come as follow ups, since they are not critical to the general specification.
A JS trait can be `@ScalaJSDefined` if it does not contain any concrete term member, and does not extend a native JS trait. Such a Scala.js-defined JS trait can be implemented by a Scala.js-defined JS class. Note that a Scala.js-defined JS trait can very well extend a (native or not) JS class. A Scala.js-defined JS class that mixes in this trait will necessarily inherit from the parent JS class, and therefore it already has all its members. The top-level trait `js.Any` is marked as `@ScalaJSDefined`, so that Scala.js-defined JS traits can directly extend it, without binding themselves to `js.Object` or another JS class.
With Scala.js-defined JS classes, it makes sense to have abstract members in native JS classes and traits, to force a Scala.js-defined JS class extending them to implement those members.
6166131 to
29314b6
Compare
Well, actually, we lack the infrastructure in desugar to emit spread calls, at the moment. So fixing it requires substantial changes which should definitely go in a different PR anyway. |
|
Refer to this link for build results (access rights to CI server needed): https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/2120/ |
Completely fine with that. |
|
LGTM |
Fix #1795: Essentials of Scala.js-defined JS classes.
|
Yeepee! :-) |
|
|
|
You have no idea :-p |
|
I don't know. But it was terrible :) After 2000 LOC PR: |
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.
This PR implements most of the specification for Scala.js-defined JavaScript classes, as discussed in #1795.
The following features are not yet supported, and are reported as implementation restrictions:
:_*to a varargs param.This PR does not implement the deprecations that would lead to nicer definitions either. These aspects will come as follow ups, since they are not critical to the general specification.