Fix crash from changing abstract class to trait#1632
Fix crash from changing abstract class to trait#1632jackkoenig wants to merge 1 commit intosbt:developfrom
Conversation
|
Thanks for getting this ball rolling again! Really like the thoughtfulness of the PR description. |
| val dependentOnRemovedClasses = removedClasses.flatMap(previous.memberRef.internal.reverse) | ||
| val modifiedClasses = classNames(modifiedSrcs) | ||
| val invalidatedClasses = removedClasses ++ dependentOnRemovedClasses ++ modifiedClasses | ||
| val inheritanceInvalidations = modifiedClasses.flatMap(previous.inheritance.internal.reverse) |
There was a problem hiding this comment.
I haven't carefully studied how the bug is occurring, or why this would fix it, but in theory the inheritance relationship is already captured as dependency relationship in Zinc. So if changing
abstract class Ato
trait Ais causing undercompilation, I wonder if we need to enrich the invalidation, specifically when the parent type changes its definition type, as if the signature has changed. The fact that this fixes the bug, but doesn't include information about the trait-vs-class I suspect that it's also might be source of potential problem.
There was a problem hiding this comment.
The bug in #598 isn't a typical undercompilation, it's crashing during the incremental compilation. This crash is sort of due to undercompilation in that if you include the right file in the incremental compilation it doesn't occur (thus why this "fix" fixes it), but it's also possible Zinc would figure it out in a later cycle if it didn't crash.
The failure mode if you back out this "fix" and run test I added "recompile subclass when superclass changes from abstract class to trait" is long so I've put it in a gist: https://gist.github.com/jackkoenig/ce190c77fda3e571d43bbe6d6534a327
There was a problem hiding this comment.
I see, so:
class Bextendstrait A, compile both.- change
Atoabstract class A, and compile onlyA.scala,scala.tools.nsc.backend.jvm.BTypescrashes with "Bad/Invalid superClass" error. - compiling both
B.scalaandA.scalawould fix the error, but always invalidating all child classes creates overcompilation, since recompilation would happen even for body code changes.
As a workaround, I wonder if we can allow Bad/Invalid superClass error only for the compile cycle 1.
2 participants
This is a redo of #1284 or a revert of the revert in #1462.
Fixes #598.
The key observation is that while #1284 used any member references, this only uses inheritance.
I am pretty confident that this will also introduce some overcompilation, so I am specifically asking for help in identifying that and reducing it. That being said, I have had issues with Zinc since #1462 so I wanted to get the ball rolling on fixing those issues and this seemed like a good place to start.
I also contributed some unit tests that I found useful while developing, but they may be redundant with the tests from #598 being re-enabled.