Fixes undercompilation on inheritance on same source#424
Conversation
|
Awesome! If it would be useful, I'm happy to take the time to check this fix on Quasar, though I've never tried to run SBT from source, so I would probably need a little hand-holding. |
|
We can publish a nightly or a milestone once it gets merged. |
|
Cool. I'm also happy to wait until a release. Whatever is the most helpful! There are other reasons that we can't upgrade to 1.0 right now, so I'm not in a huge rush. |
|
The validator has checked the following projects against Scala 2.12,
✅ The result is: SUCCESS |
|
The scripted failures might be legit. I am guessing now I am over-compiling on transitive memberref. |
0e667a4 to
0fd72d2
Compare
|
I'm reviewing this tomorrow. |
|
I can't think of a scenario that is specific to the local inheritance. I suspect this bug is actually about just inheritance dependencies. I think the transitive chain is broken when two classes are declared in the same source file. |
|
@gkossakowski You're right. When I first reproduced it in #421, I didn't know what was going on so I tried to recreate the original cake pattern found in Quasar code base. The repro can be minimized further. |
|
Let me know when this is passing CI and the reproduction test case is minimized. I'll review it then. |
|
@jvican Restarting the Drone job. The minimization is done. |
internal/zinc-core/src/main/scala/sbt/internal/inc/Relations.scala
Outdated
Show resolved
Hide resolved
internal/zinc-scripted/src/test/scala/sbt/internal/inc/IncHandler.scala
Outdated
Show resolved
Hide resolved
internal/zinc-scripted/src/test/scala/sbt/internal/inc/IncHandler.scala
Outdated
Show resolved
Hide resolved
zinc/src/sbt-test/source-dependencies/trait-trait-211/mirtest/EvaluatorSpecs.scala
Outdated
Show resolved
Hide resolved
zinc/src/sbt-test/source-dependencies/trait-trait-211/mirtest/EvaluatorSpecs.scala
Outdated
Show resolved
Hide resolved
zinc/src/sbt-test/source-dependencies/trait-trait-211/mirtest/Hello.scala
Outdated
Show resolved
Hide resolved
zinc/src/sbt-test/source-dependencies/trait-trait-211/mirtest/incOptions.properties
Outdated
Show resolved
Hide resolved
zinc/src/sbt-test/source-dependencies/trait-trait-212/mirtest/Hello.scala
Outdated
Show resolved
Hide resolved
|
@jvican I narrowed the PR to solving the under compilation happening in 2.11. The fact that it works ok some of the times for Scala 2.12 is related to 2.12 traits compile-to-interface is an independent "should we invalidate trait-trait inheritance in 2.12" question, which is a separate issue from "should we track inheritance in same source." If you define In a future PR, it would be good to take advantage of the 2.12 trait though. |
|
The latest explanation and reproductions look good to me. It's probably worth consolidating these comments back into PR's description for future readers. My experience tells me that good notes are invaluable in the long run. I have to step out of this pr and focus on other things now. |
|
@gkossakowski Cool. Thanks for your inputs. |
|
Packed more info into the commit message from this PR. |
There was a problem hiding this comment.
I had to go on the lookout of the real issue behind this bug. The essential reproduction case is described in https://gist.github.com/jvican/1fe00c898d7132dc1777f1325f86759a. I'll add it to this PR directly.
That reproduction test case shows that this is a more general issue than just 2.11's trait encoding. The current logic violates an essential invariant of the incremental compiler: all subclasses of a given class Foo should be recompiled when Foo is changed (transitively). This affects all Scala versions handled by Zinc.
We can keep the trait-trait-211 scripted test to show that Quasar's issue #417 has been fixed. What was really happening on this ticket, and which hasn't been explained, is that there is a direct dependency between StringLibSpecs and SliceTransforms because the old trait encoding synthesizes a new static forwarder from the former to the latter (the super class defining transform). The signature of this static forwarder is not valid after the recompilation of SliceTransforms and hence it fails at runtime.
I agree with the solution proposed in this pull request, but I first think that some changes to existing scripted tests have to be justified before this can be merged.
|
@jvican Please stop overwriting my commits. |
sealed test used to be implemented with `-> compile` in sbt/sbt but was marked pending in 8e65c00. This changes back to the original state.
This commit proves that the error only exists with 2.11.x.
dwijnand
left a comment
There was a problem hiding this comment.
LGTM.
My personal nit to pick/yak to shave is the irrelevancy of the names of things in the scripted test (mirtest, SliceTransforms, buildNonemptyObjects, gg.table, etc).
If this goes through another review cycle for any reason I'd love if those could be replaced with foos, bars and bippys. :)
### background In sbt 0.13 days, we could ignore the relationship between two classes defined in the same `*.scala` source file, because they will be compiled anyway, and the invalidation was done at the source file level. With class-based namehashing, the invalidation is done at the class level, so we can no longer ignore inheritance relationship coming from the same source, but we still have old assumptions scattered around the xsbt-dependency implementation. ### what we see without the fix ``` [info] Compiling 1 Scala source to ... .... [debug] [inv] internalDependencies: [debug] [inv] DependencyByInheritance Relation [ [debug] [inv] xx.B -> gg.table.A [debug] [inv] xx.Foo -> xx.C [debug] [inv] ] [debug] [inv] DependencyByMemberRef Relation [ [debug] [inv] xx.B -> gg.table.A [debug] [inv] xx.Hello -> gg.table.A [debug] [inv] xx.Foo -> xx.C [debug] [inv] ] .... Caused by: java.lang.AbstractMethodError: xx.Foo.buildNonemptyObjects(II)V ``` First, we see that `xx.C -> xx.B DependencyByInheritance` relationship is missing. Second, the error message seen is `java.lang.AbstractMethodError` happening on `xx.Foo`. ### what this changes This change changes two if expressions that was used to filter out dependency info coming from the same source. One might wonder why it's necessary to keep the local inheritance info, if two classes involved are compiled together anyways. The answer is transitive dependencies. Here's likely what was happening: 1. `gg.table.A` was changed, 2. causing `xx.B` to invalidate. 3. However, because of the missing same-source inheritance, it did not invalidate `xx.C`. 4. This meant that neither `xx.Foo` was invalidated. 5. Calling transform method on a new `xx.Foo` causes runtime error. By tracking same-source inheritance, we will now correctly invalidate `xx.C` and `xx.Foo`. I think the assumption that's broken here is that "we don't need to track inheritance that is happening between two classes in a same source." ### Is this 2.11 only issue? No. The simple trait-trait inheritance reproduction alone will not cause problem in Scala 2.12 because of the [compile-to-interface](http://www.scala-lang.org/news/2.12.0/#traits-compile-to-interfaces) traits. However, not all traits will compile to interface. This means that if we want to take advantage of the compile-to-interface traits, we still should keep track of the same-source inheritance, but introduce some more logic to determine whether recompilation is necessary. Fixes sbt#417
|
@dwijnand I'll give you foo, but no bippy. |
`source-dependencies / patMat-scope` passes locally for me. The invalidation on the CI is likely caused by: ``` [debug] Recompiling all 3 sources: invalidated sources (2) exceeded 50.0% of all sources ``` This attempts to workaround that by adding more source. This does not affect the fidelity of the original test.
background
In sbt 0.13 days, we could ignore the relationship between two classes defined
in the same
*.scalasource file, because they will be compiled anyway, andthe invalidation was done at the source file level. With class-based namehashing,
the invalidation is done at the class level, so we can no longer ignore inheritance
relationship coming from the same source, but we still have old assumptions scattered
around the xsbt-dependency implementation.
what we see without the fix
First, we see that
xx.EvaluatorSpecification -> xx.EvaluatorTestSupport DependencyByInheritancerelationship is missing. Second, the error message seen isjava.lang.AbstractMethodErrorhappening onxx.StringLibSpecs.what this changes
This change changes two if expressions that was used to filter out dependency info coming from the same source.
One might wonder why it's necessary to keep the local inheritance info, if two classes involved are compiled together anyways. The answer is transitive dependencies.
Here's likely what was happening:
gg.table.SliceTransformswas changed,xx.EvaluatorTestSupportto invalidate.xx.EvaluatorSpecification.xx.StringLibSpecswas invalidated.xx.StringLibSpecscauses runtime error.By tracking same-source inheritance, we will now correctly invalidate
xx.EvaluatorSpecificationandxx.StringLibSpecs.I think the assumption that's broken here is that "we don't need to track inheritance that is happening between two classes in a same source."
Is this 2.11 only issue?
No. The simple trait-trait inheritance reproduction alone will not cause problem in Scala 2.12
because of the compile-to-interface traits.
However, not all traits will compile to interface.
This means that if we want to take advantage of the compile-to-interface traits,
we still should keep track of the same-source inheritance, but introduce some more
logic to determine whether recompilation is necessary.
Fixes #417