Merge pull request github#10952 from smowton/smowton/fix/java-interface-redeclares-tostring

Kotlin: extract interface redeclarations of `Object` methods

Author: smowton

java/kotlin-extractor/src/main/kotlin/KotlinFileExtractor.kt

@@ -27,6 +27,7 @@ import org.jetbrains.kotlin.load.java.structure.JavaClass
 import org.jetbrains.kotlin.load.java.structure.JavaMethod
 import org.jetbrains.kotlin.load.java.structure.JavaTypeParameter
 import org.jetbrains.kotlin.load.java.structure.JavaTypeParameterListOwner
+import org.jetbrains.kotlin.load.java.structure.impl.classFiles.BinaryJavaClass
 import org.jetbrains.kotlin.name.FqName
 import org.jetbrains.kotlin.types.Variance
 import org.jetbrains.kotlin.util.OperatorNameConventions
@@ -98,15 +99,29 @@ open class KotlinFileExtractor(
         }
     }
 
+    private fun javaBinaryDeclaresMethod(c: IrClass, name: String) =
+        ((c.source as? JavaSourceElement)?.javaElement as? BinaryJavaClass)?.methods?.any { it.name.asString() == name }
+
+    private fun isJavaBinaryDeclaration(f: IrFunction) =
+        f.parentClassOrNull?.let { javaBinaryDeclaresMethod(it, f.name.asString()) } ?: false
+
+    private fun isJavaBinaryObjectMethodRedeclaration(d: IrDeclaration) =
+        when (d) {
+            is IrFunction ->
+                when (d.name.asString()) {
+                    "toString" -> d.valueParameters.isEmpty()
+                    "hashCode" -> d.valueParameters.isEmpty()
+                    "equals" -> d.valueParameters.singleOrNull()?.type?.isNullableAny() ?: false
+                    else -> false
+                } && isJavaBinaryDeclaration(d)
+            else -> false
+        }
+
     @OptIn(ObsoleteDescriptorBasedAPI::class)
     private fun isFake(d: IrDeclarationWithVisibility): Boolean {
-        val visibility = d.visibility
-        if (visibility is DelegatedDescriptorVisibility && visibility.delegate == Visibilities.InvisibleFake) {
-            return true
-        }
-        if (d.isFakeOverride) {
+        val hasFakeVisibility = d.visibility.let { it is DelegatedDescriptorVisibility && it.delegate == Visibilities.InvisibleFake } || d.isFakeOverride
+        if (hasFakeVisibility && !isJavaBinaryObjectMethodRedeclaration(d))
             return true
-        }
         try {
             if ((d as? IrFunction)?.descriptor?.isHiddenToOvercomeSignatureClash == true) {
                 return true
@@ -908,7 +923,9 @@ open class KotlinFileExtractor(
             else
                 null
         } else {
-            forceExtractFunction(f, parentId, extractBody, extractMethodAndParameterTypeAccesses, typeSubstitution, classTypeArgsIncludingOuterClasses).also {
+            // Work around an apparent bug causing redeclarations of `fun toString(): String` specifically in interfaces loaded from Java classes show up like fake overrides.
+            val overriddenVisibility = if (f.isFakeOverride && isJavaBinaryObjectMethodRedeclaration(f)) OverriddenFunctionAttributes(visibility = DescriptorVisibilities.PUBLIC) else null
+            forceExtractFunction(f, parentId, extractBody, extractMethodAndParameterTypeAccesses, typeSubstitution, classTypeArgsIncludingOuterClasses, overriddenAttributes = overriddenVisibility).also {
                 // The defaults-forwarder function is a static utility, not a member, so we only need to extract this for the unspecialised instance of this class.
                 if (classTypeArgsIncludingOuterClasses.isNullOrEmpty())
                     extractDefaultsFunction(f, parentId, extractBody, extractMethodAndParameterTypeAccesses)

java/ql/integration-tests/posix-only/kotlin/java-interface-redeclares-tostring/Test.java

@@ -0,0 +1,5 @@
+public interface Test {
+  String toString();
+  int hashCode();
+  boolean equals(Object other);
+}

java/ql/integration-tests/posix-only/kotlin/java-interface-redeclares-tostring/test.expected

@@ -0,0 +1,4 @@
+| equals | Test |
+| hashCode | Test |
+| toString | Test |
+| toString | java.lang.CharSequence |

java/ql/integration-tests/posix-only/kotlin/java-interface-redeclares-tostring/test.py

@@ -0,0 +1,4 @@
+from create_database_utils import *
+
+runSuccessfully(["javac", "Test.java", "-d", "bin"])
+run_codeql_database_create(["kotlinc user.kt -cp bin"], lang="java")

java/ql/integration-tests/posix-only/kotlin/java-interface-redeclares-tostring/test.ql

@@ -0,0 +1,7 @@
+import java
+
+from Method m
+where
+  m.getDeclaringType().getName() = ["Test", "CharSequence"] and
+  m.getName() = ["toString", "equals", "hashCode"]
+select m.getName(), m.getDeclaringType().getQualifiedName()

java/ql/integration-tests/posix-only/kotlin/java-interface-redeclares-tostring/user.kt

@@ -0,0 +1 @@
+fun f(t: Test, cs: CharSequence) = t.toString() + cs.toString() + t.equals(1) + t.hashCode()

java/ql/test/kotlin/library-tests/java-kotlin-collection-type-generic-methods/test.expected

@@ -110,6 +110,7 @@ methodWithDuplicate
 | Collection<E> | addAll | Collection<? extends E> |
 | Collection<E> | contains | Object |
 | Collection<E> | containsAll | Collection<?> |
+| Collection<E> | equals | Object |
 | Collection<E> | remove | Object |
 | Collection<E> | removeAll | Collection<?> |
 | Collection<E> | removeIf | Predicate<? super E> |
@@ -120,6 +121,7 @@ methodWithDuplicate
 | Collection<Entry<K,V>> | addAll | Collection<? extends Entry<K,V>> |
 | Collection<Entry<K,V>> | contains | Object |
 | Collection<Entry<K,V>> | containsAll | Collection<?> |
+| Collection<Entry<K,V>> | equals | Object |
 | Collection<Entry<K,V>> | remove | Object |
 | Collection<Entry<K,V>> | removeAll | Collection<?> |
 | Collection<Entry<K,V>> | removeIf | Predicate<? super Entry<K,V>> |
@@ -130,6 +132,7 @@ methodWithDuplicate
 | Collection<K> | addAll | Collection<? extends K> |
 | Collection<K> | contains | Object |
 | Collection<K> | containsAll | Collection<?> |
+| Collection<K> | equals | Object |
 | Collection<K> | remove | Object |
 | Collection<K> | removeAll | Collection<?> |
 | Collection<K> | removeIf | Predicate<? super K> |
@@ -151,6 +154,7 @@ methodWithDuplicate
 | Collection<V> | addAll | Collection<? extends V> |
 | Collection<V> | contains | Object |
 | Collection<V> | containsAll | Collection<?> |
+| Collection<V> | equals | Object |
 | Collection<V> | remove | Object |
 | Collection<V> | removeAll | Collection<?> |
 | Collection<V> | removeIf | Predicate<? super V> |
@@ -188,6 +192,7 @@ methodWithDuplicate
 | List<E> | contains | Object |
 | List<E> | containsAll | Collection<?> |
 | List<E> | copyOf | Collection<? extends E> |
+| List<E> | equals | Object |
 | List<E> | get | int |
 | List<E> | indexOf | Object |
 | List<E> | lastIndexOf | Object |
@@ -270,6 +275,7 @@ methodWithDuplicate
 | Map<Identity,Entry<?>> | copyOf | Map<? extends K,? extends V> |
 | Map<Identity,Entry<?>> | entry | K |
 | Map<Identity,Entry<?>> | entry | V |
+| Map<Identity,Entry<?>> | equals | Object |
 | Map<Identity,Entry<?>> | forEach | BiConsumer<? super Identity,? super Entry<?>> |
 | Map<Identity,Entry<?>> | get | Object |
 | Map<Identity,Entry<?>> | getOrDefault | Entry<?> |
@@ -300,6 +306,7 @@ methodWithDuplicate
 | Map<K,V> | copyOf | Map<? extends K,? extends V> |
 | Map<K,V> | entry | K |
 | Map<K,V> | entry | V |
+| Map<K,V> | equals | Object |
 | Map<K,V> | forEach | BiConsumer<? super K,? super V> |
 | Map<K,V> | get | Object |
 | Map<K,V> | getOrDefault | Object |
@@ -330,6 +337,7 @@ methodWithDuplicate
 | Map<Object,Object> | copyOf | Map<? extends K,? extends V> |
 | Map<Object,Object> | entry | K |
 | Map<Object,Object> | entry | V |
+| Map<Object,Object> | equals | Object |
 | Map<Object,Object> | forEach | BiConsumer<? super Object,? super Object> |
 | Map<Object,Object> | get | Object |
 | Map<Object,Object> | getOrDefault | Object |