Evaluate 'static final Companion' visibility in the outer class to th… (#59)

* Evaluate 'static final Companion' visibility in the outer class to the visibility of the companion class itself

Fixes #53

Author: qwwdfsad

src/main/kotlin/api/KotlinMetadataSignature.kt

@@ -159,3 +159,11 @@ data class AccessFlags(val access: Int) {
     fun getModifierString(): String = getModifiers().joinToString(" ")
 }
 
+fun FieldBinarySignature.isCompanionField(outerClassMetadata: KotlinClassMetadata?): Boolean {
+    if (!access.isFinal || !access.isStatic) return false
+    val metadata = outerClassMetadata ?: return false
+    // Non-classes are not affected by the problem
+    if (metadata !is KotlinClassMetadata.Class) return false
+    return metadata.toKmClass().companionObject == name
+}
+

src/main/kotlin/api/KotlinMetadataVisibilities.kt

@@ -121,16 +121,33 @@ fun KotlinClassMetadata.toClassVisibility(classNode: ClassNode): ClassVisibility
 
 fun ClassNode.toClassVisibility() = kotlinMetadata?.toClassVisibility(this)
 
-fun Sequence<ClassNode>.readKotlinVisibilities(): Map<String, ClassVisibility> =
-    mapNotNull { it.toClassVisibility() }
-        .associateBy { it.name }
-        .apply {
-            values.asSequence().filter { it.isCompanion }.forEach {
-                val containingClassName = it.name.substringBeforeLast('$')
-                getValue(containingClassName).companionVisibilities = it
-            }
-
-            values.asSequence().filter { it.facadeClassName != null }.forEach {
-                getValue(it.facadeClassName!!).partVisibilities.add(it)
+fun Map<String, ClassNode>.readKotlinVisibilities(visibilityFilter: (String) -> Boolean = { true }): Map<String, ClassVisibility> =
+    /*
+     * Optimized sequence of:
+     * 1) Map values to visibility
+     * 2) Filter keys by visibilityFilter
+     * 3) Post-process each value and set facade/companion
+     */
+    entries
+        .mapNotNull { (name, classNode) ->
+            if (!visibilityFilter(name)) return@mapNotNull null
+            val visibility = classNode.toClassVisibility() ?: return@mapNotNull null
+            name to visibility
+        }.toMap().apply {
+            values.forEach {
+                updateCompanionInfo(it)
+                updateFacadeInfo(it)
             }
         }
+
+private fun Map<String, ClassVisibility>.updateFacadeInfo(it: ClassVisibility) {
+    if (it.facadeClassName == null) return
+    getValue(it.facadeClassName).partVisibilities.add(it)
+}
+
+private fun Map<String, ClassVisibility>.updateCompanionInfo(it: ClassVisibility) {
+    if (!it.isCompanion) return
+    val containingClassName = it.name.substringBeforeLast('$')
+    getValue(containingClassName).companionVisibilities = it
+}
+

src/main/kotlin/api/KotlinSignaturesLoading.kt

@@ -5,10 +5,12 @@
 
 package kotlinx.validation.api
 
+import kotlinx.metadata.jvm.KotlinClassMetadata
 import kotlinx.validation.*
 import org.objectweb.asm.*
 import org.objectweb.asm.tree.*
 import java.io.*
+import java.util.*
 import java.util.jar.*
 
 @ExternalApi
@@ -26,19 +28,35 @@ public fun Sequence<InputStream>.loadApiFromJvmClasses(visibilityFilter: (String
         }
     }
 
-    val visibilityMapNew = classNodes.readKotlinVisibilities().filterKeys(visibilityFilter)
-    return classNodes
+    // Note: map is sorted, so the dump will produce stable result
+    val classNodeMap = classNodes.associateByTo(TreeMap()) { it.name }
+    val visibilityMap = classNodeMap.readKotlinVisibilities(visibilityFilter)
+    return classNodeMap
+        .values
         .map { classNode ->
             with(classNode) {
                 val metadata = kotlinMetadata
-                val mVisibility = visibilityMapNew[name]
+                val mVisibility = visibilityMap[name]
                 val classAccess = AccessFlags(effectiveAccess and Opcodes.ACC_STATIC.inv())
                 val supertypes = listOf(superName) - "java/lang/Object" + interfaces.sorted()
 
                 val fieldSignatures = fields
                     .map { it.toFieldBinarySignature() }
                     .filter {
                         it.isEffectivelyPublic(classAccess, mVisibility)
+                    }.filter {
+                        /*
+                         * Filter out 'public static final Companion' field that doesn't constitutes public API.
+                         * For that we first check if field corresponds to the 'Companion' class and then
+                         * if companion is effectively public by itself, so the 'Companion' field has the same visibility.
+                         */
+                        if (!it.isCompanionField(classNode.kotlinMetadata)) return@filter true
+                        val outerKClass = (classNode.kotlinMetadata as KotlinClassMetadata.Class).toKmClass()
+                        val companionName = name + "$" + outerKClass.companionObject
+                        // False positive is better than the crash here
+                        val companionClass = classNodeMap[companionName] ?: return@filter true
+                        val visibility = visibilityMap[companionName] ?: return@filter true
+                        companionClass.isEffectivelyPublic(visibility)
                     }
 
                 val allMethods = methods.map { it.toMethodBinarySignature() }
@@ -60,8 +78,6 @@ public fun Sequence<InputStream>.loadApiFromJvmClasses(visibilityFilter: (String
                 )
             }
         }
-        .asIterable()
-        .sortedBy { it.name }
 }
 
 internal fun List<ClassBinarySignature>.filterOutAnnotated(targetAnnotations: Set<String>): List<ClassBinarySignature> {

src/test/kotlin/cases/companions/companions.kt

@@ -18,6 +18,11 @@ object PublicClasses {
         internal companion object
     }
 
+    class PublishedApiCompanion {
+        @PublishedApi
+        internal companion object
+    }
+
     class PrivateCompanion {
         private companion object
     }
@@ -33,25 +38,56 @@ object PublicInterfaces {
     }
 }
 
+object InternalClasses {
+    internal class PublicCompanion {
+        companion object
+    }
+
+    internal class ProtectedCompanion {
+        protected companion object
+    }
 
+    internal abstract class AbstractProtectedCompanion {
+        protected companion object
+    }
 
-object InternalClasses {
+    internal class InternalCompanion {
+        internal companion object
+    }
+
+    internal class PrivateCompanion {
+        private companion object
+    }
+}
+
+object PublishedApiClasses {
+    @PublishedApi
     internal class PublicCompanion {
         companion object
     }
 
+    @PublishedApi
     internal class ProtectedCompanion {
         protected companion object
     }
 
+    @PublishedApi
     internal abstract class AbstractProtectedCompanion {
         protected companion object
     }
 
+    @PublishedApi
     internal class InternalCompanion {
         internal companion object
     }
 
+    @PublishedApi
+    internal class PublishedApiCompanion {
+        @PublishedApi
+        internal companion object
+    }
+
+    @PublishedApi
     internal class PrivateCompanion {
         private companion object
     }
@@ -67,7 +103,6 @@ object InternalInterfaces {
     }
 }
 
-
 object PrivateClasses {
     private class PublicCompanion {
         companion object
@@ -85,6 +120,11 @@ object PrivateClasses {
         internal companion object
     }
 
+    private class PublishedApiCompanion {
+        @PublishedApi
+        internal companion object
+    }
+
     private class PrivateCompanion {
         private companion object
     }

src/test/kotlin/cases/companions/companions.txt

@@ -27,7 +27,6 @@ protected final class cases/companions/PublicClasses$AbstractProtectedCompanion$
 }
 
 public final class cases/companions/PublicClasses$InternalCompanion {
-	public static final field Companion Lcases/companions/PublicClasses$InternalCompanion$Companion;
 	public fun <init> ()V
 }
 
@@ -47,12 +46,19 @@ public final class cases/companions/PublicClasses$PublicCompanion {
 public final class cases/companions/PublicClasses$PublicCompanion$Companion {
 }
 
+public final class cases/companions/PublicClasses$PublishedApiCompanion {
+	public static final field Companion Lcases/companions/PublicClasses$PublishedApiCompanion$Companion;
+	public fun <init> ()V
+}
+
+public final class cases/companions/PublicClasses$PublishedApiCompanion$Companion {
+}
+
 public final class cases/companions/PublicInterfaces {
 	public static final field INSTANCE Lcases/companions/PublicInterfaces;
 }
 
 public abstract interface class cases/companions/PublicInterfaces$PrivateCompanion {
-	public static final synthetic field Companion Lcases/companions/PublicInterfaces$PrivateCompanion$Companion;
 }
 
 public abstract interface class cases/companions/PublicInterfaces$PublicCompanion {
@@ -62,3 +68,43 @@ public abstract interface class cases/companions/PublicInterfaces$PublicCompanio
 public final class cases/companions/PublicInterfaces$PublicCompanion$Companion {
 }
 
+public final class cases/companions/PublishedApiClasses {
+	public static final field INSTANCE Lcases/companions/PublishedApiClasses;
+}
+
+public abstract class cases/companions/PublishedApiClasses$AbstractProtectedCompanion {
+	protected static final field Companion Lcases/companions/PublishedApiClasses$AbstractProtectedCompanion$Companion;
+	public fun <init> ()V
+}
+
+protected final class cases/companions/PublishedApiClasses$AbstractProtectedCompanion$Companion {
+}
+
+public final class cases/companions/PublishedApiClasses$InternalCompanion {
+	public fun <init> ()V
+}
+
+public final class cases/companions/PublishedApiClasses$PrivateCompanion {
+	public fun <init> ()V
+}
+
+public final class cases/companions/PublishedApiClasses$ProtectedCompanion {
+	public fun <init> ()V
+}
+
+public final class cases/companions/PublishedApiClasses$PublicCompanion {
+	public static final field Companion Lcases/companions/PublishedApiClasses$PublicCompanion$Companion;
+	public fun <init> ()V
+}
+
+public final class cases/companions/PublishedApiClasses$PublicCompanion$Companion {
+}
+
+public final class cases/companions/PublishedApiClasses$PublishedApiCompanion {
+	public static final field Companion Lcases/companions/PublishedApiClasses$PublishedApiCompanion$Companion;
+	public fun <init> ()V
+}
+
+public final class cases/companions/PublishedApiClasses$PublishedApiCompanion$Companion {
+}
+

src/test/kotlin/cases/internal/internal.txt

@@ -1,3 +1,16 @@
+public final class cases/internal/Companion {
+	public fun <init> ()V
+}
+
 public final class cases/internal/PublicClass {
 }
 
+public final class cases/internal/PublicClassInternalCompanionWithNameShadowing {
+	public fun <init> ()V
+	public static final fun getCompanion ()Lcases/internal/Companion;
+}
+
+public final class cases/internal/PublicClassNamedInternalCompanion {
+	public fun <init> ()V
+}
+

src/test/kotlin/cases/internal/internalCompanion.kt

@@ -0,0 +1,26 @@
+/*
+ * Copyright 2016-2021 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 cases.internal
+
+// Internal companion is not part of public API
+// neither should be outer static final companion field
+
+// Named that way so the test ensures that PublicClassInternalCompanionWithNameShadowing does properly differentiate
+// 'Companion' static final field and hand-written field with the same type
+class Companion {
+    internal companion object
+}
+
+class PublicClassNamedInternalCompanion {
+    internal companion object Foo
+}
+
+class PublicClassInternalCompanionWithNameShadowing {
+    internal companion object Companion {
+        @JvmStatic
+        public val Companion: cases.internal.Companion = Companion()
+    }
+}