azalea/kotlin-fork
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

New IC: Include inline property accessors in class/package members

Browse Source 
Problem: ClasspathChangesComputer (a core component of the new IC) uses
the existing inline function analysis from the old IC
(IncrementalJvmCache.InlineFunctionsMap). If an inline property accessor
has changed, the inline function analysis will report the name of the
property accessor, not the name of the property.
ClasspathChangesComputer doesn't see the property accessor's name in the
list of class/package members, so it will throw an exception.

Solution: There are 2 options:
  1. If an inline property accessor has changed, the inline function
     analysis can report the name of the property instead of the
     property accessor.
  2. ClasspathChangesComputer can include property accessors in the list
     of class/package members.

In this commit, we will choose option 2 as it is simpler.

Test: New KotlinOnlyClasspathChangesComputerTest.testPropertyAccessors

Small cleanup - PLS SQUASH INTO PREVIOUS COMMIT

  - Address review
  - Fix failed tests
  - Add some trivial changes

^KT-53871 Fixed
This commit is contained in:
Hung Nguyen
2022-09-04 15:39:32 +01:00
committed by nataliya.valtman
parent 2ba7c7b8a9
commit 03f83ff339
23 changed files with 376 additions and 126 deletions
Download Patch File Download Diff File Expand all files Collapse all files
build-common/src/org/jetbrains/kotlin/incremental/ChangesCollector.kt
+3 -29
View File
@@ -36,32 +36,6 @@ class ChangesCollector {
fun protoDataChanges(): Map<FqName, ProtoData> = storage
fun protoDataRemoved(): List<FqName> = removed
companion object {
fun <T> T.getNonPrivateNames(nameResolver: NameResolver, vararg members: T.() -> List<MessageLite>) =
members.flatMap { this.it().filterNot { it.isPrivate }.names(nameResolver) }.toSet()
fun ClassProtoData.getNonPrivateMemberNames(): Set<String> {
return proto.getNonPrivateNames(
nameResolver,
// The types below should match the logic at `DifferenceCalculatorForClass.difference`
ProtoBuf.Class::getConstructorList,
ProtoBuf.Class::getFunctionList,
ProtoBuf.Class::getPropertyList,
ProtoBuf.Class::getTypeAliasList
) + proto.enumEntryList.map { nameResolver.getString(it.name) }
}
fun PackagePartProtoData.getNonPrivateMemberNames(): Set<String> {
return proto.getNonPrivateNames(
nameResolver,
// The types below should match the logic at `DifferenceCalculatorForPackageFacade.difference`
ProtoBuf.Package::getFunctionList,
ProtoBuf.Package::getPropertyList,
ProtoBuf.Package::getTypeAliasList
)
}
}
fun changes(): List<ChangeInfo> {
val changes = arrayListOf<ChangeInfo>()
@@ -192,7 +166,7 @@ class ChangesCollector {
}
private fun PackagePartProtoData.collectAllFromPackage(isRemoved: Boolean) {
val memberNames = getNonPrivateMemberNames()
val memberNames = getNonPrivateMemberNames(includeInlineAccessors = true)
if (isRemoved) {
collectRemovedMembers(packageFqName, memberNames)
} else {
@@ -204,14 +178,14 @@ class ChangesCollector {
val classFqName = nameResolver.getClassId(proto.fqName).asSingleFqName()
if (proto.isCompanionObject) {
val memberNames = getNonPrivateMemberNames()
val memberNames = getNonPrivateMemberNames(includeInlineAccessors = true)
val collectMember = if (isRemoved) this@ChangesCollector::collectRemovedMember else this@ChangesCollector::collectChangedMember
collectMember(classFqName.parent(), classFqName.shortName().asString())
memberNames.forEach { collectMember(classFqName, it) }
} else {
if (!isRemoved && collectAllMembersForNewClass) {
val memberNames = getNonPrivateMemberNames()
val memberNames = getNonPrivateMemberNames(includeInlineAccessors = true)
memberNames.forEach { this@ChangesCollector.collectChangedMember(classFqName, it) }
}
build-common/src/org/jetbrains/kotlin/incremental/IncrementalJvmCache.kt
+71 -34
View File
@@ -23,8 +23,11 @@ import com.intellij.util.io.EnumeratorStringDescriptor
import gnu.trove.THashSet
import org.jetbrains.annotations.TestOnly
import org.jetbrains.kotlin.build.GeneratedJvmClass
import org.jetbrains.kotlin.incremental.DifferenceCalculatorForClass.Companion.getNonPrivateMembers
import org.jetbrains.kotlin.incremental.DifferenceCalculatorForPackageFacade.Companion.getNonPrivateMembers
import org.jetbrains.kotlin.incremental.storage.*
import org.jetbrains.kotlin.inline.inlineFunctionsJvmNames
import org.jetbrains.kotlin.inline.inlineAccessors
import org.jetbrains.kotlin.inline.inlineFunctionsAndAccessors
import org.jetbrains.kotlin.load.kotlin.header.KotlinClassHeader
import org.jetbrains.kotlin.load.kotlin.incremental.components.IncrementalCache
import org.jetbrains.kotlin.load.kotlin.incremental.components.JvmPackagePartProto
@@ -39,7 +42,8 @@ import org.jetbrains.kotlin.name.FqName
import org.jetbrains.kotlin.name.Name
import org.jetbrains.kotlin.resolve.jvm.JvmClassName
import org.jetbrains.org.objectweb.asm.*
import org.jetbrains.org.objectweb.asm.ClassReader.*
import org.jetbrains.org.objectweb.asm.ClassReader.SKIP_CODE
import org.jetbrains.org.objectweb.asm.ClassReader.SKIP_DEBUG
import java.io.File
import java.security.MessageDigest
@@ -570,19 +574,26 @@ open class IncrementalJvmCache(
val key = kotlinClassInfo.className.internalName
val oldMap = storage[key] ?: emptyMap()
val newMap = kotlinClassInfo.inlineFunctionsMap
val newMap = kotlinClassInfo.inlineFunctionsAndAccessorsMap
if (newMap.isNotEmpty()) {
storage[key] = newMap
} else {
storage.remove(key)
}
for (fn in oldMap.keys + newMap.keys) {
for (methodSignature in oldMap.keys + newMap.keys) {
val inlineFunctionOrAccessorName = functionNameBySignature(methodSignature)
// Note: If we detect a change in an inline property accessor (e.g., `getFoo`), we have two options:
// 1. Report that property `foo` has changed (and ignore `getFoo`)
// 2. Report that property accessor `getFoo` has changed (and ignore `foo`)
// Both options are possible (the compiler generates `LookupSymbol`s for both `foo` and `getFoo` when `getFoo` is inlined).
// Currently, we choose option 2 as it is simpler (i.e., even though inline property accessors are not immediate members of
// a class/package, we treat them like immediate members).
changesCollector.collectMemberIfValueWasChanged(
kotlinClassInfo.scopeFqName(),
functionNameBySignature(fn),
oldMap[fn],
newMap[fn]
inlineFunctionOrAccessorName,
oldMap[methodSignature],
newMap[methodSignature]
)
}
}
@@ -659,6 +670,28 @@ fun <K : Comparable<K>, V> Map<K, V>.dumpMap(dumpValue: (V) -> String): String =
fun <T : Comparable<T>> Collection<T>.dumpCollection(): String =
"[${sorted().joinToString(", ", transform = Any::toString)}]"
/**
* @param includeInlineAccessors If `true`, also include inline property accessors in the returned list. This is necessary because property
* accessors are not immediate members of a class/package, and in some cases we treat them like immediate members (see
* [org.jetbrains.kotlin.incremental.IncrementalJvmCache.InlineFunctionsMap.process]). Note that here we deal with *inline* property
* accessors only as [org.jetbrains.kotlin.incremental.IncrementalJvmCache.InlineFunctionsMap.process] is written only to handle *inline*
* property accessors (and functions).
*/
fun ProtoData.getNonPrivateMemberNames(includeInlineAccessors: Boolean): List<String> {
return when (this) {
is ClassProtoData -> {
getNonPrivateMembers() + (if (includeInlineAccessors) {
inlineAccessors(proto.propertyList, nameResolver, excludePrivateAccessors = true).map { it.name }
} else emptyList())
}
is PackagePartProtoData -> {
getNonPrivateMembers() + (if (includeInlineAccessors) {
inlineAccessors(proto.propertyList, nameResolver, excludePrivateAccessors = true).map { it.name }
} else emptyList())
}
}
}
/**
* Minimal information about a Kotlin class to compute recompilation-triggering changes during an incremental run of the `KotlinCompile`
* task (see [IncrementalJvmCache.saveClassToCache]).
@@ -674,7 +707,7 @@ class KotlinClassInfo constructor(
val classHeaderStrings: Array<String>, // Can be empty
val multifileClassName: String?, // Not null iff classKind == KotlinClassHeader.Kind.MULTIFILE_CLASS_PART
val constantsMap: LinkedHashMap<String, Any>,
val inlineFunctionsMap: LinkedHashMap<String, Long>
val inlineFunctionsAndAccessorsMap: LinkedHashMap<String, Long>
) {
val className: JvmClassName by lazy { JvmClassName.byClassId(classId) }
@@ -726,48 +759,55 @@ class KotlinClassInfo constructor(
}
fun createFrom(classId: ClassId, classHeader: KotlinClassHeader, classContents: ByteArray): KotlinClassInfo {
val constantsAndInlineFunctions = getConstantsAndInlineFunctions(classHeader, classContents)
val (constants, inlineFunctionsAndAccessors) = getConstantsAndInlineFunctionsOrAccessors(classHeader, classContents)
return KotlinClassInfo(
classId,
classHeader.kind,
classHeader.data ?: classHeader.incompatibleData ?: emptyArray(),
classHeader.data ?: emptyArray(),
classHeader.strings ?: emptyArray(),
classHeader.multifileClassName,
constantsMap = constantsAndInlineFunctions.first,
inlineFunctionsMap = constantsAndInlineFunctions.second
constants.mapKeysTo(LinkedHashMap()) { it.key.name },
inlineFunctionsAndAccessors.mapKeysTo(LinkedHashMap()) { it.key.asString() },
)
}
}
}
/** Parses the class file only once to get both constants and inline functions. */
private fun getConstantsAndInlineFunctions(
/**
* Parses the class file only once to get both constants and inline functions/property accessors. This is faster than getting them
* separately in two passes.
*/
private fun getConstantsAndInlineFunctionsOrAccessors(
classHeader: KotlinClassHeader,
classContents: ByteArray
): Pair<LinkedHashMap<String, Any>, LinkedHashMap<String, Long>> {
): Pair<Map<JvmMemberSignature.Field, Any>, Map<JvmMemberSignature.Method, Long>> {
val constantsClassVisitor = ConstantsClassVisitor()
val inlineFunctionSignatures = inlineFunctionsJvmNames(classHeader)
val inlineFunctionsAndAccessors = inlineFunctionsAndAccessors(classHeader)
return if (inlineFunctionSignatures.isEmpty()) {
ClassReader(classContents).accept(constantsClassVisitor, SKIP_CODE or SKIP_DEBUG or SKIP_FRAMES)
Pair(constantsClassVisitor.getResult(), LinkedHashMap())
return if (inlineFunctionsAndAccessors.isEmpty()) {
// parsingOptions = (SKIP_CODE, SKIP_DEBUG) as method bodies and debug info are not important for constants
ClassReader(classContents).accept(constantsClassVisitor, SKIP_CODE or SKIP_DEBUG)
Pair(constantsClassVisitor.getResult(), emptyMap())
} else {
val inlineFunctionsClassVisitor = InlineFunctionsClassVisitor(inlineFunctionSignatures, constantsClassVisitor)
ClassReader(classContents).accept(inlineFunctionsClassVisitor, 0)
Pair(constantsClassVisitor.getResult(), inlineFunctionsClassVisitor.getResult())
val inlineFunctionsAndAccessorsClassVisitor =
InlineFunctionsAndAccessorsClassVisitor(inlineFunctionsAndAccessors.toSet(), constantsClassVisitor)
// parsingOptions must not include (SKIP_CODE, SKIP_DEBUG) as method bodies and debug info (e.g., line numbers) are important for
// inline functions/accessors
ClassReader(classContents).accept(inlineFunctionsAndAccessorsClassVisitor, 0)
Pair(constantsClassVisitor.getResult(), inlineFunctionsAndAccessorsClassVisitor.getResult())
}
}
private class ConstantsClassVisitor : ClassVisitor(Opcodes.API_VERSION) {
private val result = LinkedHashMap<String, Any>()
private val result = mutableMapOf<JvmMemberSignature.Field, Any>()
override fun visitField(access: Int, name: String, desc: String, signature: String?, value: Any?): FieldVisitor? {
if (access and Opcodes.ACC_PRIVATE == Opcodes.ACC_PRIVATE) return null
val staticFinal = Opcodes.ACC_STATIC or Opcodes.ACC_FINAL
if (value != null && access and staticFinal == staticFinal) {
result[name] = value
result[JvmMemberSignature.Field(name, desc)] = value
}
return null
}
@@ -775,12 +815,12 @@ private class ConstantsClassVisitor : ClassVisitor(Opcodes.API_VERSION) {
fun getResult() = result
}
private class InlineFunctionsClassVisitor(
private val inlineFunctionSignatures: Set<String>,
cv: ConstantsClassVisitor // Note: cv must not override the visitMethod (it will not be called with the current implementation below)
private class InlineFunctionsAndAccessorsClassVisitor(
private val inlineFunctionsAndAccessors: Set<JvmMemberSignature.Method>,
cv: ConstantsClassVisitor // Note: cv must not override `visitMethod` (it will not be called with the current implementation below)
) : ClassVisitor(Opcodes.API_VERSION, cv) {
private val result = LinkedHashMap<String, Long>()
private val result = mutableMapOf<JvmMemberSignature.Method, Long>()
private var classVersion: Int? = null
override fun visit(version: Int, access: Int, name: String, signature: String?, superName: String?, interfaces: Array<out String>?) {
@@ -791,11 +831,8 @@ private class InlineFunctionsClassVisitor(
override fun visitMethod(access: Int, name: String, desc: String, signature: String?, exceptions: Array<out String>?): MethodVisitor? {
if (access and Opcodes.ACC_PRIVATE == Opcodes.ACC_PRIVATE) return null
// Note: Here, functionSignature = name + descriptor.
// It is different from the `signature` parameter above, which is essentially a more detailed descriptor when generics are used
// (or null otherwise).
val functionSignature = JvmMemberSignature.Method(name, desc).asString()
if (functionSignature !in inlineFunctionSignatures) return null
val method = JvmMemberSignature.Method(name, desc)
if (method !in inlineFunctionsAndAccessors) return null
val classWriter = ClassWriter(0)
@@ -804,7 +841,7 @@ private class InlineFunctionsClassVisitor(
return object : MethodVisitor(Opcodes.API_VERSION, classWriter.visitMethod(access, name, desc, signature, exceptions)) {
override fun visitEnd() {
result[functionSignature] = classWriter.toByteArray().md5()
result[method] = classWriter.toByteArray().md5()
}
}
}
build-common/src/org/jetbrains/kotlin/incremental/protoDifferenceUtils.kt
+46 -11
View File
@@ -51,17 +51,17 @@ fun ProtoMapValue.toProtoData(packageFqName: FqName): ProtoData =
}
internal val MessageLite.isPrivate: Boolean
get() = DescriptorVisibilities.isPrivate(
ProtoEnumFlags.descriptorVisibility(
when (this) {
is ProtoBuf.Constructor -> Flags.VISIBILITY.get(flags)
is ProtoBuf.Function -> Flags.VISIBILITY.get(flags)
is ProtoBuf.Property -> Flags.VISIBILITY.get(flags)
is ProtoBuf.TypeAlias -> Flags.VISIBILITY.get(flags)
else -> error("Unknown message: $this")
}
)
)
get() {
val visibility = when (this) {
is ProtoBuf.Constructor -> Flags.VISIBILITY.get(flags)
is ProtoBuf.Function -> Flags.VISIBILITY.get(flags)
is ProtoBuf.Property -> Flags.VISIBILITY.get(flags)
is ProtoBuf.TypeAlias -> Flags.VISIBILITY.get(flags)
is ProtoBuf.EnumEntry -> return false // EnumEntry doesn't have a visibility flag
else -> error("Unknown message: $this")
}
return DescriptorVisibilities.isPrivate(ProtoEnumFlags.descriptorVisibility(visibility))
}
private fun MessageLite.name(nameResolver: NameResolver): String {
return when (this) {
@@ -69,6 +69,7 @@ private fun MessageLite.name(nameResolver: NameResolver): String {
is ProtoBuf.Function -> nameResolver.getString(name)
is ProtoBuf.Property -> nameResolver.getString(name)
is ProtoBuf.TypeAlias -> nameResolver.getString(name)
is ProtoBuf.EnumEntry -> nameResolver.getString(name)
else -> error("Unknown message: $this")
}
}
@@ -307,6 +308,25 @@ class DifferenceCalculatorForClass(
return Difference(isClassAffected, areSubclassesAffected, names, changedSupertypes)
}
companion object {
fun ClassProtoData.getNonPrivateMembers(): List<String> {
val membersResolvers: List<(ProtoBuf.Class) -> List<MessageLite>> = listOf(
// This list must match the logic in `DifferenceCalculatorForClass.difference`
// TODO: Consider adding COMPANION_OBJECT_NAME and NESTED_CLASS_NAME_LIST as they are also members of a class (see
// `DifferenceCalculatorForClass.difference`)
ProtoBuf.Class::getConstructorList,
ProtoBuf.Class::getFunctionList,
ProtoBuf.Class::getPropertyList,
ProtoBuf.Class::getTypeAliasList,
ProtoBuf.Class::getEnumEntryList
)
return membersResolvers.flatMap { membersResolver ->
membersResolver(proto).filterNot { it.isPrivate }.names(nameResolver)
}
}
}
}
class DifferenceCalculatorForPackageFacade(
@@ -362,6 +382,21 @@ class DifferenceCalculatorForPackageFacade(
return Difference(changedMembersNames = names)
}
companion object {
fun PackagePartProtoData.getNonPrivateMembers(): List<String> {
val membersResolvers: List<(ProtoBuf.Package) -> List<MessageLite>> = listOf(
// This list must match the logic in `DifferenceCalculatorForPackageFacade.difference`
ProtoBuf.Package::getFunctionList,
ProtoBuf.Package::getPropertyList,
ProtoBuf.Package::getTypeAliasList
)
return membersResolvers.flatMap { membersResolver ->
membersResolver(proto).filterNot { it.isPrivate }.names(nameResolver)
}
}
}
}
private val ProtoBuf.Class.isSealed: Boolean