Skip to content

Commit

Permalink
IC: Fix regression in detecting constants in KotlinClassInfo.kt
Browse files Browse the repository at this point in the history
A constant is a static final field with non-null value. In a previous
commit (0b09be7), we accidentally removed the *non-null value*
filter when looking for constants in the bytecode.

This commit re-adds that filter to make sure the detection is correct.

Test: Added KotlinOnlyClasspathChangesComputerTest.testDelegatedProperties

^KT-58986: Fixed

(cherry picked from commit 0783561)
  • Loading branch information
hungvietnguyen authored and Space Team committed Jun 18, 2023
1 parent 5964edc commit de918fc
Show file tree
Hide file tree
Showing 7 changed files with 47 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ private fun getExtraInfo(classHeader: KotlinClassHeader, classContents: ByteArra

val constantSnapshots: Map<String, Long> = classNode.fields.associate { fieldNode ->
// Note: `fieldNode` is a constant because we kept only fields that are (non-private) constants in `classNode`
fieldNode.name to (fieldNode.value?.let { ConstantValueExternalizer.toByteArray(it).hashToLong() } ?: 0L)
fieldNode.name to ConstantValueExternalizer.toByteArray(fieldNode.value!!).hashToLong()
}

val inlineFunctionOrAccessorSnapshots: Map<InlineFunctionOrAccessor, Long> = classNode.methods.associate { methodNode ->
Expand Down Expand Up @@ -210,7 +210,11 @@ class SelectiveClassVisitor(
) : ClassVisitor(Opcodes.API_VERSION, cv) {

override fun visitField(access: Int, name: String, desc: String, signature: String?, value: Any?): FieldVisitor? {
return if (shouldVisitField(JvmMemberSignature.Field(name, desc), access.isPrivate(), access.isStaticFinal())) {
// Note: A constant's value must be not-null. A static final field with a `null` value at the bytecode declaration is not a constant
// (whether the value is initialized later in the static initializer or not, it won't be inlined by the compiler).
val isConstant = access.isStaticFinal() && value != null

return if (shouldVisitField(JvmMemberSignature.Field(name, desc), access.isPrivate(), isConstant)) {
cv.visitField(access, name, desc, signature, value)
} else null
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,19 @@ class KotlinOnlyClasspathChangesComputerTest : ClasspathChangesComputerTest() {
).assertEquals(changes)
}

/** Regression test for KT-58986.*/
@Test
fun testDelegatedProperties() {
// Check that classpath changes computation doesn't fail
val changes = computeClasspathChanges(File(testDataDir, "KotlinOnly/testDelegatedProperties/src"), tmpDir)
Changes(
lookupSymbols = setOf(
LookupSymbol(name = "delegatedProperty", scope = "com.example"),
),
fqNames = setOf("com.example")
).assertEquals(changes)
}

/** Tests [SupertypesInheritorsImpact]. */
@Test
override fun testImpactComputation_SupertypesInheritors() {
Expand Down
Binary file not shown.
Binary file not shown.
Binary file not shown.
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
package com.example

import kotlin.reflect.KProperty

var delegatedProperty: String by Delegate() // Added

class Delegate {

operator fun getValue(thisRef: Any?, property: KProperty<*>): String {
return "<Delegated>"
}

operator fun setValue(thisRef: Any?, property: KProperty<*>, value: String) {
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
package com.example

import kotlin.reflect.KProperty

class Delegate {

operator fun getValue(thisRef: Any?, property: KProperty<*>): String {
return "<Delegated>"
}

operator fun setValue(thisRef: Any?, property: KProperty<*>, value: String) {
}
}

0 comments on commit de918fc

Please sign in to comment.