WIP: SD-228 eliminate static super accessors in traits #5415
Conversation
|
FYI, I suspect this might break Scala.js, more specifically the commit "Make |
lrytz
force-pushed
the
traitSuperAccessors
branch
3 times, most recently
from
445be20
to
51298b5
Compare
For details see SD-228. No longer generate static accessor methods for every non-private trait method. Super calls are implemented using invokespecial. If necessary, the target interface is added as a direct parent to the current class.
All callsites to $init$ methods are generated during erasure. By having the method already static at this point, we can avoid the non-local transform and generate T.$init$(this) from the start, instead of this.$init$(). This also allows eliminating some now unneeded code in the backend.
Now that we re-introduced the logic to add additional parent interfaces in the backend to support arbitrary super calls to indirect parent interfaces, we can do the same if the interface is java-defined. Before, when we were using static accessors to emit super calls, indirect java interfaces were excluded.
When adding an interface to a class during code gen, record that fact in the corresponding BType. This ensures that ClassBTypes constructed from symbol and classfile are identical. This is basically the same commit as 3fca034 (which was later reverted).
This reverts 91b066a. Interfaces required for an invokespecial call are added as direct parent to the class, no matter if they are defined in Java or Scala.
lrytz
force-pushed
the
traitSuperAccessors
branch
from
51298b5
to
31505fe
Compare
|
From the community build: That looks like an invalid AST from the Scala compiler (https://github.com/scala-js/scala-js/blob/master/compiler/src/main/scala/org/scalajs/core/compiler/GenJSCode.scala#L1625-L1635). I'll try to reproduce.. The rest of the community build is running here: https://scala-ci.typesafe.com/view/scala-2.12.x/job/scala-2.12.x-integrate-community-build/620/console |
|
@sjrd here's a minimized example - it doesn't take much: trait T { val x = 0 } // has an $init$ method
class C extends T // calls T.$init$(C.this)The
The current result of The reason things don't crash in the JVM backend (we have the same assertion in place that crashed in SJS) is that for static calls, we simply skip code gen for the qualifier: https://github.com/scala/scala/blob/2.12.x/src/compiler/scala/tools/nsc/backend/jvm/BCodeBodyBuilder.scala#L669 This is not the case in the SJS code gen, and What do you think is the best way forward? I guess so far you don't have support for static methods in SJS? |
|
The rest of the community build passed: https://scala-ci.typesafe.com/view/scala-2.12.x/job/scala-2.12.x-integrate-community-build/620/console |
|
@lrytz Thanks for the detailed explanation. I guess the best way forward is for SJS to do a similar exception to avoid The SJS IR has static methods (we've been using them for impl class methods for ages), so that is not a problem. I'll work on this as soon as the camera ready for my Scala'16 paper is submitted (the deadline is tomorrow noon-ish). |
|
@sjrd thanks! No rush, and if it looks non-trivial, please wait until we reach a decision whether this PR gets merged or not - it's still open (scala/scala-dev#228). |
|
@lrytz What resolvers do I need to get nightlies of scala-xml? The following was not enough: gave me |
@sjrd I started this Jenkins run https://scala-ci.typesafe.com/view/scala-2.12.0/job/scala-2.12.0-release-main/82/ which should publish what you need. |
|
alternatively, you can clone the scala-xml, repo add one more resolver |
|
Thanks @SethTisue, I manage to get it from the artifacts published by the build you mentioned. I think the necessary changes are those in scala-js/scala-js@master...sjrd:static-trait-init-rc2 Note that this is about class files being broken. If I revert the change at declaration site of the If I revert the change when compiling the library ( Only accessing I'm a bit lost right now. I think I'll let it rest until Monday, unless you guys have a hint for me. FTR, I'm on Windows atm. |
|
@sjrd I did I used the same scala version that's used in this PR as starr, it's published (together with modules) on |
|
At least it looks to me like it succeeded: some warnings are reported in sbt as |
|
Indeed, with your suggested settings to use the nightly version, I can compile and run the helloworld, as well as the test suite.
Those |
|
Here is a PR for Scala.js: scala-js/scala-js#2608 so that our full CI runs on it. |
|
Thanks for checking, @sjrd! Here's a re-run of the community built with scala-js pinned to your branch. |
|
we decided that we're not going to merge this, see scala/scala-dev#228 (comment) |
Recovered and adapted some test cases for super calls from scala#5415
Recovered and adapted some test cases for super calls from scala#5415
Recovered and adapted some test cases for super calls from scala#5415
Recovered and adapted some test cases for super calls from scala#5415
Recovered and adapted some test cases for super calls from scala#5415
Recovered and adapted some test cases for super calls from scala#5415
Recovered and adapted some test cases for super calls from scala#5415
Recovered and adapted some test cases for super calls from scala#5415
Recovered and adapted some test cases for super calls from scala#5415
Recovered and adapted some test cases for super calls from scala#5415
WIP, don't merge: the first commit updates starr to a version on scala-release-temp that corresponds to the tip of this PR. This way I can find out if all commits in this PR pass CI validation.
The temporary starr is build from this branch: https://github.com/lrytz/scala/tree/traitSuperAccessorsTmp
It's the same as this PR, but without the starr update in the first commit. The bootstrap job that built it passed all tests (https://scala-ci.typesafe.com/view/scala-2.12.x/job/scala-2.12.x-integrate-bootstrap/593/)
For submission, the right procedure is probably to keep the current starr, have CI fail on the commits, and then re-starr right after merging it.