From ae15eccd9c7aea2b4336ea454d974aed66d5ec16 Mon Sep 17 00:00:00 2001 From: Grzegorz Kossakowski Date: Wed, 4 Dec 2013 13:22:08 +0100 Subject: [PATCH 01/15] Introduce a companion `Incremental` class. Move most of the functionality from `Incremental` object to its companion class. This commit is a preparation for making it possible to have two different implementation of logic in `Incremental` object. --- .../src/main/scala/sbt/inc/Incremental.scala | 41 +++++++++++-------- 1 file changed, 24 insertions(+), 17 deletions(-) diff --git a/compile/inc/src/main/scala/sbt/inc/Incremental.scala b/compile/inc/src/main/scala/sbt/inc/Incremental.scala index 74ad517ac..518e16538 100644 --- a/compile/inc/src/main/scala/sbt/inc/Incremental.scala +++ b/compile/inc/src/main/scala/sbt/inc/Incremental.scala @@ -21,20 +21,33 @@ object Incremental log: Logger, options: IncOptions)(implicit equivS: Equiv[Stamp]): (Boolean, Analysis) = { - val initialChanges = changedInitial(entry, sources, previous, current, forEntry, options, log) + val incremental = new Incremental + val initialChanges = incremental.changedInitial(entry, sources, previous, current, forEntry, options, log) val binaryChanges = new DependencyChanges { val modifiedBinaries = initialChanges.binaryDeps.toArray val modifiedClasses = initialChanges.external.allModified.toArray def isEmpty = modifiedBinaries.isEmpty && modifiedClasses.isEmpty } - val initialInv = invalidateInitial(previous.relations, initialChanges, log) + val initialInv = incremental.invalidateInitial(previous.relations, initialChanges, log) log.debug("All initially invalidated sources: " + initialInv + "\n") val analysis = manageClassfiles(options) { classfileManager => - cycle(initialInv, sources, binaryChanges, previous, doCompile, classfileManager, 1, log, options) + incremental.cycle(initialInv, sources, binaryChanges, previous, doCompile, classfileManager, 1, log, options) } (!initialInv.isEmpty, analysis) } + private[inc] val apiDebugProp = "xsbt.api.debug" + private[inc] def apiDebug(options: IncOptions): Boolean = options.apiDebug || java.lang.Boolean.getBoolean(apiDebugProp) + + private[sbt] def prune(invalidatedSrcs: Set[File], previous: Analysis): Analysis = + prune(invalidatedSrcs, previous, ClassfileManager.deleteImmediately()) + + private[sbt] def prune(invalidatedSrcs: Set[File], previous: Analysis, classfileManager: ClassfileManager): Analysis = + { + classfileManager.delete( invalidatedSrcs.flatMap(previous.relations.products) ) + previous -- invalidatedSrcs + } + private[this] def manageClassfiles[T](options: IncOptions)(run: ClassfileManager => T): T = { val classfileManager = options.newClassfileManager() @@ -46,10 +59,13 @@ object Incremental result } +} + + +private class Incremental { + val incDebugProp = "xsbt.inc.debug" private def incDebug(options: IncOptions): Boolean = options.relationsDebug || java.lang.Boolean.getBoolean(incDebugProp) - val apiDebugProp = "xsbt.api.debug" - def apiDebug(options: IncOptions): Boolean = options.apiDebug || java.lang.Boolean.getBoolean(apiDebugProp) // setting the related system property to true will skip checking that the class name // still comes from the same classpath entry. This can workaround bugs in classpath construction, @@ -58,7 +74,7 @@ object Incremental // TODO: the Analysis for the last successful compilation should get returned + Boolean indicating success // TODO: full external name changes, scopeInvalidations - @tailrec def cycle(invalidatedRaw: Set[File], allSources: Set[File], binaryChanges: DependencyChanges, previous: Analysis, + @tailrec final def cycle(invalidatedRaw: Set[File], allSources: Set[File], binaryChanges: DependencyChanges, previous: Analysis, doCompile: (Set[File], DependencyChanges) => Analysis, classfileManager: ClassfileManager, cycleNum: Int, log: Logger, options: IncOptions): Analysis = if(invalidatedRaw.isEmpty) previous @@ -67,7 +83,7 @@ object Incremental def debug(s: => String) = if (incDebug(options)) log.debug(s) else () val withPackageObjects = invalidatedRaw ++ invalidatedPackageObjects(invalidatedRaw, previous.relations) val invalidated = expand(withPackageObjects, allSources, log, options) - val pruned = prune(invalidated, previous, classfileManager) + val pruned = Incremental.prune(invalidated, previous, classfileManager) debug("********* Pruned: \n" + pruned.relations + "\n*********") val fresh = doCompile(invalidated, binaryChanges) @@ -144,7 +160,7 @@ object Incremental val newApis = lastSources.toSeq map newAPI val apiChanges = (lastSources, oldApis, newApis).zipped.flatMap { (src, oldApi, newApi) => sameSource(src, oldApi, newApi, log, options) } - if (apiDebug(options) && apiChanges.nonEmpty) { + if (Incremental.apiDebug(options) && apiChanges.nonEmpty) { logApiChanges(apiChanges, oldAPI, newAPI, log, options) } @@ -321,15 +337,6 @@ object Incremental newInv ++ initialDependsOnNew } - def prune(invalidatedSrcs: Set[File], previous: Analysis): Analysis = - prune(invalidatedSrcs, previous, ClassfileManager.deleteImmediately()) - - def prune(invalidatedSrcs: Set[File], previous: Analysis, classfileManager: ClassfileManager): Analysis = - { - classfileManager.delete( invalidatedSrcs.flatMap(previous.relations.products) ) - previous -- invalidatedSrcs - } - def externalBinaryModified(entry: String => Option[File], analysis: File => Option[Analysis], previous: Stamps, current: ReadStamps, log: Logger)(implicit equivS: Equiv[Stamp]): File => Boolean = dependsOn => { From 1de2900a67847a6718642062acf522b990af6aa2 Mon Sep 17 00:00:00 2001 From: Grzegorz Kossakowski Date: Wed, 4 Dec 2013 17:45:45 +0100 Subject: [PATCH 02/15] Add Logger and IncOptions as Incremental class constructor args Both Logger and IncOptions instances were passed around Incremental class implementation unmodified. Given the fact that entire implementation of the class uses exactly the same values for those types it makes sense to extract them as constructor arguments so they are accessible everywhere. This helps reducing signatures of other methods to more essential parameters that are more specific to given method. --- .../src/main/scala/sbt/inc/Incremental.scala | 80 +++++++++---------- 1 file changed, 40 insertions(+), 40 deletions(-) diff --git a/compile/inc/src/main/scala/sbt/inc/Incremental.scala b/compile/inc/src/main/scala/sbt/inc/Incremental.scala index 518e16538..a55021c87 100644 --- a/compile/inc/src/main/scala/sbt/inc/Incremental.scala +++ b/compile/inc/src/main/scala/sbt/inc/Incremental.scala @@ -21,17 +21,17 @@ object Incremental log: Logger, options: IncOptions)(implicit equivS: Equiv[Stamp]): (Boolean, Analysis) = { - val incremental = new Incremental - val initialChanges = incremental.changedInitial(entry, sources, previous, current, forEntry, options, log) + val incremental = new Incremental(log, options) + val initialChanges = incremental.changedInitial(entry, sources, previous, current, forEntry) val binaryChanges = new DependencyChanges { val modifiedBinaries = initialChanges.binaryDeps.toArray val modifiedClasses = initialChanges.external.allModified.toArray def isEmpty = modifiedBinaries.isEmpty && modifiedClasses.isEmpty } - val initialInv = incremental.invalidateInitial(previous.relations, initialChanges, log) + val initialInv = incremental.invalidateInitial(previous.relations, initialChanges) log.debug("All initially invalidated sources: " + initialInv + "\n") val analysis = manageClassfiles(options) { classfileManager => - incremental.cycle(initialInv, sources, binaryChanges, previous, doCompile, classfileManager, 1, log, options) + incremental.cycle(initialInv, sources, binaryChanges, previous, doCompile, classfileManager, 1) } (!initialInv.isEmpty, analysis) } @@ -62,7 +62,7 @@ object Incremental } -private class Incremental { +private class Incremental(log: Logger, options: IncOptions) { val incDebugProp = "xsbt.inc.debug" private def incDebug(options: IncOptions): Boolean = options.relationsDebug || java.lang.Boolean.getBoolean(incDebugProp) @@ -75,14 +75,14 @@ private class Incremental { // TODO: the Analysis for the last successful compilation should get returned + Boolean indicating success // TODO: full external name changes, scopeInvalidations @tailrec final def cycle(invalidatedRaw: Set[File], allSources: Set[File], binaryChanges: DependencyChanges, previous: Analysis, - doCompile: (Set[File], DependencyChanges) => Analysis, classfileManager: ClassfileManager, cycleNum: Int, log: Logger, options: IncOptions): Analysis = + doCompile: (Set[File], DependencyChanges) => Analysis, classfileManager: ClassfileManager, cycleNum: Int): Analysis = if(invalidatedRaw.isEmpty) previous else { def debug(s: => String) = if (incDebug(options)) log.debug(s) else () val withPackageObjects = invalidatedRaw ++ invalidatedPackageObjects(invalidatedRaw, previous.relations) - val invalidated = expand(withPackageObjects, allSources, log, options) + val invalidated = expand(withPackageObjects, allSources) val pruned = Incremental.prune(invalidated, previous, classfileManager) debug("********* Pruned: \n" + pruned.relations + "\n*********") @@ -92,18 +92,18 @@ private class Incremental { val merged = pruned ++ fresh//.copy(relations = pruned.relations ++ fresh.relations, apis = pruned.apis ++ fresh.apis) debug("********* Merged: \n" + merged.relations + "\n*********") - val incChanges = changedIncremental(invalidated, previous.apis.internalAPI _, merged.apis.internalAPI _, log, options) + val incChanges = changedIncremental(invalidated, previous.apis.internalAPI _, merged.apis.internalAPI _) debug("\nChanges:\n" + incChanges) val transitiveStep = options.transitiveStep - val incInv = invalidateIncremental(merged.relations, merged.apis, incChanges, invalidated, cycleNum >= transitiveStep, log) - cycle(incInv, allSources, emptyChanges, merged, doCompile, classfileManager, cycleNum+1, log, options) + val incInv = invalidateIncremental(merged.relations, merged.apis, incChanges, invalidated, cycleNum >= transitiveStep) + cycle(incInv, allSources, emptyChanges, merged, doCompile, classfileManager, cycleNum+1) } private[this] def emptyChanges: DependencyChanges = new DependencyChanges { val modifiedBinaries = new Array[File](0) val modifiedClasses = new Array[String](0) def isEmpty = true } - private[this] def expand(invalidated: Set[File], all: Set[File], log: Logger, options: IncOptions): Set[File] = { + private[this] def expand(invalidated: Set[File], all: Set[File]): Set[File] = { val recompileAllFraction = options.recompileAllFraction if(invalidated.size > all.size * recompileAllFraction) { log.debug("Recompiling all " + all.size + " sources: invalidated sources (" + invalidated.size + ") exceeded " + (recompileAllFraction*100.0) + "% of all sources") @@ -123,7 +123,7 @@ private class Incremental { * NOTE: This method creates a new APIDiff instance on every invocation. */ private def logApiChanges[T](apiChanges: Iterable[APIChange[T]], oldAPIMapping: T => Source, - newAPIMapping: T => Source, log: Logger, options: IncOptions): Unit = { + newAPIMapping: T => Source): Unit = { val contextSize = options.apiDiffContextSize try { val apiDiff = new APIDiff @@ -154,19 +154,19 @@ private class Incremental { * providing the API before and after the last step. The functions should return * an empty API if the file did not/does not exist. */ - def changedIncremental[T](lastSources: collection.Set[T], oldAPI: T => Source, newAPI: T => Source, log: Logger, options: IncOptions): APIChanges[T] = + def changedIncremental[T](lastSources: collection.Set[T], oldAPI: T => Source, newAPI: T => Source): APIChanges[T] = { val oldApis = lastSources.toSeq map oldAPI val newApis = lastSources.toSeq map newAPI - val apiChanges = (lastSources, oldApis, newApis).zipped.flatMap { (src, oldApi, newApi) => sameSource(src, oldApi, newApi, log, options) } + val apiChanges = (lastSources, oldApis, newApis).zipped.flatMap { (src, oldApi, newApi) => sameSource(src, oldApi, newApi) } if (Incremental.apiDebug(options) && apiChanges.nonEmpty) { - logApiChanges(apiChanges, oldAPI, newAPI, log, options) + logApiChanges(apiChanges, oldAPI, newAPI) } new APIChanges(apiChanges) } - def sameSource[T](src: T, a: Source, b: Source, log: Logger, options: IncOptions): Option[APIChange[T]] = { + def sameSource[T](src: T, a: Source, b: Source): Option[APIChange[T]] = { // Clients of a modified source file (ie, one that doesn't satisfy `shortcutSameSource`) containing macros must be recompiled. val hasMacro = a.hasMacro || b.hasMacro if (shortcutSameSource(a, b)) { @@ -174,11 +174,11 @@ private class Incremental { } else { if (hasMacro && options.recompileOnMacroDef) { Some(APIChangeDueToMacroDefinition(src)) - } else sameAPI(src, a, b, log) + } else sameAPI(src, a, b) } } - def sameAPI[T](src: T, a: Source, b: Source, log: Logger): Option[SourceAPIChange[T]] = { + def sameAPI[T](src: T, a: Source, b: Source): Option[SourceAPIChange[T]] = { if (SameAPI(a,b)) None else { @@ -193,15 +193,15 @@ private class Incremental { } def changedInitial(entry: String => Option[File], sources: Set[File], previousAnalysis: Analysis, current: ReadStamps, - forEntry: File => Option[Analysis], options: IncOptions, log: Logger)(implicit equivS: Equiv[Stamp]): InitialChanges = + forEntry: File => Option[Analysis])(implicit equivS: Equiv[Stamp]): InitialChanges = { val previous = previousAnalysis.stamps val previousAPIs = previousAnalysis.apis val srcChanges = changes(previous.allInternalSources.toSet, sources, f => !equivS.equiv( previous.internalSource(f), current.internalSource(f) ) ) val removedProducts = previous.allProducts.filter( p => !equivS.equiv( previous.product(p), current.product(p) ) ).toSet - val binaryDepChanges = previous.allBinaries.filter( externalBinaryModified(entry, forEntry, previous, current, log)).toSet - val extChanges = changedIncremental(previousAPIs.allExternals, previousAPIs.externalAPI _, currentExternalAPI(entry, forEntry), log, options) + val binaryDepChanges = previous.allBinaries.filter( externalBinaryModified(entry, forEntry, previous, current)).toSet + val extChanges = changedIncremental(previousAPIs.allExternals, previousAPIs.externalAPI _, currentExternalAPI(entry, forEntry)) InitialChanges(srcChanges, removedProducts, binaryDepChanges, extChanges ) } @@ -215,14 +215,14 @@ private class Incremental { val (changed, unmodified) = inBoth.partition(existingModified) } - def invalidateIncremental(previous: Relations, apis: APIs, changes: APIChanges[File], recompiledSources: Set[File], transitive: Boolean, log: Logger): Set[File] = + def invalidateIncremental(previous: Relations, apis: APIs, changes: APIChanges[File], recompiledSources: Set[File], transitive: Boolean): Set[File] = { val dependsOnSrc = previous.usesInternalSrc _ val propagated = if(transitive) - transitiveDependencies(dependsOnSrc, changes.allModified.toSet, log) + transitiveDependencies(dependsOnSrc, changes.allModified.toSet) else - invalidateIntermediate(previous, changes, log) + invalidateIntermediate(previous, changes) val dups = invalidateDuplicates(previous) if(dups.nonEmpty) @@ -243,23 +243,23 @@ private class Incremental { /** Returns the transitive source dependencies of `initial`. * Because the intermediate steps do not pull in cycles, this result includes the initial files * if they are part of a cycle containing newly invalidated files . */ - def transitiveDependencies(dependsOnSrc: File => Set[File], initial: Set[File], log: Logger): Set[File] = + def transitiveDependencies(dependsOnSrc: File => Set[File], initial: Set[File]): Set[File] = { - val transitiveWithInitial = transitiveDeps(initial, log)(dependsOnSrc) - val transitivePartial = includeInitialCond(initial, transitiveWithInitial, dependsOnSrc, log) + val transitiveWithInitial = transitiveDeps(initial)(dependsOnSrc) + val transitivePartial = includeInitialCond(initial, transitiveWithInitial, dependsOnSrc) log.debug("Final step, transitive dependencies:\n\t" + transitivePartial) transitivePartial } /** Invalidates sources based on initially detected 'changes' to the sources, products, and dependencies.*/ - def invalidateInitial(previous: Relations, changes: InitialChanges, log: Logger): Set[File] = + def invalidateInitial(previous: Relations, changes: InitialChanges): Set[File] = { val srcChanges = changes.internalSrc val srcDirect = srcChanges.removed ++ srcChanges.removed.flatMap(previous.usesInternalSrc) ++ srcChanges.added ++ srcChanges.changed val byProduct = changes.removedProducts.flatMap(previous.produced) val byBinaryDep = changes.binaryDeps.flatMap(previous.usesBinary) val externalModifiedSources = changes.external.allModified.toSet - val byExtSrcDep = invalidateByExternal(previous, externalModifiedSources, log) //changes.external.modified.flatMap(previous.usesExternal) // ++ scopeInvalidations + val byExtSrcDep = invalidateByExternal(previous, externalModifiedSources) //changes.external.modified.flatMap(previous.usesExternal) // ++ scopeInvalidations checkAbsolute(srcChanges.added.toList) log.debug( "\nInitial source changes: \n\tremoved:" + srcChanges.removed + "\n\tadded: " + srcChanges.added + "\n\tmodified: " + srcChanges.changed + @@ -290,14 +290,14 @@ private class Incremental { } /** Sources invalidated by `external` sources in other projects according to the previous `relations`. */ - def invalidateByExternal(relations: Relations, external: Set[String], log: Logger): Set[File] = + def invalidateByExternal(relations: Relations, external: Set[String]): Set[File] = { // Propagate public inheritance dependencies transitively. // This differs from normal because we need the initial crossing from externals to sources in this project. val externalInheritedR = relations.publicInherited.external val byExternalInherited = external flatMap externalInheritedR.reverse val internalInheritedR = relations.publicInherited.internal - val transitiveInherited = transitiveDeps(byExternalInherited, log)(internalInheritedR.reverse _) + val transitiveInherited = transitiveDeps(byExternalInherited)(internalInheritedR.reverse _) // Get the direct dependencies of all sources transitively invalidated by inheritance val directA = transitiveInherited flatMap relations.direct.internal.reverse @@ -306,38 +306,38 @@ private class Incremental { transitiveInherited ++ directA ++ directB } /** Intermediate invalidation step: steps after the initial invalidation, but before the final transitive invalidation. */ - def invalidateIntermediate(relations: Relations, changes: APIChanges[File], log: Logger): Set[File] = + def invalidateIntermediate(relations: Relations, changes: APIChanges[File]): Set[File] = { def reverse(r: Relations.Source) = r.internal.reverse _ - invalidateSources(reverse(relations.direct), reverse(relations.publicInherited), changes, log) + invalidateSources(reverse(relations.direct), reverse(relations.publicInherited), changes) } /** Invalidates inheritance dependencies, transitively. Then, invalidates direct dependencies. Finally, excludes initial dependencies not * included in a cycle with newly invalidated sources. */ - private[this] def invalidateSources(directDeps: File => Set[File], publicInherited: File => Set[File], changes: APIChanges[File], log: Logger): Set[File] = + private[this] def invalidateSources(directDeps: File => Set[File], publicInherited: File => Set[File], changes: APIChanges[File]): Set[File] = { val initial = changes.allModified.toSet log.debug("Invalidating by inheritance (transitively)...") - val transitiveInherited = transitiveDeps(initial, log)(publicInherited) + val transitiveInherited = transitiveDeps(initial)(publicInherited) log.debug("Invalidated by transitive public inheritance: " + transitiveInherited) val direct = transitiveInherited flatMap directDeps log.debug("Invalidated by direct dependency: " + direct) val all = transitiveInherited ++ direct - includeInitialCond(initial, all, f => directDeps(f) ++ publicInherited(f), log) + includeInitialCond(initial, all, f => directDeps(f) ++ publicInherited(f)) } /** Conditionally include initial sources that are dependencies of newly invalidated sources. ** Initial sources included in this step can be because of a cycle, but not always. */ - private[this] def includeInitialCond(initial: Set[File], currentInvalidations: Set[File], allDeps: File => Set[File], log: Logger): Set[File] = + private[this] def includeInitialCond(initial: Set[File], currentInvalidations: Set[File], allDeps: File => Set[File]): Set[File] = { val newInv = currentInvalidations -- initial log.debug("New invalidations:\n\t" + newInv) - val transitiveOfNew = transitiveDeps(newInv, log)(allDeps) + val transitiveOfNew = transitiveDeps(newInv)(allDeps) val initialDependsOnNew = transitiveOfNew & initial log.debug("Previously invalidated, but (transitively) depend on new invalidations:\n\t" + initialDependsOnNew) newInv ++ initialDependsOnNew } - def externalBinaryModified(entry: String => Option[File], analysis: File => Option[Analysis], previous: Stamps, current: ReadStamps, log: Logger)(implicit equivS: Equiv[Stamp]): File => Boolean = + def externalBinaryModified(entry: String => Option[File], analysis: File => Option[Analysis], previous: Stamps, current: ReadStamps)(implicit equivS: Equiv[Stamp]): File => Boolean = dependsOn => { def inv(reason: String): Boolean = { @@ -389,7 +389,7 @@ private class Incremental { def orEmpty(o: Option[Source]): Source = o getOrElse APIs.emptySource def orTrue(o: Option[Boolean]): Boolean = o getOrElse true - private[this] def transitiveDeps[T](nodes: Iterable[T], log: Logger)(dependencies: T => Iterable[T]): Set[T] = + private[this] def transitiveDeps[T](nodes: Iterable[T])(dependencies: T => Iterable[T]): Set[T] = { val xs = new collection.mutable.HashSet[T] def all(from: T, tos: Iterable[T]): Unit = tos.foreach(to => visit(from, to)) From 946fd53a73a9fb247c5917e37bd1a30ed83ba073 Mon Sep 17 00:00:00 2001 From: Grzegorz Kossakowski Date: Thu, 5 Dec 2013 23:08:21 +0100 Subject: [PATCH 03/15] Introduce abstract `IncrementalCommon` class. Introduce an abstract `IncrementalCommon class that holds the implementation of incremental compiler that was previously done in `Incremental` class. Also, introduce `IncrementalDefaultImpl` that inherits from IncrementalCommon. This is the first step to introduce a design where most of incremental compiler's logic lives in IncrementalCommon and we have two subclasses: 1. Default, which holds implementation specific to the old algorithm known from sbt 0.13.0 2. NameHashing, which holds implementation specific to the name hashing algorithm This commit is purely a refactoring and does not change any behavior. --- compile/inc/src/main/scala/sbt/inc/Incremental.scala | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/compile/inc/src/main/scala/sbt/inc/Incremental.scala b/compile/inc/src/main/scala/sbt/inc/Incremental.scala index a55021c87..fe4f03861 100644 --- a/compile/inc/src/main/scala/sbt/inc/Incremental.scala +++ b/compile/inc/src/main/scala/sbt/inc/Incremental.scala @@ -21,7 +21,7 @@ object Incremental log: Logger, options: IncOptions)(implicit equivS: Equiv[Stamp]): (Boolean, Analysis) = { - val incremental = new Incremental(log, options) + val incremental = new IncrementalDefaultImpl(log, options) val initialChanges = incremental.changedInitial(entry, sources, previous, current, forEntry) val binaryChanges = new DependencyChanges { val modifiedBinaries = initialChanges.binaryDeps.toArray @@ -62,7 +62,7 @@ object Incremental } -private class Incremental(log: Logger, options: IncOptions) { +private abstract class IncrementalCommon(log: Logger, options: IncOptions) { val incDebugProp = "xsbt.inc.debug" private def incDebug(options: IncOptions): Boolean = options.relationsDebug || java.lang.Boolean.getBoolean(incDebugProp) @@ -449,3 +449,5 @@ private class Incremental(log: Logger, options: IncOptions) { def properSubPkg(testParent: Seq[String], testSub: Seq[String]) = testParent.length < testSub.length && testSub.startsWith(testParent) def pkgs(api: Source) = names(api :: Nil).map(pkg)*/ } + +private final class IncrementalDefaultImpl(log: Logger, options: IncOptions) extends IncrementalCommon(log, options) From 83a131e4f5c73e1b20899d5deb8ef7e4c0fb1517 Mon Sep 17 00:00:00 2001 From: Grzegorz Kossakowski Date: Thu, 5 Dec 2013 07:24:17 +0100 Subject: [PATCH 04/15] Introduce `IncrementalCommon.invalidateSource` method. In addition to `invalidateSources` we introduce `invalidateSource` that invalidates dependencies of a single source. This is needed for the name hashing algorithm because its invalidation logic depends on information about API changes of each source file individually. The refactoring is done in `IncrementalCommon` class so it affects the default implementation as well. However, this refactoring does not affect the result of invalidation in the default implementation. --- compile/inc/src/main/scala/sbt/inc/Incremental.scala | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/compile/inc/src/main/scala/sbt/inc/Incremental.scala b/compile/inc/src/main/scala/sbt/inc/Incremental.scala index fe4f03861..d4c6d7871 100644 --- a/compile/inc/src/main/scala/sbt/inc/Incremental.scala +++ b/compile/inc/src/main/scala/sbt/inc/Incremental.scala @@ -316,13 +316,20 @@ private abstract class IncrementalCommon(log: Logger, options: IncOptions) { private[this] def invalidateSources(directDeps: File => Set[File], publicInherited: File => Set[File], changes: APIChanges[File]): Set[File] = { val initial = changes.allModified.toSet + val all = (changes.apiChanges flatMap { change => + invalidateSource(directDeps, publicInherited, change) + }).toSet + includeInitialCond(initial, all, f => directDeps(f) ++ publicInherited(f)) + } + + private[this] def invalidateSource(directDeps: File => Set[File], publicInherited: File => Set[File], change: APIChange[File]): Set[File] = { log.debug("Invalidating by inheritance (transitively)...") - val transitiveInherited = transitiveDeps(initial)(publicInherited) + val transitiveInherited = transitiveDeps(Set(change.modified))(publicInherited) log.debug("Invalidated by transitive public inheritance: " + transitiveInherited) val direct = transitiveInherited flatMap directDeps log.debug("Invalidated by direct dependency: " + direct) val all = transitiveInherited ++ direct - includeInitialCond(initial, all, f => directDeps(f) ++ publicInherited(f)) + all } /** Conditionally include initial sources that are dependencies of newly invalidated sources. From 3643419e7c157bfa3c64f4ffd9584804856c189d Mon Sep 17 00:00:00 2001 From: Grzegorz Kossakowski Date: Thu, 5 Dec 2013 08:50:19 +0100 Subject: [PATCH 05/15] Make `invalidateSource` to take Relations. This way we'll be able to have a polymorphic implementation of this method in the future. One implementation will use the old dependency tracking mechanism and the other will use the new one (implemented for name hashing). --- .../src/main/scala/sbt/inc/Incremental.scala | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/compile/inc/src/main/scala/sbt/inc/Incremental.scala b/compile/inc/src/main/scala/sbt/inc/Incremental.scala index d4c6d7871..8d2235303 100644 --- a/compile/inc/src/main/scala/sbt/inc/Incremental.scala +++ b/compile/inc/src/main/scala/sbt/inc/Incremental.scala @@ -308,21 +308,26 @@ private abstract class IncrementalCommon(log: Logger, options: IncOptions) { /** Intermediate invalidation step: steps after the initial invalidation, but before the final transitive invalidation. */ def invalidateIntermediate(relations: Relations, changes: APIChanges[File]): Set[File] = { - def reverse(r: Relations.Source) = r.internal.reverse _ - invalidateSources(reverse(relations.direct), reverse(relations.publicInherited), changes) + invalidateSources(relations, changes) } /** Invalidates inheritance dependencies, transitively. Then, invalidates direct dependencies. Finally, excludes initial dependencies not * included in a cycle with newly invalidated sources. */ - private[this] def invalidateSources(directDeps: File => Set[File], publicInherited: File => Set[File], changes: APIChanges[File]): Set[File] = + private[this] def invalidateSources(relations: Relations, changes: APIChanges[File]): Set[File] = { val initial = changes.allModified.toSet val all = (changes.apiChanges flatMap { change => - invalidateSource(directDeps, publicInherited, change) + invalidateSource(relations, change) }).toSet - includeInitialCond(initial, all, f => directDeps(f) ++ publicInherited(f)) + includeInitialCond(initial, all, allDeps(relations)) } - private[this] def invalidateSource(directDeps: File => Set[File], publicInherited: File => Set[File], change: APIChange[File]): Set[File] = { + private[this] def allDeps(relations: Relations): File => Set[File] = + f => relations.direct.internal.reverse(f) + + private[this] def invalidateSource(relations: Relations, change: APIChange[File]): Set[File] = { + def reverse(r: Relations.Source) = r.internal.reverse _ + val directDeps: File => Set[File] = reverse(relations.direct) + val publicInherited: File => Set[File] = reverse(relations.publicInherited) log.debug("Invalidating by inheritance (transitively)...") val transitiveInherited = transitiveDeps(Set(change.modified))(publicInherited) log.debug("Invalidated by transitive public inheritance: " + transitiveInherited) From fdc72f3744143d9271f08e156c2eb0257f2f6b0e Mon Sep 17 00:00:00 2001 From: Grzegorz Kossakowski Date: Thu, 5 Dec 2013 20:47:57 +0100 Subject: [PATCH 06/15] The invalidateByExternal takes single external api change. Refactor the `invalidateByExternal` method to take single, external api change. Introduce `invalidateByAllExternal` that takes all APIChanges object. This way `invalidateByExternal` will have an access to APIChange object that represents changed name hashes once name hashing is merged. --- .../src/main/scala/sbt/inc/Incremental.scala | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/compile/inc/src/main/scala/sbt/inc/Incremental.scala b/compile/inc/src/main/scala/sbt/inc/Incremental.scala index 8d2235303..c434541fe 100644 --- a/compile/inc/src/main/scala/sbt/inc/Incremental.scala +++ b/compile/inc/src/main/scala/sbt/inc/Incremental.scala @@ -258,13 +258,12 @@ private abstract class IncrementalCommon(log: Logger, options: IncOptions) { val srcDirect = srcChanges.removed ++ srcChanges.removed.flatMap(previous.usesInternalSrc) ++ srcChanges.added ++ srcChanges.changed val byProduct = changes.removedProducts.flatMap(previous.produced) val byBinaryDep = changes.binaryDeps.flatMap(previous.usesBinary) - val externalModifiedSources = changes.external.allModified.toSet - val byExtSrcDep = invalidateByExternal(previous, externalModifiedSources) //changes.external.modified.flatMap(previous.usesExternal) // ++ scopeInvalidations + val byExtSrcDep = invalidateByAllExternal(previous, changes.external) //changes.external.modified.flatMap(previous.usesExternal) // ++ scopeInvalidations checkAbsolute(srcChanges.added.toList) log.debug( "\nInitial source changes: \n\tremoved:" + srcChanges.removed + "\n\tadded: " + srcChanges.added + "\n\tmodified: " + srcChanges.changed + "\nRemoved products: " + changes.removedProducts + - "\nModified external sources: " + externalModifiedSources + + "\nExternal API changes: " + changes.external + "\nModified binary dependencies: " + changes.binaryDeps + "\nInitial directly invalidated sources: " + srcDirect + "\n\nSources indirectly invalidated by:" + @@ -289,20 +288,26 @@ private abstract class IncrementalCommon(log: Logger, options: IncOptions) { } } + def invalidateByAllExternal(relations: Relations, externalAPIChanges: APIChanges[String]): Set[File] = { + (externalAPIChanges.apiChanges.flatMap { externalAPIChange => + invalidateByExternal(relations, externalAPIChange) + }).toSet + } + /** Sources invalidated by `external` sources in other projects according to the previous `relations`. */ - def invalidateByExternal(relations: Relations, external: Set[String]): Set[File] = - { + private def invalidateByExternal(relations: Relations, externalAPIChange: APIChange[String]): Set[File] = { + val modified = externalAPIChange.modified // Propagate public inheritance dependencies transitively. // This differs from normal because we need the initial crossing from externals to sources in this project. val externalInheritedR = relations.publicInherited.external - val byExternalInherited = external flatMap externalInheritedR.reverse + val byExternalInherited = externalInheritedR.reverse(modified) val internalInheritedR = relations.publicInherited.internal val transitiveInherited = transitiveDeps(byExternalInherited)(internalInheritedR.reverse _) // Get the direct dependencies of all sources transitively invalidated by inheritance val directA = transitiveInherited flatMap relations.direct.internal.reverse // Get the sources that directly depend on externals. This includes non-inheritance dependencies and is not transitive. - val directB = external flatMap relations.direct.external.reverse + val directB = relations.direct.external.reverse(modified) transitiveInherited ++ directA ++ directB } /** Intermediate invalidation step: steps after the initial invalidation, but before the final transitive invalidation. */ From 4ebbf3fb8b53773bfb68ed81554cd0cbc1db78a1 Mon Sep 17 00:00:00 2001 From: Grzegorz Kossakowski Date: Thu, 5 Dec 2013 22:52:45 +0100 Subject: [PATCH 07/15] Refactor code in IncrementalCommon and IncrementalDefaultImpl Move implementation of the following methods from IncrementalCommon to IncrementalDefaultImpl: * invalidatedPackageObjects * sameAPI * invalidateByExternal * allDeps * invalidateSource These are the methods that are expected to have different implementation in the name hashing algorithm. Hence, we make them abstract in IncrementalCommon so they can be implemented differently in subclasses. --- .../src/main/scala/sbt/inc/Incremental.scala | 98 +++++++++++-------- 1 file changed, 56 insertions(+), 42 deletions(-) diff --git a/compile/inc/src/main/scala/sbt/inc/Incremental.scala b/compile/inc/src/main/scala/sbt/inc/Incremental.scala index c434541fe..79c5ee8dd 100644 --- a/compile/inc/src/main/scala/sbt/inc/Incremental.scala +++ b/compile/inc/src/main/scala/sbt/inc/Incremental.scala @@ -112,10 +112,7 @@ private abstract class IncrementalCommon(log: Logger, options: IncOptions) { else invalidated } - // Package objects are fragile: if they inherit from an invalidated source, get "class file needed by package is missing" error - // This might be too conservative: we probably only need package objects for packages of invalidated sources. - private[this] def invalidatedPackageObjects(invalidated: Set[File], relations: Relations): Set[File] = - invalidated flatMap relations.publicInherited.internal.reverse filter { _.getName == "package.scala" } + protected def invalidatedPackageObjects(invalidated: Set[File], relations: Relations): Set[File] /** * Logs API changes using debug-level logging. The API are obtained using the APIDiff class. @@ -178,14 +175,7 @@ private abstract class IncrementalCommon(log: Logger, options: IncOptions) { } } - def sameAPI[T](src: T, a: Source, b: Source): Option[SourceAPIChange[T]] = { - if (SameAPI(a,b)) - None - else { - val sourceApiChange = SourceAPIChange(src) - Some(sourceApiChange) - } - } + protected def sameAPI[T](src: T, a: Source, b: Source): Option[SourceAPIChange[T]] def shortcutSameSource(a: Source, b: Source): Boolean = !a.hash.isEmpty && !b.hash.isEmpty && sameCompilation(a.compilation, b.compilation) && (a.hash.deep equals b.hash.deep) def sameCompilation(a: Compilation, b: Compilation): Boolean = a.startTime == b.startTime && a.outputs.corresponds(b.outputs){ @@ -295,21 +285,8 @@ private abstract class IncrementalCommon(log: Logger, options: IncOptions) { } /** Sources invalidated by `external` sources in other projects according to the previous `relations`. */ - private def invalidateByExternal(relations: Relations, externalAPIChange: APIChange[String]): Set[File] = { - val modified = externalAPIChange.modified - // Propagate public inheritance dependencies transitively. - // This differs from normal because we need the initial crossing from externals to sources in this project. - val externalInheritedR = relations.publicInherited.external - val byExternalInherited = externalInheritedR.reverse(modified) - val internalInheritedR = relations.publicInherited.internal - val transitiveInherited = transitiveDeps(byExternalInherited)(internalInheritedR.reverse _) + protected def invalidateByExternal(relations: Relations, externalAPIChange: APIChange[String]): Set[File] - // Get the direct dependencies of all sources transitively invalidated by inheritance - val directA = transitiveInherited flatMap relations.direct.internal.reverse - // Get the sources that directly depend on externals. This includes non-inheritance dependencies and is not transitive. - val directB = relations.direct.external.reverse(modified) - transitiveInherited ++ directA ++ directB - } /** Intermediate invalidation step: steps after the initial invalidation, but before the final transitive invalidation. */ def invalidateIntermediate(relations: Relations, changes: APIChanges[File]): Set[File] = { @@ -326,21 +303,9 @@ private abstract class IncrementalCommon(log: Logger, options: IncOptions) { includeInitialCond(initial, all, allDeps(relations)) } - private[this] def allDeps(relations: Relations): File => Set[File] = - f => relations.direct.internal.reverse(f) + protected def allDeps(relations: Relations): File => Set[File] - private[this] def invalidateSource(relations: Relations, change: APIChange[File]): Set[File] = { - def reverse(r: Relations.Source) = r.internal.reverse _ - val directDeps: File => Set[File] = reverse(relations.direct) - val publicInherited: File => Set[File] = reverse(relations.publicInherited) - log.debug("Invalidating by inheritance (transitively)...") - val transitiveInherited = transitiveDeps(Set(change.modified))(publicInherited) - log.debug("Invalidated by transitive public inheritance: " + transitiveInherited) - val direct = transitiveInherited flatMap directDeps - log.debug("Invalidated by direct dependency: " + direct) - val all = transitiveInherited ++ direct - all - } + protected def invalidateSource(relations: Relations, change: APIChange[File]): Set[File] /** Conditionally include initial sources that are dependencies of newly invalidated sources. ** Initial sources included in this step can be because of a cycle, but not always. */ @@ -406,7 +371,7 @@ private abstract class IncrementalCommon(log: Logger, options: IncOptions) { def orEmpty(o: Option[Source]): Source = o getOrElse APIs.emptySource def orTrue(o: Option[Boolean]): Boolean = o getOrElse true - private[this] def transitiveDeps[T](nodes: Iterable[T])(dependencies: T => Iterable[T]): Set[T] = + protected def transitiveDeps[T](nodes: Iterable[T])(dependencies: T => Iterable[T]): Set[T] = { val xs = new collection.mutable.HashSet[T] def all(from: T, tos: Iterable[T]): Unit = tos.foreach(to => visit(from, to)) @@ -467,4 +432,53 @@ private abstract class IncrementalCommon(log: Logger, options: IncOptions) { def pkgs(api: Source) = names(api :: Nil).map(pkg)*/ } -private final class IncrementalDefaultImpl(log: Logger, options: IncOptions) extends IncrementalCommon(log, options) +private final class IncrementalDefaultImpl(log: Logger, options: IncOptions) extends IncrementalCommon(log, options) { + + // Package objects are fragile: if they inherit from an invalidated source, get "class file needed by package is missing" error + // This might be too conservative: we probably only need package objects for packages of invalidated sources. + override protected def invalidatedPackageObjects(invalidated: Set[File], relations: Relations): Set[File] = + invalidated flatMap relations.publicInherited.internal.reverse filter { _.getName == "package.scala" } + + override protected def sameAPI[T](src: T, a: Source, b: Source): Option[SourceAPIChange[T]] = { + if (SameAPI(a,b)) + None + else { + val sourceApiChange = SourceAPIChange(src) + Some(sourceApiChange) + } + } + + /** Invalidates sources based on initially detected 'changes' to the sources, products, and dependencies.*/ + override protected def invalidateByExternal(relations: Relations, externalAPIChange: APIChange[String]): Set[File] = { + val modified = externalAPIChange.modified + // Propagate public inheritance dependencies transitively. + // This differs from normal because we need the initial crossing from externals to sources in this project. + val externalInheritedR = relations.publicInherited.external + val byExternalInherited = externalInheritedR.reverse(modified) + val internalInheritedR = relations.publicInherited.internal + val transitiveInherited = transitiveDeps(byExternalInherited)(internalInheritedR.reverse _) + + // Get the direct dependencies of all sources transitively invalidated by inheritance + val directA = transitiveInherited flatMap relations.direct.internal.reverse + // Get the sources that directly depend on externals. This includes non-inheritance dependencies and is not transitive. + val directB = relations.direct.external.reverse(modified) + transitiveInherited ++ directA ++ directB + } + + override protected def invalidateSource(relations: Relations, change: APIChange[File]): Set[File] = { + def reverse(r: Relations.Source) = r.internal.reverse _ + val directDeps: File => Set[File] = reverse(relations.direct) + val publicInherited: File => Set[File] = reverse(relations.publicInherited) + log.debug("Invalidating by inheritance (transitively)...") + val transitiveInherited = transitiveDeps(Set(change.modified))(publicInherited) + log.debug("Invalidated by transitive public inheritance: " + transitiveInherited) + val direct = transitiveInherited flatMap directDeps + log.debug("Invalidated by direct dependency: " + direct) + val all = transitiveInherited ++ direct + all + } + + override protected def allDeps(relations: Relations): File => Set[File] = + f => relations.direct.internal.reverse(f) + +} From 88528a43cba84031de63e4a2e5634f0a09f6a930 Mon Sep 17 00:00:00 2001 From: Grzegorz Kossakowski Date: Fri, 6 Dec 2013 15:03:53 +0100 Subject: [PATCH 08/15] Fix a few mistakes related to IncOptions.recompileOnMacroDef The 39036e7c2097c5597df5e66a9d4923dd5154a510 introduced `recompileOnMacroDef` option to IncOptions. However, not all necessary logic has been changed. This commit fixes that: * `copy` method does not forget the value of the `recompileOnMacroDef` flag * `productArity` has been increased to match the arity of the class * `productElement` returns the value of `recompileOnMacroDef` flag * `hashCode` and `equals` methods take into account value of `recompileOnMacroDef` flag * fix the name of the key for `recompileOnMacroDef` flag --- compile/inc/src/main/scala/sbt/inc/IncOptions.scala | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/compile/inc/src/main/scala/sbt/inc/IncOptions.scala b/compile/inc/src/main/scala/sbt/inc/IncOptions.scala index 7077d2291..9b366460b 100644 --- a/compile/inc/src/main/scala/sbt/inc/IncOptions.scala +++ b/compile/inc/src/main/scala/sbt/inc/IncOptions.scala @@ -112,14 +112,14 @@ final class IncOptions( apiDumpDirectory: Option[java.io.File] = this.apiDumpDirectory, newClassfileManager: () => ClassfileManager = this.newClassfileManager): IncOptions = { new IncOptions(transitiveStep, recompileAllFraction, relationsDebug, apiDebug, apiDiffContextSize, - apiDumpDirectory, newClassfileManager) + apiDumpDirectory, newClassfileManager, recompileOnMacroDef) } @deprecated("Methods generated for case class will be removed in the future.", "0.13.2") override def productPrefix: String = "IncOptions" @deprecated("Methods generated for case class will be removed in the future.", "0.13.2") - def productArity: Int = 7 + def productArity: Int = 8 @deprecated("Methods generated for case class will be removed in the future.", "0.13.2") def productElement(x$1: Int): Any = x$1 match { @@ -130,6 +130,7 @@ final class IncOptions( case 4 => IncOptions.this.apiDiffContextSize case 5 => IncOptions.this.apiDumpDirectory case 6 => IncOptions.this.newClassfileManager + case 7 => IncOptions.this.recompileOnMacroDef case _ => throw new IndexOutOfBoundsException(x$1.toString()) } @@ -149,7 +150,8 @@ final class IncOptions( acc = Statics.mix(acc, apiDiffContextSize) acc = Statics.mix(acc, Statics.anyHash(apiDumpDirectory)) acc = Statics.mix(acc, Statics.anyHash(newClassfileManager)) - Statics.finalizeHash(acc, 7) + acc = Statics.mix(acc, if (recompileOnMacroDef) 1231 else 1237) + Statics.finalizeHash(acc, 8) } override def toString(): String = scala.runtime.ScalaRunTime._toString(IncOptions.this) @@ -160,7 +162,8 @@ final class IncOptions( transitiveStep == IncOptions$1.transitiveStep && recompileAllFraction == IncOptions$1.recompileAllFraction && relationsDebug == IncOptions$1.relationsDebug && apiDebug == IncOptions$1.apiDebug && apiDiffContextSize == IncOptions$1.apiDiffContextSize && apiDumpDirectory == IncOptions$1.apiDumpDirectory && - newClassfileManager == IncOptions$1.newClassfileManager + newClassfileManager == IncOptions$1.newClassfileManager && + recompileOnMacroDef == IncOptions$1.recompileOnMacroDef })) } //- EXPANDED CASE CLASS METHOD END -// @@ -217,7 +220,7 @@ object IncOptions extends Serializable { private val apiDebugKey = "apiDebug" private val apiDumpDirectoryKey = "apiDumpDirectory" private val apiDiffContextSizeKey = "apiDiffContextSize" - private val recompileOnMacroDefKey = "recompileOnMacroDefKey" + private val recompileOnMacroDefKey = "recompileOnMacroDef" def fromStringMap(m: java.util.Map[String, String]): IncOptions = { // all the code below doesn't look like idiomatic Scala for a good reason: we are working with Java API From cacb17fb2ea8210d9c8b5c00b0466c4ed4fba44a Mon Sep 17 00:00:00 2001 From: Grzegorz Kossakowski Date: Fri, 6 Dec 2013 15:44:03 +0100 Subject: [PATCH 09/15] Add `nameHashing` option to IncOptions This option is not used anywhere yet. This commit just contains all the boilerplate needed in order to introduce a new field to IncOptions class. --- .../src/main/scala/sbt/inc/IncOptions.scala | 68 +++++++++++++------ 1 file changed, 49 insertions(+), 19 deletions(-) diff --git a/compile/inc/src/main/scala/sbt/inc/IncOptions.scala b/compile/inc/src/main/scala/sbt/inc/IncOptions.scala index 9b366460b..0e634aa4f 100644 --- a/compile/inc/src/main/scala/sbt/inc/IncOptions.scala +++ b/compile/inc/src/main/scala/sbt/inc/IncOptions.scala @@ -51,57 +51,76 @@ final class IncOptions( * Determines whether incremental compiler should recompile all dependencies of a file * that contains a macro definition. */ - val recompileOnMacroDef: Boolean + val recompileOnMacroDef: Boolean, + /** + * Determines whether incremental compiler uses the new algorithm known as name hashing. + * + * This flag is disabled by default so incremental compiler's behavior is the same as in sbt 0.13.0. + * + * IMPLEMENTATION NOTE: + * Enabling this flag enables a few additional functionalities that are needed by the name hashing algorithm: + * + * 1. New dependency source tracking is used. See `sbt.inc.Relations` for details. + * 2. Used names extraction and tracking is enabled. See `sbt.inc.Relations` for details as well. + * 3. Hashing of public names is enabled. See `sbt.inc.AnalysisCallback` for details. + * + */ + val nameHashing: Boolean ) extends Product with Serializable { /** * Secondary constructor introduced to make IncOptions to be binary compatible with version that didn't have - * `recompileOnMacroDef` filed defined. + * `recompileOnMacroDef` and `nameHashing` fields defined. */ def this(transitiveStep: Int, recompileAllFraction: Double, relationsDebug: Boolean, apiDebug: Boolean, apiDiffContextSize: Int, apiDumpDirectory: Option[java.io.File], newClassfileManager: () => ClassfileManager) = { this(transitiveStep, recompileAllFraction, relationsDebug, apiDebug, apiDiffContextSize, - apiDumpDirectory, newClassfileManager, IncOptions.recompileOnMacroDefDefault) + apiDumpDirectory, newClassfileManager, IncOptions.recompileOnMacroDefDefault, IncOptions.nameHashingDefault) } def withTransitiveStep(transitiveStep: Int): IncOptions = { new IncOptions(transitiveStep, recompileAllFraction, relationsDebug, apiDebug, apiDiffContextSize, - apiDumpDirectory, newClassfileManager, recompileOnMacroDef) + apiDumpDirectory, newClassfileManager, recompileOnMacroDef, nameHashing) } def withRecompileAllFraction(recompileAllFraction: Double): IncOptions = { new IncOptions(transitiveStep, recompileAllFraction, relationsDebug, apiDebug, apiDiffContextSize, - apiDumpDirectory, newClassfileManager, recompileOnMacroDef) + apiDumpDirectory, newClassfileManager, recompileOnMacroDef, nameHashing) } def withRelationsDebug(relationsDebug: Boolean): IncOptions = { new IncOptions(transitiveStep, recompileAllFraction, relationsDebug, apiDebug, apiDiffContextSize, - apiDumpDirectory, newClassfileManager, recompileOnMacroDef) + apiDumpDirectory, newClassfileManager, recompileOnMacroDef, nameHashing) } def withApiDebug(apiDebug: Boolean): IncOptions = { new IncOptions(transitiveStep, recompileAllFraction, relationsDebug, apiDebug, apiDiffContextSize, - apiDumpDirectory, newClassfileManager, recompileOnMacroDef) + apiDumpDirectory, newClassfileManager, recompileOnMacroDef, nameHashing) } def withApiDiffContextSize(apiDiffContextSize: Int): IncOptions = { new IncOptions(transitiveStep, recompileAllFraction, relationsDebug, apiDebug, apiDiffContextSize, - apiDumpDirectory, newClassfileManager, recompileOnMacroDef) + apiDumpDirectory, newClassfileManager, recompileOnMacroDef, nameHashing) } def withApiDumpDirectory(apiDumpDirectory: Option[File]): IncOptions = { new IncOptions(transitiveStep, recompileAllFraction, relationsDebug, apiDebug, apiDiffContextSize, - apiDumpDirectory, newClassfileManager, recompileOnMacroDef) + apiDumpDirectory, newClassfileManager, recompileOnMacroDef, nameHashing) } def withNewClassfileManager(newClassfileManager: () => ClassfileManager): IncOptions = { new IncOptions(transitiveStep, recompileAllFraction, relationsDebug, apiDebug, apiDiffContextSize, - apiDumpDirectory, newClassfileManager, recompileOnMacroDef) + apiDumpDirectory, newClassfileManager, recompileOnMacroDef, nameHashing) } def withRecompileOnMacroDef(recompileOnMacroDef: Boolean): IncOptions = { new IncOptions(transitiveStep, recompileAllFraction, relationsDebug, apiDebug, apiDiffContextSize, - apiDumpDirectory, newClassfileManager, recompileOnMacroDef) + apiDumpDirectory, newClassfileManager, recompileOnMacroDef, nameHashing) + } + + def withNameHashing(nameHashing: Boolean): IncOptions = { + new IncOptions(transitiveStep, recompileAllFraction, relationsDebug, apiDebug, apiDiffContextSize, + apiDumpDirectory, newClassfileManager, recompileOnMacroDef, nameHashing) } //- EXPANDED CASE CLASS METHOD BEGIN -// @@ -112,14 +131,14 @@ final class IncOptions( apiDumpDirectory: Option[java.io.File] = this.apiDumpDirectory, newClassfileManager: () => ClassfileManager = this.newClassfileManager): IncOptions = { new IncOptions(transitiveStep, recompileAllFraction, relationsDebug, apiDebug, apiDiffContextSize, - apiDumpDirectory, newClassfileManager, recompileOnMacroDef) + apiDumpDirectory, newClassfileManager, recompileOnMacroDef, nameHashing) } @deprecated("Methods generated for case class will be removed in the future.", "0.13.2") override def productPrefix: String = "IncOptions" @deprecated("Methods generated for case class will be removed in the future.", "0.13.2") - def productArity: Int = 8 + def productArity: Int = 9 @deprecated("Methods generated for case class will be removed in the future.", "0.13.2") def productElement(x$1: Int): Any = x$1 match { @@ -131,6 +150,7 @@ final class IncOptions( case 5 => IncOptions.this.apiDumpDirectory case 6 => IncOptions.this.newClassfileManager case 7 => IncOptions.this.recompileOnMacroDef + case 8 => IncOptions.this.nameHashing case _ => throw new IndexOutOfBoundsException(x$1.toString()) } @@ -151,7 +171,8 @@ final class IncOptions( acc = Statics.mix(acc, Statics.anyHash(apiDumpDirectory)) acc = Statics.mix(acc, Statics.anyHash(newClassfileManager)) acc = Statics.mix(acc, if (recompileOnMacroDef) 1231 else 1237) - Statics.finalizeHash(acc, 8) + acc = Statics.mix(acc, if (nameHashing) 1231 else 1237) + Statics.finalizeHash(acc, 9) } override def toString(): String = scala.runtime.ScalaRunTime._toString(IncOptions.this) @@ -163,7 +184,7 @@ final class IncOptions( relationsDebug == IncOptions$1.relationsDebug && apiDebug == IncOptions$1.apiDebug && apiDiffContextSize == IncOptions$1.apiDiffContextSize && apiDumpDirectory == IncOptions$1.apiDumpDirectory && newClassfileManager == IncOptions$1.newClassfileManager && - recompileOnMacroDef == IncOptions$1.recompileOnMacroDef + recompileOnMacroDef == IncOptions$1.recompileOnMacroDef && nameHashing == IncOptions$1.nameHashing })) } //- EXPANDED CASE CLASS METHOD END -// @@ -171,6 +192,7 @@ final class IncOptions( object IncOptions extends Serializable { private val recompileOnMacroDefDefault: Boolean = true + private val nameHashingDefault: Boolean = false val Default = IncOptions( // 1. recompile changed sources // 2(3). recompile direct dependencies and transitive public inheritance dependencies of sources with API changes in 1(2). @@ -182,7 +204,8 @@ object IncOptions extends Serializable { apiDiffContextSize = 5, apiDumpDirectory = None, newClassfileManager = ClassfileManager.deleteImmediately, - recompileOnMacroDef = recompileOnMacroDefDefault + recompileOnMacroDef = recompileOnMacroDefDefault, + nameHashing = nameHashingDefault ) //- EXPANDED CASE CLASS METHOD BEGIN -// final override def toString(): String = "IncOptions" @@ -195,9 +218,10 @@ object IncOptions extends Serializable { } def apply(transitiveStep: Int, recompileAllFraction: Double, relationsDebug: Boolean, apiDebug: Boolean, apiDiffContextSize: Int, apiDumpDirectory: Option[java.io.File], - newClassfileManager: () => ClassfileManager, recompileOnMacroDef: Boolean): IncOptions = { + newClassfileManager: () => ClassfileManager, recompileOnMacroDef: Boolean, + nameHashing: Boolean): IncOptions = { new IncOptions(transitiveStep, recompileAllFraction, relationsDebug, apiDebug, apiDiffContextSize, - apiDumpDirectory, newClassfileManager, recompileOnMacroDef) + apiDumpDirectory, newClassfileManager, recompileOnMacroDef, nameHashing) } @deprecated("Methods generated for case class will be removed in the future.", "0.13.2") def unapply(x$0: IncOptions): Option[(Int, Double, Boolean, Boolean, Int, Option[java.io.File], () => AnyRef)] = { @@ -221,6 +245,7 @@ object IncOptions extends Serializable { private val apiDumpDirectoryKey = "apiDumpDirectory" private val apiDiffContextSizeKey = "apiDiffContextSize" private val recompileOnMacroDefKey = "recompileOnMacroDef" + private val nameHashingKey = "nameHashing" def fromStringMap(m: java.util.Map[String, String]): IncOptions = { // all the code below doesn't look like idiomatic Scala for a good reason: we are working with Java API @@ -254,9 +279,13 @@ object IncOptions extends Serializable { val k = recompileOnMacroDefKey if (m.containsKey(k)) m.get(k).toBoolean else Default.recompileOnMacroDef } + def getNameHashing: Boolean = { + val k = nameHashingKey + if (m.containsKey(k)) m.get(k).toBoolean else Default.nameHashing + } new IncOptions(getTransitiveStep, getRecompileAllFraction, getRelationsDebug, getApiDebug, getApiDiffContextSize, - getApiDumpDirectory, ClassfileManager.deleteImmediately, getRecompileOnMacroDef) + getApiDumpDirectory, ClassfileManager.deleteImmediately, getRecompileOnMacroDef, getNameHashing) } def toStringMap(o: IncOptions): java.util.Map[String, String] = { @@ -268,6 +297,7 @@ object IncOptions extends Serializable { o.apiDumpDirectory.foreach(f => m.put(apiDumpDirectoryKey, f.toString)) m.put(apiDiffContextSizeKey, o.apiDiffContextSize.toString) m.put(recompileOnMacroDefKey, o.recompileOnMacroDef.toString) + m.put(nameHashingKey, o.nameHashing.toString) m } } From d70bc51b6d077fac3c112f7b61189b4eb422a9b2 Mon Sep 17 00:00:00 2001 From: Grzegorz Kossakowski Date: Fri, 6 Dec 2013 15:53:33 +0100 Subject: [PATCH 10/15] Use `nameHashing` option throughout incremental compiler There are two categories of places in the code that need to refer to `nameHashing` option: * places where Analysis object is created so it gets proper implementation of underlying Relations object * places with logic that is specifically designed to be enabled by that option This commit covers both cases. --- compile/inc/src/main/scala/sbt/inc/Analysis.scala | 2 ++ compile/inc/src/main/scala/sbt/inc/Compile.scala | 4 ++-- compile/inc/src/main/scala/sbt/inc/Incremental.scala | 1 + .../src/main/scala/sbt/compiler/AggressiveCompile.scala | 6 +++--- .../src/main/scala/sbt/compiler/IncrementalCompiler.scala | 6 ++++++ 5 files changed, 14 insertions(+), 5 deletions(-) diff --git a/compile/inc/src/main/scala/sbt/inc/Analysis.scala b/compile/inc/src/main/scala/sbt/inc/Analysis.scala index 212b6fc6d..aaa63918d 100644 --- a/compile/inc/src/main/scala/sbt/inc/Analysis.scala +++ b/compile/inc/src/main/scala/sbt/inc/Analysis.scala @@ -56,6 +56,8 @@ trait Analysis object Analysis { lazy val Empty: Analysis = new MAnalysis(Stamps.empty, APIs.empty, Relations.empty, SourceInfos.empty, Compilations.empty) + private[sbt] def empty(nameHashing: Boolean): Analysis = new MAnalysis(Stamps.empty, APIs.empty, + Relations.empty(nameHashing = nameHashing), SourceInfos.empty, Compilations.empty) /** Merge multiple analysis objects into one. Deps will be internalized as needed. */ def merge(analyses: Traversable[Analysis]): Analysis = { diff --git a/compile/inc/src/main/scala/sbt/inc/Compile.scala b/compile/inc/src/main/scala/sbt/inc/Compile.scala index a5b56a5c5..edf714f06 100644 --- a/compile/inc/src/main/scala/sbt/inc/Compile.scala +++ b/compile/inc/src/main/scala/sbt/inc/Compile.scala @@ -157,9 +157,9 @@ private final class AnalysisCallback(internalMap: File => Option[File], external def usedName(sourceFile: File, name: String) = add(usedNames, sourceFile, name) - def nameHashing: Boolean = false // TODO: define the flag in IncOptions which controls this + def nameHashing: Boolean = options.nameHashing - def get: Analysis = addUsedNames( addCompilation( addExternals( addBinaries( addProducts( addSources(Analysis.Empty) ) ) ) ) ) + def get: Analysis = addUsedNames( addCompilation( addExternals( addBinaries( addProducts( addSources(Analysis.empty(nameHashing = nameHashing)) ) ) ) ) ) def addProducts(base: Analysis): Analysis = addAll(base, classes) { case (a, src, (prod, name)) => a.addProduct(src, prod, current product prod, name ) } def addBinaries(base: Analysis): Analysis = addAll(base, binaryDeps)( (a, src, bin) => a.addBinaryDep(src, bin, binaryClassName(bin), current binary bin) ) def addSources(base: Analysis): Analysis = diff --git a/compile/inc/src/main/scala/sbt/inc/Incremental.scala b/compile/inc/src/main/scala/sbt/inc/Incremental.scala index 79c5ee8dd..dc86f818d 100644 --- a/compile/inc/src/main/scala/sbt/inc/Incremental.scala +++ b/compile/inc/src/main/scala/sbt/inc/Incremental.scala @@ -21,6 +21,7 @@ object Incremental log: Logger, options: IncOptions)(implicit equivS: Equiv[Stamp]): (Boolean, Analysis) = { + assert(!options.nameHashing, "We don't support name hashing algorithm yet.") val incremental = new IncrementalDefaultImpl(log, options) val initialChanges = incremental.changedInitial(entry, sources, previous, current, forEntry) val binaryChanges = new DependencyChanges { diff --git a/compile/integration/src/main/scala/sbt/compiler/AggressiveCompile.scala b/compile/integration/src/main/scala/sbt/compiler/AggressiveCompile.scala index 5fc1bafba..fec36db56 100644 --- a/compile/integration/src/main/scala/sbt/compiler/AggressiveCompile.scala +++ b/compile/integration/src/main/scala/sbt/compiler/AggressiveCompile.scala @@ -61,7 +61,7 @@ class AggressiveCompile(cacheFile: File) cache: GlobalsCache, incrementalCompilerOptions: IncOptions)(implicit log: Logger): Analysis = { - val (previousAnalysis, previousSetup) = extract(store.get()) + val (previousAnalysis, previousSetup) = extract(store.get(), incrementalCompilerOptions) if(skip) previousAnalysis else { @@ -169,11 +169,11 @@ class AggressiveCompile(cacheFile: File) if(!combined.isEmpty) log.info(combined.mkString("Compiling ", " and ", " to " + outputDirs.map(_.getAbsolutePath).mkString(",") + "...")) } - private def extract(previous: Option[(Analysis, CompileSetup)]): (Analysis, Option[CompileSetup]) = + private def extract(previous: Option[(Analysis, CompileSetup)], incOptions: IncOptions): (Analysis, Option[CompileSetup]) = previous match { case Some((an, setup)) => (an, Some(setup)) - case None => (Analysis.Empty, None) + case None => (Analysis.empty(nameHashing = incOptions.nameHashing), None) } def javaOnly(f: File) = f.getName.endsWith(".java") diff --git a/compile/integration/src/main/scala/sbt/compiler/IncrementalCompiler.scala b/compile/integration/src/main/scala/sbt/compiler/IncrementalCompiler.scala index 8600f6a70..68ad63f2c 100644 --- a/compile/integration/src/main/scala/sbt/compiler/IncrementalCompiler.scala +++ b/compile/integration/src/main/scala/sbt/compiler/IncrementalCompiler.scala @@ -36,9 +36,15 @@ object IC extends IncrementalCompiler[Analysis, AnalyzingCompiler] def readCache(file: File): Maybe[(Analysis, CompileSetup)] = try { Maybe.just(readCacheUncaught(file)) } catch { case _: Exception => Maybe.nothing() } + @deprecated("Use overloaded variant which takes `IncOptions` as parameter.", "0.13.2") def readAnalysis(file: File): Analysis = try { readCacheUncaught(file)._1 } catch { case _: Exception => Analysis.Empty } + def readAnalysis(file: File, incOptions: IncOptions): Analysis = + try { readCacheUncaught(file)._1 } catch { + case _: Exception => Analysis.empty(nameHashing = incOptions.nameHashing) + } + def readCacheUncaught(file: File): (Analysis, CompileSetup) = Using.fileReader(IO.utf8)(file) { reader => TextAnalysisFormat.read(reader) } } From c2dc6cd529c0c945699012c291036873f06954f6 Mon Sep 17 00:00:00 2001 From: Grzegorz Kossakowski Date: Thu, 12 Dec 2013 15:57:55 +0100 Subject: [PATCH 11/15] Test for inheritance and member references. Add a test-case that documents current behavior of incremental compiler when it comes to invalidating dependencies that arise from inheritance and member references. --- .../transitive-memberRef/build.sbt | 36 +++++++++++++++++++ .../transitive-memberRef/changes/A1.scala | 5 +++ .../src/main/scala/A.scala | 3 ++ .../src/main/scala/B.scala | 3 ++ .../src/main/scala/C.scala | 3 ++ .../src/main/scala/D.scala | 3 ++ .../src/main/scala/X.scala | 5 +++ .../src/main/scala/Y.scala | 5 +++ .../transitive-memberRef/test | 11 ++++++ 9 files changed, 74 insertions(+) create mode 100644 sbt/src/sbt-test/source-dependencies/transitive-memberRef/build.sbt create mode 100644 sbt/src/sbt-test/source-dependencies/transitive-memberRef/changes/A1.scala create mode 100644 sbt/src/sbt-test/source-dependencies/transitive-memberRef/src/main/scala/A.scala create mode 100644 sbt/src/sbt-test/source-dependencies/transitive-memberRef/src/main/scala/B.scala create mode 100644 sbt/src/sbt-test/source-dependencies/transitive-memberRef/src/main/scala/C.scala create mode 100644 sbt/src/sbt-test/source-dependencies/transitive-memberRef/src/main/scala/D.scala create mode 100644 sbt/src/sbt-test/source-dependencies/transitive-memberRef/src/main/scala/X.scala create mode 100644 sbt/src/sbt-test/source-dependencies/transitive-memberRef/src/main/scala/Y.scala create mode 100644 sbt/src/sbt-test/source-dependencies/transitive-memberRef/test diff --git a/sbt/src/sbt-test/source-dependencies/transitive-memberRef/build.sbt b/sbt/src/sbt-test/source-dependencies/transitive-memberRef/build.sbt new file mode 100644 index 000000000..ef32473dc --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/transitive-memberRef/build.sbt @@ -0,0 +1,36 @@ +logLevel := Level.Debug + +// disable sbt's heauristic which recompiles everything in case +// some fraction (e.g. 50%) of files is scheduled to be recompiled +// in this test we want precise information about recompiled files +// which that heuristic would distort +incOptions := incOptions.value.copy(recompileAllFraction = 1.0) + +/* Performs checks related to compilations: + * a) checks in which compilation given set of files was recompiled + * b) checks overall number of compilations performed + */ +TaskKey[Unit]("check-compilations") <<= (compile in Compile, scalaSource in Compile) map { (a: sbt.inc.Analysis, src: java.io.File) => + def relative(f: java.io.File): java.io.File = f.relativeTo(src) getOrElse f + val allCompilations = a.compilations.allCompilations + val recompiledFiles: Seq[Set[java.io.File]] = allCompilations map { c => + val recompiledFiles = a.apis.internal.collect { + case (file, api) if api.compilation.startTime == c.startTime => relative(file) + } + recompiledFiles.toSet + } + def recompiledFilesInIteration(iteration: Int, fileNames: Set[String]) = { + val files = fileNames.map(new java.io.File(_)) + assert(recompiledFiles(iteration) == files, "%s != %s".format(recompiledFiles(iteration), files)) + } + // Y.scala is compiled only at the beginning as changes to A.scala do not affect it + recompiledFilesInIteration(0, Set("Y.scala")) + // A.scala is changed and recompiled + recompiledFilesInIteration(1, Set("A.scala")) + // change in A.scala causes recompilation of B.scala, C.scala, D.scala which depend on transtiviely + // and by inheritance on A.scala + // X.scala is also recompiled because it depends by member reference on B.scala + // Note that Y.scala is not recompiled because it depends just on X through member reference dependency + recompiledFilesInIteration(2, Set("B.scala", "C.scala", "D.scala", "X.scala")) + assert(allCompilations.size == 3) +} diff --git a/sbt/src/sbt-test/source-dependencies/transitive-memberRef/changes/A1.scala b/sbt/src/sbt-test/source-dependencies/transitive-memberRef/changes/A1.scala new file mode 100644 index 000000000..63a2739e1 --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/transitive-memberRef/changes/A1.scala @@ -0,0 +1,5 @@ +package test + +class A { + def foo: Int = 23 +} diff --git a/sbt/src/sbt-test/source-dependencies/transitive-memberRef/src/main/scala/A.scala b/sbt/src/sbt-test/source-dependencies/transitive-memberRef/src/main/scala/A.scala new file mode 100644 index 000000000..1b0178fd9 --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/transitive-memberRef/src/main/scala/A.scala @@ -0,0 +1,3 @@ +package test + +class A diff --git a/sbt/src/sbt-test/source-dependencies/transitive-memberRef/src/main/scala/B.scala b/sbt/src/sbt-test/source-dependencies/transitive-memberRef/src/main/scala/B.scala new file mode 100644 index 000000000..b9913245b --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/transitive-memberRef/src/main/scala/B.scala @@ -0,0 +1,3 @@ +package test + +class B extends A diff --git a/sbt/src/sbt-test/source-dependencies/transitive-memberRef/src/main/scala/C.scala b/sbt/src/sbt-test/source-dependencies/transitive-memberRef/src/main/scala/C.scala new file mode 100644 index 000000000..4ce04f8bf --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/transitive-memberRef/src/main/scala/C.scala @@ -0,0 +1,3 @@ +package test + +class C extends B diff --git a/sbt/src/sbt-test/source-dependencies/transitive-memberRef/src/main/scala/D.scala b/sbt/src/sbt-test/source-dependencies/transitive-memberRef/src/main/scala/D.scala new file mode 100644 index 000000000..eff328ce5 --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/transitive-memberRef/src/main/scala/D.scala @@ -0,0 +1,3 @@ +package test + +class D extends C diff --git a/sbt/src/sbt-test/source-dependencies/transitive-memberRef/src/main/scala/X.scala b/sbt/src/sbt-test/source-dependencies/transitive-memberRef/src/main/scala/X.scala new file mode 100644 index 000000000..8c0d9edf8 --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/transitive-memberRef/src/main/scala/X.scala @@ -0,0 +1,5 @@ +package test + +class X { + def bar(b: B) = b +} diff --git a/sbt/src/sbt-test/source-dependencies/transitive-memberRef/src/main/scala/Y.scala b/sbt/src/sbt-test/source-dependencies/transitive-memberRef/src/main/scala/Y.scala new file mode 100644 index 000000000..df53c3c5c --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/transitive-memberRef/src/main/scala/Y.scala @@ -0,0 +1,5 @@ +package test + +class Y { + def baz(x: X) = x +} diff --git a/sbt/src/sbt-test/source-dependencies/transitive-memberRef/test b/sbt/src/sbt-test/source-dependencies/transitive-memberRef/test new file mode 100644 index 000000000..395f90229 --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/transitive-memberRef/test @@ -0,0 +1,11 @@ +# introduces first compile iteration +> compile +# adds a new method to A which will cause transitive invalidation +# of all source files that inherit from it +# also, all direct dependencies of files that inherit from A will +# be invalidated (in our case that's X.scala) +$ copy-file changes/A1.scala src/main/scala/A.scala +# second iteration +> compile +# check in which compile iteration given source file got recompiled +> check-compilations From a48ab0d38b73aa89eb3a592d411c9ef103d83cdd Mon Sep 17 00:00:00 2001 From: Grzegorz Kossakowski Date: Sun, 15 Dec 2013 18:59:14 +0100 Subject: [PATCH 12/15] Implement name hashing algorithm in incremental compiler. Provide implementation of invalidation logic that takes computed name hashes into account. The implementation is spread amongst two classes: 1. `IncrementalNameHashing` which implements a variant of incremental compilation algorithm that computes modified names and delegates to `MemberReferenceInvalidationStrategy` when invalidating member reference dependencies 2. `MemberReferenceInvalidationStrategy` which implements the core logic of dealing with dependencies introduced by member reference. See documentation of that class for details. The name hashing optimization is applied when invalidating source files having both internal and external dependencies (in initial iteration), check `invalidateByExternal` and `invalidateSource` methods for details. As seen in implementation of `MemberReferenceInvalidationStrategy` the name hashing optimization is not applied when implicit members change. NOTE: All functionality introduced in this commit is enabled only when `IncOptions.nameHashing` flag is set to true. The `source-dependencies/transitive-memberRef` test has been changed to test name hashing variant of incremental compilation. The change to invalidated files reflects the difference between the old and the new algorithm. Also, there a few new tests added that cover issues previously found while testing name hashing algorithm and are fixed in this commit. Each paragraph describes a single test. Add a test case which shows that detect properly changes to type aliases in the name hashing algorithm. See gkossakowski/sbt#6 for details. Add test covering bug with use of symbolic names (issue gkossakowski/sbt#5). Add a test which covers the case where we refer to a name that is declared in the same file. See issue gkossakowski/sbt#3 for details. --- .../inc/src/main/scala/sbt/inc/Changes.scala | 33 +++++ .../src/main/scala/sbt/inc/Incremental.scala | 95 +++++++++++++- .../scala/sbt/inc/MemberRefInvalidator.scala | 124 ++++++++++++++++++ .../backtick-quoted-names/A.scala | 3 + .../backtick-quoted-names/B.scala | 3 + .../backtick-quoted-names/build.sbt | 1 + .../backtick-quoted-names/changes/A.scala | 3 + .../backtick-quoted-names/test | 7 + .../same-file-used-names/A.scala | 8 ++ .../same-file-used-names/B.scala | 3 + .../same-file-used-names/build.sbt | 1 + .../same-file-used-names/changes/B.scala | 3 + .../same-file-used-names/test | 7 + .../transitive-memberRef/build.sbt | 6 +- .../source-dependencies/type-alias/A.scala | 4 + .../source-dependencies/type-alias/B.scala | 3 + .../source-dependencies/type-alias/build.sbt | 3 + .../type-alias/changes/A.scala | 3 + .../source-dependencies/type-alias/test | 7 + 19 files changed, 309 insertions(+), 8 deletions(-) create mode 100644 compile/inc/src/main/scala/sbt/inc/MemberRefInvalidator.scala create mode 100644 sbt/src/sbt-test/source-dependencies/backtick-quoted-names/A.scala create mode 100644 sbt/src/sbt-test/source-dependencies/backtick-quoted-names/B.scala create mode 100644 sbt/src/sbt-test/source-dependencies/backtick-quoted-names/build.sbt create mode 100644 sbt/src/sbt-test/source-dependencies/backtick-quoted-names/changes/A.scala create mode 100644 sbt/src/sbt-test/source-dependencies/backtick-quoted-names/test create mode 100644 sbt/src/sbt-test/source-dependencies/same-file-used-names/A.scala create mode 100644 sbt/src/sbt-test/source-dependencies/same-file-used-names/B.scala create mode 100644 sbt/src/sbt-test/source-dependencies/same-file-used-names/build.sbt create mode 100644 sbt/src/sbt-test/source-dependencies/same-file-used-names/changes/B.scala create mode 100644 sbt/src/sbt-test/source-dependencies/same-file-used-names/test create mode 100644 sbt/src/sbt-test/source-dependencies/type-alias/A.scala create mode 100644 sbt/src/sbt-test/source-dependencies/type-alias/B.scala create mode 100644 sbt/src/sbt-test/source-dependencies/type-alias/build.sbt create mode 100644 sbt/src/sbt-test/source-dependencies/type-alias/changes/A.scala create mode 100644 sbt/src/sbt-test/source-dependencies/type-alias/test diff --git a/compile/inc/src/main/scala/sbt/inc/Changes.scala b/compile/inc/src/main/scala/sbt/inc/Changes.scala index 3fce46738..f1de55044 100644 --- a/compile/inc/src/main/scala/sbt/inc/Changes.scala +++ b/compile/inc/src/main/scala/sbt/inc/Changes.scala @@ -6,6 +6,8 @@ package inc import xsbt.api.NameChanges import java.io.File +import xsbti.api.{_internalOnly_NameHashes => NameHashes} +import xsbti.api.{_internalOnly_NameHash => NameHash} final case class InitialChanges(internalSrc: Changes[File], removedProducts: Set[File], binaryDeps: Set[File], external: APIChanges[String]) final class APIChanges[T](val apiChanges: Iterable[APIChange[T]]) @@ -22,6 +24,37 @@ sealed abstract class APIChange[T](val modified: T) */ case class APIChangeDueToMacroDefinition[T](modified0: T) extends APIChange(modified0) case class SourceAPIChange[T](modified0: T) extends APIChange(modified0) +/** + * An APIChange that carries information about modified names. + * + * This class is used only when name hashing algorithm is enabled. + */ +case class NamesChange[T](modified0: T, modifiedNames: ModifiedNames) extends APIChange(modified0) + +/** + * ModifiedNames are determined by comparing name hashes in two versions of an API representation. + * + * Note that we distinguish between sets of regular (non-implicit) and implicit modified names. + * This distinction is needed because the name hashing algorithm makes different decisions based + * on whether modified name is implicit or not. Implicit names are much more difficult to handle + * due to difficulty of reasoning about the implicit scope. + */ +case class ModifiedNames(regularNames: Set[String], implicitNames: Set[String]) { + override def toString: String = + s"ModifiedNames(regularNames = ${regularNames mkString ", "}, implicitNames = ${implicitNames mkString ", "})" +} +object ModifiedNames { + def compareTwoNameHashes(a: NameHashes, b: NameHashes): ModifiedNames = { + val modifiedRegularNames = calculateModifiedNames(a.regularMembers.toSet, b.regularMembers.toSet) + val modifiedImplicitNames = calculateModifiedNames(a.implicitMembers.toSet, b.implicitMembers.toSet) + ModifiedNames(modifiedRegularNames, modifiedImplicitNames) + } + private def calculateModifiedNames(xs: Set[NameHash], ys: Set[NameHash]): Set[String] = { + val differentNameHashes = (xs union ys) diff (xs intersect ys) + differentNameHashes.map(_.name) + } +} + trait Changes[A] { diff --git a/compile/inc/src/main/scala/sbt/inc/Incremental.scala b/compile/inc/src/main/scala/sbt/inc/Incremental.scala index dc86f818d..ee4352787 100644 --- a/compile/inc/src/main/scala/sbt/inc/Incremental.scala +++ b/compile/inc/src/main/scala/sbt/inc/Incremental.scala @@ -21,8 +21,11 @@ object Incremental log: Logger, options: IncOptions)(implicit equivS: Equiv[Stamp]): (Boolean, Analysis) = { - assert(!options.nameHashing, "We don't support name hashing algorithm yet.") - val incremental = new IncrementalDefaultImpl(log, options) + val incremental: IncrementalCommon = + if (!options.nameHashing) + new IncrementalDefaultImpl(log, options) + else + new IncrementalNameHashing(log, options) val initialChanges = incremental.changedInitial(entry, sources, previous, current, forEntry) val binaryChanges = new DependencyChanges { val modifiedBinaries = initialChanges.binaryDeps.toArray @@ -128,7 +131,8 @@ private abstract class IncrementalCommon(log: Logger, options: IncOptions) { apiChanges foreach { case APIChangeDueToMacroDefinition(src) => log.debug(s"Public API is considered to be changed because $src contains a macro definition.") - case SourceAPIChange(src) => + case apiChange@(_: SourceAPIChange[T] | _: NamesChange[T]) => + val src = apiChange.modified val oldApi = oldAPIMapping(src) val newApi = newAPIMapping(src) val apiUnifiedPatch = apiDiff.generateApiDiff(src.toString, oldApi.api, newApi.api, contextSize) @@ -176,7 +180,7 @@ private abstract class IncrementalCommon(log: Logger, options: IncOptions) { } } - protected def sameAPI[T](src: T, a: Source, b: Source): Option[SourceAPIChange[T]] + protected def sameAPI[T](src: T, a: Source, b: Source): Option[APIChange[T]] def shortcutSameSource(a: Source, b: Source): Boolean = !a.hash.isEmpty && !b.hash.isEmpty && sameCompilation(a.compilation, b.compilation) && (a.hash.deep equals b.hash.deep) def sameCompilation(a: Compilation, b: Compilation): Boolean = a.startTime == b.startTime && a.outputs.corresponds(b.outputs){ @@ -475,11 +479,90 @@ private final class IncrementalDefaultImpl(log: Logger, options: IncOptions) ext log.debug("Invalidated by transitive public inheritance: " + transitiveInherited) val direct = transitiveInherited flatMap directDeps log.debug("Invalidated by direct dependency: " + direct) - val all = transitiveInherited ++ direct - all + transitiveInherited ++ direct } override protected def allDeps(relations: Relations): File => Set[File] = f => relations.direct.internal.reverse(f) } + +/** + * Implementation of incremental algorithm known as "name hashing". It differs from the default implementation + * by applying pruning (filter) of member reference dependencies based on used and modified simple names. + * + * See MemberReferenceInvalidationStrategy for some more information. + */ +private final class IncrementalNameHashing(log: Logger, options: IncOptions) extends IncrementalCommon(log, options) { + + private val memberRefInvalidator = new MemberRefInvalidator(log) + + // Package objects are fragile: if they inherit from an invalidated source, get "class file needed by package is missing" error + // This might be too conservative: we probably only need package objects for packages of invalidated sources. + override protected def invalidatedPackageObjects(invalidated: Set[File], relations: Relations): Set[File] = + invalidated flatMap relations.inheritance.internal.reverse filter { _.getName == "package.scala" } + + override protected def sameAPI[T](src: T, a: Source, b: Source): Option[APIChange[T]] = { + if (SameAPI(a,b)) + None + else { + val aNameHashes = a._internalOnly_nameHashes + val bNameHashes = b._internalOnly_nameHashes + val modifiedNames = ModifiedNames.compareTwoNameHashes(aNameHashes, bNameHashes) + val apiChange = NamesChange(src, modifiedNames) + Some(apiChange) + } + } + + /** Invalidates sources based on initially detected 'changes' to the sources, products, and dependencies.*/ + override protected def invalidateByExternal(relations: Relations, externalAPIChange: APIChange[String]): Set[File] = { + val modified = externalAPIChange.modified + val invalidationReason = memberRefInvalidator.invalidationReason(externalAPIChange) + log.debug(s"$invalidationReason\nAll member reference dependencies will be considered within this context.") + // Propagate inheritance dependencies transitively. + // This differs from normal because we need the initial crossing from externals to sources in this project. + val externalInheritanceR = relations.inheritance.external + val byExternalInheritance = externalInheritanceR.reverse(modified) + log.debug(s"Files invalidated by inheriting from (external) $modified: $byExternalInheritance; now invalidating by inheritance (internally).") + val transitiveInheritance = byExternalInheritance flatMap { file => + invalidateByInheritance(relations, file) + } + val memberRefInvalidationInternal = memberRefInvalidator.get(relations.memberRef.internal, + relations.names, externalAPIChange) + val memberRefInvalidationExternal = memberRefInvalidator.get(relations.memberRef.external, + relations.names, externalAPIChange) + + // Get the member reference dependencies of all sources transitively invalidated by inheritance + log.debug("Getting direct dependencies of all sources transitively invalidated by inheritance.") + val memberRefA = transitiveInheritance flatMap memberRefInvalidationInternal + // Get the sources that depend on externals by member reference. + // This includes non-inheritance dependencies and is not transitive. + log.debug(s"Getting sources that directly depend on (external) $modified.") + val memberRefB = memberRefInvalidationExternal(modified) + transitiveInheritance ++ memberRefA ++ memberRefB + } + + private def invalidateByInheritance(relations: Relations, modified: File): Set[File] = { + val inheritanceDeps = relations.inheritance.internal.reverse _ + log.debug(s"Invalidating (transitively) by inheritance from $modified...") + val transitiveInheritance = transitiveDeps(Set(modified))(inheritanceDeps) + log.debug("Invalidated by transitive inheritance dependency: " + transitiveInheritance) + transitiveInheritance + } + + override protected def invalidateSource(relations: Relations, change: APIChange[File]): Set[File] = { + log.debug(s"Invalidating ${change.modified}...") + val transitiveInheritance = invalidateByInheritance(relations, change.modified) + val reasonForInvalidation = memberRefInvalidator.invalidationReason(change) + log.debug(s"$reasonForInvalidation\nAll member reference dependencies will be considered within this context.") + val memberRefInvalidation = memberRefInvalidator.get(relations.memberRef.internal, + relations.names, change) + val memberRef = transitiveInheritance flatMap memberRefInvalidation + val all = transitiveInheritance ++ memberRef + all + } + + override protected def allDeps(relations: Relations): File => Set[File] = + f => relations.memberRef.internal.reverse(f) + +} diff --git a/compile/inc/src/main/scala/sbt/inc/MemberRefInvalidator.scala b/compile/inc/src/main/scala/sbt/inc/MemberRefInvalidator.scala new file mode 100644 index 000000000..22537c78d --- /dev/null +++ b/compile/inc/src/main/scala/sbt/inc/MemberRefInvalidator.scala @@ -0,0 +1,124 @@ +package sbt.inc + +import sbt.Relation +import java.io.File +import sbt.Logger +import xsbt.api.APIUtil + +/** + * Implements various strategies for invalidating dependencies introduced by member reference. + * + * The strategy is represented as function T => Set[File] where T is a source file that other + * source files depend on. When you apply that function to given element `src` you get set of + * files that depend on `src` by member reference and should be invalidated due to api change + * that was passed to a method constructing that function. There are two questions that arise: + * + * 1. Why is signature T => Set[File] and not T => Set[T] or File => Set[File]? + * 2. Why would we apply that function to any other `src` that then one that got modified + * and the modification is described by APIChange? + * + * Let's address the second question with the following example of source code structure: + * + * // A.scala + * class A + * + * // B.scala + * class B extends A + * + * // C.scala + * class C { def foo(a: A) = ??? } + * + * // D.scala + * class D { def bar(b: B) = ??? } + * + * Member reference dependencies on A.scala are B.scala, C.scala. When the api of A changes + * then we would consider B and C for invalidation. However, B is also a dependency by inheritance + * so we always invalidate it. The api change to A is relevant when B is considered (because + * of how inheritance works) so we would invalidate B by inheritance and then we would like to + * invalidate member reference dependencies of B as well. In other words, we have a function + * because we want to apply it (with the same api change in mind) to all src files invalidated + * by inheritance of the originally modified file. + * + * The first question is a bit more straightforward to answer. We always invalidate internal + * source files (in given project) that are represented as File but they might depend either on + * internal source files (then T=File) or they can depend on external class name (then T=String). + * + * The specific invalidation strategy is determined based on APIChange that describes a change to api + * of a single source file. + * + * For example, if we get APIChangeDueToMacroDefinition then we invalidate all member reference + * dependencies unconditionally. On the other hand, if api change is due to modified name hashes + * of regular members then we'll invalidate sources that use those names. + */ +private[inc] class MemberRefInvalidator(log: Logger) { + def get[T](memberRef: Relation[File, T], usedNames: Relation[File, String], apiChange: APIChange[_]): + T => Set[File] = apiChange match { + case _: APIChangeDueToMacroDefinition[_] => + new InvalidateUnconditionally(memberRef) + case NamesChange(_, modifiedNames) if !modifiedNames.implicitNames.isEmpty => + new InvalidateUnconditionally(memberRef) + case NamesChange(modifiedSrcFile, modifiedNames) => + new NameHashFilteredInvalidator[T](usedNames, memberRef, modifiedNames.regularNames) + case _: SourceAPIChange[_] => + sys.error(wrongAPIChangeMsg) + } + + def invalidationReason(apiChange: APIChange[_]): String = apiChange match { + case APIChangeDueToMacroDefinition(modifiedSrcFile) => + s"The $modifiedSrcFile source file declares a macro." + case NamesChange(modifiedSrcFile, modifiedNames) if !modifiedNames.implicitNames.isEmpty => + s"""|The $modifiedSrcFile source file has the following implicit definitions changed: + |\t${modifiedNames.implicitNames.mkString(", ")}.""".stripMargin + case NamesChange(modifiedSrcFile, modifiedNames) => + s"""|The $modifiedSrcFile source file has the following regular definitions changed: + |\t${modifiedNames.regularNames.mkString(", ")}.""".stripMargin + case _: SourceAPIChange[_] => + sys.error(wrongAPIChangeMsg) + } + + private val wrongAPIChangeMsg = + "MemberReferenceInvalidator.get should be called when name hashing is enabled " + + "and in that case we shouldn't have SourceAPIChange as an api change." + + private class InvalidateUnconditionally[T](memberRef: Relation[File, T]) extends (T => Set[File]) { + def apply(from: T): Set[File] = { + val invalidated = memberRef.reverse(from) + if (!invalidated.isEmpty) + log.debug(s"The following member ref dependencies of $from are invalidated:\n" + + formatInvalidated(invalidated)) + invalidated + } + private def formatInvalidated(invalidated: Set[File]): String = { + val sortedFiles = invalidated.toSeq.sortBy(_.getAbsolutePath) + sortedFiles.map(file => "\t"+file).mkString("\n") + } + } + + private class NameHashFilteredInvalidator[T]( + usedNames: Relation[File, String], + memberRef: Relation[File, T], + modifiedNames: Set[String]) extends (T => Set[File]) { + + def apply(to: T): Set[File] = { + val dependent = memberRef.reverse(to) + filteredDependencies(dependent) + } + private def filteredDependencies(dependent: Set[File]): Set[File] = { + dependent.filter { + case from if APIUtil.isScalaSourceName(from.getName) => + val usedNamesInDependent = usedNames.forward(from) + val modifiedAndUsedNames = modifiedNames intersect usedNamesInDependent + if (modifiedAndUsedNames.isEmpty) { + log.debug(s"None of the modified names appears in $from. This dependency is not being considered for invalidation.") + false + } else { + log.debug(s"The following modified names cause invalidation of $from: $modifiedAndUsedNames") + true + } + case from => + log.debug(s"Name hashing optimization doesn't apply to non-Scala dependency: $from") + true + } + } + } +} diff --git a/sbt/src/sbt-test/source-dependencies/backtick-quoted-names/A.scala b/sbt/src/sbt-test/source-dependencies/backtick-quoted-names/A.scala new file mode 100644 index 000000000..1d3a976a8 --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/backtick-quoted-names/A.scala @@ -0,0 +1,3 @@ +object A { + def `=` = 3 +} diff --git a/sbt/src/sbt-test/source-dependencies/backtick-quoted-names/B.scala b/sbt/src/sbt-test/source-dependencies/backtick-quoted-names/B.scala new file mode 100644 index 000000000..7cbd62e1d --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/backtick-quoted-names/B.scala @@ -0,0 +1,3 @@ +object B extends App { + println(A.`=`) +} diff --git a/sbt/src/sbt-test/source-dependencies/backtick-quoted-names/build.sbt b/sbt/src/sbt-test/source-dependencies/backtick-quoted-names/build.sbt new file mode 100644 index 000000000..8a38ef414 --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/backtick-quoted-names/build.sbt @@ -0,0 +1 @@ +incOptions := incOptions.value.withNameHashing(true) diff --git a/sbt/src/sbt-test/source-dependencies/backtick-quoted-names/changes/A.scala b/sbt/src/sbt-test/source-dependencies/backtick-quoted-names/changes/A.scala new file mode 100644 index 000000000..b473714fa --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/backtick-quoted-names/changes/A.scala @@ -0,0 +1,3 @@ +object A { + def asdf = 3 +} diff --git a/sbt/src/sbt-test/source-dependencies/backtick-quoted-names/test b/sbt/src/sbt-test/source-dependencies/backtick-quoted-names/test new file mode 100644 index 000000000..d4d386615 --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/backtick-quoted-names/test @@ -0,0 +1,7 @@ +> compile + +# rename def with symbolic name (`=`) +$ copy-file changes/A.scala A.scala + +# Both A.scala and B.scala should be recompiled, producing a compile error +-> compile diff --git a/sbt/src/sbt-test/source-dependencies/same-file-used-names/A.scala b/sbt/src/sbt-test/source-dependencies/same-file-used-names/A.scala new file mode 100644 index 000000000..d91afb5ca --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/same-file-used-names/A.scala @@ -0,0 +1,8 @@ +object A { + def x = 3 + + def y = { + import B._ + x + } +} diff --git a/sbt/src/sbt-test/source-dependencies/same-file-used-names/B.scala b/sbt/src/sbt-test/source-dependencies/same-file-used-names/B.scala new file mode 100644 index 000000000..5e34efa4d --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/same-file-used-names/B.scala @@ -0,0 +1,3 @@ +object B { +// def x = 3 +} diff --git a/sbt/src/sbt-test/source-dependencies/same-file-used-names/build.sbt b/sbt/src/sbt-test/source-dependencies/same-file-used-names/build.sbt new file mode 100644 index 000000000..8a38ef414 --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/same-file-used-names/build.sbt @@ -0,0 +1 @@ +incOptions := incOptions.value.withNameHashing(true) diff --git a/sbt/src/sbt-test/source-dependencies/same-file-used-names/changes/B.scala b/sbt/src/sbt-test/source-dependencies/same-file-used-names/changes/B.scala new file mode 100644 index 000000000..4bf188fb2 --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/same-file-used-names/changes/B.scala @@ -0,0 +1,3 @@ +object B { + def x = 3 +} diff --git a/sbt/src/sbt-test/source-dependencies/same-file-used-names/test b/sbt/src/sbt-test/source-dependencies/same-file-used-names/test new file mode 100644 index 000000000..781b4aafb --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/same-file-used-names/test @@ -0,0 +1,7 @@ +> compile + +# uncomment definition of `x` that leads to ambiguity error in A +$ copy-file changes/B.scala B.scala + +# Both A.scala and B.scala should be recompiled, producing a compile error +-> compile diff --git a/sbt/src/sbt-test/source-dependencies/transitive-memberRef/build.sbt b/sbt/src/sbt-test/source-dependencies/transitive-memberRef/build.sbt index ef32473dc..de908146c 100644 --- a/sbt/src/sbt-test/source-dependencies/transitive-memberRef/build.sbt +++ b/sbt/src/sbt-test/source-dependencies/transitive-memberRef/build.sbt @@ -1,5 +1,7 @@ logLevel := Level.Debug +incOptions := incOptions.value.withNameHashing(true) + // disable sbt's heauristic which recompiles everything in case // some fraction (e.g. 50%) of files is scheduled to be recompiled // in this test we want precise information about recompiled files @@ -24,13 +26,13 @@ TaskKey[Unit]("check-compilations") <<= (compile in Compile, scalaSource in Comp assert(recompiledFiles(iteration) == files, "%s != %s".format(recompiledFiles(iteration), files)) } // Y.scala is compiled only at the beginning as changes to A.scala do not affect it - recompiledFilesInIteration(0, Set("Y.scala")) + recompiledFilesInIteration(0, Set("X.scala", "Y.scala")) // A.scala is changed and recompiled recompiledFilesInIteration(1, Set("A.scala")) // change in A.scala causes recompilation of B.scala, C.scala, D.scala which depend on transtiviely // and by inheritance on A.scala // X.scala is also recompiled because it depends by member reference on B.scala // Note that Y.scala is not recompiled because it depends just on X through member reference dependency - recompiledFilesInIteration(2, Set("B.scala", "C.scala", "D.scala", "X.scala")) + recompiledFilesInIteration(2, Set("B.scala", "C.scala", "D.scala")) assert(allCompilations.size == 3) } diff --git a/sbt/src/sbt-test/source-dependencies/type-alias/A.scala b/sbt/src/sbt-test/source-dependencies/type-alias/A.scala new file mode 100644 index 000000000..c0c8794a7 --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/type-alias/A.scala @@ -0,0 +1,4 @@ +object A { + type X = Option[Int] +} + diff --git a/sbt/src/sbt-test/source-dependencies/type-alias/B.scala b/sbt/src/sbt-test/source-dependencies/type-alias/B.scala new file mode 100644 index 000000000..81640ed8d --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/type-alias/B.scala @@ -0,0 +1,3 @@ +object B { + def y: A.X = Option(3) +} diff --git a/sbt/src/sbt-test/source-dependencies/type-alias/build.sbt b/sbt/src/sbt-test/source-dependencies/type-alias/build.sbt new file mode 100644 index 000000000..c5a1099aa --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/type-alias/build.sbt @@ -0,0 +1,3 @@ +logLevel in compile := Level.Debug + +incOptions := incOptions.value.withNameHashing(true) diff --git a/sbt/src/sbt-test/source-dependencies/type-alias/changes/A.scala b/sbt/src/sbt-test/source-dependencies/type-alias/changes/A.scala new file mode 100644 index 000000000..53aee1626 --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/type-alias/changes/A.scala @@ -0,0 +1,3 @@ +object A { + type X = Int +} diff --git a/sbt/src/sbt-test/source-dependencies/type-alias/test b/sbt/src/sbt-test/source-dependencies/type-alias/test new file mode 100644 index 000000000..f0a7fe8a1 --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/type-alias/test @@ -0,0 +1,7 @@ +> compile + +# change type alias +$ copy-file changes/A.scala A.scala + +# Both A.scala and B.scala should be recompiled, producing a compile error +-> compile From cd6b2a2a8cd4903848c815660025b2472160bbc4 Mon Sep 17 00:00:00 2001 From: Grzegorz Kossakowski Date: Sun, 15 Dec 2013 23:46:07 +0100 Subject: [PATCH 13/15] Mark tests that regress when name hashing is enabled. There are number of scripted tests that fail if we switch to name hashing being enabled by default. There's no easy way to mark those tests as pending only when name hashing flag is enabled so I decided to "mark" them by copying those tests, enabling name hashing in each of them and mark those copies as pending. Here's explanation of each failing test: * `constants` and `java-static` fail due to typer inlining constants so we can't track dependencies properly (see SI-7173) * `macro` fails for similar reasons as above: typer expands macros and we can't track dependencies properly * `struct` fails because it turns out that we need to handle structural types in a special way both at declaration and use sites. At the moment we handle them explicitly at declaration site so `struct-usage` passes but `struct` fails --- .../constants-name-hashing/build.sbt | 1 + .../constants-name-hashing/changes/A1.scala | 1 + .../constants-name-hashing/changes/A2.scala | 1 + .../constants-name-hashing/changes/B.scala | 4 +++ .../constants-name-hashing/pending | 11 +++++++ .../java-static-name-hashing/build.sbt | 1 + .../java-static-name-hashing/changes/J1.java | 4 +++ .../java-static-name-hashing/changes/J2.java | 4 +++ .../java-static-name-hashing/changes/S.scala | 4 +++ .../java-static-name-hashing/pending | 24 +++++++++++++++ .../macro-client/Client.scala | 5 ++++ .../macro-provider/Provider.scala | 8 +++++ .../macro-provider/changes/Provider.scala | 8 +++++ .../macro-name-hashing/pending | 13 +++++++++ .../macro-name-hashing/project/build.scala | 29 +++++++++++++++++++ .../struct-name-hashing/A.scala | 3 ++ .../struct-name-hashing/B.scala | 4 +++ .../struct-name-hashing/C.scala | 4 +++ .../struct-name-hashing/build.sbt | 1 + .../struct-name-hashing/changes/A.scala | 3 ++ .../struct-name-hashing/pending | 6 ++++ 21 files changed, 139 insertions(+) create mode 100644 sbt/src/sbt-test/source-dependencies/constants-name-hashing/build.sbt create mode 100644 sbt/src/sbt-test/source-dependencies/constants-name-hashing/changes/A1.scala create mode 100644 sbt/src/sbt-test/source-dependencies/constants-name-hashing/changes/A2.scala create mode 100644 sbt/src/sbt-test/source-dependencies/constants-name-hashing/changes/B.scala create mode 100644 sbt/src/sbt-test/source-dependencies/constants-name-hashing/pending create mode 100644 sbt/src/sbt-test/source-dependencies/java-static-name-hashing/build.sbt create mode 100644 sbt/src/sbt-test/source-dependencies/java-static-name-hashing/changes/J1.java create mode 100644 sbt/src/sbt-test/source-dependencies/java-static-name-hashing/changes/J2.java create mode 100644 sbt/src/sbt-test/source-dependencies/java-static-name-hashing/changes/S.scala create mode 100644 sbt/src/sbt-test/source-dependencies/java-static-name-hashing/pending create mode 100644 sbt/src/sbt-test/source-dependencies/macro-name-hashing/macro-client/Client.scala create mode 100644 sbt/src/sbt-test/source-dependencies/macro-name-hashing/macro-provider/Provider.scala create mode 100644 sbt/src/sbt-test/source-dependencies/macro-name-hashing/macro-provider/changes/Provider.scala create mode 100644 sbt/src/sbt-test/source-dependencies/macro-name-hashing/pending create mode 100644 sbt/src/sbt-test/source-dependencies/macro-name-hashing/project/build.scala create mode 100644 sbt/src/sbt-test/source-dependencies/struct-name-hashing/A.scala create mode 100644 sbt/src/sbt-test/source-dependencies/struct-name-hashing/B.scala create mode 100644 sbt/src/sbt-test/source-dependencies/struct-name-hashing/C.scala create mode 100644 sbt/src/sbt-test/source-dependencies/struct-name-hashing/build.sbt create mode 100644 sbt/src/sbt-test/source-dependencies/struct-name-hashing/changes/A.scala create mode 100644 sbt/src/sbt-test/source-dependencies/struct-name-hashing/pending diff --git a/sbt/src/sbt-test/source-dependencies/constants-name-hashing/build.sbt b/sbt/src/sbt-test/source-dependencies/constants-name-hashing/build.sbt new file mode 100644 index 000000000..8a38ef414 --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/constants-name-hashing/build.sbt @@ -0,0 +1 @@ +incOptions := incOptions.value.withNameHashing(true) diff --git a/sbt/src/sbt-test/source-dependencies/constants-name-hashing/changes/A1.scala b/sbt/src/sbt-test/source-dependencies/constants-name-hashing/changes/A1.scala new file mode 100644 index 000000000..f67b6f474 --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/constants-name-hashing/changes/A1.scala @@ -0,0 +1 @@ +object A { final val x = 1 } diff --git a/sbt/src/sbt-test/source-dependencies/constants-name-hashing/changes/A2.scala b/sbt/src/sbt-test/source-dependencies/constants-name-hashing/changes/A2.scala new file mode 100644 index 000000000..4f9396f13 --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/constants-name-hashing/changes/A2.scala @@ -0,0 +1 @@ +object A { final val x = 2 } diff --git a/sbt/src/sbt-test/source-dependencies/constants-name-hashing/changes/B.scala b/sbt/src/sbt-test/source-dependencies/constants-name-hashing/changes/B.scala new file mode 100644 index 000000000..058527993 --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/constants-name-hashing/changes/B.scala @@ -0,0 +1,4 @@ +object B +{ + def main(args: Array[String]) = assert(args(0).toInt == A.x ) +} \ No newline at end of file diff --git a/sbt/src/sbt-test/source-dependencies/constants-name-hashing/pending b/sbt/src/sbt-test/source-dependencies/constants-name-hashing/pending new file mode 100644 index 000000000..61df26ef6 --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/constants-name-hashing/pending @@ -0,0 +1,11 @@ +# Tests if source dependencies are tracked properly +# for compile-time constants (like final vals in top-level objects) +# see https://issues.scala-lang.org/browse/SI-7173 for details +# why compile-time constants can be tricky to track due to early inlining + +$ copy-file changes/B.scala B.scala + +$ copy-file changes/A1.scala A.scala +> run 1 +$ copy-file changes/A2.scala A.scala +> run 2 diff --git a/sbt/src/sbt-test/source-dependencies/java-static-name-hashing/build.sbt b/sbt/src/sbt-test/source-dependencies/java-static-name-hashing/build.sbt new file mode 100644 index 000000000..8a38ef414 --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/java-static-name-hashing/build.sbt @@ -0,0 +1 @@ +incOptions := incOptions.value.withNameHashing(true) diff --git a/sbt/src/sbt-test/source-dependencies/java-static-name-hashing/changes/J1.java b/sbt/src/sbt-test/source-dependencies/java-static-name-hashing/changes/J1.java new file mode 100644 index 000000000..a3a75fefd --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/java-static-name-hashing/changes/J1.java @@ -0,0 +1,4 @@ +public class J +{ + public static final int x = 3; +} \ No newline at end of file diff --git a/sbt/src/sbt-test/source-dependencies/java-static-name-hashing/changes/J2.java b/sbt/src/sbt-test/source-dependencies/java-static-name-hashing/changes/J2.java new file mode 100644 index 000000000..8ff2e24c6 --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/java-static-name-hashing/changes/J2.java @@ -0,0 +1,4 @@ +public class J +{ + public static final String x = "3"; +} \ No newline at end of file diff --git a/sbt/src/sbt-test/source-dependencies/java-static-name-hashing/changes/S.scala b/sbt/src/sbt-test/source-dependencies/java-static-name-hashing/changes/S.scala new file mode 100644 index 000000000..45436972b --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/java-static-name-hashing/changes/S.scala @@ -0,0 +1,4 @@ +object S +{ + val y: Int = J.x +} diff --git a/sbt/src/sbt-test/source-dependencies/java-static-name-hashing/pending b/sbt/src/sbt-test/source-dependencies/java-static-name-hashing/pending new file mode 100644 index 000000000..42890ca74 --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/java-static-name-hashing/pending @@ -0,0 +1,24 @@ +# When a Java class is loaded from a class file and not parsed from a source file, scalac reports +# the statics as an object without a file and so the Analyzer must know to look for the +# object's linked class. +# This test verifies this happens. +# The test compiles a Java class with a static field. +# It then adds a Scala object that references the static field. Because the object only depends on a +# static member and because the Java source is not included in the compilation (since it didn't change), +# this triggers the special case above. + +# add and compile the Java source +$ copy-file changes/J1.java src/main/java/J.java +> compile + +# add and compile the Scala source +$ copy-file changes/S.scala src/main/scala/S.scala +> compile + +# change the Java source so that a compile error should occur if S.scala is also recompiled (which will happen if the dependency was properly recorded) +$ copy-file changes/J2.java src/main/java/J.java +-> compile + +# verify it should have failed by doing a full recompilation +> clean +-> compile \ No newline at end of file diff --git a/sbt/src/sbt-test/source-dependencies/macro-name-hashing/macro-client/Client.scala b/sbt/src/sbt-test/source-dependencies/macro-name-hashing/macro-client/Client.scala new file mode 100644 index 000000000..90932d136 --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/macro-name-hashing/macro-client/Client.scala @@ -0,0 +1,5 @@ +package macro + +object Client { + Provider.tree(0) +} diff --git a/sbt/src/sbt-test/source-dependencies/macro-name-hashing/macro-provider/Provider.scala b/sbt/src/sbt-test/source-dependencies/macro-name-hashing/macro-provider/Provider.scala new file mode 100644 index 000000000..9b6d27676 --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/macro-name-hashing/macro-provider/Provider.scala @@ -0,0 +1,8 @@ +package macro +import scala.language.experimental.macros +import scala.reflect.macros._ + +object Provider { + def tree(args: Any) = macro treeImpl + def treeImpl(c: Context)(args: c.Expr[Any]) = c.universe.reify(args.splice) +} diff --git a/sbt/src/sbt-test/source-dependencies/macro-name-hashing/macro-provider/changes/Provider.scala b/sbt/src/sbt-test/source-dependencies/macro-name-hashing/macro-provider/changes/Provider.scala new file mode 100644 index 000000000..711989b32 --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/macro-name-hashing/macro-provider/changes/Provider.scala @@ -0,0 +1,8 @@ +package macro +import scala.language.experimental.macros +import scala.reflect.macros._ + +object Provider { + def tree(args: Any) = macro treeImpl + def treeImpl(c: Context)(args: c.Expr[Any]) = sys.error("no macro for you!") +} diff --git a/sbt/src/sbt-test/source-dependencies/macro-name-hashing/pending b/sbt/src/sbt-test/source-dependencies/macro-name-hashing/pending new file mode 100644 index 000000000..b3755d4ee --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/macro-name-hashing/pending @@ -0,0 +1,13 @@ +> compile + +# replace macro with one that throws an error + +$ copy-file macro-provider/changes/Provider.scala macro-provider/Provider.scala + +> macro-provider/compile + +-> macro-client/compile + +> clean + +-> compile diff --git a/sbt/src/sbt-test/source-dependencies/macro-name-hashing/project/build.scala b/sbt/src/sbt-test/source-dependencies/macro-name-hashing/project/build.scala new file mode 100644 index 000000000..a5382240f --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/macro-name-hashing/project/build.scala @@ -0,0 +1,29 @@ +import sbt._ +import Keys._ + +object build extends Build { + val defaultSettings = Seq( + libraryDependencies <+= scalaVersion("org.scala-lang" % "scala-reflect" % _ ), + incOptions := incOptions.value.withNameHashing(true) + ) + + lazy val root = Project( + base = file("."), + id = "macro", + aggregate = Seq(macroProvider, macroClient), + settings = Defaults.defaultSettings ++ defaultSettings + ) + + lazy val macroProvider = Project( + base = file("macro-provider"), + id = "macro-provider", + settings = Defaults.defaultSettings ++ defaultSettings + ) + + lazy val macroClient = Project( + base = file("macro-client"), + id = "macro-client", + dependencies = Seq(macroProvider), + settings = Defaults.defaultSettings ++ defaultSettings + ) +} diff --git a/sbt/src/sbt-test/source-dependencies/struct-name-hashing/A.scala b/sbt/src/sbt-test/source-dependencies/struct-name-hashing/A.scala new file mode 100644 index 000000000..d17a6e20a --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/struct-name-hashing/A.scala @@ -0,0 +1,3 @@ +object A { + def x: Int = 3 +} \ No newline at end of file diff --git a/sbt/src/sbt-test/source-dependencies/struct-name-hashing/B.scala b/sbt/src/sbt-test/source-dependencies/struct-name-hashing/B.scala new file mode 100644 index 000000000..635568727 --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/struct-name-hashing/B.scala @@ -0,0 +1,4 @@ +object B { + def onX(m: { def x: Int } ) = + m.x +} \ No newline at end of file diff --git a/sbt/src/sbt-test/source-dependencies/struct-name-hashing/C.scala b/sbt/src/sbt-test/source-dependencies/struct-name-hashing/C.scala new file mode 100644 index 000000000..413cd6d63 --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/struct-name-hashing/C.scala @@ -0,0 +1,4 @@ +object C { + def main(args: Array[String]) = + println(B.onX(A)) +} \ No newline at end of file diff --git a/sbt/src/sbt-test/source-dependencies/struct-name-hashing/build.sbt b/sbt/src/sbt-test/source-dependencies/struct-name-hashing/build.sbt new file mode 100644 index 000000000..8a38ef414 --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/struct-name-hashing/build.sbt @@ -0,0 +1 @@ +incOptions := incOptions.value.withNameHashing(true) diff --git a/sbt/src/sbt-test/source-dependencies/struct-name-hashing/changes/A.scala b/sbt/src/sbt-test/source-dependencies/struct-name-hashing/changes/A.scala new file mode 100644 index 000000000..dc9bbd3c0 --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/struct-name-hashing/changes/A.scala @@ -0,0 +1,3 @@ +object A { + def x: Byte = 3 +} diff --git a/sbt/src/sbt-test/source-dependencies/struct-name-hashing/pending b/sbt/src/sbt-test/source-dependencies/struct-name-hashing/pending new file mode 100644 index 000000000..8c7328ea4 --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/struct-name-hashing/pending @@ -0,0 +1,6 @@ +> compile + +# modify A.scala so that it does not conform to the structural type in B.scala +$ copy-file changes/A.scala A.scala + +-> compile \ No newline at end of file From 6cf79aba08da5145b1a85466305a23928a031ba2 Mon Sep 17 00:00:00 2001 From: Grzegorz Kossakowski Date: Sun, 15 Dec 2013 23:51:46 +0100 Subject: [PATCH 14/15] Mark test that passes when name hashing is enabled. There's one test that starts to pass when we enable name hashing. It's `import-class` which tests whether tracking of dependencies that arise from imports is properly tracked. The name hashing algorithm uses different dependency tracking compared to the old algorithm and the new dependency extraction logic does handle import tree nodes properly so the test passes. We "mark" the test passing by copying it and enabling the name hashing flag in it. This is done similarly as in 940f7ff46d. --- .../source-dependencies/import-class-name-hashing/A.scala | 3 +++ .../source-dependencies/import-class-name-hashing/B.scala | 1 + .../import-class-name-hashing/build.sbt | 1 + .../import-class-name-hashing/changes/A.scala | 1 + .../source-dependencies/import-class-name-hashing/test | 8 ++++++++ 5 files changed, 14 insertions(+) create mode 100644 sbt/src/sbt-test/source-dependencies/import-class-name-hashing/A.scala create mode 100644 sbt/src/sbt-test/source-dependencies/import-class-name-hashing/B.scala create mode 100644 sbt/src/sbt-test/source-dependencies/import-class-name-hashing/build.sbt create mode 100644 sbt/src/sbt-test/source-dependencies/import-class-name-hashing/changes/A.scala create mode 100644 sbt/src/sbt-test/source-dependencies/import-class-name-hashing/test diff --git a/sbt/src/sbt-test/source-dependencies/import-class-name-hashing/A.scala b/sbt/src/sbt-test/source-dependencies/import-class-name-hashing/A.scala new file mode 100644 index 000000000..a93bbe535 --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/import-class-name-hashing/A.scala @@ -0,0 +1,3 @@ +package a + +class A diff --git a/sbt/src/sbt-test/source-dependencies/import-class-name-hashing/B.scala b/sbt/src/sbt-test/source-dependencies/import-class-name-hashing/B.scala new file mode 100644 index 000000000..0489f4a26 --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/import-class-name-hashing/B.scala @@ -0,0 +1 @@ +import a.A diff --git a/sbt/src/sbt-test/source-dependencies/import-class-name-hashing/build.sbt b/sbt/src/sbt-test/source-dependencies/import-class-name-hashing/build.sbt new file mode 100644 index 000000000..8a38ef414 --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/import-class-name-hashing/build.sbt @@ -0,0 +1 @@ +incOptions := incOptions.value.withNameHashing(true) diff --git a/sbt/src/sbt-test/source-dependencies/import-class-name-hashing/changes/A.scala b/sbt/src/sbt-test/source-dependencies/import-class-name-hashing/changes/A.scala new file mode 100644 index 000000000..2a93cdef5 --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/import-class-name-hashing/changes/A.scala @@ -0,0 +1 @@ +package a diff --git a/sbt/src/sbt-test/source-dependencies/import-class-name-hashing/test b/sbt/src/sbt-test/source-dependencies/import-class-name-hashing/test new file mode 100644 index 000000000..7679ba52c --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/import-class-name-hashing/test @@ -0,0 +1,8 @@ +> compile + +# remove class a.A +$ copy-file changes/A.scala A.scala + +# 'import a.A' should now fail in B.scala +# succeeds because scalac doesn't track this dependency +-> compile From f4940df48d0af3a068669b320f238dd7c25db451 Mon Sep 17 00:00:00 2001 From: Grzegorz Kossakowski Date: Mon, 6 Jan 2014 22:24:00 +0100 Subject: [PATCH 15/15] Make all APIChange subclasses final. They should have been final from the beginning. We are fixing that omission now. --- compile/inc/src/main/scala/sbt/inc/Changes.scala | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/compile/inc/src/main/scala/sbt/inc/Changes.scala b/compile/inc/src/main/scala/sbt/inc/Changes.scala index f1de55044..94bb1ec18 100644 --- a/compile/inc/src/main/scala/sbt/inc/Changes.scala +++ b/compile/inc/src/main/scala/sbt/inc/Changes.scala @@ -22,14 +22,14 @@ sealed abstract class APIChange[T](val modified: T) * api has changed. The reason is that there's no way to determine if changes to macros implementation * are affecting its users or not. Therefore we err on the side of caution. */ -case class APIChangeDueToMacroDefinition[T](modified0: T) extends APIChange(modified0) -case class SourceAPIChange[T](modified0: T) extends APIChange(modified0) +final case class APIChangeDueToMacroDefinition[T](modified0: T) extends APIChange(modified0) +final case class SourceAPIChange[T](modified0: T) extends APIChange(modified0) /** * An APIChange that carries information about modified names. * * This class is used only when name hashing algorithm is enabled. */ -case class NamesChange[T](modified0: T, modifiedNames: ModifiedNames) extends APIChange(modified0) +final case class NamesChange[T](modified0: T, modifiedNames: ModifiedNames) extends APIChange(modified0) /** * ModifiedNames are determined by comparing name hashes in two versions of an API representation. @@ -39,7 +39,7 @@ case class NamesChange[T](modified0: T, modifiedNames: ModifiedNames) extends AP * on whether modified name is implicit or not. Implicit names are much more difficult to handle * due to difficulty of reasoning about the implicit scope. */ -case class ModifiedNames(regularNames: Set[String], implicitNames: Set[String]) { +final case class ModifiedNames(regularNames: Set[String], implicitNames: Set[String]) { override def toString: String = s"ModifiedNames(regularNames = ${regularNames mkString ", "}, implicitNames = ${implicitNames mkString ", "})" }