From 2ad047340fd22e7c530cf0ec99ebd60bcb4d5fdf Mon Sep 17 00:00:00 2001 From: Hung Nguyen Date: Fri, 5 Aug 2022 14:57:15 +0100 Subject: [PATCH] New IC: Add constants-in-companion-objects impact computation When computing impacted symbols of changed symbols, previously we considered only the supertypes-inheritors type of impact, which is the most common type. This commit adds the constants-in-companion-objects type of impact to address KT-53266. We've also cleaned up impact computation to make it easier to add new types of impact in the future. ^KT-53266 In progress --- .../kotlin/incremental/ChangesCollector.kt | 4 +- .../kotlin/incremental/IncrementalJvmCache.kt | 57 +++-- .../incremental/protoDifferenceUtils.kt | 15 ++ .../classpathDiff/ClasspathChangesComputer.kt | 237 +++++++++-------- .../classpathDiff/ClasspathSnapshot.kt | 9 +- .../ClasspathSnapshotSerializer.kt | 6 +- .../ClasspathSnapshotShrinker.kt | 37 ++- .../classpathDiff/ClasspathSnapshotter.kt | 7 +- .../incremental/classpathDiff/Impact.kt | 240 ++++++++++++++++++ .../classpathDiff/ProgramSymbol.kt | 3 + .../ClasspathChangesComputerTest.kt | 3 + .../ClasspathSnapshotTestCommon.kt | 8 +- .../com/example/SimpleClass.json | 8 + 13 files changed, 462 insertions(+), 172 deletions(-) create mode 100644 compiler/incremental-compilation-impl/src/org/jetbrains/kotlin/incremental/classpathDiff/Impact.kt diff --git a/build-common/src/org/jetbrains/kotlin/incremental/ChangesCollector.kt b/build-common/src/org/jetbrains/kotlin/incremental/ChangesCollector.kt index 44b1d485c9c..564042f283b 100644 --- a/build-common/src/org/jetbrains/kotlin/incremental/ChangesCollector.kt +++ b/build-common/src/org/jetbrains/kotlin/incremental/ChangesCollector.kt @@ -17,7 +17,6 @@ package org.jetbrains.kotlin.incremental import org.jetbrains.kotlin.metadata.ProtoBuf -import org.jetbrains.kotlin.metadata.deserialization.Flags import org.jetbrains.kotlin.metadata.deserialization.NameResolver import org.jetbrains.kotlin.name.FqName import org.jetbrains.kotlin.protobuf.MessageLite @@ -203,9 +202,8 @@ class ChangesCollector { private fun ClassProtoData.collectAllFromClass(isRemoved: Boolean, isAdded: Boolean, collectAllMembersForNewClass: Boolean = false) { val classFqName = nameResolver.getClassId(proto.fqName).asSingleFqName() - val kind = Flags.CLASS_KIND.get(proto.flags) - if (kind == ProtoBuf.Class.Kind.COMPANION_OBJECT) { + if (proto.isCompanionObject) { val memberNames = getNonPrivateMemberNames() val collectMember = if (isRemoved) this@ChangesCollector::collectRemovedMember else this@ChangesCollector::collectChangedMember diff --git a/build-common/src/org/jetbrains/kotlin/incremental/IncrementalJvmCache.kt b/build-common/src/org/jetbrains/kotlin/incremental/IncrementalJvmCache.kt index dd11c9b3a63..ef7abb55439 100644 --- a/build-common/src/org/jetbrains/kotlin/incremental/IncrementalJvmCache.kt +++ b/build-common/src/org/jetbrains/kotlin/incremental/IncrementalJvmCache.kt @@ -455,27 +455,27 @@ open class IncrementalJvmCache( val allConstants = oldMap.keys + newMap.keys if (allConstants.isEmpty()) return - // If a constant is defined in a companion object, it will be found in the constantsMap of the containing class, not the - // companion object's class, so we will need to correct its scope. - // (See https://youtrack.jetbrains.com/issue/KT-44741#focus=Comments-27-5659564.0-0 for more details.) - // Note: This only applies to a *constant* defined in a *companion object* (it's not an issue for inline functions, or top-level - // constants, or constants in non-companion objects). - val companionObjectClassId = if (kotlinClassInfo.classKind == KotlinClassHeader.Kind.CLASS) { - val protoData = kotlinClassInfo.protoData as ClassProtoData - if (protoData.proto.hasCompanionObjectName()) { - val companionObjectName = Name.identifier(protoData.nameResolver.getString(protoData.proto.companionObjectName)) - kotlinClassInfo.classId.createNestedClassId(companionObjectName) - } else null - } else null - val scope = companionObjectClassId?.asSingleFqName() ?: kotlinClassInfo.scopeFqName() - - // Here we assume that the old and new classes have the same KotlinClassHeader.Kind, so that the scopes of the old and new - // constants are the same and their values can be compared. - // If the class kinds are different, the changes will be detected when comparing protos (in that case, the changes collected - // here will be a subset of those changes). + val scope = kotlinClassInfo.scopeFqName() for (const in allConstants) { changesCollector.collectMemberIfValueWasChanged(scope, const, oldMap[const], newMap[const]) } + + // If a constant is defined in a companion object of class A, its name and type will be found in the Kotlin metadata of + // `A$Companion.class`, but its value will only be found in the Java bytecode code of `A.class` (see + // `org.jetbrains.kotlin.incremental.classpathDiff.ConstantsInCompanionObjectImpact` for more details). + // Therefore, if the value of `CONSTANT` in `A.class` has changed, we will report that `A.CONSTANT` has changed in the code + // above, and report that `A.Companion.CONSTANT` is impacted in the code below. + kotlinClassInfo.companionObject?.let { companionObjectClassId -> + // Note that `companionObjectClassId` is the companion object of the current class. Here we assume that the previous class + // also has a companion object with the same name. If that is not the case, that change will be detected when comparing + // protos, and the report below will be imprecise/redundant, but it's okay to over-approximate the result. + val companionObjectFqName = companionObjectClassId.asSingleFqName() + for (const in allConstants) { + changesCollector.collectMemberIfValueWasChanged( + scope = companionObjectFqName, name = const, oldMap[const], newMap[const] + ) + } + } } @Synchronized @@ -688,7 +688,7 @@ class KotlinClassInfo constructor( } /** - * Returns the [ProtoData] of this class. + * The [ProtoData] of this class. * * NOTE: The caller needs to ensure `classKind != KotlinClassHeader.Kind.MULTIFILE_CLASS` first, as the compiler doesn't write proto * data to [KotlinClassHeader.Kind.MULTIFILE_CLASS] classes. @@ -700,6 +700,25 @@ class KotlinClassInfo constructor( protoMapValue.toProtoData(classId.packageFqName) } + /** Name of the companion object of this class (default is "Companion") iff this class HAS a companion object, or null otherwise. */ + val companionObject: ClassId? by lazy { + if (classKind == KotlinClassHeader.Kind.CLASS) { + (protoData as ClassProtoData).getCompanionObjectName()?.let { + classId.createNestedClassId(Name.identifier(it)) + } + } else null + } + + /** List of constants defined in this class iff this class IS a companion object, or null otherwise. The list could be empty. */ + val constantsInCompanionObject: List? by lazy { + if (classKind == KotlinClassHeader.Kind.CLASS) { + val classProtoData = protoData as ClassProtoData + if (classProtoData.proto.isCompanionObject) { + classProtoData.getConstants() + } else null + } else null + } + companion object { fun createFrom(kotlinClass: LocalFileKotlinClass): KotlinClassInfo { diff --git a/build-common/src/org/jetbrains/kotlin/incremental/protoDifferenceUtils.kt b/build-common/src/org/jetbrains/kotlin/incremental/protoDifferenceUtils.kt index 06ec105f7e5..53c7c83a450 100644 --- a/build-common/src/org/jetbrains/kotlin/incremental/protoDifferenceUtils.kt +++ b/build-common/src/org/jetbrains/kotlin/incremental/protoDifferenceUtils.kt @@ -367,8 +367,23 @@ class DifferenceCalculatorForPackageFacade( private val ProtoBuf.Class.isSealed: Boolean get() = ProtoBuf.Modality.SEALED == Flags.MODALITY.get(flags) +internal val ProtoBuf.Class.isCompanionObject: Boolean + get() = ProtoBuf.Class.Kind.COMPANION_OBJECT == Flags.CLASS_KIND.get(flags) + val ProtoBuf.Class.typeTableOrNull: ProtoBuf.TypeTable? get() = if (hasTypeTable()) typeTable else null val ProtoBuf.Package.typeTableOrNull: ProtoBuf.TypeTable? get() = if (hasTypeTable()) typeTable else null + +internal fun ClassProtoData.getCompanionObjectName(): String? { + return if (proto.hasCompanionObjectName()) { + nameResolver.getString(proto.companionObjectName) + } else null +} + +internal fun ClassProtoData.getConstants(): List { + return proto.propertyList + .filter { Flags.IS_CONST.get(it.flags) } + .map { nameResolver.getString(it.name) } +} diff --git a/compiler/incremental-compilation-impl/src/org/jetbrains/kotlin/incremental/classpathDiff/ClasspathChangesComputer.kt b/compiler/incremental-compilation-impl/src/org/jetbrains/kotlin/incremental/classpathDiff/ClasspathChangesComputer.kt index 6b004c31184..7848d87fbca 100644 --- a/compiler/incremental-compilation-impl/src/org/jetbrains/kotlin/incremental/classpathDiff/ClasspathChangesComputer.kt +++ b/compiler/incremental-compilation-impl/src/org/jetbrains/kotlin/incremental/classpathDiff/ClasspathChangesComputer.kt @@ -12,13 +12,15 @@ import org.jetbrains.kotlin.build.report.metrics.BuildMetricsReporter import org.jetbrains.kotlin.build.report.metrics.BuildTime import org.jetbrains.kotlin.build.report.metrics.measure import org.jetbrains.kotlin.incremental.* +import org.jetbrains.kotlin.incremental.classpathDiff.BreadthFirstSearch.findReachableNodes import org.jetbrains.kotlin.incremental.classpathDiff.ClasspathSnapshotShrinker.shrinkClasspath -import org.jetbrains.kotlin.incremental.classpathDiff.ImpactAnalysis.computeImpactedSetInclusive +import org.jetbrains.kotlin.incremental.classpathDiff.ImpactedSymbolsComputer.computeImpactedSymbols import org.jetbrains.kotlin.incremental.storage.FileToCanonicalPathConverter import org.jetbrains.kotlin.incremental.storage.ListExternalizer import org.jetbrains.kotlin.incremental.storage.loadFromFile import org.jetbrains.kotlin.name.ClassId -import org.jetbrains.kotlin.resolve.jvm.JvmClassName +import org.jetbrains.kotlin.name.FqName +import org.jetbrains.kotlin.name.Name import org.jetbrains.kotlin.resolve.sam.SAM_LOOKUP_NAME import java.util.* @@ -110,14 +112,16 @@ object ClasspathChangesComputer { val changedAndImpactedSet = reporter.measure(BuildTime.COMPUTE_IMPACTED_SET) { // Note that changes may contain added symbols (they can also impact recompilation -- see examples in JavaClassChangesComputer). // So ideally, the result should be: - // computeImpactedSetInclusive(changes = changesOnPreviousClasspath, allClasses = classesOnPreviousClasspath) + - // computeImpactedSetInclusive(changes = changesOnCurrentClasspath, allClasses = classesOnCurrentClasspath) + // computeImpactedSymbols(changes = changesOnPreviousClasspath, allClasses = classesOnPreviousClasspath) + + // computeImpactedSymbols(changes = changesOnCurrentClasspath, allClasses = classesOnCurrentClasspath) // However, here we only have the combined changes on both the previous and current classpath, and because it's okay to // over-approximate the result, we will modify the above computation into: - // computeImpactedSetInclusive(changes, allClasses = classesOnPreviousClasspath + classesOnCurrentClasspath) - // Note: `allClasses` may contain overlapping ClassIds, but it won't be an issue. Also, we will replace - // classesOnCurrentClasspath with changedClassesOnCurrentClasspath to avoid listing unchanged classes twice. - computeImpactedSetInclusive( + // computeImpactedSet( + // changes = changesOnPreviousAndCurrentClasspath, + // allClasses = classesOnPreviousClasspath + classesOnCurrentClasspath) + // Note: We will replace `classesOnCurrentClasspath` with `changedClassesOnCurrentClasspath` to avoid listing unchanged classes + // twice. `allClasses` may contain overlapping ClassIds of modified classes, but it won't be an issue. + computeImpactedSymbols( changes = changedSet, allClasses = (previousClassSnapshots.asSequence() + changedCurrentClasses.asSequence()).asIterable() ) @@ -240,158 +244,143 @@ object ClasspathChangesComputer { // classes, and symbols in removed classes. incrementalJvmCache.clearCacheForRemovedClasses(changesCollector) - // Normalize the changes and clean up + // Get the changes and clean up val dirtyData = changesCollector.getDirtyData(listOf(incrementalJvmCache), DoNothingICReporter) workingDir.deleteRecursively() - return dirtyData.toProgramSymbols(currentClassSnapshots, previousClassSnapshots) - } - - private fun DirtyData.toProgramSymbols( - currentClassSnapshots: List, - previousClassSnapshots: List - ): ProgramSymbolSet { - // Note that dirtyLookupSymbols may contain added symbols (they can also impact recompilation -- see examples in - // JavaClassChangesComputer). Therefore, we need to consider classes on both the previous and current classpath. The combined list - // may contain overlapping ClassIds, but it won't be an issue. - val allClasses = (previousClassSnapshots.asSequence() + currentClassSnapshots.asSequence()).asIterable() - return dirtyLookupSymbols.toProgramSymbolSet(allClasses).also { - checkDirtyDataNormalization(this, it) - } + // Normalize the changes (convert DirtyData to `ProgramSymbol`s) + // Note: + // - DirtyData may contain added symbols (they can also impact recompilation -- see examples in JavaClassChangesComputer). + // Therefore, we need to consider classes on both the previous and current classpath when converting DirtyData. + // - We have removed unchanged classes earlier in computeChangedAndImpactedSet method, so here we only have changed classes. + // - `changedClasses` may contain overlapping ClassIds of modified classes, but it won't be an issue. + val changedClasses = (previousClassSnapshots.asSequence() + currentClassSnapshots.asSequence()).asIterable() + return dirtyData.toProgramSymbols(changedClasses) } /** - * Checks whether [DirtyData.toProgramSymbols] completed without any inconsistencies. + * Converts this [DirtyData] to [ProgramSymbol]s. * * Specifically, [DirtyData] consists of: * - dirtyLookupSymbols (Collection) * - dirtyClassesFqNamesForceRecompile (Collection) * - * In [DirtyData.toProgramSymbols], we converted only dirtyLookupSymbols to [ProgramSymbol]s as dirtyLookupSymbols should contain all - * the changes. + * First, we will convert `dirtyLookupSymbols` to [ProgramSymbol]s as `dirtyLookupSymbols` should contain all the changes. * - * In the following, we'll check that: - * 1. There are no items in dirtyLookupSymbols that have not yet been converted to [ProgramSymbol]s. - * 2. dirtyClassesFqNames and dirtyClassesFqNamesForceRecompile do not contain new information that can't be derived from - * dirtyLookupSymbols. + * Then, we will check that: + * 1. There are no items in `dirtyLookupSymbols` that have not yet been converted to [ProgramSymbol]s. + * 2. `dirtyClassesFqNames` and `dirtyClassesFqNamesForceRecompile` must not contain new information that can't be derived from + * `dirtyLookupSymbols`. */ - private fun checkDirtyDataNormalization(dirtyData: DirtyData, programSymbols: ProgramSymbolSet) { - val changes = programSymbols.toChangesEither() + private fun DirtyData.toProgramSymbols(changedClasses: Iterable): ProgramSymbolSet { + val changedProgramSymbols = dirtyLookupSymbols.toProgramSymbolSet(changedClasses) - val unmatchedLookupSymbols = dirtyData.dirtyLookupSymbols.toMutableSet().also { - it.removeAll(changes.lookupSymbols.toSet()) + // Check whether there is any info in this DirtyData that has not yet been converted to `changedProgramSymbols` + val (changedLookupSymbols, changedFqNames) = changedProgramSymbols.toChangesEither().let { + it.lookupSymbols.toSet() to it.fqNames.toSet() } - val unmatchedFqNames = (dirtyData.dirtyClassesFqNames).toMutableSet().also { - it.removeAll(changes.fqNames.toSet()) + val unmatchedLookupSymbols = this.dirtyLookupSymbols.toMutableSet().also { + it.removeAll(changedLookupSymbols) + } + val unmatchedFqNames = this.dirtyClassesFqNames.toMutableSet().also { + it.addAll(this.dirtyClassesFqNamesForceRecompile) + it.removeAll(changedFqNames) } - // Some LookupSymbols (reported by IncrementalJvmCache) are invalid. Examples: - // - LookupSymbol(name=, scope=com.example) (detected by - // KotlinOnlyClasspathChangesComputerTest.testTopLevelMembers): SAM-CONSTRUCTOR should have a class scope not a package scope. - // - LookupSymbol(name=BarUseABKt, scope=bar) (detected by - // IncrementalCompilationClasspathSnapshotJvmMultiProjectIT.testMoveFunctionFromLibToApp): The name of a LookupSymbol should not - // end with "Kt" (unless there is a Kotlin class (CLASS kind) whose name ends with "Kt", which is almost never the case). - // Ignore these for now. - // TODO: Fix them later - if (unmatchedLookupSymbols.isNotEmpty()) { - unmatchedLookupSymbols.removeAll { it.name == SAM_LOOKUP_NAME.asString() || it.name.endsWith("Kt") } - } - if (unmatchedFqNames.isNotEmpty()) { - unmatchedFqNames.removeAll { it.asString().endsWith("Kt") } - } + /* When `unmatchedLookupSymbols` or `unmatchedFqNames` is not empty, there are two cases: + * 1. The unmatched LookupSymbols/FqNames are redundant. This is not ideal but because it does not cause incremental compilation + * to be incorrect, we can fix these issues later if they are not easy to fix immediately. + * 2. The unmatched LookupSymbols/FqNames are valid changes. Since they are required for incremental compilation to be correct, we + * must fix these issues immediately. + * In the following, we'll list the known issues for case 1 (and it must be case 1 only). + * TODO: We'll fix these issues later. + */ + // Known issue 1: DirtyData reported by IncrementalJvmCache may include both a class and class member (e.g., + // LookupSymbol("com.example", "A") and LookupSymbol("com.example.A", "someProperty")). When the class LookupSymbol is present, the + // class member LookupSymbol is redundant. When converting DirtyData to ProgramSymbols, we remove redundant class member + // `ProgramSymbol`s, so here we will find that LookupSymbol("com.example.A", "someProperty") is not yet matched. Ignore these + // `LookupSymbol`s for now. + val classesFqNames = changedProgramSymbols.classes.mapTo(mutableSetOf()) { it.asSingleFqName() } + unmatchedLookupSymbols.removeAll { FqName(it.scope) in classesFqNames } + + // Known issue 2: If class A has a companion object containing a constant `CONSTANT`, and if the value of `CONSTANT` has changed, + // then only `A.class` will change, not `A.Companion.class` (see `ConstantsInCompanionObjectImpact`). Since we distinguish between + // changed symbols and impacted symbols, we should detect that: + // - A.CONSTANT has changed + // - A.Companion.CONSTANT is unchanged but impacted (this detection happens after the step here) + // + // However, currently IncrementalJvmCache will report that both `A.CONSTANT` and `A.Companion.CONSTANT` have changed (see + // `IncrementalJvmCache.ConstantsMap.process`) as it needs to work with both the old IC and the new IC (in the old IC, changed + // symbols and impacted symbols are not clearly separated). + // + // With the new IC, when converting DirtyData to ProgramSymbols (this method), because we consider only changed classes and + // `A.Companion.class` is unchanged, we will not convert `A.Companion.CONSTANT`. Therefore, `A.Companion.CONSTANT` is unmatched, + // and we'll need to ignore it here. + // + // Note: Once we are able to remove this workaround, we can remove RegularKotlinClassSnapshot.companionObjectName as this is the only + // usage of that property. + val companionObjectFqNames = changedClasses.mapNotNullTo(mutableSetOf()) { clazz -> + (clazz as? RegularKotlinClassSnapshot)?.companionObjectName?.let { it -> + clazz.classId.createNestedClassId(Name.identifier(it)).asSingleFqName() + } + } + unmatchedLookupSymbols.removeAll { FqName(it.scope) in companionObjectFqNames } + unmatchedFqNames.removeAll(companionObjectFqNames) + + // Known issue 3: LookupSymbol(name=, scope=com.example) reported by IncrementalJvmCache is invalid (detected by + // KotlinOnlyClasspathChangesComputerTest.testTopLevelMembers): SAM-CONSTRUCTOR should have a class scope, not a package scope. + unmatchedLookupSymbols.removeAll { it.name == SAM_LOOKUP_NAME.asString() && FqName(it.scope) !in classesFqNames } + + // Known issue 4: LookupSymbol(name=BarUseABKt, scope=bar) reported by IncrementalJvmCache is invalid (detected by + // IncrementalCompilationClasspathSnapshotJvmMultiProjectIT.testMoveFunctionFromLibToApp): The name of a LookupSymbol should not + // end with "Kt" (unless there is a Kotlin class (CLASS kind) whose name ends with "Kt", which is almost never the case). + unmatchedLookupSymbols.removeAll { it.name.endsWith("Kt") } + unmatchedFqNames.removeAll { it.asString().endsWith("Kt") } + + /* + * End of known issues, throw an Exception. + */ check(unmatchedLookupSymbols.isEmpty()) { "The following LookupSymbols are not yet converted to ProgramSymbols: ${unmatchedLookupSymbols.joinToString(", ")}" } check(unmatchedFqNames.isEmpty()) { - "The following FqNames are not found in DirtyData.dirtyLookupSymbols: ${unmatchedFqNames.joinToString(", ")}.\n" + - "DirtyData = $dirtyData" + "The following FqNames can't be derived from DirtyData.dirtyLookupSymbols: ${unmatchedFqNames.joinToString(", ")}.\n" + + "DirtyData = $this" } + + return changedProgramSymbols } } -internal object ImpactAnalysis { +private object ImpactedSymbolsComputer { /** - * Computes the set of [ProgramSymbol]s that are impacted by the given changes. + * Computes the set of [ProgramSymbol]s that are *transitively* impacted by the given set of [ProgramSymbol]s. For example, if a + * superclass has changed/been impacted, its subclasses will be impacted. * - * For example, if a superclass has changed, any of its subclasses will be impacted even if it has not changed because unchanged source - * files in the previous compilation that depended on the subclasses will need to be recompiled. - * - * The returned set includes the given changes plus the impacted ones. + * The returned set is *inclusive* (it contains the given set + the directly/transitively impacted ones). */ - fun computeImpactedSetInclusive(changes: ProgramSymbolSet, allClasses: Iterable): ProgramSymbolSet { - val classIdToSubclasses = getClassIdToSubclassesMap(allClasses) - val impactedClassesResolver = { classId: ClassId -> classIdToSubclasses[classId] ?: emptySet() } + fun computeImpactedSymbols(changes: ProgramSymbolSet, allClasses: Iterable): ProgramSymbolSet { + val impactedSymbolsResolver = AllImpacts.getResolver(allClasses) + return ProgramSymbolSet.Collector().apply { + // Add impacted classes + val impactedClasses = findReachableNodes(changes.classes, impactedSymbolsResolver::getImpactedClasses) + addClasses(impactedClasses) - return ProgramSymbolSet.Collector().run { - addClasses(findImpactedClassesInclusive(changes.classes, impactedClassesResolver)) - - for ((classId, memberNames) in changes.classMembers) { - findImpactedClassesInclusive(setOf(classId), impactedClassesResolver).forEach { impactedClassId -> - addClassMembers(impactedClassId, memberNames) - } + // Add impacted class members + val classMembers = changes.classMembers.map { ClassMembers(it.key, it.value) } + val impactedClassMembers = findReachableNodes(classMembers, impactedSymbolsResolver::getImpactedClassMembers) + impactedClassMembers.forEach { + addClassMembers(it.classId, it.memberNames) } - for ((packageFqName, memberNames) in changes.packageMembers) { + // Package members are currently not impacted, so we just copy the original set over + changes.packageMembers.forEach { (packageFqName, memberNames) -> addPackageMembers(packageFqName, memberNames) } - - getResult() - } + }.getResult() } - private fun getClassIdToSubclassesMap(allClasses: Iterable): Map> { - val classIds: Set = allClasses.mapTo(mutableSetOf()) { it.classId } // Use Set for presence check - val classNameToClassId = classIds.associateBy { JvmClassName.byClassId(it) } - val classNameToClassIdResolver = { className: JvmClassName -> classNameToClassId[className] } - - val classIdToSubclasses = mutableMapOf>() - allClasses.forEach { classSnapshot -> - // No need to collect supertypes outside the given set of classes (e.g., "java/lang/Object") - classSnapshot.getSupertypes(classNameToClassIdResolver).intersect(classIds).forEach { supertype -> - classIdToSubclasses.getOrPut(supertype) { mutableSetOf() }.add(classSnapshot.classId) - } - } - return classIdToSubclasses - } - - /** - * Finds directly and transitively impacted classes of the given classes. The return set includes both the given classes and the - * impacted classes. - */ - fun findImpactedClassesInclusive(classIds: Set, impactedClassesResolver: (ClassId) -> Set): Set { - // Standard Breadth-First Search - val visitedAndToVisitClasses = classIds.toMutableSet() - val classesToVisit = ArrayDeque(classIds) - - while (classesToVisit.isNotEmpty()) { - val classToVisit = classesToVisit.removeFirst() - val nextClassesToVisit = impactedClassesResolver.invoke(classToVisit) - visitedAndToVisitClasses - visitedAndToVisitClasses.addAll(nextClassesToVisit) - classesToVisit.addAll(nextClassesToVisit) - } - return visitedAndToVisitClasses - } -} - -/** - * Returns the [ClassId]s of the supertypes of this class (could be empty in some cases). - * - * @param classIdResolver Resolves the [ClassId] from the [JvmClassName] of a supertype. It may return null if the supertype is outside the - * considered set of classes (e.g., "java/lang/Object"). Those supertypes do not need to be included in the returned result. - */ -internal fun AccessibleClassSnapshot.getSupertypes(classIdResolver: (JvmClassName) -> ClassId?): Set { - return when (this) { - is RegularKotlinClassSnapshot -> supertypes.mapNotNullTo(mutableSetOf()) { classIdResolver.invoke(it) } - is PackageFacadeKotlinClassSnapshot, is MultifileClassKotlinClassSnapshot -> { - // These classes may have supertypes (e.g., kotlin/collections/ArraysKt (MULTIFILE_CLASS) extends - // kotlin/collections/ArraysKt___ArraysKt (MULTIFILE_CLASS_PART)), but we don't have to use that info during impact analysis - // because those inheritors and supertypes should have the same package names, and in package facades only the package names and - // member names matter. - emptySet() - } - is JavaClassSnapshot -> supertypes.mapNotNullTo(mutableSetOf()) { classIdResolver.invoke(it) } - } } diff --git a/compiler/incremental-compilation-impl/src/org/jetbrains/kotlin/incremental/classpathDiff/ClasspathSnapshot.kt b/compiler/incremental-compilation-impl/src/org/jetbrains/kotlin/incremental/classpathDiff/ClasspathSnapshot.kt index b36d6dd0aa5..27685fc99d1 100644 --- a/compiler/incremental-compilation-impl/src/org/jetbrains/kotlin/incremental/classpathDiff/ClasspathSnapshot.kt +++ b/compiler/incremental-compilation-impl/src/org/jetbrains/kotlin/incremental/classpathDiff/ClasspathSnapshot.kt @@ -62,7 +62,14 @@ class RegularKotlinClassSnapshot( override val classId: ClassId, override val classAbiHash: Long, override val classMemberLevelSnapshot: KotlinClassInfo?, - val supertypes: List + val supertypes: List, + + /** Name of the companion object of this class (default is "Companion") iff this class HAS a companion object, or null otherwise. */ + val companionObjectName: String?, + + /** List of constants defined in this class iff this class IS a companion object, or null otherwise. The list could be empty. */ + val constantsInCompanionObject: List? + ) : KotlinClassSnapshot() /** [KotlinClassSnapshot] where class kind == [FILE_FACADE] or [MULTIFILE_CLASS_PART]. */ diff --git a/compiler/incremental-compilation-impl/src/org/jetbrains/kotlin/incremental/classpathDiff/ClasspathSnapshotSerializer.kt b/compiler/incremental-compilation-impl/src/org/jetbrains/kotlin/incremental/classpathDiff/ClasspathSnapshotSerializer.kt index b2b05c69257..4a9d8a51f5e 100644 --- a/compiler/incremental-compilation-impl/src/org/jetbrains/kotlin/incremental/classpathDiff/ClasspathSnapshotSerializer.kt +++ b/compiler/incremental-compilation-impl/src/org/jetbrains/kotlin/incremental/classpathDiff/ClasspathSnapshotSerializer.kt @@ -120,6 +120,8 @@ private object RegularKotlinClassSnapshotExternalizer : DataExternalizer, lookupSymbols: Collection, @@ -57,7 +62,7 @@ object ClasspathSnapshotShrinker { } /** - * Finds classes that are referenced by the given lookup symbols. + * Finds classes that are *directly* referenced by the given lookup symbols. * * Note: It's okay to over-approximate the result. */ @@ -90,31 +95,19 @@ object ClasspathSnapshotShrinker { } /** - * Finds classes that are transitively referenced. For example, if a subclass is referenced, its supertypes will potentially be - * referenced. + * Finds classes that are *transitively* referenced from the given classes. For example, if a subclass is referenced, its supertypes + * will be transitively referenced. * - * The returned list includes the given referenced classes plus the transitively referenced ones. + * The returned list is *inclusive* (it contains the given list + the transitively referenced ones). */ private fun findTransitivelyReferencedClasses( allClasses: List, referencedClasses: List ): List { - val classIdToClassSnapshot = allClasses.associateBy { it.classId } - val classIds: Set = classIdToClassSnapshot.keys // Use Set for presence check - val classNameToClassId = classIds.associateBy { JvmClassName.byClassId(it) } - val classNameToClassIdResolver = { className: JvmClassName -> classNameToClassId[className] } - - val supertypesResolver = { classId: ClassId -> - // No need to collect supertypes outside the given set of classes (e.g., "java/lang/Object") - @Suppress("SimpleRedundantLet") - classIdToClassSnapshot[classId]?.let { - it.getSupertypes(classNameToClassIdResolver).intersect(classIds) - } ?: emptySet() - } - - val referencedClassIds = referencedClasses.mapTo(mutableSetOf()) { it.classId } - val transitivelyReferencedClassIds: Set = /* Use Set for presence check */ - ImpactAnalysis.findImpactedClassesInclusive(referencedClassIds, supertypesResolver) + val referencedClassIds = referencedClasses.map { it.classId } + val impactingClassesResolver = AllImpacts.getReverseResolver(allClasses) + val transitivelyReferencedClassIds: Set = /* Must be a Set for the presence check below */ + findReachableNodes(referencedClassIds, impactingClassesResolver::getImpactingClasses) return allClasses.filter { it.classId in transitivelyReferencedClassIds } } diff --git a/compiler/incremental-compilation-impl/src/org/jetbrains/kotlin/incremental/classpathDiff/ClasspathSnapshotter.kt b/compiler/incremental-compilation-impl/src/org/jetbrains/kotlin/incremental/classpathDiff/ClasspathSnapshotter.kt index bafb60cac7f..cbdd5cb00d4 100644 --- a/compiler/incremental-compilation-impl/src/org/jetbrains/kotlin/incremental/classpathDiff/ClasspathSnapshotter.kt +++ b/compiler/incremental-compilation-impl/src/org/jetbrains/kotlin/incremental/classpathDiff/ClasspathSnapshotter.kt @@ -89,7 +89,12 @@ object ClassSnapshotter { val classMemberLevelSnapshot = kotlinClassInfo.takeIf { granularity == CLASS_MEMBER_LEVEL } return when (kotlinClassInfo.classKind) { - CLASS -> RegularKotlinClassSnapshot(classId, classAbiHash, classMemberLevelSnapshot, classFile.classInfo.supertypes) + CLASS -> RegularKotlinClassSnapshot( + classId, classAbiHash, classMemberLevelSnapshot, + supertypes = classFile.classInfo.supertypes, + companionObjectName = kotlinClassInfo.companionObject?.shortClassName?.identifier, + constantsInCompanionObject = kotlinClassInfo.constantsInCompanionObject + ) FILE_FACADE, MULTIFILE_CLASS_PART -> PackageFacadeKotlinClassSnapshot( classId, classAbiHash, classMemberLevelSnapshot, packageMemberNames = (kotlinClassInfo.protoData as PackagePartProtoData).getNonPrivateMemberNames() diff --git a/compiler/incremental-compilation-impl/src/org/jetbrains/kotlin/incremental/classpathDiff/Impact.kt b/compiler/incremental-compilation-impl/src/org/jetbrains/kotlin/incremental/classpathDiff/Impact.kt new file mode 100644 index 00000000000..5d3819a7c40 --- /dev/null +++ b/compiler/incremental-compilation-impl/src/org/jetbrains/kotlin/incremental/classpathDiff/Impact.kt @@ -0,0 +1,240 @@ +/* + * Copyright 2010-2022 JetBrains s.r.o. and Kotlin Programming Language contributors. + * Use of this source code is governed by the Apache 2.0 license that can be found in the license/LICENSE.txt file. + */ + +package org.jetbrains.kotlin.incremental.classpathDiff + +import org.jetbrains.kotlin.name.ClassId +import org.jetbrains.kotlin.resolve.jvm.JvmClassName +import java.util.* + +/** + * A common interface for all types of impact among classes. For example, if class B extends class A, then class A impacts class B, because + * if class A has changed, a source file that references class B will need to be recompiled (even though class B has not changed). + */ +internal sealed interface Impact { + + /** Provides an [ImpactedSymbolsResolver] to compute the set of [ProgramSymbol]s impacted by a given set of [ProgramSymbol]s. */ + fun getResolver(allClasses: Iterable): ImpactedSymbolsResolver + + /** + * Provides an [ImpactingClassesResolver] to compute the set of classes impacting a given set of classes (the reverse of [getResolver]). + */ + fun getReverseResolver(allClasses: Iterable): ImpactingClassesResolver +} + +/** + * Computes the set of [ProgramSymbol]s that are *directly* impacted by a given set of [ProgramSymbol]s. + * + * The returned set is *inclusive* (it contains the given set + the directly impacted ones). + * + * This is typically used when computing classpath changes: If class A has changed, and it impacts class B, then a source file that + * references class B will need to be recompiled (even though class B has not changed). + */ +internal interface ImpactedSymbolsResolver { + fun getImpactedClasses(classId: ClassId): Set + fun getImpactedClassMembers(classMembers: ClassMembers): Set +} + +/** + * Computes the set of classes *directly* impacting a given set of classes. + * + * The returned set is *inclusive* (it contains the given set + the directly impacting ones). + * + * This is typically used when shrinking classpath snapshots: If class A impacts class B, and class B is referenced by a source file, then + * class A will need to be retained in the shrunk classpath snapshot because the classpath changes computation will need to see class A + * (see [ImpactedSymbolsResolver]). + */ +internal interface ImpactingClassesResolver { + fun getImpactingClasses(classId: ClassId): Set +} + +/** + * A composite [Impact] containing all possible concrete impacts. Currently, the types of impact include: + * 1. [SupertypesInheritorsImpact] + * 2. [ConstantsInCompanionObjectsImpact] + */ +internal object AllImpacts : Impact { + + private val allImpacts = listOf(SupertypesInheritorsImpact, ConstantsInCompanionObjectsImpact) + + override fun getResolver(allClasses: Iterable): ImpactedSymbolsResolver { + val resolvers = allImpacts.map { it.getResolver(allClasses) } + return object : ImpactedSymbolsResolver { + override fun getImpactedClasses(classId: ClassId): Set { + return resolvers.flatMapTo(mutableSetOf()) { it.getImpactedClasses(classId) } + } + + override fun getImpactedClassMembers(classMembers: ClassMembers): Set { + return resolvers.flatMapTo(mutableSetOf()) { it.getImpactedClassMembers(classMembers) } + } + } + } + + override fun getReverseResolver(allClasses: Iterable): ImpactingClassesResolver { + val reverseResolvers = allImpacts.map { it.getReverseResolver(allClasses) } + return object : ImpactingClassesResolver { + override fun getImpactingClasses(classId: ClassId): Set { + return reverseResolvers.flatMapTo(mutableSetOf()) { it.getImpactingClasses(classId) } + } + } + } +} + +/** + * Describes the impact between supertypes and inheritors: If a superclass/interface has changed, its subclasses/sub-interfaces will be + * impacted. + */ +private object SupertypesInheritorsImpact : Impact { + + override fun getResolver(allClasses: Iterable): ImpactedSymbolsResolver { + val classIdToSubclasses: Map> = getClassIdToSubclassesMap(allClasses) + return object : ImpactedSymbolsResolver { + override fun getImpactedClasses(classId: ClassId): Set { + return classIdToSubclasses[classId] ?: emptySet() + } + + override fun getImpactedClassMembers(classMembers: ClassMembers): Set { + return classIdToSubclasses[classMembers.classId]?.let { subclasses -> + subclasses.mapTo(mutableSetOf()) { subclass -> + ClassMembers(subclass, classMembers.memberNames) + } + } ?: emptySet() + } + } + } + + override fun getReverseResolver(allClasses: Iterable): ImpactingClassesResolver { + val classIdToSupertypesMap: Map> = getClassIdToSupertypesMap(allClasses) + return object : ImpactingClassesResolver { + override fun getImpactingClasses(classId: ClassId): Set { + return classIdToSupertypesMap[classId] ?: emptySet() + } + } + } + + private fun getClassIdToSubclassesMap(allClasses: Iterable): Map> { + val classIdToSubclasses = mutableMapOf>() + getClassIdToSupertypesMap(allClasses).forEach { (classId, supertypes) -> + supertypes.forEach { supertype -> + classIdToSubclasses.getOrPut(supertype) { mutableSetOf() }.add(classId) + } + } + return classIdToSubclasses + } + + private fun getClassIdToSupertypesMap(allClasses: Iterable): Map> { + val classNameToClassId = allClasses.associate { JvmClassName.byClassId(it.classId) to it.classId } + return allClasses.mapNotNull { clazz -> + // Find supertypes that are within `allClasses`, we don't care about those outside `allClasses` (e.g., `java/lang/Object`) + val supertypes = when (clazz) { + is RegularKotlinClassSnapshot -> clazz.supertypes.mapNotNullTo(mutableSetOf()) { classNameToClassId[it] } + is PackageFacadeKotlinClassSnapshot, is MultifileClassKotlinClassSnapshot -> { + // These classes may have supertypes (e.g., kotlin/collections/ArraysKt (MULTIFILE_CLASS) extends + // kotlin/collections/ArraysKt___ArraysKt (MULTIFILE_CLASS_PART)), but we don't have to use that info during impact + // analysis because those inheritors and supertypes should have the same package names, and in package facades only the + // package names and member names matter. + emptySet() + } + is JavaClassSnapshot -> clazz.supertypes.mapNotNullTo(mutableSetOf()) { classNameToClassId[it] } + } + if (supertypes.isNotEmpty()) { + clazz.classId to supertypes + } else null + }.toMap() + } +} + +/** + * Describes the impact between a class and its companion object when the companion object defines some constants. + * + * Consider the following source file: + * class A { + * companion object { + * const val CONSTANT = 1 + * } + * } + * + * This source file will compile into 2 .class files: + * - `A.Companion.class`'s Kotlin metadata describes the name and type of `CONSTANT` but not its value. Its Java bytecode does not define + * the constant. + * - `A.class`'s Kotlin metadata does not contain `CONSTANT`. However, its Java bytecode defines the constant as follows: + * public static final int CONSTANT = 1; + * + * Therefore, if the value of the constant has changed in the source file, we will only see a change in the Java bytecode of `A.class`, not + * in `A.Companion.class` or in the Kotlin metadata of either class. + * + * Hence, we will need to detect that `A.CONSTANT` impacts `A.Companion.CONSTANT` because if a source file references + * `A.Companion.CONSTANT`, it will need to be recompiled when `A.CONSTANT`'s value in `A.class` has changed (even though `A.Companion.class` + * has not changed). + * + * Note: This corner case only applies to *constants' values* defined in *companion objects* (it does not apply to constants' names and + * types, or top-level constants, or constants in non-companion objects, or inline functions). + */ +private object ConstantsInCompanionObjectsImpact : Impact { + + override fun getResolver(allClasses: Iterable): ImpactedSymbolsResolver { + val companionObjectToConstants: Map> = allClasses.mapNotNull { clazz -> + (clazz as? RegularKotlinClassSnapshot)?.constantsInCompanionObject?.let { constants -> + // We only care about companion objects that define some constants + if (constants.isNotEmpty()) { + clazz.classId to constants + } else null + } + }.toMap() + val classToCompanionObject: Map = companionObjectToConstants.keys.associateBy { it.parentClassId!! } + + return object : ImpactedSymbolsResolver { + override fun getImpactedClasses(classId: ClassId): Set { + return setOfNotNull(classToCompanionObject[classId]) + } + + override fun getImpactedClassMembers(classMembers: ClassMembers): Set { + return classToCompanionObject[classMembers.classId]?.let { companionObject -> + val constantsInCompanionObject = companionObjectToConstants[companionObject]!! + val impactedConstants = classMembers.memberNames.intersect(constantsInCompanionObject.toSet()) + setOf(ClassMembers(companionObject, impactedConstants)) + } ?: emptySet() + } + } + } + + override fun getReverseResolver(allClasses: Iterable): ImpactingClassesResolver { + val companionObjects: Set = allClasses.mapNotNullTo(mutableSetOf()) { clazz -> + (clazz as? RegularKotlinClassSnapshot)?.constantsInCompanionObject?.let { constants -> + // We only care about companion objects that define some constants + if (constants.isNotEmpty()) clazz.classId else null + } + } + + return object : ImpactingClassesResolver { + override fun getImpactingClasses(classId: ClassId): Set { + return if (classId in companionObjects) { + setOf(classId.parentClassId!!) + } else emptySet() + } + } + } +} + +internal object BreadthFirstSearch { + + /** + * Finds the set of nodes that are *transitively* reachable from the given set of nodes. + * + * The returned set is *inclusive* (it contains the given set + the directly/transitively reachable ones). + */ + fun findReachableNodes(nodes: Iterable, edgesProvider: (T) -> Iterable): Set { + val visitedAndToVisitNodes = nodes.toMutableSet() + val nodesToVisit = ArrayDeque(nodes.toSet()) + + while (nodesToVisit.isNotEmpty()) { + val nodeToVisit = nodesToVisit.removeFirst() + val nextNodesToVisit = edgesProvider.invoke(nodeToVisit) - visitedAndToVisitNodes + visitedAndToVisitNodes.addAll(nextNodesToVisit) + nodesToVisit.addAll(nextNodesToVisit) + } + return visitedAndToVisitNodes + } +} diff --git a/compiler/incremental-compilation-impl/src/org/jetbrains/kotlin/incremental/classpathDiff/ProgramSymbol.kt b/compiler/incremental-compilation-impl/src/org/jetbrains/kotlin/incremental/classpathDiff/ProgramSymbol.kt index 72c48ab64d7..24fda1ead91 100644 --- a/compiler/incremental-compilation-impl/src/org/jetbrains/kotlin/incremental/classpathDiff/ProgramSymbol.kt +++ b/compiler/incremental-compilation-impl/src/org/jetbrains/kotlin/incremental/classpathDiff/ProgramSymbol.kt @@ -21,6 +21,9 @@ data class ClassMember(val classId: ClassId, val memberName: String) : ProgramSy data class PackageMember(val packageFqName: FqName, val memberName: String) : ProgramSymbol() +/** Compact representation for set of [ClassMember]s having the same [ClassId]. */ +data class ClassMembers(val classId: ClassId, val memberNames: Set) + /** Compact representation for a set of [ProgramSymbol]s. */ class ProgramSymbolSet private constructor( diff --git a/compiler/incremental-compilation-impl/test/org/jetbrains/kotlin/incremental/classpathDiff/ClasspathChangesComputerTest.kt b/compiler/incremental-compilation-impl/test/org/jetbrains/kotlin/incremental/classpathDiff/ClasspathChangesComputerTest.kt index c81add82ea4..e5c28d32538 100644 --- a/compiler/incremental-compilation-impl/test/org/jetbrains/kotlin/incremental/classpathDiff/ClasspathChangesComputerTest.kt +++ b/compiler/incremental-compilation-impl/test/org/jetbrains/kotlin/incremental/classpathDiff/ClasspathChangesComputerTest.kt @@ -217,6 +217,9 @@ class KotlinOnlyClasspathChangesComputerTest : ClasspathChangesComputerTest() { val changes = computeClasspathChanges(File(testDataDir, "KotlinOnly/testConstantsAndInlineFunctions/src"), tmpDir) Changes( lookupSymbols = setOf( + LookupSymbol(name = "constantChangedType", scope = "com.example.SomeClass"), + LookupSymbol(name = "constantChangedValue", scope = "com.example.SomeClass"), + LookupSymbol(name = "constantChangedType", scope = "com.example.SomeClass.CompanionObject"), LookupSymbol(name = "constantChangedValue", scope = "com.example.SomeClass.CompanionObject"), diff --git a/compiler/incremental-compilation-impl/test/org/jetbrains/kotlin/incremental/classpathDiff/ClasspathSnapshotTestCommon.kt b/compiler/incremental-compilation-impl/test/org/jetbrains/kotlin/incremental/classpathDiff/ClasspathSnapshotTestCommon.kt index 59920509008..518b1b289bf 100644 --- a/compiler/incremental-compilation-impl/test/org/jetbrains/kotlin/incremental/classpathDiff/ClasspathSnapshotTestCommon.kt +++ b/compiler/incremental-compilation-impl/test/org/jetbrains/kotlin/incremental/classpathDiff/ClasspathSnapshotTestCommon.kt @@ -12,6 +12,8 @@ import org.jetbrains.kotlin.incremental.classpathDiff.ClasspathSnapshotTestCommo import org.jetbrains.kotlin.incremental.classpathDiff.ClasspathSnapshotTestCommon.CompileUtil.compile import org.jetbrains.kotlin.incremental.classpathDiff.ClasspathSnapshotTestCommon.CompileUtil.compileAll import org.jetbrains.kotlin.incremental.classpathDiff.ClasspathSnapshotTestCommon.SourceFile.KotlinSourceFile +import org.jetbrains.kotlin.incremental.storage.fromByteArray +import org.jetbrains.kotlin.incremental.storage.toByteArray import org.jetbrains.kotlin.test.KotlinTestUtils import org.junit.Rule import org.junit.rules.TemporaryFolder @@ -25,7 +27,11 @@ abstract class ClasspathSnapshotTestCommon { // Use Gson to compare objects private val gson by lazy { GsonBuilder().setPrettyPrinting().create() } - protected fun Any.toGson(): String = gson.toJson(this) + protected fun ClassSnapshot.toGson(): String = gson.toJson( + // Serialize and deserialize the object to unset lazy properties' values as they are not essential and can add noise when comparing + // objects + ClassSnapshotExternalizer.fromByteArray(ClassSnapshotExternalizer.toByteArray(this)) + ) sealed class SourceFile(val baseDir: File, relativePath: String) { val unixStyleRelativePath: String diff --git a/compiler/incremental-compilation-impl/testData/org/jetbrains/kotlin/incremental/classpathDiff/ClasspathSnapshotterTest/kotlin/testSimpleClass/expected-snapshot/com/example/SimpleClass.json b/compiler/incremental-compilation-impl/testData/org/jetbrains/kotlin/incremental/classpathDiff/ClasspathSnapshotterTest/kotlin/testSimpleClass/expected-snapshot/com/example/SimpleClass.json index 0f17a1a22ce..88e67fc50cb 100644 --- a/compiler/incremental-compilation-impl/testData/org/jetbrains/kotlin/incremental/classpathDiff/ClasspathSnapshotterTest/kotlin/testSimpleClass/expected-snapshot/com/example/SimpleClass.json +++ b/compiler/incremental-compilation-impl/testData/org/jetbrains/kotlin/incremental/classpathDiff/ClasspathSnapshotterTest/kotlin/testSimpleClass/expected-snapshot/com/example/SimpleClass.json @@ -56,6 +56,14 @@ "protoData$delegate": { "initializer": {}, "_value": {} + }, + "companionObject$delegate": { + "initializer": {}, + "_value": {} + }, + "constantsInCompanionObject$delegate": { + "initializer": {}, + "_value": {} } }, "supertypes": [