Fixes undercompilation on inheritance on same source #424

eed3si9n · 2017-10-07T20:00:50Z

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.EvaluatorTestSupport -> gg.table.SliceTransforms
[debug] [inv]     xx.StringLibSpecs -> xx.EvaluatorSpecification
[debug] [inv] ]
[debug] [inv]     DependencyByMemberRef Relation [
[debug] [inv]     xx.EvaluatorTestSupport -> gg.table.SliceTransforms
[debug] [inv]     xx.Hello -> gg.table.SliceTransforms
[debug] [inv]     xx.StringLibSpecs -> xx.EvaluatorSpecification
[debug] [inv] ]
....
Caused by: java.lang.AbstractMethodError: xx.StringLibSpecs.buildNonemptyObjects(II)V

First, we see that xx.EvaluatorSpecification -> xx.EvaluatorTestSupport DependencyByInheritance relationship is missing. Second, the error message seen is java.lang.AbstractMethodError happening on xx.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.SliceTransforms was changed,
causing xx.EvaluatorTestSupport to invalidate.
However, because of the missing same-source inheritance, it did not invalidate xx.EvaluatorSpecification.
This meant that neither xx.StringLibSpecs was invalidated.
Calling transform method on a new xx.StringLibSpecs causes runtime error.

By tracking same-source inheritance, we will now correctly invalidate xx.EvaluatorSpecification and xx.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

djspiewak · 2017-10-07T20:25:20Z

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.

eed3si9n · 2017-10-07T20:27:26Z

We can publish a nightly or a milestone once it gets merged.

djspiewak · 2017-10-07T20:31:46Z

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.

typesafe-tools · 2017-10-07T23:05:55Z

The validator has checked the following projects against Scala 2.12,
tested using dbuild, projects built on top of each other.

Project	Reference	Commit
sbt	1.x	sbt/sbt@`84dafd0`
zinc	pull/424/head	`67d5870`
io	1.x	sbt/io@`62004b2`
librarymanagement	1.x	sbt/librarymanagement@`293666f`
util	1.x	sbt/util@`f4eadfc`
website	1.x

✅ The result is: SUCCESS
(restart)

eed3si9n · 2017-10-08T00:10:59Z

The scripted failures might be legit. I am guessing now I am over-compiling on transitive memberref.

jvican · 2017-10-08T12:21:28Z

I'm reviewing this tomorrow.

gkossakowski · 2017-10-09T02:54:39Z

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.
I suggest minimizing the test case further. From the description, it sounds like you don't need type constructors or self-types for the reproduction of this bug and they muddle the picture unnecessarily.

eed3si9n · 2017-10-09T03:05:34Z

@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.

jvican · 2017-10-09T08:34:09Z

Let me know when this is passing CI and the reproduction test case is minimized. I'll review it then.

eed3si9n · 2017-10-09T08:37:05Z

@jvican Restarting the Drone job. The minimization is done.

jvican · 2017-10-09T08:39:01Z

internal/zinc-core/src/main/scala/sbt/internal/inc/Relations.scala

