From df90c773acd97261a62050352cbabf4a81d141d1 Mon Sep 17 00:00:00 2001 From: Martin Bonnin Date: Thu, 27 Jun 2024 23:50:41 +0200 Subject: [PATCH 1/4] Fix for #90 --- .../validation/test/NonPublicMarkersTest.kt | 26 +++++++++++++ .../examples/classes/ConstProperty.dump | 11 ++++++ .../examples/classes/ConstProperty.kt | 15 ++++++++ .../kotlin/api/KotlinMetadataSignature.kt | 8 +++- .../kotlin/api/KotlinSignaturesLoading.kt | 37 +++++++++++++++++-- 5 files changed, 92 insertions(+), 5 deletions(-) create mode 100644 src/functionalTest/resources/examples/classes/ConstProperty.dump create mode 100644 src/functionalTest/resources/examples/classes/ConstProperty.kt diff --git a/src/functionalTest/kotlin/kotlinx/validation/test/NonPublicMarkersTest.kt b/src/functionalTest/kotlin/kotlinx/validation/test/NonPublicMarkersTest.kt index 4bb804b7..3539c0a3 100644 --- a/src/functionalTest/kotlin/kotlinx/validation/test/NonPublicMarkersTest.kt +++ b/src/functionalTest/kotlin/kotlinx/validation/test/NonPublicMarkersTest.kt @@ -104,4 +104,30 @@ class NonPublicMarkersTest : BaseKotlinGradleTest() { Assertions.assertThat(rootProjectApiDump.readText()).isEqualToIgnoringNewLines(expected) } } + + @Test + fun testIgnoredMarkersOnConstProperties() { + val runner = test { + buildGradleKts { + resolve("/examples/gradle/base/withPlugin.gradle.kts") + resolve("/examples/gradle/configuration/nonPublicMarkers/markers.gradle.kts") + } + + kotlin("ConstProperty.kt") { + resolve("/examples/classes/ConstProperty.kt") + } + + apiFile(projectName = rootProjectDir.name) { + resolve("/examples/classes/ConstProperty.dump") + } + + runner { + arguments.add(":apiCheck") + } + } + + runner.withDebug(true).build().apply { + assertTaskSuccess(":apiCheck") + } + } } diff --git a/src/functionalTest/resources/examples/classes/ConstProperty.dump b/src/functionalTest/resources/examples/classes/ConstProperty.dump new file mode 100644 index 00000000..7030d368 --- /dev/null +++ b/src/functionalTest/resources/examples/classes/ConstProperty.dump @@ -0,0 +1,11 @@ +public final class foo/Foo { + public static final field Companion Lfoo/Foo$Companion; + public fun ()V +} + +public final class foo/Foo$Companion { +} + +public abstract interface annotation class foo/HiddenProperty : java/lang/annotation/Annotation { +} + diff --git a/src/functionalTest/resources/examples/classes/ConstProperty.kt b/src/functionalTest/resources/examples/classes/ConstProperty.kt new file mode 100644 index 00000000..c8656eb8 --- /dev/null +++ b/src/functionalTest/resources/examples/classes/ConstProperty.kt @@ -0,0 +1,15 @@ +/* + * Copyright 2016-2022 JetBrains s.r.o. + * Use of this source code is governed by the Apache 2.0 License that can be found in the LICENSE.txt file. + */ +package foo + +@Target(AnnotationTarget.PROPERTY) +annotation class HiddenProperty + +public class Foo { + companion object { + @HiddenProperty + const val bar = "barValue" + } +} \ No newline at end of file diff --git a/src/main/kotlin/api/KotlinMetadataSignature.kt b/src/main/kotlin/api/KotlinMetadataSignature.kt index 93f571b9..4fb6d426 100644 --- a/src/main/kotlin/api/KotlinMetadataSignature.kt +++ b/src/main/kotlin/api/KotlinMetadataSignature.kt @@ -175,7 +175,11 @@ internal fun FieldNode.isCompanionField(outerClassMetadata: KotlinClassMetadata? return metadata.toKmClass().companionObject == name } -internal fun ClassNode.companionName(outerClassMetadata: KotlinClassMetadata?): String { - val outerKClass = (outerClassMetadata as KotlinClassMetadata.Class).toKmClass() +internal fun ClassNode.companionName(outerClassMetadata: KotlinClassMetadata?): String? { + if (outerClassMetadata !is KotlinClassMetadata.Class) { + // Happens when outerClassMetadata == KotlinClassMetadata$FileFacade for an example + return null + } + val outerKClass = outerClassMetadata.toKmClass() return name + "$" + outerKClass.companionObject } diff --git a/src/main/kotlin/api/KotlinSignaturesLoading.kt b/src/main/kotlin/api/KotlinSignaturesLoading.kt index 801926fb..6224173a 100644 --- a/src/main/kotlin/api/KotlinSignaturesLoading.kt +++ b/src/main/kotlin/api/KotlinSignaturesLoading.kt @@ -5,6 +5,9 @@ package kotlinx.validation.api +import kotlinx.metadata.Flag +import kotlinx.metadata.KmProperty +import kotlinx.metadata.internal.metadata.jvm.deserialization.JvmFlags import kotlinx.metadata.jvm.* import kotlinx.validation.* import org.objectweb.asm.* @@ -119,10 +122,26 @@ private fun FieldNode.buildFieldSignature( * as non-public API using some annotation, then we won't be able to filter out * the companion field. */ - val companionName = ownerClass.companionName(ownerClass.kotlinMetadata) + val companionName = ownerClass.companionName(ownerClass.kotlinMetadata)!! companionClass = classes[companionName] foundAnnotations.addAll(companionClass?.visibleAnnotations.orEmpty()) foundAnnotations.addAll(companionClass?.invisibleAnnotations.orEmpty()) + } else if (access.and(Opcodes.ACC_STATIC) != 0 && access.and(Opcodes.ACC_FINAL) != 0) { + companionClass = ownerClass.companionName(ownerClass.kotlinMetadata)?.let { + classes[it] + } + + val property = companionClass?.kmProperty(name) + + if (property != null && JvmFlag.Property.IS_MOVED_FROM_INTERFACE_COMPANION(property.flags)) { + /* + * The property was moved from the companion object. Take all the annotations from there + * to be able to filter out the non-public markers. + * + * See https://github.com/Kotlin/binary-compatibility-validator/issues/90 + */ + foundAnnotations.addAll(companionClass!!.methods.annotationsFor(property.syntheticMethodForAnnotations)) + } } val fieldSignature = toFieldBinarySignature(foundAnnotations) @@ -133,6 +152,18 @@ private fun FieldNode.buildFieldSignature( } } +private fun ClassNode.kmProperty(name: String?): KmProperty? { + val metadata = kotlinMetadata ?: return null + + if (metadata !is KotlinClassMetadata.Class) { + return null + } + + return metadata.toKmClass().properties.firstOrNull { + it.name == name + } +} + private fun MethodNode.buildMethodSignature( ownerVisibility: ClassVisibility?, ownerClass: ClassNode @@ -236,8 +267,8 @@ public fun List.extractAnnotatedPackages(targetAnnotations // package-info classes are private synthetic abstract interfaces since 2005 (JDK-6232928). it.access.isInterface && it.access.isSynthetic && it.access.isAbstract }.filter { - it.annotations.any { - ann -> targetAnnotations.any { ann.refersToName(it) } + it.annotations.any { ann -> + targetAnnotations.any { ann.refersToName(it) } } }.map { val res = it.name.substring(0, it.name.length - "/package-info".length) From 5f1d3847c31841aee4054e8219f88a25b57fa080 Mon Sep 17 00:00:00 2001 From: Martin Bonnin Date: Fri, 28 Jun 2024 10:45:30 +0200 Subject: [PATCH 2/4] Use isStatic and isFinal --- src/main/kotlin/api/KotlinSignaturesLoading.kt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main/kotlin/api/KotlinSignaturesLoading.kt b/src/main/kotlin/api/KotlinSignaturesLoading.kt index 6224173a..7a259bc6 100644 --- a/src/main/kotlin/api/KotlinSignaturesLoading.kt +++ b/src/main/kotlin/api/KotlinSignaturesLoading.kt @@ -126,7 +126,7 @@ private fun FieldNode.buildFieldSignature( companionClass = classes[companionName] foundAnnotations.addAll(companionClass?.visibleAnnotations.orEmpty()) foundAnnotations.addAll(companionClass?.invisibleAnnotations.orEmpty()) - } else if (access.and(Opcodes.ACC_STATIC) != 0 && access.and(Opcodes.ACC_FINAL) != 0) { + } else if (isStatic(access) && isFinal(access)) { companionClass = ownerClass.companionName(ownerClass.kotlinMetadata)?.let { classes[it] } @@ -137,7 +137,7 @@ private fun FieldNode.buildFieldSignature( /* * The property was moved from the companion object. Take all the annotations from there * to be able to filter out the non-public markers. - * + * * See https://github.com/Kotlin/binary-compatibility-validator/issues/90 */ foundAnnotations.addAll(companionClass!!.methods.annotationsFor(property.syntheticMethodForAnnotations)) From ff78c5bb619272eb6742153c59919f139386af75 Mon Sep 17 00:00:00 2001 From: Martin Bonnin Date: Fri, 28 Jun 2024 10:54:43 +0200 Subject: [PATCH 3/4] update test case --- .../kotlin/cases/companions/companions.kt | 22 +++++++++++++++++++ .../kotlin/cases/companions/companions.txt | 17 ++++++++++++++ 2 files changed, 39 insertions(+) diff --git a/src/test/kotlin/cases/companions/companions.kt b/src/test/kotlin/cases/companions/companions.kt index a5eea596..a8827c48 100644 --- a/src/test/kotlin/cases/companions/companions.kt +++ b/src/test/kotlin/cases/companions/companions.kt @@ -157,3 +157,25 @@ class FilteredNamedCompanionObjectHolder private constructor() { val F: Int = 42 } } + +class FilteredCompanionProperties private constructor() { + companion object { + public val F1: Int = 1 + public const val F2: Int = 2 + private val F3: Int = 3 + private const val F4: Int = 4 + @PrivateApi + val F5: Int = 5 + @PrivateApi + const val F6: Int = 6 + } +} + +class FilteredCompanionFunctions private constructor() { + companion object { + public fun f1(): Int = 1 + private fun f2(): Int = 2 + @PrivateApi + public fun f3(): Int = 3 + } +} diff --git a/src/test/kotlin/cases/companions/companions.txt b/src/test/kotlin/cases/companions/companions.txt index 9138da81..1dde5edc 100644 --- a/src/test/kotlin/cases/companions/companions.txt +++ b/src/test/kotlin/cases/companions/companions.txt @@ -1,6 +1,23 @@ +public final class cases/companions/FilteredCompanionFunctions { + public static final field Companion Lcases/companions/FilteredCompanionFunctions$Companion; +} + +public final class cases/companions/FilteredCompanionFunctions$Companion { + public final fun f1 ()I +} + public final class cases/companions/FilteredCompanionObjectHolder { } +public final class cases/companions/FilteredCompanionProperties { + public static final field Companion Lcases/companions/FilteredCompanionProperties$Companion; + public static final field F2 I +} + +public final class cases/companions/FilteredCompanionProperties$Companion { + public final fun getF1 ()I +} + public final class cases/companions/FilteredNamedCompanionObjectHolder { } From ab771fd73f0a8797395158dc569c028aa55a0814 Mon Sep 17 00:00:00 2001 From: Martin Bonnin Date: Fri, 28 Jun 2024 11:08:03 +0200 Subject: [PATCH 4/4] Add multiple const val properties --- src/test/kotlin/cases/companions/companions.kt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/test/kotlin/cases/companions/companions.kt b/src/test/kotlin/cases/companions/companions.kt index a8827c48..6590923a 100644 --- a/src/test/kotlin/cases/companions/companions.kt +++ b/src/test/kotlin/cases/companions/companions.kt @@ -168,6 +168,8 @@ class FilteredCompanionProperties private constructor() { val F5: Int = 5 @PrivateApi const val F6: Int = 6 + @PrivateApi + const val F7: Int = 7 } }