Fix #2285: Adapt to 2.12 trait encoding (removal of impl classes). #2288
Conversation
|
Review by @retronym, please. |
|
Nightly for this PR: https://scala-webapps.epfl.ch/jenkins/job/scalajs-matrix-build/3771/ |
|
Refer to this link for build results (access rights to CI server needed): https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/2917/ |
|
Do we have no stack trace tests at all? (Otherwise I will of course ask you to add one for default methods :P) |
| * but we use this method in places where, previously, we had only | ||
| * `isInterface`. | ||
| */ | ||
| def isTraitOrInterface: Boolean = self.isInterface |
There was a problem hiding this comment.
How hard is it to write an assertion that no one calls this from PrepJSInterop? IIUC, the method would not do the same in 2.11 and 2.12, there, right?
There was a problem hiding this comment.
I don't understand. Could you elaborate?
There was a problem hiding this comment.
If somebody (in the future), uses this method in PrepJSInterop (or any phase pre-mixin for that matter) and expects it to return isTrait || isInterface (which exists pre-2.12 IIUC), this will work in 2.12. However, in pre-2.12, this test will fail for traits that have implementations. (Btw. why can't you just implement this with self.isInterface || self.isTrait?)
There was a problem hiding this comment.
Btw. why can't you just implement this with self.isInterface || self.isTrait?
Er ... I didn't try ^^ I probably can, indeed. -_-'
There was a problem hiding this comment.
It works just as well, so I'll take that.
They come with the second commit, for free ;-) |
failed in two jobs, so it's probably not spurious |
scalac gives us this absurd tree after mixin in 2.11.7: abstract trait A extends Object {
<accessor> def helloworld$A$_setter_$s_=(x$1: String): Unit;
def foo(d: String, d2: String): helloworld.Base = {
<synthetic> val d2$1: String = (d2: String);
<empty>
}.$asInstanceOf[helloworld.Base]();
<stable> <accessor> def s(): String;
def bar(): Unit
};Note that |
12d78d1 to
e6eefd8
Compare
|
@gzm0 Updated with your comments + fix run/t6443.scala (there's a comment). |
|
Refer to this link for build results (access rights to CI server needed): https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/2918/ |
|
@sjrd I can confirm that this branch builds successfully in a run of Scala community build using the new trait encoding. |
|
This patch LGTM, too, although I'm not familiar enough with ScalaJS to offer more than a superficial review. |
…ses). The main changes are actually related to keeping source compatibility with older versions, through more compat hacks. Also: * Use isTraitOrInterface instead of isInterface to identify something that should be an interface at the IR level. * A method is abstract iff its body is EmptyTree.
e6eefd8 to
eefb2cb
Compare
|
@retronym Thanks! I had to re-change the @gzm0 Would you have time to have a second look today, please? |
|
LGTM |
|
Refer to this link for build results (access rights to CI server needed): https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/2919/ |
|
New nighlty build: https://scala-webapps.epfl.ch/jenkins/job/scalajs-matrix-build/3776/ |
|
Nightly passed :-) |
Fix #2285: Adapt to 2.12 trait encoding (removal of impl classes).
The main changes are actually related to keeping source compatibility with older versions, through more compat hacks.
Also:
isTraitOrInterfaceinstead ofisInterfaceto identify something that should be an interface at the IR level.EmptyTree.