[ABI Validation] For synthetic $default methods, lookup the annotations on their corresponding non-synthetic method

martinbonnin · Space Team · commit 1bc4bc928ca3 · 2025-01-03T19:05:18.000Z
Fixes Kotlin/binary-compatibility-validator#58 Pull request Kotlin/binary-compatibility-validator#85 Moved from Kotlin/binary-compatibility-validator@66031d9
diff --git a/libraries/tools/abi-validation/src/main/kotlin/api/AsmMetadataLoading.kt b/libraries/tools/abi-validation/src/main/kotlin/api/AsmMetadataLoading.kt
@@ -47,9 +47,7 @@ val ClassNode.outerClassName: String? get() = innerClassNode?.outerName
 
 const val publishedApiAnnotationName = "kotlin/PublishedApi"
 fun ClassNode.isPublishedApi() = findAnnotation(publishedApiAnnotationName, includeInvisible = true) != null
-fun MethodNode.isPublishedApi() = findAnnotation(publishedApiAnnotationName, includeInvisible = true) != null
-fun FieldNode.isPublishedApi() = findAnnotation(publishedApiAnnotationName, includeInvisible = true) != null
-
+fun List<AnnotationNode>.isPublishedApi() = firstOrNull { it.refersToName(publishedApiAnnotationName) } != null
 
 fun ClassNode.isDefaultImpls(metadata: KotlinClassMetadata?) = isInner() && name.endsWith("\$DefaultImpls") && metadata.isSyntheticClass()
 
diff --git a/libraries/tools/abi-validation/src/main/kotlin/api/KotlinMetadataSignature.kt b/libraries/tools/abi-validation/src/main/kotlin/api/KotlinMetadataSignature.kt
@@ -7,6 +7,7 @@ package kotlinx.validation.api
 
 import kotlinx.metadata.jvm.*
 import kotlinx.validation.*
+import org.objectweb.asm.Opcodes
 import org.objectweb.asm.tree.*
 
 @ExternalApi // Only name is part of the API, nothing else is used by stdlib
