Assorted incremental compiler bug fixes.#2343
Conversation
|
Hi @romanowski, Thank you for your contribution! We really value the time you've taken to put this together. Before we proceed with reviewing this pull request, please sign the Typesafe Contributors License Agreement: |
|
I know that this is WIP, but just wanted to mention, it would be great to have lots of notes about these assorted changes for the release. |
|
For refinement types, the Structure was already restricted
to declarations (and not inherited members), but all base types
were still included for a refinement's parents, which would
create unwieldy, and even erroneous (cyclic) types by expanding
all constituents of an intersection type to add all base types.
Since the logic already disregarded inherited members, it seems
logical to only include direct parents, and not all ancestor types.
Incremental compilation no longer includes a huge cyclic type,
and thus `ShowAPI` in principle does not need to deal with cycles,
but we'll keep that functionality in there for now.
```
class Dep {
def bla(c: Boolean) = if (c) new Value else "bla"
}
class Value extends java.lang.Comparable[Value] { def compareTo(that: Value): Int = 1 }
```
Call `initialize` in case symbol's `info` hadn't been completed during normal compilation. Also, normalize to the class symbol immediately. Add a TODO regarding only looking at class symbols, and thus ignoring the term symbol for objects, as the corresponding class symbol has all the relevant info.
The only aspect of the self variable that's relevant for incremental compilation is its explicitly declared type, and only when it's different from the type of the class that declares it. Technically, any self type that's a super type of the class could be ignored, as it cannot affect external use (instantiation/subclassing) of the class.
Motivated because we want to make it more robust & configurable.
Original motivation was to diagnose a cyclic type representation,
likely due to an f-bounded existential type, as illustrated by the following:
```
class Dep {
// The API representation for `bla`'s result type contains a cycle
// (an existential's type variable's bound is the existential type itself)
// This results in a stack overflow while showing the API diff.
// Note that the actual result type in the compiler is not cyclic
// (the f-bounded existential for Comparable is truncated)
def bla(c: Boolean) = if (c) new Value else "bla"
}
class Value extends java.lang.Comparable[Value] { def compareTo(that: Value): Int = 1 }
```
Limit nesting (`-Dsbt.inc.apidiff.depth=N`, where N defaults to `2`),
and number of declarations shown for a class/structural type
(via `sbt.inc.apidiff.decls`, which defaults to `0` -- no limit).
Limiting nesting is crucial in keeping the size of api diffs of large programs
within a reasonable amount of RAM...
For example, compiling the Scala library, the API diff with nesting at `4`
exhausts 4G of RAM...
For refinement types, the Structure was already restricted
to declarations (and not inherited members), but all base types
were still included for a refinement's parents, which would
create unwieldy, and even erroneous (cyclic) types by expanding
all constituents of an intersection type to add all base types.
Since the logic already disregarded inherited members, it seems
logical to only include direct parents, and not all ancestor types.
```
class Dep {
def bla(c: Boolean) = if (c) new Value else "bla"
}
class Value extends java.lang.Comparable[Value] { def compareTo(that: Value): Int = 1 }
```
Specialize two implementations for each value of the `inherit` boolean argument. Also use a more direct way of distinguishing declared and inherited members. backwards compat for source-dependencies/inherited-dependencies
Also a bit more complete: handle SelectFromTypeTree, consider the self type an inheritance dependency, and flatten any refinement types in inherited types, to get to the symbols of their parents, instead of the useless symbol of the refinement class. Include inheritance dependencies in regular ones Also, update test to reflect the self type is now seen as an inheritance dependency. self types are local, so don't treat them like inherited types note inheritanceSymbols dealiases, where allSymbols is constructed differently fix NPE in source-dependencies/macro-annotation
Since pickled annotated types and symbols only mention static annotations, whereas compilation from source sees all annotations, we must explicitly filter annotations in the API representation using the same criteria as the pickler, so that we generate the same API when compiling from source as when we're loading classfiles.
Further categorize debug output as api diff ([diff]), and invalidation ([inv]). Since we log a ton of diagnostics, make it a bit easier to find the relevant bits.
a6c6e53 to
81786a2
Compare
|
Hi @romanowski, Thank you for your contribution! We really value the time you've taken to put this together. Before we proceed with reviewing this pull request, please sign the Typesafe Contributors License Agreement: |
|
Hey, @eed3si9n, thanks for turning this into a PR! When looking at the release notes, I noticed I hadn't cleaned up 0f61629's commit message yet. It probably wasn't clear what it's doing, and so it's not included in the notes, although we may want to. We should probably also mention the changes to logging (are they ok for you?). For example, you can now limit nesting ( I'm happy to do the rewording. /cc @gkossakowski for some double checking of my changes & fixes. |
|
@adriaanm I summarized it to "Fixes the tracking of self types in the incremental compiler." I'd be happy to take better wording on a one-liner summary of this change. As per logging, I was a bit sad to see beautiful |
|
Correct, the logs are meant to simplify reporting bugs. Honestly, I'm stumped that someone like the implicit approach to logging -- it took me a long time to wrap my head around it 😵 Regarding 0f61629, this fixed a performance problem with the old traversal, which would visit the same AST multiple times (first with |
|
Out of curiosity are these changes coming from use of sbt inside the scala repo, or something else? |
|
No, we're battle testing it on a customer's MLoC code base ;-) |
|
Hi @romanowski, Thank you for your contribution! We really value the time you've taken to put this together. Before we proceed with reviewing this pull request, please sign the Typesafe Contributors License Agreement: |
Assorted incremental compiler bug fixes.
|
I wish @adriaanm had a chance to clean up commits before merging. Notes for future archeologists:
In aggregate, changes to the source code look good to me. |
|
Due to sbt's omission, we weren't running the unit tests on the compiler bridge, which @gkossakowski caught in #2357. On my machine the test that was changed now fails: This is consistent with earlier comment:
|
|
Yeah, that was one of the mistakes that @gkossakowski pointed out in my original code during an over-the-shoulder review. I should have been quicker with fixing the commit messages and test... |
|
I'd be happy to roll the commit history back before this got merged if you're up for fixing the commits etc. |
|
I had planned to improve my branch before submitting the PR anyway, so, yes, I'm happy to make the fixes next week. I'll leave it up to you whether you want to |
|
Unfortunately, I have rebased my development branch on top of 0.13 already. |
|
+1, thanks @gkossakowski |
|
I submitted a fix in #2358. I'll wait for Travis to validate the change. |
Rewritten from sbt/zinc@3792b80
Cherry picked adriaanm#1 on top of 0.13 to prepare for 0.13.10.
/cc @adriaanm