+    |  products: ${relation_s(srcProd)}
+    |  library deps: ${relation_s(libraryDep)}
+    |  library class names: ${relation_s(libraryClassName)}
+    |  internalDependencies: ${internalDependencies.dependencies map {


Can you keep these expressions out of the string interpolation?

jvican · 2017-10-09T08:40:48Z

internal/zinc-scripted/src/test/scala/sbt/internal/inc/IncHandler.scala

@@ -156,8 +156,8 @@ final class IncHandler(directory: File, cacheDir: File, scriptedLog: ManagedLogg
        val analysis = p.compile(i)
        p.discoverMainClasses(Some(analysis.apis)) match {
          case Seq(mainClassName) =>
-            val classpath = i.si.allJars :+ p.classesDir
-            val loader = ClasspathUtilities.makeLoader(classpath, i.si, directory)
+            val cp = p.classpath(i)


This change is modifying the semantics of the classpath handling. You're including internalClasspath and unmanagedJars for something that before didn't require it.

I advise to remove this change altogether. It's cluttering up the fix's diff for nothing.

jvican · 2017-10-09T08:41:55Z

internal/zinc-scripted/src/test/scala/sbt/internal/inc/IncHandler.scala

@@ -343,6 +343,10 @@ case class ProjectStructure(
    ()
  }

+  def classpath(i: IncInstance): Array[File] = {
+    (i.si.allJars.toList ++ (unmanagedJars :+ classesDir) ++ internalClasspath).toArray


I suspect this line reveals a potential bug. Classes dir should always come before unmanagedJars, since unmanagedJars could be overriding classes defined in classesDir. I suspect unmanagedJars has to come at the end of the classpath.

This is for emulating subproject dependencies.

???. An elaboration would be good.

I was able to reproduce this without multiple subprojects, so I'll send this change in another PR.

You have not answered why unmanagedJars has to be before classesDir.

jvican · 2017-10-09T08:42:24Z

zinc/src/sbt-test/source-dependencies/sealed/test

@@ -5,5 +5,5 @@ $ copy-file changes/A.scala A.scala

 # D.scala needs recompiling because the pattern match in D
 # is no longer exhaustive, which emits a warning
-> checkRecompilations 1 A B C E
-> checkRecompilations 2 D
+> checkRecompilations 1


Can you explain this change?

jvican · 2017-10-09T08:43:04Z

zinc/src/sbt-test/source-dependencies/trait-trait-211/mirtest/EvaluatorSpecs.scala

+
+import gg.table._
+
+trait EvaluatorSpecification extends EvaluatorTestSupport { self =>


Why is the self annotation required here?

jvican · 2017-10-09T08:43:17Z

zinc/src/sbt-test/source-dependencies/trait-trait-211/mirtest/EvaluatorSpecs.scala

+trait EvaluatorSpecification extends EvaluatorTestSupport { self =>
+}
+
+trait EvaluatorTestSupport extends SliceTransforms { outer =>


Why is the self annotation required here?

I can remove that.

jvican · 2017-10-09T08:43:39Z

zinc/src/sbt-test/source-dependencies/trait-trait-211/mirtest/Hello.scala

+  testEval
+}
+
+trait StringLibSpecs extends EvaluatorSpecification { self =>


Why is the self annotation required here?

jvican · 2017-10-09T08:44:00Z

zinc/src/sbt-test/source-dependencies/trait-trait-211/mirtest/incOptions.properties

@@ -0,0 +1,2 @@
+apiDebug = true


Should be removed after fix passes CI.

Actually I think it's ok for this stay, since the debug logs are displayed only when the test fails.

jvican · 2017-10-10T09:35:20Z

I've added myself the scripted test for 2.12.x before your changes @eed3si9n .This addition proves that this only happens with 2.12.x (to double-check it, you need to check it out and run the test). For some reason, GitHub does not display the commit in the appropriate order, I can ensure you it's the first one.

I leave my second request "Remove the assumption only if the binary Scala version is 2.11" for you to implement.

jvican · 2017-10-10T10:26:15Z

zinc/src/sbt-test/source-dependencies/trait-trait-212/mirtest/Hello.scala

+package xx
+
+object Hello extends App {
+  val consumer = new StringLibSpecs


This can be minimized further by removing StringLibSpecs:

package xx object Hello extends App { (new EvaluatorSpecification).transform }

My earlier reproduction had that too, but because new trait {} captures the enclosing scope from Hello, I think it reduces the number of feature interaction if I used an actual class there.

Caused by: java.lang.AbstractMethodError: xx.StringLibSpecs.buildNonemptyObjects(II)V

as opposed to

Caused by: java.lang.AbstractMethodError: xx.Hello$$anon$1.buildNonemptyObjects(II)V

eed3si9n · 2017-10-10T15:42:30Z

@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 var in the trait you are going to get in trouble right back.

In a future PR, it would be good to take advantage of the 2.12 trait though.

gkossakowski · 2017-10-11T04:02:56Z

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.

eed3si9n · 2017-10-11T04:04:14Z

@gkossakowski Cool. Thanks for your inputs.

eed3si9n · 2017-10-11T04:36:41Z

Packed more info into the commit message from this PR.

jvican

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 · 2017-10-11T07:52:04Z

zinc/src/sbt-test/source-dependencies/patMat-scope/test

@@ -12,9 +12,7 @@ $ sleep 1000
 $ exists target/classes/foo/SealedUsedInPatMatScope.class
 $ exists target/classes/foo/SealedNameUsedInDefaultScope.class

-# Default scopes should not change
-$ newer beforeFirstCompilation target/classes/foo/SealedNameUsedInDefaultScope.class


This change requires an explanation.

I am honestly not really sure about this one. I think we should be able to put the test back.
In earlier phase of this PR, I had over-tracking of member ref, so I am guessing that affected the behavior around here.

jvican · 2017-10-11T07:52:51Z

zinc/src/sbt-test/source-dependencies/sealed/test

@@ -5,5 +5,4 @@ $ copy-file changes/A.scala A.scala

 # D.scala needs recompiling because the pattern match in D
 # is no longer exhaustive, which emits a warning
-> checkRecompilations 1 A B C E
-> checkRecompilations 2 D
+-> compile


Why has this been changed?

The reformulation of this test is less precise than the previous one, and I think we should always favor checkRecompilations since it gives room to less errors than just "expect a random compilation failure".

Why has this been changed?

Because what I expect as output is failure of compilation, as it was tested in sbt/sbt, and it's more direct result of what we've tested using this test. The comment right above says

# D.scala needs recompiling because the pattern match in D # is no longer exhaustive, which emits a warning

See also source-dependencies/check-recompilation test for a more normal usage. checkRecompilations looks into the Analysis so see what step each classes were recompiled. The expected output we want is that the final compilation to show warning when the pattern match is no longer exhaustive. We are testing the behavior using -Xfatal-warnings flag.

> checkRecompilations 1 A B C E > checkRecompilations 2 D

says, in 1st compilation step it compiled A, B, C, and E. And in the 2nd step, it compiled D. This does not tell us about the warning. In other words, this is a weaker test than what we had in sbt 0.13.

Now thanks to the -Xfatal-warnings flag, it fails to compile. So I could potentially put -> checkRecompilations 1 A B C E (with minus sign), but that value would be nonsensical.

The issue here is that checkRecompilations runs both compile and checks the value of the analysis file. IMO checkRecompilations should only read the last persisted analysis file and make sure that the invariants are met. I'm happy to leave that for another pull request.

eed3si9n · 2017-10-11T15:00:25Z

@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

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. :)

jvican

Tests are failing. Happy to LGTM after tests are passing.

jvican · 2017-10-12T14:57:25Z

zinc/src/sbt-test/source-dependencies/sealed/test

@@ -5,5 +5,4 @@ $ copy-file changes/A.scala A.scala

 # D.scala needs recompiling because the pattern match in D
 # is no longer exhaustive, which emits a warning
-> checkRecompilations 1 A B C E
-> checkRecompilations 2 D
+-> compile


The issue here is that checkRecompilations runs both compile and checks the value of the analysis file. IMO checkRecompilations should only read the last persisted analysis file and make sure that the invariants are met. I'm happy to leave that for another pull request.

### 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

eed3si9n · 2017-10-12T16:52:00Z

@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.

eed3si9n · 2017-10-12T17:04:51Z

zinc/src/sbt-test/source-dependencies/patMat-scope/src/main/scala/foo/Bar.scala

@@ -0,0 +1,6 @@
+package foo
+
+// This class is used to pad the number of source code.


@jvican Plz see the commit message. Basically it works on my machine, but on Drone I am seeing:

[debug] Recompiling all 3 sources: invalidated sources (2) exceeded 50.0% of all sources

KOh wow, unexpected. I opened a ticket so that we can keep track of this one in the future. #430

To disable this heuristic, we can use this:

zinc/zinc/src/sbt-test/source-dependencies/specify-inc-options/incOptions.properties

Line 2 in 3a64b8a

recompileAllFraction = 99

jvican

Thank you for your work and patience @eed3si9n. This now LGTM. I'm happy this issue has been addressed early in the 1.x series.

eed3si9n requested review from gkossakowski, dwijnand and jvican October 7, 2017 20:00

eed3si9n mentioned this pull request Oct 7, 2017

Inherited trait definitions do not seem to be handled gracefully by Zinc #417

Closed

eed3si9n force-pushed the wip/417 branch 3 times, most recently from 0e667a4 to 0fd72d2 Compare October 8, 2017 03:04

jvican self-assigned this Oct 8, 2017

jvican added the bug label Oct 8, 2017

eed3si9n changed the title ~~Fixes undercompilation on local inheritance~~ Fixes undercompilation on inheritance on same source Oct 9, 2017

eed3si9n mentioned this pull request Oct 9, 2017

Reproduce #417 #421

Closed

eed3si9n force-pushed the wip/417 branch from 0fd72d2 to b4078d0 Compare October 9, 2017 05:22

jvican reviewed Oct 9, 2017

View reviewed changes

jvican force-pushed the wip/417 branch from a32968a to 5c99e5e Compare October 10, 2017 09:32

jvican reviewed Oct 10, 2017

View reviewed changes

eed3si9n force-pushed the wip/417 branch from 5c99e5e to eab9dbc Compare October 11, 2017 04:26

jvican requested changes Oct 11, 2017

View reviewed changes

jvican force-pushed the wip/417 branch from eab9dbc to b2a0c7c Compare October 11, 2017 09:29

eed3si9n and others added 3 commits October 12, 2017 02:57

Fix source-dependencies/sealed

456e071

sealed test used to be implemented with `-> compile` in sbt/sbt but was marked pending in 8e65c00. This changes back to the original state.

Add trait-trait-212 for Scala 2.12.3

6a829c2

This commit proves that the error only exists with 2.11.x.

Add real reproduction case for sbt#417

91c0a69

eed3si9n force-pushed the wip/417 branch from b2a0c7c to e5250d3 Compare October 12, 2017 07:02

dwijnand previously approved these changes Oct 12, 2017

View reviewed changes

jvican reviewed Oct 12, 2017

View reviewed changes

eed3si9n dismissed dwijnand’s stale review via 9403160 October 12, 2017 17:00

eed3si9n force-pushed the wip/417 branch from e5250d3 to 9403160 Compare October 12, 2017 17:00

eed3si9n commented Oct 12, 2017

View reviewed changes

jvican approved these changes Oct 12, 2017

View reviewed changes

dwijnand approved these changes Oct 12, 2017

View reviewed changes

dwijnand merged commit 1a7dc5d into sbt:1.0.x Oct 12, 2017

dwijnand added this to the 1.0.3 milestone Oct 13, 2017

eed3si9n mentioned this pull request Oct 13, 2017

Negative test (-compile, or expectation of compilation error) #432

Open

shonzilla mentioned this pull request Oct 25, 2017

Zinc sometimes doesn't invalidate files where trait definitions are inherited sbt/sbt#3586

Closed

eed3si9n mentioned this pull request Jul 25, 2018

Default arguments cause issues in separate compilation #559

Closed


		import gg.table._

		trait EvaluatorSpecification extends EvaluatorTestSupport { self =>

		@@ -0,0 +1,6 @@
		package foo

		// This class is used to pad the number of source code.

Fixes undercompilation on inheritance on same source #424

Fixes undercompilation on inheritance on same source #424

Conversation

eed3si9n commented Oct 7, 2017 • edited Loading

background

what we see without the fix

what this changes

Is this 2.11 only issue?

djspiewak commented Oct 7, 2017

eed3si9n commented Oct 7, 2017

djspiewak commented Oct 7, 2017

typesafe-tools commented Oct 7, 2017

eed3si9n commented Oct 8, 2017

jvican commented Oct 8, 2017

gkossakowski commented Oct 9, 2017 • edited Loading

eed3si9n commented Oct 9, 2017

jvican commented Oct 9, 2017

eed3si9n commented Oct 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvican commented Oct 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eed3si9n commented Oct 10, 2017

gkossakowski commented Oct 11, 2017

eed3si9n commented Oct 11, 2017

eed3si9n commented Oct 11, 2017

jvican left a comment • edited Loading

Choose a reason for hiding this comment

jvican Oct 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvican Oct 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eed3si9n commented Oct 11, 2017

dwijnand left a comment

Choose a reason for hiding this comment

jvican left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eed3si9n commented Oct 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvican left a comment • edited Loading

Choose a reason for hiding this comment

eed3si9n commented Oct 7, 2017 •

edited

Loading

gkossakowski commented Oct 9, 2017 •

edited

Loading

jvican commented Oct 10, 2017 •

edited

Loading

jvican left a comment •

edited

Loading

jvican Oct 11, 2017 •

edited

Loading

jvican Oct 11, 2017 •

edited

Loading

jvican left a comment •

edited

Loading

jvican left a comment •

edited

Loading