Revert to the Scala 2 encoding for trait initialization and super calls #8652
Conversation
|
FYI, the dotty backend is missing some code to handle super like scala 2 does: |
|
Yes, Scala 2 handles all of that directly in the back-end. Both rewriting calls and generating the static forwarders. Should we do that in dotty as well (that would mean that the |
|
I am not sure. We'd like to avoid pushing too much stuff into the backend, unless it is really JVM specific. On the other hand, if that's what Scala-2 does, it's easiest to duplicate it here. |
|
It is actually JVM-specific. Scala.js doesn't do that (in fact the entire phase could be skipped for Scala.js). I'm not sure about Scala Native, but there's no reason that it would do it either. |
|
AFAIK the only way to force an invokeSpecial is with a Super(...) receiver or maybe something else that has a SuperType. But both requires that the underlying value is a This. So I think right now there's no way to tell the backend to do an invokeSpecial on something that is not a this. |
|
Why do we need an |
|
Because the scheme is that, for a concrete I'll try the solution to move that logic to the back-end like scalac does. |
Ah I see. So this is basically a workaround for the problem that you can't call an arbitrary method in some indirect parent with invokeSuper. Because if you could, you would not need the static forwarder method. So yes, this is JVM weirdness, I can't see other platforms having that problem. |
sjrd
force-pushed
the
trait-init-like-scala-2
branch
2 times, most recently
from
ae7bc0e to
493cb6e
Compare
sjrd
force-pushed
the
trait-init-like-scala-2
branch
from
8fcb94e to
4370981
Compare
sjrd
force-pushed
the
trait-init-like-scala-2
branch
5 times, most recently
from
64b95d2 to
975a4be
Compare
|
Well, finally all non-bootstrap tests pass. The TBH I have no idea how to start debugging this, so I'll need help here. In the meantime, I believe this is good enough to be reviewed. Perhaps the reviewer will immediately spot what's wrong with the Bad symbolic reference in tastyBootstrap. |
sjrd
changed the title
I would start by checking what classpath is being passed to the compiler in this test, it looks like the compiler-interface jar from sbt is missing somehow, which is weird. Are you seeing this locally or on the CI? |
|
Both locally and on the CI. |
|
ah, looking at the CI log, I think the stub symbol for xsbti.Position is not the root issue but a follow-on error after: 2020-06-24T16:10:32.7076646Z error while transforming method problem
2020-06-24T16:10:32.7077152Z error while transforming trait AnalysisCallback
2020-06-24T16:10:32.7081971Z exception while typing ... (caught cyclic reference) ... of class class dotty.tools.dotc.ast.Trees$DefDef # 399145855
2020-06-24T16:10:32.8077990Z exception while typing abstract class Context(base: dotty.tools.dotc.core.Contexts.ContextBase) extends dotty.tools.dotc.core.Periods,
[...]
2020-06-24T16:10:33.2807471Z *** error while checking compiler/src/dotty/tools/dotc/core/Contexts.scala after phase capturedVars ***I can't be sure what's going on without digging further (which I don't have the time to do right now), but my guess is that there's a mini-phase in the same group as CapturedVars whose denotation transformer can now force other denotations in a way that can lead to a cycle, which means Mixin#transformSym is likely to be the culprit, especially since it seems to be doing some phase-travelling. |
AnalysisCallback is Java-defined, but we do set the Trait flag on Java interfaces, and Mixin#transformSym does: else if sym.is(Trait) thenso maybe you just need to do |
|
(this is a bit misleading, maybe the flag should be called TraitOrInterface and we should have an isScalaTrait method) |
|
Yes! Thanks a lot @smarter! That was exactly the lead I needed. There were actually two issues: a) the Java-defined interfaces and b) some cycles. I broke the cycles with some more phase traveling. Now |
|
Minimization of the NPE in scalaz: class Foo
trait GenericTrait[A] {
def bar: A
}
trait ConcreteTrait extends GenericTrait[Foo] {
val bar: Foo = new Foo
}
class ConcreteClass extends ConcreteTrait
object Test {
def main(args: Array[String]): Unit = {
val obj = new ConcreteClass
println(obj)
println(obj.bar)
assert(obj.bar != null) // AssertionError
}
}I expect ScalaTest is the same issue. I'll work on fixing those next. For fastparse, there is an error in the Scala2Unpickler. I'm pretty sure this is a bug that already existed before my PR, but that was not exposed. Who is responsible for Scala2Unpicker? Any idea what might be going on there, off the top of your head, before I dive in there? |
|
Note in particular Apparently the A few lines above in That said I have no idea what's going on in this code, so I'm bit lost. |
This might not be the best way to fix t4753.scala, but this is the last run test and I'll take anything that makes the tests pass at this point.
The round-trip is useless, and causes issues with some Scala2-emitted modules that do not have module classes (seems to be related to modules inside traits).
* Avoid transforming members of Java interfaces. * Avoid cloning a scope if we do not need it.
This allows to entirely get rid of the phase as well as the `Scala2xAugmented` flag.
sjrd
force-pushed
the
trait-init-like-scala-2
branch
from
7276641 to
77e2925
Compare
|
Rebased after #9343. If there's no objection by the time the CI's green, I'll merge this. |
This reverts a change in scala#8652, which breaks the benchmarking of stdlib: ``` Exception in thread "main" java.lang.AssertionError: assertion failed: moduleRoot = val <none>, moduleClassRoot = module class volatile$, classRoot = class volatile at dotty.DottyPredef$.assertFail(DottyPredef.scala:17) at dotty.tools.dotc.core.unpickleScala2.Scala2Unpickler.<init>(Scala2Unpickler.scala:150) at dotty.tools.dotc.core.classfile.ClassfileParser.unpickleScala$1(ClassfileParser.scala:754) at dotty.tools.dotc.core.classfile.ClassfileParser.unpickleOrParseInnerClasses(ClassfileParser.scala:861) at dotty.tools.dotc.core.classfile.ClassfileParser.parseClass(ClassfileParser.scala:165) at dotty.tools.dotc.core.classfile.ClassfileParser.run(ClassfileParser.scala:90) at dotty.tools.dotc.core.ClassfileLoader.load(SymbolLoaders.scala:402) at dotty.tools.dotc.core.ClassfileLoader.doComplete(SymbolLoaders.scala:397) at dotty.tools.dotc.core.SymbolLoader.complete(SymbolLoaders.scala:342)y.scala, community-build/community-projects/std ```
Not fit for review yet. This doesn't work. See my line comment below, on which I would like to get some advice.