Skip to content

Assorted incremental compiler bug fixes.#1

Open
adriaanm wants to merge 76 commits into0.13from
inc-comp-stable-self
Open

Assorted incremental compiler bug fixes.#1
adriaanm wants to merge 76 commits into0.13from
inc-comp-stable-self

Conversation

@adriaanm
Copy link
Owner

WIP PR. @eed3si9n, any suggestions on how to polish (/test) it so that it can be accepted?

@adriaanm
Copy link
Owner Author

/cc @gkossakowski ;-)

@eed3si9n
Copy link

/cc @Duhemm

@adriaanm
Copy link
Owner Author

Thanks -- any pointers to developer docs on which test targets to run before submitting the PR are welcome.

@gkossakowski
Copy link

Do you have an example of scenario you want to address?
You might want to run these tests: https://github.com/sbt/sbt/tree/0.13/sbt/src/sbt-test/source-dependencies. The exact command escaped my memory.

@eed3si9n
Copy link

The scenarios are described using scripted test (see http://eed3si9n.com/testing-sbt-plugins), which is what @gkossakowski showed.

To run it for foo in source-dependencies, run scripted source-dependencies/foo.

@Duhemm
Copy link

Duhemm commented Oct 26, 2015

The command to run the tests is scripted source-dependencies/*. The only docs that I know of for scripted tests is this: http://eed3si9n.com/testing-sbt-plugins

You may also want to try using this: https://github.com/sbt/incrementalcompiler/blob/1.0/internal/incrementalcompiler-core/src/test/scala/sbt/inc/IncrementalTest.scala. It may be easier to make ensure that what you want to have recompiled is actually recompiled.

@eed3si9n
Copy link

Also note that for sbt 1.0, we've split incremental compiler into https://github.com/sbt/incrementalcompiler so you might have to send us the PR there too.

@adriaanm
Copy link
Owner Author

This will need to be backported as it's for a customer that I assume is still on 0.13. What's the preferred direction? Submit to 1.0 and backport or other way around? (This patch needs to be sent to them before the end of the month, though that's not super relevant for the immediate discussion.)

@adriaanm
Copy link
Owner Author

The bug I'm trying to fix is spurious recompilation, triggered for classes without self types. This was spotted in the wild, and I don't have a repro, but it looked like the representation of their self type in the extracted API would vary between no type and the type of the class, causing a rebuild.

@adriaanm
Copy link
Owner Author

/cc @retronym

@retronym
Copy link

Regarding the intermittent NoType you observed in the wild, I noticed that handling of refinement types has some logic to use NoType rather than the type refs to the refinement class symbol: https://github.com/sbt/sbt/blob/0.13/compile/interface/src/main/scala/xsbt/ExtractAPI.scala#L372-L397

Here's the bug I mentioned in our chat today related to a type causes a loop in typeToString: https://issues.scala-lang.org/browse/SI-7744

@eed3si9n
Copy link

This will need to be backported as it's for a customer that I assume is still on 0.13. What's the preferred direction?

If 0.13.10 needs to be issued, then we should get it into 0.13 first.

sbt was reporting warning abouts inconsistent versions of dependencies
even if these dependencies didn't have the same configuration (as in
`provided` vs `compile`).

This commit fixes this problem by comparing the dependencies by
organization, artifact name and configuration.

Fixes sbt#1933
@adriaanm
Copy link
Owner Author

We don't immediately need a new release, though this should end up in one.

@adriaanm adriaanm force-pushed the inc-comp-stable-self branch from 7b05583 to ef7d3c3 Compare October 29, 2015 16:40
@adriaanm adriaanm force-pushed the inc-comp-stable-self branch 2 times, most recently from 7d85ebb to c093d42 Compare December 21, 2015 07:19
@adriaanm
Copy link
Owner Author

I've rebased on top of #2 to get some more backports from 0.13 on top of 0.13.8, including the not-yet-merged sbt#2325

The crash manifested as a `NotSerializableException`,
because we don't visit the private subtrees of the API graph,
which would previously force all Lazy stubs as a side-effect.
There was a backstop in `AbstractLazy` (`writeReplace`), but
somehow that didn't work.

```
java.io.NotSerializableException: sun.reflect.generics.reflectiveObjects.ParameterizedTypeImpl
[...]
 at java.io.ObjectOutputStream.writeObject(ObjectOutputStream.java:329)
 at xsbt.api.SourceFormat$.writes(SourceFormat.scala:24)
 at xsbt.api.SourceFormat$.writes(SourceFormat.scala:15)
 at sbt.inc.TextAnalysisFormat21661anonfun.apply(TextAnalysisFormat.scala:329)
 at sbt.inc.TextAnalysisFormat21661anonfun.apply(TextAnalysisFormat.scala:329)
 at sbt.inc.TextAnalysisFormat21661anonfun at java.io.ObjectOutputStream.writeObject0(ObjectOutputStream.javr$.aggregate(TextAnalysisFormat.scala:18)
 at sbt.inc.TextAnalysisFormat$.objToString(TextAnalysisFormat.scala:329)
 at sbt.inc.TextAnalysisFormat21661anonfun.apply(TextAnalysisFormat.scala:213)
 at sbt.inc.TextAnalysisFormat21661anonfun.apply(TextAnalysisFormat.scala:213)
 at sbt.inc.TextAnalysisFormat21661anonfun21661writeMap.apply(TextAnalysisFormat.scala:381)
 at sbt.inc.TextAnalysisFormat21661anonfun21661writeMap.apply(TextAnalysisFormat.scala:377)
 at scala.collection.mutable.ResizableArray.foreach(Resizable at sbt.inc.TextAnalysisFormalection.mutable.ArrayBuffer.foreach(ArrayBuffer.scala:47)
 at sbt.inc.TextAnalysisFormat$.sbt21661writeMap(TextAnalysisFormat.scala:377)
 at sbt.inc.TextAnalysisFormat$.write(TextAnalysisFormat.scala:216)
 at sbt.inc.TextAnalysisFormat21661anonfun.apply(TextAnalysisFormat.scala:64)
 at sbt.inc.TextAnalysisFormat21661anonfun.apply(TextAnalysisFormat.scala:64)
 at sbt.inc.TextAnalysisFormat21661anonfun.apply(TextAnalysisFormat.scala:64)
 at sbt.inc.FormatTimer$.aggregate(TextAnalysisFo at sbt.inc.TextAnalysisFormat21661anonfun.scala:25)
 at sbt.inc.TextAnalysisFormat$.write(TextAnalysisFormat.scala:64)
 at sbt.inc.FileBasedStore21661anon21661anonfun.apply(FileBasedStore.scala:12)
 at sbt.inc.FileBasedStore21661anon21661anonfun.apply(FileBasedStore.scala:12)
 at sbt.Using.apply(Using.scala:24)
 at sbt.inc.FileBasedStore21661anon.set(FileBasedStore.scala:12)
 at sbt.inc.AnalysisStore21661anon.set(AnalysisStore.scala:16)
 at sbt.inc.AnalysisStore21661anon.set(AnalysisStore.scala:27)
 at sbt.Defaults21661anonfun.apply(Defaults.scala:799)
 at sbt.Defaults21661anonfun at sbt.inc.TextAts.scala:793)
 at scala.Function121661anonfun.apply(Function1.scala:47)
 at sbt.21661anonfun21661u2219.apply(TypeFunctions.scala:40)
 at sbt.std.Transform21661anon.work(System.scala:63)
 at sbt.Execute21661anonfun21661anonfun.apply(Execute.scala:226)
 at sbt.Execute21661anonfun21661anonfun.apply(Execute.scala:226)
 at sbt.ErrorHandling$.wideConvert(ErrorHandling.scala:17)
 at sbt.Execute.work(Execute.scala:235)
 at sbt.Execute21661anonfun.apply(Exe at sbt.Using.apply(Using.scala:24)
 mit.apply(Execute.scala:226)
 at sbt.ConcurrentRestrictions21661anon21661anonfun.apply(ConcurrentRestrictions.scala:159)
 at sbt.CompletionService21661anon.call(CompletionService.scala:28)
 at java.util.concurrent.FutureTask.innerRun(FutureTask.java:303)
 at java.util.concurrent.FutureTask.run(FutureTask.java:138)
 at java.util.concurrent.Executors.call(Executors.java:439)
 at java.util.concurrent.FutureTask.innerRun(FutureTask.java:303 at sbt.std.Transform21661anon.work(System(FutureTask.java:138)
 at java.util.concurrent.ThreadPoolExecutor.runTask(ThreadPoolExecutor.java:895)
 at java.util.concurrent.ThreadPoolExecutor.run(ThreadPoolExecutor.java:918)
 at java.lang.Thread.run(Thread.java:695)

[error] (compile:compile) java.io.NotSerializableException: sun.reflect.generics.reflectiveObjects.ParameterizedTypeImpl
[error] Total time: 2 s, completed Dec 21, 2015 12:05:19 PM
```
@adriaanm adriaanm force-pushed the inc-comp-stable-self branch from c093d42 to 9f5fd6c Compare December 22, 2015 06:12
Fix for regression triggered by sbt#2325 -- apparently, the API visitor would
force all the lazy stubs so the `AbstractLazy` `writeReplace` was not exercised.
With the private subtrees being ignored, the visitor didn't force the lazy stub,
and the `writeProtected` method was not inherited in `SafeLazy.Impl`.

From https://docs.oracle.com/javase/7/docs/api/java/io/Serializable.html
`writeReplace` must be protected for its override to be inherited.

> `Serializable` classes that need to designate an alternative object to be used
> when writing an object to the stream should implement this special method with
> the exact signature:
> `ANY-ACCESS-MODIFIER Object writeReplace() throws ObjectStreamException;`
>
> This `writeReplace` method is invoked by serialization if the method exists and
> it would be accessible from a method defined within the class of the object
> being serialized. Thus, the method can have `private`, `protected` and
> `package-private` access.
>
> **Subclass access to this method follows java accessibility rules.**

(Thanks to retronym for digging up the docs.)

The fix is captured, indirectly, by the scripted test
`source-dependencies/java-analysis-serialization-error`.
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 }
```
@adriaanm adriaanm force-pushed the inc-comp-stable-self branch from de25c20 to 0d32d1b Compare January 5, 2016 21:10
adriaanm and others added 6 commits January 5, 2016 13:15
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.
@adriaanm adriaanm force-pushed the inc-comp-stable-self branch from 0d32d1b to 9969e82 Compare January 5, 2016 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants