-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Scala.js: Implement non-native JS classes. #9774
Conversation
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.
Hello, and thank you for opening this PR! 🎉
All contributors have signed the CLA, thank you! ❤️
Commit Messages
We want to keep history, but for that to actually be useful we have
some rules on how to format our commit messages (relevant xkcd).
Please stick to these guidelines for commit messages:
- Separate subject from body with a blank line
- When fixing an issue, start your commit message with
Fix #<ISSUE-NBR>:
- Limit the subject line to 72 characters
- Capitalize the subject line
- Do not end the subject line with a period
- Use the imperative mood in the subject line ("Add" instead of "Added")
- Wrap the body at 80 characters
- Use the body to explain what and why vs. how
adapted from https://chris.beams.io/posts/git-commit
Have an awesome day! ☀️
9f9df05
to
3744aec
Compare
* We are only manipulating IR trees and types. | ||
* | ||
* The only difference is the two parameters `overloadIdent` and `reportError`, | ||
* which are added so that this entire file can be even more isolated. |
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.
Reviewers: Given the verbatim copy from Scala 2, it is probably completely useless to review this file.
def typeInfo: String = sym.info.toString | ||
} | ||
|
||
private sealed abstract class RTTypeTest |
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.
Reviewers: From here until the end of this file, the contents are basically copied verbatim from Scala 2.
* have wrapped them in a `withContextualJSClassValue`, not knowing where | ||
* they belong in the larger tree. | ||
* We now unwrap those, canceling out that effect. | ||
* TODO Is there a better way to do this? |
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.
@smarter said on gitter:
parents are transformed in a special context, that might help: https://github.com/lampepfl/dotty/blob/master/compiler/src/dotty/tools/dotc/transform/MegaPhase.scala#L360
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.
Unfortunately, that special context is not so special. It's just built with a particular set of (owner, outer context, scope), but we cannot otherwise identify it. So that trail is a dead end.
compiler/src/dotty/tools/dotc/transform/sjs/ExplicitJSClasses.scala
Outdated
Show resolved
Hide resolved
def fakeNew(cls: ClassSymbol, ctor: TermSymbol)(using Context): Tree = { | ||
/* TODO This is not entirely good enough, as it break -Ycheck for generic | ||
* classes. Erasure restores the consistency of the fake invocations. | ||
* Improving this is left for later. |
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.
@smarter Any idea how to improve this?
My best idea so far is to leave holes at this phase, and add another phase between Erasure and LambdaLift that would create the fake New nodes. Indeed, after erasure, the problem of type parameters does not exist anymore, and we wouldn't break consistency.
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 think the best option would be to tweak LambdaLift to make it do what you want. But failing that, I agree that adding the new after Erasure would be nicer than doing it here indeed.
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.
Regarding making lambdalift do what I want, I've been trying to come up with exactly what I want. And in fact I cannot think of a better way to receive the information I want than the one I have now (at least for ways that maintain semantic meaning of the Trees we have in the compiler at the phases we have).
So the question remains how to produce that information, and AFAICT modifying lambdalift will not be easier than having a miniphase create those fake New nodes, so I'm more inclined to go that route.
compiler/src/dotty/tools/dotc/transform/sjs/ExplicitJSClasses.scala
Outdated
Show resolved
Hide resolved
compiler/src/dotty/tools/dotc/transform/sjs/ExplicitJSClasses.scala
Outdated
Show resolved
Hide resolved
1eba262
to
ffbd08c
Compare
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.
Not much to say about the PR in its current form (only minor things). However (as I have already mentioned offline), it seems that a lot here is working around the current functioning of dotc, rather than integrating with it. LMK how you would like to proceed in that regard (both short- and long-term) and I'm happy to give further input.
* dotc inserts statements before the super constructor call for param | ||
* accessor initializers (including val's and var's declared in the params). | ||
* We move those after the super constructor call, and are therefore | ||
* executed later than for a Scala class. |
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 take it this is user visible if I do something like this:
abstract class A extends js.Object {
def a: Int
println(a)
}
class B(val a: Int) extends A
OTOH, this only applies to JS classes, so its just a semantic difference (even though a subtle one).
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.
Yes. That semantic difference already exists in Scala 2, and is unavoidable.
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 have expanded the comment to make it clear that it is related to Scala/Scala.js as languages, rather than a workaround for any particular compiler.
* those two methods as primitives to completely eliminate them. | ||
* | ||
* Hopefully this will become unnecessary when/if we manage to | ||
* reinterpret js.| as a true Dotty union type. |
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 seems we're amassing quite a bit of tech debt that can be fixed dotty internally. That is OK, especially if it is to bootstrap into something usable we can improve on.
My recommendation would be to track these in some form (can be issue tracker, doesn't have to be) so we can keep a grasp on them.
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 intend to submit an issue for this one, but I haven't spent time on minimizing it yet. I'd like to find a minimization that is independent of Scala.js, since this is a typechecker issue.
} | ||
|
||
def genJSConstructorDispatch(alts: List[Symbol]): (Option[List[js.ParamDef]], js.JSMethodDef) = { | ||
val exporteds = alts.map(Exported) |
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.
This imports quite a bit of tech debt. (Exported
as a class IMO does not make sense anymore, I have tried removing it, but it requires control flow changes, so I haven't gotten around to it yet).
Maybe this is also OK, just want to point 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.
I'm not sure what's wrong, here. Exported
gathers the information about how a method behaves in the context of the run-time overloading dispatch that JSExportsGen
has to build. Since constructing that information is non-obvious, and is then reused several times over the run-time overloading dispatch generation algorithm, it makes sense to me to store that info somewhere, and it seems to me that a class is perfectly applicable for 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.
What is strange IMO is that we calculate things inside the class itself (rather than in the flow of export generation). An Exported
has no more semantic meaning than a Symbol
(it used to, but not anymore). So IMO a better thing would be something more semantically meaningful.
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.
So ... IIUC, it would be better to
- Move the computations in a separate method that then instantiates the
Exported
with all the already computed data (rather than put everything in the constructor), and - Rename it to something more meaningful, perhaps
SymbolExportInfo
?
Would that address your concerns, or am I missing something?
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.
Yes. My feeling is that with a better control flow, some of this data becomes more local. But it's just a gut feeling. I haven't tried.
private def isApplicableOwner(sym: Symbol)(using Context): Boolean = { | ||
!sym.isStaticOwner || ( | ||
sym.is(ModuleClass) && | ||
sym.hasAnnotation(jsdefn.JSTypeAnnot) && |
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.
How is this different form sym.isJSType
? (also below)
else | ||
ref1 | ||
} else { | ||
// JS traits never have an initializer, no matter what dotc thinks |
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.
Can we make the rest of dotc more JS aware to not "think" 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.
Perhaps the choice of words is not appropriate, here.
In a way, adding the NoInits
flag here precisely makes dotty aware that a JS trait does not have an initializer. It's just that the initial computation of whether or not to put NoInits
does not add the flag to JS traits, since it is not directly aware of the specificity of JS traits. That initial computation happens in Namer. Maybe I could try to add that directly in Namer, but then I would also have to do something in Scala2Unpickler to fix up things coming from Scala 2 (there is already some logic in there to patch NoInits
on some well-known Scala 2 traits, because they never have the NoInits flag in the pickles).
compiler/src/dotty/tools/dotc/transform/sjs/ExplicitJSClasses.scala
Outdated
Show resolved
Hide resolved
case typeRef: TypeRef => typeRef | ||
case _ => | ||
// This should not have passed the checks in PrepJSInterop | ||
report.error(i"class type required but found $tpe0", tree) |
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.
Assert instead? (if it is a compiler bug?)
compiler/src/dotty/tools/dotc/transform/sjs/ExplicitJSClasses.scala
Outdated
Show resolved
Hide resolved
Oh, seems a bunch of stuff has changed from my review snapshot :-/ I'll have a look at that as well. |
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.
@gzm0 I've added a commit that I think addresses all your comments except the ones that are of the kind "can we change dotty to make this simpler". I need to go prepare dinner now; I'll come back for those later.
* those two methods as primitives to completely eliminate them. | ||
* | ||
* Hopefully this will become unnecessary when/if we manage to | ||
* reinterpret js.| as a true Dotty union type. |
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 intend to submit an issue for this one, but I haven't spent time on minimizing it yet. I'd like to find a minimization that is independent of Scala.js, since this is a typechecker issue.
} | ||
|
||
def genJSConstructorDispatch(alts: List[Symbol]): (Option[List[js.ParamDef]], js.JSMethodDef) = { | ||
val exporteds = alts.map(Exported) |
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'm not sure what's wrong, here. Exported
gathers the information about how a method behaves in the context of the run-time overloading dispatch that JSExportsGen
has to build. Since constructing that information is non-obvious, and is then reused several times over the run-time overloading dispatch generation algorithm, it makes sense to me to store that info somewhere, and it seems to me that a class is perfectly applicable for that.
|
||
val hasVarArg = alt.hasRepeatedParam | ||
val maxArgc = if (hasVarArg) paramsSize - 1 else paramsSize | ||
val needsRestParam = maxArgc != minArgc || hasVarArg |
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.
Indeed. I have significantly reworked this area of the code now to address that and other comments below. The logic to determine the required args is now in genExportMethod
.
val hasVarArg = alt.hasRepeatedParam | ||
val maxArgc = if (hasVarArg) paramsSize - 1 else paramsSize | ||
val needsRestParam = maxArgc != minArgc || hasVarArg | ||
val formalArgsRegistry = new FormalArgsRegistry(minArgc, needsRestParam) |
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 would rather keep FormalArgsRegistry
as is since it is 100% in common with Scala 2. I'm not really sure how to improve it anyway.
js.Assign(js.JSSuperSelect(superClass, receiver, nameTree), | ||
allArgs.head.asInstanceOf[js.Tree]) | ||
} else { | ||
js.JSSuperMethodCall(superClass, receiver, nameTree, allArgs) |
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.
No, we're passing exactly the arguments that we received (the rest param being passed with a spread to re-expand it).
allArgs.head.asInstanceOf[js.Tree]) | ||
} else { | ||
js.JSSuperMethodCall(superClass, receiver, nameTree, allArgs) | ||
} |
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.
That was actually very tempting, but it doesn't work because genApplyJSMethodGeneric
unboxes the result, and here we must keep it boxed.
val verifiedOrDefault = if (param.hasDefault) { | ||
js.If(js.BinaryOp(js.BinaryOp.===, jsArg, js.Undefined()), { | ||
genCallDefaultGetter(exported.sym, i, param.sym.sourcePos, static) { | ||
prevArgsCount => result.take(prevArgsCount).toList.map(_.ref) |
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 tried to do as you suggest, but it gets uglier than expected because we then also need to compute the tpe
s in advance (not just the idents), and to compute the tpes we need to also look at whether it's repeated param or not (and if it is, it's a Scala Seq
type, so it's annoying to produce), etc. In the end it's harder to reason about than the mutable state, IMO.
I have looked at the new commits, nothing to add. |
3f940aa
to
7b638aa
Compare
Regarding adding dedicated First, this generally goes against the current in dotc. The current has been in favor of less kinds of Trees, and instead represent things as methods where possible. A clear example is the absence of a One concern, which went in favor of reducing the number of tree kinds, is that every additional kind of Tree degrades ever so slightly the performance of the entire compiler, for all programs, because the type test chains in the pattern match get a bit longer. If some kind of tree is not used pervasively (like For two of the methods, namely That has to be balanced with an entirely different concern. If we create special things that appear in the compiler only in the JS pipeline, we create a big risk regarding compiler plugins. If a compiler plugin does not actively take steps and tests to support Scala.js-related features, they can end up breaking on Scala.js code. If instead, they simply see methods and process them like any other, and it works like that, it's a win. Finally, I'll talk a bit more about In conclusion, although there are potential gains to reap from having dedicated Tree nodes, there are also downsides. The current approach has the merit of being well established and known to work in Scala 2. We could try a different approach, but tht requires some more exploration, and it is not clear to me that it will be overall better. |
I never thought about it that way. That's actually a very neat way of simplifying things.
Yep. Sold.
Could we make them lambdas so they are less hacky?
Seems like there are multiple constructs in the compiler (in general that need this). This might be a generalization that is worth it.
Could we add methods instead ( |
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 was in fact much easier than I thought. Unlike in scalac, where we forcefully reattach captures as normal constructor arguments, here we keep them as captures. This is much more natural, and yields a simpler implementation.
We don't use `paramSymss` anymore. We now only work with `paramNamess` and `paramInfoss`, which are reliable. We also clean up a bunch of things by making things "more general", requiring less explanation comments. In particular, we make no difference between the outer param and other capture params.
We enable Ycheck for our phases in the test suite to make sure that we do not regress in the future.
7b638aa
to
a91beae
Compare
Rebased.
Hum, I don't understand. Do you mean putting the
I believe those are good things we could explore, but at this point I suggest to leave them for future work. They would be internal improvements in the compiler, but I don't think they will directly improve things for users. I'd rather validate this port PR and give it to users before researching further. |
Yep, sounds fair. Is there anything specific I still should look at? |
I don't think so. It seems I'm only waiting for a review by @nicolasstucki at this point, mostly to make sure that I'm not doing anything dotty-stupid. :) |
I will review it this afternoon |
Thanks a lot, @gzm0 @nicolasstucki and @smarter for your reviews on this complex 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:
js.Symbol
s in@JSName
annotationsHowever, 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:
ExplicitJSClasses
phase, which reifies all nested JS classes, and has extensive documentation in the code.JSExportsGen
component of the back-end, which creates dispatchers for run-time overloading, default parameters and variadic arguments (equivalent toGenJSExports
in Scala 2).JSConstructorGen
component, which massages the constructors of JS classes and their dispatcher into a unique JS constructor.JSCodeGen
, notably to generate thejs.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 ofGenJSCode
in Scala 2, since it works onjs.IRNode
s, without knowledge of scalac/dotc data structures.Edit: I've now also added the behavior of anonymous JS classes, whose equivalent in Scala 2 was scala-js/scala-js#2292