Upgrade to Scala.js 1.6.0. #13070
Upgrade to Scala.js 1.6.0. #13070
Conversation
smarter
added
the
needs-minor-release
|
Assigning this to @prolativ as he's the one who'll be doing the next minor release. |
| * scalac inserts statements before the super constructor call for early | ||
| * initializers and param accessor initializers (including val's and var's |
There was a problem hiding this comment.
There's no early initializer in Scala 3.
| * declared in the params). We move those after the super constructor | ||
| * call, and are therefore executed later than for a Scala class. |
| } yield { | ||
| val sym = vparam.symbol | ||
| val tpe = toIRType(sym.info) | ||
| js.VarDef(encodeLocalSym(sym), originalNameOfLocal(sym), tpe, mutable = true, jstpe.zeroOf(tpe))(vparam.span) |
There was a problem hiding this comment.
I noticed that the corresponding scala-js code uses more linebreaks in expressions (for example this line is split in two in scala-js), wouldn't it be better to mimic the scala-js style to simplify comparisons?
There was a problem hiding this comment.
Yes, perhaps that would be better. Is it worth spending my time reformatting things as close as possible to the scala-js code?
There was a problem hiding this comment.
Is it worth spending my time reformatting things
No, since you're maintaining this code you get to do decide how to format it :).
| ctorArgs, afterThisCall.result()) | ||
| } | ||
|
|
||
| private def genJSClassCtorDispatch(sym: Symbol, allParams: List[Symbol], |
There was a problem hiding this comment.
The comments and code in this method appear to be somewhat different from the scala-js version, what are the high-level differences here?
There was a problem hiding this comment.
The main difference is that in dotc we don't have reliable param symbols. We can only work with the paramNames and paramInfos of a MethodType. This significantly complicates jsParamInfos, which cannot store param symbols, and cannot know whether params have a default value or not. For default values, we have to do some expensive checks to test whether a default getter exists or not. Since we only need that information at definition site of a method (but not at call site), we avoid the expensive checks in jsParamInfos, and instead we merge that information in here, by taking into account the param symbols found in the Trees.
There was a problem hiding this comment.
The main difference is that in dotc we don't have reliable param symbols.
Note that as of last year we do have https://github.com/lampepfl/dotty/blob/6922c1dd7a0be17cead711cdeb9f216327b4b368/compiler/src/dotty/tools/dotc/core/SymDenotations.scala#L300-L306, I don't know if it would help you here.
| rank1 <= rank2 | ||
|
|
||
| case (InstanceOfTypeTest(t1), InstanceOfTypeTest(t2)) => | ||
| t1 <:< t2 |
There was a problem hiding this comment.
Aside, but you can turn a partial ordering based on subtyping into a total ordering by replacing it with a check for the length of the base type sequence (171cea2), which should allow you to use a regular sort function instead of doing a topological sort.
There was a problem hiding this comment.
I don't think that's true. Given
class A
class B extends A
class C extends AB and C have the same base type sequence length, yet they are not the same. That's not a total ordering. Perhaps it works in 171cea2 because it's known to be in the same superclass lineage? But in this case we don't know that.
There was a problem hiding this comment.
That's not a total ordering.
It gives you a total ordering for equivalence classes, where two types with the same base length are considered to be equivalent which is good enough for our purposes (I've also recently learned this is called a strict weak ordering: https://en.wikipedia.org/wiki/Weak_ordering#Strict_weak_orderings)
|
We'd like to merge this in this week, so it can get a week's worth of testing by being in the nightlies, before cutting 3.1.0. Most of the comments don't seem blockers to me, but the "Doesn't that affect semantics?" one looks potentially more merge-blocking. I'm assuming it would be a shame for this to slide to 3.2.0. |
I think it's fine to merge it just before 3.1.0-RC1, after all that's why we have an RC period.
scala-js has the same comment in its codebase, so whatever this means it's probably not an issue. |
|
I'm still on vacation this week. I won't be able to address comments before Monday. The "affects semantics": yes it's a change of semantics compared to Scala classes. The same change exists in Scala 2. It's not the only semantics difference between Scala and JS classes. It has to be like that. (It's also already like that in dotty master; this is not changed by this PR) |
This is consistent with genClass. Forward port of scala-js/scala-js@6022605
The following commit will introduce another call-site. Separating it to reduce noise. Forward port of scala-js/scala-js@587e194
Instead of post-transforming the trees generated by JSExportsGen, we refactor the right parts and generate the proper trees right away. This was neccessary to give the generated trees the right type (partial scala-js/scala-js#4442). Forward port of scala-js/scala-js@8093f28
In statement position, they must always be typed as NoType, since their children might be typed as such. Forward port of scala-js/scala-js@e440de9
This includes changes in the compiler and build from the following upstream commits: * scala-js/scala-js@c4aa18b Allow to specify target versions of ECMAScript > 2015. * scala-js/scala-js@89dfc48 Use a dedicated function for Long-to-Float conversions.
….meta. These changes are not tested in this repo, because we do not have the infrastructure for tests requiring ECMAScript Modules. They should be straightforward enough that it is not really necessary. All the magic of these two features happens in the linker, not in the compiler.
No description provided.