Skip to content

Fixes undercompilation on inheritance on same source#424

Merged
dwijnand merged 5 commits intosbt:1.0.xfrom
eed3si9n:wip/417
Oct 12, 2017
Merged

Fixes undercompilation on inheritance on same source#424
dwijnand merged 5 commits intosbt:1.0.xfrom
eed3si9n:wip/417

Conversation

@eed3si9n
Copy link
Member

@eed3si9n eed3si9n commented Oct 7, 2017

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:

  1. gg.table.SliceTransforms was changed,
  2. causing xx.EvaluatorTestSupport to invalidate.
  3. However, because of the missing same-source inheritance, it did not invalidate xx.EvaluatorSpecification.
  4. This meant that neither xx.StringLibSpecs was invalidated.
  5. 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
Copy link

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
Copy link
Member Author

eed3si9n commented Oct 7, 2017

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

@djspiewak
Copy link

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
Copy link

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
Copy link
Member Author

eed3si9n commented Oct 8, 2017

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

@eed3si9n eed3si9n force-pushed the wip/417 branch 3 times, most recently from 0e667a4 to 0fd72d2 Compare October 8, 2017 03:04
@jvican jvican self-assigned this Oct 8, 2017
@jvican jvican added the bug label Oct 8, 2017
@jvican
Copy link
Member

jvican commented Oct 8, 2017

I'm reviewing this tomorrow.

@gkossakowski
Copy link
Contributor

gkossakowski commented Oct 9, 2017

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 eed3si9n changed the title Fixes undercompilation on local inheritance Fixes undercompilation on inheritance on same source Oct 9, 2017
@eed3si9n eed3si9n mentioned this pull request Oct 9, 2017
@eed3si9n
Copy link
Member Author

eed3si9n commented Oct 9, 2017

@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
Copy link
Member

jvican commented Oct 9, 2017

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

@eed3si9n
Copy link
Member Author

eed3si9n commented Oct 9, 2017

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

@eed3si9n
Copy link
Member Author

@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
Copy link
Contributor

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
Copy link
Member Author

@gkossakowski Cool. Thanks for your inputs.

@eed3si9n
Copy link
Member Author

Packed more info into the commit message from this PR.

Copy link
Member

@jvican jvican left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@eed3si9n
Copy link
Member Author

@jvican Please stop overwriting my commits.

eed3si9n and others added 3 commits October 12, 2017 02:57
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
dwijnand previously approved these changes Oct 12, 2017
Copy link
Member

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@jvican jvican left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

### 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
Copy link
Member Author

@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.
Copy link
Member

@jvican jvican left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants