Merge pull request #1528 from rochala/undercompilation-local-dependency

[1.x] Local dependency invalidation improvement

Author: eed3si9n

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

@@ -98,7 +98,6 @@ private[inc] abstract class IncrementalCommon(
         invalidatedSources,
         classfileManager,
         pruned,
-        previous,
         classesToRecompile,
         profiler.registerCycle(
           invalidatedClasses,
@@ -149,11 +148,11 @@ private[inc] abstract class IncrementalCommon(
         invalidatedSources: Set[VirtualFile],
         classFileManager: XClassFileManager,
         pruned: Analysis,
-        override val previousAnalysis: Analysis,
         classesToRecompile: Set[String],
         registerCycle: (Set[String], APIChanges, Set[String], Boolean) => Unit
     ) extends IncrementalCallback(classFileManager) {
       override val isFullCompilation: Boolean = allSources.subsetOf(invalidatedSources)
+      override val previousAnalysis: Analysis = previous
       override val previousAnalysisPruned: Analysis = pruned
 
       override def mergeAndInvalidate(
@@ -165,10 +164,12 @@ private[inc] abstract class IncrementalCommon(
             partialAnalysis.copy(compilations = pruned.compilations ++ partialAnalysis.compilations)
           else pruned ++ partialAnalysis
 
-        // Represents classes detected as changed externally and internally (by a previous cycle)
+        // Represents all classes that were compiled as a result of external and internal invalidation (by a previous cycle)
         // Maps the changed sources by the user to class names we can count as invalidated
         val getClasses = (a: Analysis) => initialChangedSources.flatMap(a.relations.classNames)
-        val recompiledClasses = classesToRecompile ++ getClasses(previous) ++ getClasses(analysis)
+        val recompiledClasses = classesToRecompile ++
+          getClasses(previous) ++ getClasses(analysis) ++
+          invalidatedSources.flatMap(previous.relations.classNames)
 
         val newApiChanges =
           detectAPIChanges(recompiledClasses, previous.apis.internalAPI, analysis.apis.internalAPI)
@@ -499,11 +500,11 @@ private[inc] abstract class IncrementalCommon(
       Set.empty
     } else {
       if (invalidateTransitively) {
-        val firstClassTransitiveInvalidation = includeTransitiveInitialInvalidations(
-          initial,
-          IncrementalCommon.transitiveDeps(initial, log)(dependsOnClass),
-          dependsOnClass
-        )
+        // NOTE: As member reference relations do not include local relations, this invalidation will fully propagate
+        // thus we can't rely solely on `firstClassTransitiveInvalidation`. Better bet is to try to find transitive
+        // dependencies from result of `firstClassInvalidation`
+        val firstClassTransitiveInvalidation =
+          IncrementalCommon.transitiveDeps(firstClassInvalidation, log)(dependsOnClass)
         log.debug("Invalidate by brute force:\n\t" + firstClassTransitiveInvalidation)
         firstClassTransitiveInvalidation ++ secondClassInvalidation ++ thirdClassInvalidation ++ recompiledClasses
       } else {

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

@@ -184,14 +184,15 @@ trait Relations {
   private[inc] def externalDependencies: ExternalDependencies
 
   /**
-   * The class dependency relation between classes introduced by member reference.
+   * The class dependency relation between classes introduced
+   * by member reference excluding excluding same-source references.
    *
    * NOTE: All inheritance dependencies are included in this relation because in order to
    * inherit from a member you have to refer to it. If you check documentation of `inheritance`
    * you'll see that there's small oddity related to traits being the first parent of a
    * class/trait that results in additional parents being introduced due to normalization.
-   * This relation properly accounts for that so the invariant that `memberRef` is a superset
-   * of `inheritance` is preserved.
+   *
+   * Because `inheritance` includes same-source references, `memberRef` is not a superset of `inheritance`
    */
   private[inc] def memberRef: ClassDependencies
 

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

@@ -810,7 +810,10 @@ case class ProjectStructure(
     import scala.collection.JavaConverters._
     val map = new java.util.HashMap[String, String]
     properties.asScala foreach { case (k: String, v: String) => map.put(k, v) }
-    val externalHooks = new DefaultExternalHooks(Optional.empty[ExternalHooks.Lookup], Optional.empty[XClassFileManager])
+    val externalHooks = new DefaultExternalHooks(
+      Optional.empty[ExternalHooks.Lookup],
+      Optional.empty[XClassFileManager]
+    )
       .withInvalidationProfiler(profiler)
     val base = IncOptions
       .of()

zinc/src/sbt-test/source-dependencies/same-source-transitive-invalidation-trait/A.scala

@@ -0,0 +1,3 @@
+trait A {
+  def buildNonemptyObjects(a: Int): Int = a
+}

zinc/src/sbt-test/source-dependencies/same-source-transitive-invalidation-trait/App.scala

@@ -0,0 +1,4 @@
+object Main extends App {
+  val x: C = new C { }
+  val z: Int = x.x
+}

zinc/src/sbt-test/source-dependencies/same-source-transitive-invalidation-trait/B.scala

@@ -0,0 +1,6 @@
+trait C extends B {
+  def x = something
+}
+trait B extends A {
+  def something = buildNonemptyObjects(5)
+}

zinc/src/sbt-test/source-dependencies/same-source-transitive-invalidation-trait/changes/A.scala

@@ -0,0 +1,4 @@
+trait A {
+  // change return type Int => String
+  def buildNonemptyObjects(a: Int): String = ""
+}

zinc/src/sbt-test/source-dependencies/same-source-transitive-invalidation-trait/changes/OriginalA.scala

@@ -0,0 +1,3 @@
+trait A {
+  def buildNonemptyObjects(a: Int): Int = a
+}

zinc/src/sbt-test/source-dependencies/same-source-transitive-invalidation-trait/changes/incOptions.properties

@@ -0,0 +1 @@
+transitiveStep = 3

zinc/src/sbt-test/source-dependencies/same-source-transitive-invalidation-trait/incOptions.properties

@@ -0,0 +1 @@
+transitiveStep = 1

zinc/src/sbt-test/source-dependencies/same-source-transitive-invalidation-trait/test

@@ -0,0 +1,21 @@
+#####################  FAILURE  ############################
+# clean build that compiles with transitiveStep = 1
+> compile
+
+# incremental build that should fail with transitiveStep = 1
+$ copy-file changes/A.scala A.scala
+-> compile
+
+#####################  FAILURE  ############################
+
+# rerun the test with transitiveStep = 3
+> clean
+$ copy-file changes/OriginalA.scala A.scala
+$ copy-file changes/incOptions.properties incOptions.properties
+
+# clean build that compiles with transitiveStep = 3
+> compile
+
+# incremental build that should fail with transitiveStep = 3
+$ copy-file changes/A.scala A.scala
+-> compile

zinc/src/sbt-test/source-dependencies/same-source-transitive-invalidation/A.scala

@@ -0,0 +1,3 @@
+object A {
+  def buildNonemptyObjects(a: Int): Int = a
+}

zinc/src/sbt-test/source-dependencies/same-source-transitive-invalidation/App.scala

@@ -0,0 +1,3 @@
+object Main extends App {
+  val z: Int = C.x
+}

zinc/src/sbt-test/source-dependencies/same-source-transitive-invalidation/B.scala

@@ -0,0 +1,7 @@
+object C {
+  def x = B.something
+}
+
+object B {
+  def something = A.buildNonemptyObjects(5)
+}

zinc/src/sbt-test/source-dependencies/same-source-transitive-invalidation/changes/A.scala

@@ -0,0 +1,4 @@
+object A {
+  // change return type Int => String
+  def buildNonemptyObjects(a: Int): String = ""
+}

zinc/src/sbt-test/source-dependencies/same-source-transitive-invalidation/changes/OriginalA.scala

@@ -0,0 +1,3 @@
+object A {
+  def buildNonemptyObjects(a: Int): Int = a
+}

zinc/src/sbt-test/source-dependencies/same-source-transitive-invalidation/changes/WorkingA.scala

@@ -0,0 +1,4 @@
+object A {
+  // add default param
+  def buildNonemptyObjects(a: Int, b: Int = 5): Int = a
+}

zinc/src/sbt-test/source-dependencies/same-source-transitive-invalidation/changes/incOptions.properties

@@ -0,0 +1 @@
+transitiveStep = 3

zinc/src/sbt-test/source-dependencies/same-source-transitive-invalidation/incOptions.properties

@@ -0,0 +1 @@
+transitiveStep = 1

zinc/src/sbt-test/source-dependencies/same-source-transitive-invalidation/test

@@ -0,0 +1,33 @@
+#####################  SUCCESS  ############################
+# clean build that compiles with transitiveStep = 1
+> compile
+
+# incremental build that should work with transitiveStep = 1
+$ copy-file changes/WorkingA.scala A.scala
+> compile
+# > checkIterations 2 this is not yet working, but it should be 2
+
+#####################  FAILURE  ############################
+
+# clean build that compiles with transitiveStep = 1
+> clean
+$ copy-file changes/OriginalA.scala A.scala
+> compile
+
+# incremental build that should fail with transitiveStep = 1
+$ copy-file changes/A.scala A.scala
+-> compile
+
+#####################  FAILURE  ############################
+
+# rerun the test with transitiveStep = 3
+> clean
+$ copy-file changes/OriginalA.scala A.scala
+$ copy-file changes/incOptions.properties incOptions.properties
+
+# clean build that compiles with transitiveStep = 3
+> compile
+
+# incremental build that should fail with transitiveStep = 3
+$ copy-file changes/A.scala A.scala
+-> compile