KT-44741: Improve handling of constants defined in companion objects

build-common/src/org/jetbrains/kotlin/incremental/AbstractIncrementalCache.kt

@@ -20,7 +20,6 @@ import com.intellij.util.io.EnumeratorStringDescriptor
 import org.jetbrains.kotlin.incremental.storage.*
 import org.jetbrains.kotlin.metadata.ProtoBuf
 import org.jetbrains.kotlin.metadata.deserialization.Flags
-import org.jetbrains.kotlin.metadata.deserialization.NameResolver
 import org.jetbrains.kotlin.metadata.deserialization.TypeTable
 import org.jetbrains.kotlin.metadata.deserialization.supertypes
 import org.jetbrains.kotlin.name.FqName
@@ -125,7 +124,9 @@ abstract class AbstractIncrementalCache<ClassName>(
      *
      * The `srcFile` argument may be `null` (e.g., if we are processing .class files in jars where source files are not available).
      */
-    protected fun addToClassStorage(proto: ProtoBuf.Class, nameResolver: NameResolver, srcFile: File?) {
+    protected fun addToClassStorage(classProtoData: ClassProtoData, srcFile: File?) {
+        val (proto, nameResolver) = classProtoData
+
         val supertypes = proto.supertypes(TypeTable(proto.typeTable))
         val parents = supertypes.map { nameResolver.getClassId(it.className).asSingleFqName() }
             .filter { it.asString() != "kotlin.Any" }

build-common/src/org/jetbrains/kotlin/incremental/IncrementalJsCache.kt

@@ -117,7 +117,7 @@ open class IncrementalJsCache(
                 registerOutputForFile(srcFile, classId.asSingleFqName())
 
                 if (protoData is ClassProtoData) {
-                    addToClassStorage(protoData.proto, protoData.nameResolver, srcFile)
+                    addToClassStorage(protoData, srcFile)
                 }
             }
 

build-common/src/org/jetbrains/kotlin/incremental/IncrementalJvmCache.kt

@@ -30,11 +30,10 @@ 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
 import org.jetbrains.kotlin.metadata.jvm.deserialization.BitEncoding
-import org.jetbrains.kotlin.metadata.jvm.deserialization.JvmProtoBufUtil
 import org.jetbrains.kotlin.metadata.jvm.deserialization.ModuleMapping
 import org.jetbrains.kotlin.name.ClassId
 import org.jetbrains.kotlin.name.FqName
-import org.jetbrains.kotlin.name.SpecialNames.DEFAULT_NAME_FOR_COMPANION_OBJECT
+import org.jetbrains.kotlin.name.Name
 import org.jetbrains.kotlin.resolve.jvm.AsmTypes
 import org.jetbrains.kotlin.resolve.jvm.JvmClassName
 import org.jetbrains.org.objectweb.asm.*
@@ -182,12 +181,7 @@ open class IncrementalJvmCache(
                 inlineFunctionsMap.process(kotlinClassInfo, changesCollector)
             }
             KotlinClassHeader.Kind.CLASS -> {
-                if (sourceFiles != null) {
+                addToClassStorage(kotlinClassInfo.protoData as ClassProtoData, sourceFiles?.let { sourceFiles.single() })
-                    assert(sourceFiles.size == 1) { "Class is expected to have only one source file: $sourceFiles" }
-                    addToClassStorage(kotlinClassInfo, sourceFiles.single())
-                } else {
-                    addToClassStorage(kotlinClassInfo, null)
-                }
 
                 protoMap.process(kotlinClassInfo, changesCollector)
                 constantsMap.process(kotlinClassInfo, changesCollector)
@@ -202,8 +196,7 @@ open class IncrementalJvmCache(
         val jvmClassName = JvmClassName.byClassId(serializedJavaClass.classId)
         javaSourcesProtoMap.process(jvmClassName, serializedJavaClass, collector)
         source?.let { sourceToClassesMap.add(source, jvmClassName) }
-        val (proto, nameResolver) = serializedJavaClass.toProtoData()
+        addToClassStorage(serializedJavaClass.toProtoData(), source)
-        addToClassStorage(proto, nameResolver, source)
 //        collector.addJavaProto(ClassProtoData(proto, nameResolver))
         dirtyOutputClassesMap.notDirty(jvmClassName)
     }
@@ -317,15 +310,11 @@ open class IncrementalJvmCache(
         private fun put(kotlinClassInfo: KotlinClassInfo, changesCollector: ChangesCollector) {
             val key = kotlinClassInfo.className.internalName
             val oldData = storage[key]
-            val newData = ProtoMapValue(
+            val newData = kotlinClassInfo.protoMapValue
-                kotlinClassInfo.classKind != KotlinClassHeader.Kind.CLASS,
-                BitEncoding.decodeBytes(kotlinClassInfo.classHeaderData),
-                kotlinClassInfo.classHeaderStrings
-            )
             storage[key] = newData
 
             val packageFqName = kotlinClassInfo.className.packageFqName
-            changesCollector.collectProtoChanges(oldData?.toProtoData(packageFqName), newData.toProtoData(packageFqName), packageProtoKey = key)
+            changesCollector.collectProtoChanges(oldData?.toProtoData(packageFqName), kotlinClassInfo.protoData, packageProtoKey = key)
         }
 
         operator fun contains(className: JvmClassName): Boolean =
@@ -402,20 +391,29 @@ open class IncrementalJvmCache(
                 storage.remove(key)
             }
 
-            for (const in oldMap.keys + newMap.keys) {
+            val allConstants = oldMap.keys + newMap.keys
-                //Constant can be declared via companion object or via const field declaration
+            if (allConstants.isEmpty()) return
-                changesCollector.collectMemberIfValueWasChanged(
+
-                    kotlinClassInfo.scopeFqName(companion = true),
+            // If a constant is defined in a companion object, it will be found in the constantsMap of the containing class, not the
-                    const,
+            // companion object's class, so we will need to correct its scope.
-                    oldMap[const],
+            // (See https://youtrack.jetbrains.com/issue/KT-44741#focus=Comments-27-5659564.0-0 for more details.)
-                    newMap[const]
+            // 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).
-                changesCollector.collectMemberIfValueWasChanged(
+            val companionObjectClassId = if (kotlinClassInfo.classKind == KotlinClassHeader.Kind.CLASS) {
-                    kotlinClassInfo.scopeFqName(companion = false),
+                val protoData = kotlinClassInfo.protoData as ClassProtoData
-                    const,
+                if (protoData.proto.hasCompanionObjectName()) {
-                    oldMap[const],
+                    val companionObjectName = Name.identifier(protoData.nameResolver.getString(protoData.proto.companionObjectName))
-                    newMap[const]
+                    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).
+            for (const in allConstants) {
+                changesCollector.collectMemberIfValueWasChanged(scope, const, oldMap[const], newMap[const])
             }
         }
 
@@ -503,11 +501,6 @@ open class IncrementalJvmCache(
             value.dumpCollection()
     }
 
-    private fun addToClassStorage(classInfo: KotlinClassInfo, srcFile: File?) {
-        val (nameResolver, proto) = JvmProtoBufUtil.readClassDataFrom(classInfo.classHeaderData, classInfo.classHeaderStrings)
-        addToClassStorage(proto, nameResolver, srcFile)
-    }
-
     private inner class InlineFunctionsMap(storageFile: File) :
         BasicStringMap<LinkedHashMap<String, Long>>(storageFile, LinkedHashMapExternalizer(StringExternalizer, LongExternalizer)) {
 
@@ -545,6 +538,11 @@ open class IncrementalJvmCache(
         override fun dumpValue(value: LinkedHashMap<String, Long>): String =
             value.dumpMap { java.lang.Long.toHexString(it) }
     }
+
+    private fun KotlinClassInfo.scopeFqName() = when (classKind) {
+        KotlinClassHeader.Kind.CLASS -> classId.asSingleFqName()
+        else -> classId.packageFqName
+    }
 }
 
 private object PathCollectionExternalizer :
@@ -613,18 +611,32 @@ class KotlinClassInfo constructor(
     val classKind: KotlinClassHeader.Kind,
     val classHeaderData: Array<String>, // Can be empty
     val classHeaderStrings: Array<String>, // Can be empty
-    @Suppress("SpellCheckingInspection") val multifileClassName: String?,
+    val multifileClassName: String?,
     val constantsMap: LinkedHashMap<String, Any>,
     val inlineFunctionsMap: LinkedHashMap<String, Long>
 ) {
 
     val className: JvmClassName by lazy { JvmClassName.byClassId(classId) }
 
-    fun scopeFqName(companion: Boolean = false) = when (classKind) {
+    val protoMapValue: ProtoMapValue by lazy {
-        KotlinClassHeader.Kind.CLASS -> {
+        ProtoMapValue(
-            className.fqNameForClassNameWithoutDollars.let { if (companion) it.child(DEFAULT_NAME_FOR_COMPANION_OBJECT) else it }
+            isPackageFacade = classKind != KotlinClassHeader.Kind.CLASS,
+            BitEncoding.decodeBytes(classHeaderData),
+            classHeaderStrings
+        )
+    }
+
+    /**
+     * Returns 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.
+     */
+    val protoData: ProtoData by lazy {
+        check(classKind != KotlinClassHeader.Kind.MULTIFILE_CLASS) {
+            "Proto data is not available for KotlinClassHeader.Kind.MULTIFILE_CLASS: $classId"
         }
-        else -> className.packageFqName
+        protoMapValue.toProtoData(className.packageFqName)
     }
 
     companion object {

compiler/incremental-compilation-impl/test/org/jetbrains/kotlin/incremental/classpathDiff/ClasspathChangesComputerTest.kt

@@ -171,27 +171,17 @@ class KotlinOnlyClasspathChangesComputerTest : ClasspathChangesComputerTest() {
         Changes(
             lookupSymbols = setOf(
                 LookupSymbol(name = "constantChangedType", scope = "com.example.SomeClass.CompanionObject"),
-                // TODO (Fix in next commit). Missing:
+                LookupSymbol(name = "constantChangedValue", scope = "com.example.SomeClass.CompanionObject"),
-                // LookupSymbol(name = "constantChangedValue", scope = "com.example.SomeClass.CompanionObject")
 
                 LookupSymbol(name = "inlineFunctionChangedSignature", scope = "com.example.SomeClass"),
                 LookupSymbol(name = "inlineFunctionChangedImplementation", scope = "com.example.SomeClass"),
 
                 LookupSymbol(name = SAM_LOOKUP_NAME.asString(), scope = "com.example.SomeClass"),
                 LookupSymbol(name = SAM_LOOKUP_NAME.asString(), scope = "com.example.SomeClass.CompanionObject"),
-
-                // TODO (Fix in next commit). Incorrect:
-                LookupSymbol(name = "constantChangedType", scope = "com.example.SomeClass"),
-                LookupSymbol(name = "constantChangedType", scope = "com.example.SomeClass.Companion"),
-                LookupSymbol(name = "constantChangedValue", scope = "com.example.SomeClass"),
-                LookupSymbol(name = "constantChangedValue", scope = "com.example.SomeClass.Companion"),
-                LookupSymbol(name = SAM_LOOKUP_NAME.asString(), scope = "com.example.SomeClass.Companion"),
             ),
             fqNames = setOf(
                 "com.example.SomeClass",
                 "com.example.SomeClass.CompanionObject",
-                // TODO (Fix in next commit). Incorrect:
-                "com.example.SomeClass.Companion"
             )
         ).assertEquals(changes)
     }

compiler/incremental-compilation-impl/testData/org/jetbrains/kotlin/incremental/classpathDiff/ClasspathSnapshotterTest/expected-snapshot/kotlin/com/example/SimpleClass.json

@@ -34,6 +34,14 @@
     "className$delegate": {
       "initializer": {},
       "_value": {}
+    },
+    "protoMapValue$delegate": {
+      "initializer": {},
+      "_value": {}
+    },
+    "protoData$delegate": {
+      "initializer": {},
+      "_value": {}
     }
   },
   "supertypes": [