Fix #1750: Alternative fix for cyclic references due to illegal class overrides #1913
Conversation
|
The CI failure Iis an OOM during dotty bootstrap. |
| case ex: CyclicReference => | ||
| // See neg/i1750a for an example where a cyclic error can arise. | ||
| // The root cause in this example is an illegal "override" of an inner trait | ||
| ctx.error(cyclicErrorMsg(ex), base.pos) |
There was a problem hiding this comment.
As a general rule in development, may I understand that try/catch can only be used as the last resort? Just want to know.
There was a problem hiding this comment.
It's complicated. Cyclic references can only be handled with try-catch, because at the points they are raised we typically do not have a way to back out cleanly. So if a cyclic reference is a possibility we need to handle it with try/catch. And we should try to minimize the places where cyclic references can be raised.
I would think twice before we introduce other exceptions and handle them with try/catch.
| class A { // error: classes cannot be overridden | ||
| bug() | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
why remove the class here?
There was a problem hiding this comment.
It now masks the other errors because the error is raised now in typer, whereas the others would be raised in RefChecks.
|
We still get an OOM on dotty bootstrapped. @smarter @DarkDimius do we even need this test anymore, now that we bootstrap already as part of the build? |
Yes, the bootstrap can now be done through sbt, but it still has to be done explicitly by using the The OOM we're getting here is similar to what I experienced while working on the sbt bootstrap, I thought I fixed them by increasing the heap size that sbt can use in 239a850, but it looks like it wasn't enough. To recap: the OOM does not happen in one of the forked JVM where we run the tests, instead it happens in the original JVM used by sbt to run its tasks. Since |
|
... In fact you can disregard everything I said above because this branch is not rebased on top of master, so it should not be using the new sbt-based bootstrap and the new CI settings, I'll rebase it to master and everything should be fine. |
Illegal class overrides are fundamentally at odds with the way dotty represents types and therefore can cause lots of low-level problems. Two measures in this commit First, we detect direct illegal class overrides on completion instead of during RefChecks. Break the override by making the previously overriding type private. This fixes i1750.scala, but still fails for indirect overrides between two unrelated outer traits/classes that are inherited by the same class or trait. We fix this by catching the previously thrown ClassCastException in both ExtractAPI and RefChecks. Test case for indirect overrides is in i1750a.scala.
|
@smarter Thanks for rebasing! |
Illegal class overrides are fundamentally at odds with the way dotty
represents types and therefore can cause lots of low-level problems.
Two measures in this commit:
First, we detect direct illegal class overrides on completion instead of
during RefChecks. Break the override by making the previously
overriding type private.
This fixes i1750.scala, but still fails for indirect overrides between
two unrelated outer traits/classes that are inherited by the same class or trait.
We fix this by catching the previously thrown ClassCastException
in both ExtractAPI and RefChecks.
Test case for indirect overrides is in i1750a.scala.
Review by @liufengyun