@@ -48,7 +49,8 @@ data class MethodBinarySignature(
     override val jvmMember: JvmMethodSignature,
     override val isPublishedApi: Boolean,
     override val access: AccessFlags,
-    override val annotations: List<AnnotationNode>
+    override val annotations: List<AnnotationNode>,
+    private val alternateDefaultSignature: JvmMethodSignature?
 ) : MemberBinarySignature {
     override val signature: String
         get() = "${access.getModifierString()} fun $name $desc"
@@ -59,60 +61,51 @@ data class MethodBinarySignature(
                 && !isDummyDefaultConstructor()
 
     override fun findMemberVisibility(classVisibility: ClassVisibility?): MemberVisibility? {
-        return super.findMemberVisibility(classVisibility) ?: classVisibility?.let { alternateDefaultSignature(it.name)?.let(it::findMember) }
+        return super.findMemberVisibility(classVisibility)
+            ?: classVisibility?.let { alternateDefaultSignature?.let(it::findMember) }
     }
 
-    /**
-     * Checks whether the method is a $default counterpart of internal @PublishedApi method
-     */
-    public fun isPublishedApiWithDefaultArguments(
-        classVisibility: ClassVisibility?,
-        publishedApiSignatures: Set<JvmMethodSignature>
-    ): Boolean {
-        // Fast-path
-        findMemberVisibility(classVisibility)?.isInternal() ?: return false
-        val name = jvmMember.name
-        if (!name.endsWith("\$default")) return false
-        // Leverage the knowledge about modified signature
-        val expectedPublishedApiCounterPart = JvmMethodSignature(
-            name.removeSuffix("\$default"),
-            jvmMember.desc.replace( ";ILjava/lang/Object;)", ";)"))
-        return expectedPublishedApiCounterPart in publishedApiSignatures
-    }
+    private fun isAccessOrAnnotationsMethod() =
+        access.isSynthetic && (name.startsWith("access\$") || name.endsWith("\$annotations"))
 
-    private fun isAccessOrAnnotationsMethod() = access.isSynthetic && (name.startsWith("access\$") || name.endsWith("\$annotations"))
-
-    private fun isDummyDefaultConstructor() = access.isSynthetic && name == "<init>" && desc == "(Lkotlin/jvm/internal/DefaultConstructorMarker;)V"
-
-    /**
-     * Calculates the signature of this method without default parameters
-     *
-     * Returns `null` if this method isn't an entry point of a function
-     * or a constructor with default parameters.
-     * Returns an incorrect result, if there are more than 31 default parameters.
-     */
-    private fun alternateDefaultSignature(className: String): JvmMethodSignature? {
-        return when {
-            !access.isSynthetic -> null
-            name == "<init>" && "ILkotlin/jvm/internal/DefaultConstructorMarker;" in desc ->
-                JvmMethodSignature(name, desc.replace("ILkotlin/jvm/internal/DefaultConstructorMarker;", ""))
-            name.endsWith("\$default") && "ILjava/lang/Object;)" in desc ->
-                JvmMethodSignature(
-                    name.removeSuffix("\$default"),
-                    desc.replace("ILjava/lang/Object;)", ")").replace("(L$className;", "(")
-                )
-            else -> null
-        }
+    private fun isDummyDefaultConstructor() =
+        access.isSynthetic && name == "<init>" && desc == "(Lkotlin/jvm/internal/DefaultConstructorMarker;)V"
+}
+
+/**
+ * Calculates the signature of this method without default parameters
+ *
+ * Returns `null` if this method isn't an entry point of a function
+ * or a constructor with default parameters.
+ * Returns an incorrect result, if there are more than 31 default parameters.
+ */
+internal fun MethodNode.alternateDefaultSignature(className: String): JvmMethodSignature? {
+    return when {
+        access and Opcodes.ACC_SYNTHETIC == 0 -> null
+        name == "<init>" && "ILkotlin/jvm/internal/DefaultConstructorMarker;" in desc ->
+            JvmMethodSignature(name, desc.replace("ILkotlin/jvm/internal/DefaultConstructorMarker;", ""))
+        name.endsWith("\$default") && "ILjava/lang/Object;)" in desc ->
+            JvmMethodSignature(
+                name.removeSuffix("\$default"),
+                desc.replace("ILjava/lang/Object;)", ")").replace("(L$className;", "(")
+            )
+        else -> null
     }
 }
 
-fun MethodNode.toMethodBinarySignature(propertyAnnotations: List<AnnotationNode>) =
-    MethodBinarySignature(
+fun MethodNode.toMethodBinarySignature(
+    extraAnnotations: List<AnnotationNode>,
+    alternateDefaultSignature: JvmMethodSignature?
+): MethodBinarySignature {
+    val allAnnotations = visibleAnnotations.orEmpty() + invisibleAnnotations.orEmpty() + extraAnnotations
+    return MethodBinarySignature(
         JvmMethodSignature(name, desc),
-        isPublishedApi(),
+        allAnnotations.isPublishedApi(),
         AccessFlags(access),
-        visibleAnnotations.orEmpty() + invisibleAnnotations.orEmpty() + propertyAnnotations
+        allAnnotations,
+        alternateDefaultSignature
     )
+}
 
 data class FieldBinarySignature(
     override val jvmMember: JvmFieldSignature,
@@ -129,13 +122,15 @@ data class FieldBinarySignature(
     }
 }
 
-fun FieldNode.toFieldBinarySignature() =
-    FieldBinarySignature(
+fun FieldNode.toFieldBinarySignature(extraAnnotations: List<AnnotationNode>): FieldBinarySignature {
+    val allAnnotations = visibleAnnotations.orEmpty() + invisibleAnnotations.orEmpty() + extraAnnotations
+    return FieldBinarySignature(
         JvmFieldSignature(name, desc),
-        isPublishedApi(),
+        allAnnotations.isPublishedApi(),
         AccessFlags(access),
-        annotations(visibleAnnotations, invisibleAnnotations))
-
+        allAnnotations
+    )
+}
 private val MemberBinarySignature.kind: Int
     get() = when (this) {
         is FieldBinarySignature -> 1
@@ -156,7 +151,9 @@ data class AccessFlags(val access: Int) {
     val isFinal: Boolean get() = isFinal(access)
     val isSynthetic: Boolean get() = isSynthetic(access)
 
-    fun getModifiers(): List<String> = ACCESS_NAMES.entries.mapNotNull { if (access and it.key != 0) it.value else null }
+    fun getModifiers(): List<String> =
+        ACCESS_NAMES.entries.mapNotNull { if (access and it.key != 0) it.value else null }
+
     fun getModifierString(): String = getModifiers().joinToString(" ")
 }
 
diff --git a/libraries/tools/abi-validation/src/main/kotlin/api/KotlinMetadataVisibilities.kt b/libraries/tools/abi-validation/src/main/kotlin/api/KotlinMetadataVisibilities.kt
@@ -87,7 +87,7 @@ fun KotlinClassMetadata?.isSyntheticClass() = this is KotlinClassMetadata.Synthe
 
 // Auxiliary class that stores signatures of corresponding field and method for a property.
 class PropertyAnnotationHolders(
-    val field: JvmMemberSignature?,
+    val field: JvmFieldSignature?,
     val method: JvmMethodSignature?,
 )
 
@@ -143,7 +143,7 @@ fun KotlinClassMetadata.toClassVisibility(classNode: ClassNode): ClassVisibility
                 property.getterSignature == null && property.setterSignature == null -> property.flags // JvmField or const case
                 else -> flagsOf(Flag.IS_PRIVATE)
             }
-            addMember(property.fieldSignature, fieldVisibility, isReified = false)
+            addMember(property.fieldSignature, fieldVisibility, isReified = false, propertyAnnotation = propertyAnnotations)
         }
     }
 
diff --git a/libraries/tools/abi-validation/src/main/kotlin/api/KotlinSignaturesLoading.kt b/libraries/tools/abi-validation/src/main/kotlin/api/KotlinSignaturesLoading.kt
@@ -43,8 +43,12 @@ public fun Sequence<InputStream>.loadApiFromJvmClasses(visibilityFilter: (String
                 val supertypes = listOf(superName) - "java/lang/Object" + interfaces.sorted()
 
                 val fieldSignatures = fields
-                    .map { it.toFieldBinarySignature() }
-                    .filter {
+                    .map {
+                        val annotationHolders =
+                            mVisibility?.members?.get(JvmFieldSignature(it.name, it.desc))?.propertyAnnotation
+                        val foundAnnotations = methods.annotationsFor(annotationHolders?.method)
+                        it.toFieldBinarySignature(foundAnnotations)
+                    }.filter {
                         it.isEffectivelyPublic(classAccess, mVisibility)
                     }.filter {
                         /*
@@ -62,40 +66,29 @@ public fun Sequence<InputStream>.loadApiFromJvmClasses(visibilityFilter: (String
                     }
 
                 // NB: this 'map' is O(methods + properties * methods) which may accidentally be quadratic
-                val allMethods = methods.map {
+                val methodSignatures = methods.map {
                     /**
                      * For getters/setters, pull the annotations from the property
                      * This is either on the field if any or in a '$annotations' synthetic function.
                      */
                     val annotationHolders =
                         mVisibility?.members?.get(JvmMethodSignature(it.name, it.desc))?.propertyAnnotation
                     val foundAnnotations = ArrayList<AnnotationNode>()
-                    // lookup annotations from $annotations()
-                    val syntheticPropertyMethod = annotationHolders?.method
-                    if (syntheticPropertyMethod != null) {
-                        foundAnnotations += methods
-                            .firstOrNull { it.name == syntheticPropertyMethod.name && it.desc == syntheticPropertyMethod.desc }
-                            ?.visibleAnnotations ?: emptyList()
-                    }
+                    foundAnnotations += fields.annotationsFor(annotationHolders?.field)
+                    foundAnnotations += methods.annotationsFor(annotationHolders?.method)
 
-                    val backingField = annotationHolders?.field
-                    if (backingField != null) {
-                        foundAnnotations += fields
-                            .firstOrNull { it.name == backingField.name && it.desc == backingField.desc }
-                            ?.visibleAnnotations ?: emptyList()
+                    /**
+                     * For synthetic $default methods, pull the annotations from the corresponding method
+                     */
+                    val alternateDefaultSignature = mVisibility?.name?.let { className ->
+                        it.alternateDefaultSignature(className)
                     }
+                    foundAnnotations += methods.annotationsFor(alternateDefaultSignature)
 
-                    it.toMethodBinarySignature(foundAnnotations)
+                    it.toMethodBinarySignature(foundAnnotations, alternateDefaultSignature)
+                }.filter {
+                    it.isEffectivelyPublic(classAccess, mVisibility)
                 }
-                // Signatures marked with @PublishedApi
-                val publishedApiSignatures = allMethods.filter {
-                    it.isPublishedApi
-                }.map { it.jvmMember }.toSet()
-                val methodSignatures = allMethods
-                    .filter {
-                        it.isEffectivelyPublic(classAccess, mVisibility) ||
-                                it.isPublishedApiWithDefaultArguments(mVisibility, publishedApiSignatures)
-                    }
 
                 ClassBinarySignature(
                     name, superName, outerClassName, supertypes, fieldSignatures + methodSignatures, classAccess,
@@ -107,6 +100,24 @@ public fun Sequence<InputStream>.loadApiFromJvmClasses(visibilityFilter: (String
         }
 }
 
+private fun List<MethodNode>.annotationsFor(methodSignature: JvmMethodSignature?): List<AnnotationNode> {
+    if (methodSignature == null) return emptyList()
+
+    return firstOrNull { it.name == methodSignature.name && it.desc == methodSignature.desc }
+        ?.run {
+            visibleAnnotations.orEmpty() + invisibleAnnotations.orEmpty()
+        } ?: emptyList()
+}
+
+private fun List<FieldNode>.annotationsFor(fieldSignature: JvmFieldSignature?): List<AnnotationNode> {
+    if (fieldSignature == null) return emptyList()
+
+    return firstOrNull { it.name == fieldSignature.name && it.desc == fieldSignature.desc }
+        ?.run {
+            visibleAnnotations.orEmpty() + invisibleAnnotations.orEmpty()
+        } ?: emptyList()
+}
+
 @ExternalApi
 public fun List<ClassBinarySignature>.filterOutAnnotated(targetAnnotations: Set<String>): List<ClassBinarySignature> {
     if (targetAnnotations.isEmpty()) return this
diff --git a/libraries/tools/abi-validation/src/test/kotlin/cases/inline/inline.txt b/libraries/tools/abi-validation/src/test/kotlin/cases/inline/inline.txt
@@ -3,7 +3,10 @@ public final class cases/inline/InlineExposedKt {
 }
 
 public final class cases/inline/InternalClassExposed {
+	public field fieldExposed Ljava/lang/String;
 	public fun <init> ()V
 	public final fun funExposed ()V
+	public final fun getPropertyExposed ()Ljava/lang/String;
+	public final fun setPropertyExposed (Ljava/lang/String;)V
 }
 
diff --git a/libraries/tools/abi-validation/src/test/kotlin/cases/inline/inlineExposed.kt b/libraries/tools/abi-validation/src/test/kotlin/cases/inline/inlineExposed.kt
@@ -11,14 +11,10 @@ internal class InternalClassExposed
     @PublishedApi
     internal fun funExposed() {}
 
-    // TODO: Cover unsupported cases: requires correctly reflecting annotations from properties
-    /*
     @PublishedApi
     internal var propertyExposed: String? = null
 
     @JvmField
     @PublishedApi
     internal var fieldExposed: String? = null
-    */
-
 }
diff --git a/libraries/tools/abi-validation/src/test/kotlin/cases/marker/marker.kt b/libraries/tools/abi-validation/src/test/kotlin/cases/marker/marker.kt
@@ -6,6 +6,8 @@ annotation class HiddenField
 @Target(AnnotationTarget.PROPERTY)
 annotation class HiddenProperty
 
+annotation class HiddenMethod
+
 public class Foo {
     // HiddenField will be on the field
     @HiddenField
@@ -14,5 +16,9 @@ public class Foo {
     // HiddenField will be on a synthetic `$annotations()` method
     @HiddenProperty
     var bar2 = 42
+
+    @HiddenMethod
+    fun hiddenMethod(bar: Int = 42) {
+    }
 }
 
diff --git a/libraries/tools/abi-validation/src/test/kotlin/cases/marker/marker.txt b/libraries/tools/abi-validation/src/test/kotlin/cases/marker/marker.txt
@@ -5,6 +5,9 @@ public final class cases/marker/Foo {
 public abstract interface annotation class cases/marker/HiddenField : java/lang/annotation/Annotation {
 }
 
+public abstract interface annotation class cases/marker/HiddenMethod : java/lang/annotation/Annotation {
+}
+
 public abstract interface annotation class cases/marker/HiddenProperty : java/lang/annotation/Annotation {
 }
 
diff --git a/libraries/tools/abi-validation/src/test/kotlin/tests/CasesPublicAPITest.kt b/libraries/tools/abi-validation/src/test/kotlin/tests/CasesPublicAPITest.kt
@@ -41,7 +41,7 @@ class CasesPublicAPITest {
 
     @Test fun localClasses() { snapshotAPIAndCompare(testName.methodName) }
 
-    @Test fun marker() { snapshotAPIAndCompare(testName.methodName, setOf("cases/marker/HiddenField", "cases/marker/HiddenProperty")) }
+    @Test fun marker() { snapshotAPIAndCompare(testName.methodName, setOf("cases/marker/HiddenField", "cases/marker/HiddenProperty", "cases/marker/HiddenMethod")) }
 
     @Test fun nestedClasses() { snapshotAPIAndCompare(testName.methodName) }
 

Original file line number	Diff line number	Diff line change
`@@ -87,7 +87,7 @@ fun KotlinClassMetadata?.isSyntheticClass() = this is KotlinClassMetadata.Synthe`
`87`	`87`
`88`	`88`	`// Auxiliary class that stores signatures of corresponding field and method for a property.`
`89`	`89`	`class PropertyAnnotationHolders(`
`90`		`- val field: JvmMemberSignature?,`
	`90`	`+ val field: JvmFieldSignature?,`
`91`	`91`	`val method: JvmMethodSignature?,`
`92`	`92`	`)`
`93`	`93`
`@@ -143,7 +143,7 @@ fun KotlinClassMetadata.toClassVisibility(classNode: ClassNode): ClassVisibility`
`143`	`143`	`property.getterSignature == null && property.setterSignature == null -> property.flags // JvmField or const case`
`144`	`144`	`else -> flagsOf(Flag.IS_PRIVATE)`
`145`	`145`	`}`
`146`		`- addMember(property.fieldSignature, fieldVisibility, isReified = false)`
	`146`	`+ addMember(property.fieldSignature, fieldVisibility, isReified = false, propertyAnnotation = propertyAnnotations)`
`147`	`147`	`}`
`148`	`148`	`}`
`149`	`149`
Original file line number	Diff line number	Diff line change
`@@ -3,7 +3,10 @@ public final class cases/inline/InlineExposedKt {`
`3`	`3`	`}`
`4`	`4`
`5`	`5`	`public final class cases/inline/InternalClassExposed {`
	`6`	`+ public field fieldExposed Ljava/lang/String;`
`6`	`7`	`public fun <init> ()V`
`7`	`8`	`public final fun funExposed ()V`
	`9`	`+ public final fun getPropertyExposed ()Ljava/lang/String;`
	`10`	`+ public final fun setPropertyExposed (Ljava/lang/String;)V`
`8`	`11`	`}`
`9`	`12`
Original file line number	Diff line number	Diff line change
`@@ -5,6 +5,9 @@ public final class cases/marker/Foo {`
`5`	`5`	`public abstract interface annotation class cases/marker/HiddenField : java/lang/annotation/Annotation {`
`6`	`6`	`}`
`7`	`7`
	`8`	`+public abstract interface annotation class cases/marker/HiddenMethod : java/lang/annotation/Annotation {`
	`9`	`+}`
	`10`	`+`
`8`	`11`	`public abstract interface annotation class cases/marker/HiddenProperty : java/lang/annotation/Annotation {`
`9`	`12`	`}`
`10`	`13`