Merge pull request #9768 from smowton/smowton/fix/internal-method-name-mangling

smowton · web-flow · commit 4c6a9772af4a · 2022-07-01T14:33:32.000+01:00
Kotlin: Mangle names of internal functions to match JVM symbols
diff --git a/java/kotlin-extractor/src/main/kotlin/KotlinUsesExtractor.kt b/java/kotlin-extractor/src/main/kotlin/KotlinUsesExtractor.kt
@@ -11,6 +11,7 @@ import org.jetbrains.kotlin.backend.common.lower.parents
 import org.jetbrains.kotlin.backend.common.lower.parentsWithSelf
 import org.jetbrains.kotlin.backend.jvm.ir.propertyIfAccessor
 import org.jetbrains.kotlin.builtins.StandardNames
+import org.jetbrains.kotlin.codegen.JvmCodegenUtil
 import org.jetbrains.kotlin.descriptors.*
 import org.jetbrains.kotlin.ir.ObsoleteDescriptorBasedAPI
 import org.jetbrains.kotlin.ir.declarations.*
@@ -23,8 +24,10 @@ import org.jetbrains.kotlin.load.java.BuiltinMethodsWithSpecialGenericSignature
 import org.jetbrains.kotlin.load.java.JvmAbi
 import org.jetbrains.kotlin.load.java.sources.JavaSourceElement
 import org.jetbrains.kotlin.load.java.structure.*
+import org.jetbrains.kotlin.load.kotlin.getJvmModuleNameForDeserializedDescriptor
 import org.jetbrains.kotlin.name.FqName
 import org.jetbrains.kotlin.name.Name
+import org.jetbrains.kotlin.name.NameUtils
 import org.jetbrains.kotlin.name.SpecialNames
 import org.jetbrains.kotlin.types.Variance
 import org.jetbrains.kotlin.util.OperatorNameConventions
@@ -754,11 +757,25 @@ open class KotlinUsesExtractor(
 
     data class FunctionNames(val nameInDB: String, val kotlinName: String)
 
+    @OptIn(ObsoleteDescriptorBasedAPI::class)
+    private fun getJvmModuleName(f: IrFunction) =
+        NameUtils.sanitizeAsJavaIdentifier(
+            getJvmModuleNameForDeserializedDescriptor(f.descriptor) ?: JvmCodegenUtil.getModuleName(pluginContext.moduleDescriptor)
+        )
+
     fun getFunctionShortName(f: IrFunction) : FunctionNames {
         if (f.origin == IrDeclarationOrigin.LOCAL_FUNCTION_FOR_LAMBDA || f.isAnonymousFunction)
             return FunctionNames(
                 OperatorNameConventions.INVOKE.asString(),
                 OperatorNameConventions.INVOKE.asString())
+
+        fun getSuffixIfInternal() =
+            if (f.visibility == DescriptorVisibilities.INTERNAL) {
+                "\$" + getJvmModuleName(f)
+            } else {
+                ""
+            }
+
         (f as? IrSimpleFunction)?.correspondingPropertySymbol?.let {
             val propName = it.owner.name.asString()
             val getter = it.owner.getter
@@ -774,35 +791,26 @@ open class KotlinUsesExtractor(
                 }
             }
 
-            when (f) {
-                getter -> {
-                    val defaultFunctionName = JvmAbi.getterName(propName)
-                    val defaultDbName = if (getter.visibility == DescriptorVisibilities.PRIVATE && getter.origin == IrDeclarationOrigin.DEFAULT_PROPERTY_ACCESSOR) {
-                        // In JVM these functions don't exist, instead the backing field is accessed directly
-                        defaultFunctionName + "\$private"
-                    } else {
-                        defaultFunctionName
-                    }
-                    return FunctionNames(getJvmName(getter) ?: defaultDbName, defaultFunctionName)
-                }
-                setter -> {
-                    val defaultFunctionName = JvmAbi.setterName(propName)
-                    val defaultDbName = if (setter.visibility == DescriptorVisibilities.PRIVATE && setter.origin == IrDeclarationOrigin.DEFAULT_PROPERTY_ACCESSOR) {
-                        // In JVM these functions don't exist, instead the backing field is accessed directly
-                        defaultFunctionName + "\$private"
-                    } else {
-                        defaultFunctionName
-                    }
-                    return FunctionNames(getJvmName(setter) ?: defaultDbName, defaultFunctionName)
-                }
+            val maybeFunctionName = when (f) {
+                getter -> JvmAbi.getterName(propName)
+                setter -> JvmAbi.setterName(propName)
                 else -> {
                     logger.error(
                         "Function has a corresponding property, but is neither the getter nor the setter"
                     )
+                    null
+                }
+            }
+            maybeFunctionName?.let { defaultFunctionName ->
+                val suffix = if (f.visibility == DescriptorVisibilities.PRIVATE && f.origin == IrDeclarationOrigin.DEFAULT_PROPERTY_ACCESSOR) {
+                    "\$private"
+                } else {
+                    getSuffixIfInternal()
                 }
+                return FunctionNames(getJvmName(f) ?: "$defaultFunctionName$suffix", defaultFunctionName)
             }
         }
-        return FunctionNames(getJvmName(f) ?: f.name.asString(), f.name.asString())
+        return FunctionNames(getJvmName(f) ?: "${f.name.asString()}${getSuffixIfInternal()}", f.name.asString())
     }
 
     // This excludes class type parameters that show up in (at least) constructors' typeParameters list.
diff --git a/java/ql/consistency-queries/visibility.ql b/java/ql/consistency-queries/visibility.ql
@@ -18,5 +18,6 @@ where
   m.getFile().isKotlinSourceFile() and
   // TODO: This ought to have visibility information
   not m.getName() = "<clinit>" and
-  count(visibility(m)) != 1
+  count(visibility(m)) != 1 and
+  not (count(visibility(m)) = 2 and visibility(m) = "public" and visibility(m) = "internal") // This is a reasonable result, since the JVM symbol is declared public, but Kotlin metadata flags it as internal
 select m, concat(visibility(m), ", ")
diff --git a/java/ql/integration-tests/posix-only/kotlin/module_mangled_names/User.java b/java/ql/integration-tests/posix-only/kotlin/module_mangled_names/User.java
@@ -0,0 +1,9 @@
+public class User {
+
+  public static int test(Test1 test1, Test2 test2, Test3 test3) {
+
+    return test1.f$main() + test2.f$mymodule() + test3.f$reservedchars___();
+
+  }
+
+}
diff --git a/java/ql/integration-tests/posix-only/kotlin/module_mangled_names/test.expected b/java/ql/integration-tests/posix-only/kotlin/module_mangled_names/test.expected
@@ -0,0 +1,4 @@
+| User.java:3:21:3:24 | test |
+| test1.kt:3:12:3:22 | f$main |
+| test2.kt:3:12:3:22 | f$mymodule |
+| test3.kt:3:12:3:22 | f$reservedchars___ |
diff --git a/java/ql/integration-tests/posix-only/kotlin/module_mangled_names/test.py b/java/ql/integration-tests/posix-only/kotlin/module_mangled_names/test.py
@@ -0,0 +1,3 @@
+from create_database_utils import *
+
+run_codeql_database_create(["kotlinc test1.kt", "kotlinc test2.kt -module-name mymodule", "kotlinc test3.kt -module-name reservedchars\\\"${}/", "javac User.java -cp ." ], lang="java")
diff --git a/java/ql/integration-tests/posix-only/kotlin/module_mangled_names/test.ql b/java/ql/integration-tests/posix-only/kotlin/module_mangled_names/test.ql
@@ -0,0 +1,5 @@
+import java
+
+from Method m
+where m.fromSource()
+select m
diff --git a/java/ql/integration-tests/posix-only/kotlin/module_mangled_names/test1.kt b/java/ql/integration-tests/posix-only/kotlin/module_mangled_names/test1.kt
@@ -0,0 +1,5 @@
+public class Test1 {
+
+  internal fun f() = 1
+
+}
diff --git a/java/ql/integration-tests/posix-only/kotlin/module_mangled_names/test2.kt b/java/ql/integration-tests/posix-only/kotlin/module_mangled_names/test2.kt
@@ -0,0 +1,5 @@
+public class Test2 {
+
+  internal fun f() = 2
+
+}
diff --git a/java/ql/integration-tests/posix-only/kotlin/module_mangled_names/test3.kt b/java/ql/integration-tests/posix-only/kotlin/module_mangled_names/test3.kt
@@ -0,0 +1,5 @@
+public class Test3 {
+
+  internal fun f() = 3
+
+}
diff --git a/java/ql/test/kotlin/library-tests/internal-public-alias/User.java b/java/ql/test/kotlin/library-tests/internal-public-alias/User.java
@@ -0,0 +1,11 @@
+public class User {
+
+  public static int test(Test t) {
+
+    t.setInternalVar$main(t.getInternalVal$main());
+
+    return t.internalFun$main();
+
+  }
+
+}
diff --git a/java/ql/test/kotlin/library-tests/internal-public-alias/test.expected b/java/ql/test/kotlin/library-tests/internal-public-alias/test.expected
@@ -0,0 +1,6 @@
+| User.java:3:21:3:24 | test |
+| test.kt:3:12:3:30 | getInternalVal$main |
+| test.kt:6:3:6:36 | getInternalVal |
+| test.kt:8:12:8:30 | getInternalVar$main |
+| test.kt:8:12:8:30 | setInternalVar$main |
+| test.kt:10:12:10:32 | internalFun$main |
diff --git a/java/ql/test/kotlin/library-tests/internal-public-alias/test.kt b/java/ql/test/kotlin/library-tests/internal-public-alias/test.kt
@@ -0,0 +1,12 @@
+public class Test {
+
+  internal val internalVal = 1
+
+  // Would clash with the internal val's getter without name mangling and provoke a database inconsistency:
+  fun getInternalVal() = internalVal
+
+  internal var internalVar = 2
+
+  internal fun internalFun() = 3
+
+}
diff --git a/java/ql/test/kotlin/library-tests/internal-public-alias/test.ql b/java/ql/test/kotlin/library-tests/internal-public-alias/test.ql
@@ -0,0 +1,5 @@
+import java
+
+from Method m
+where m.fromSource()
+select m
diff --git a/java/ql/test/kotlin/library-tests/java-kotlin-collection-type-generic-methods/test.expected b/java/ql/test/kotlin/library-tests/java-kotlin-collection-type-generic-methods/test.expected
@@ -54,7 +54,7 @@ methodWithDuplicate
 | AbstractList<E> | set | int |
 | AbstractList<E> | subList | int |
 | AbstractList<E> | subListRangeCheck | int |
-| AbstractMap | containsEntry | Entry<?,?> |
+| AbstractMap | containsEntry$kotlin_stdlib | Entry<?,?> |
 | AbstractMap | containsKey | Object |
 | AbstractMap | containsValue | Object |
 | AbstractMap | equals | Object |
@@ -79,7 +79,7 @@ methodWithDuplicate
 | AbstractMap<K,V> | put | V |
 | AbstractMap<K,V> | putAll | Map<? extends K,? extends V> |
 | AbstractMap<K,V> | remove | Object |
-| AbstractMap<String,String> | containsEntry | Entry<?,?> |
+| AbstractMap<String,String> | containsEntry$kotlin_stdlib | Entry<?,?> |
 | AbstractMap<String,String> | containsKey | Object |
 | AbstractMap<String,String> | containsValue | Object |
 | AbstractMap<String,String> | equals | Object |
diff --git a/java/ql/test/kotlin/library-tests/methods/methods.expected b/java/ql/test/kotlin/library-tests/methods/methods.expected
@@ -42,7 +42,7 @@ methods
 | methods.kt:5:1:20:1 | Class | methods.kt:14:12:14:29 | publicFun | publicFun() | public |  |
 | methods.kt:5:1:20:1 | Class | methods.kt:15:15:15:35 | protectedFun | protectedFun() | protected |  |
 | methods.kt:5:1:20:1 | Class | methods.kt:16:13:16:31 | privateFun | privateFun() | private |  |
-| methods.kt:5:1:20:1 | Class | methods.kt:17:14:17:33 | internalFun | internalFun() | internal |  |
+| methods.kt:5:1:20:1 | Class | methods.kt:17:14:17:33 | internalFun$main | internalFun$main() | internal |  |
 | methods.kt:5:1:20:1 | Class | methods.kt:18:5:18:36 | noExplicitVisibilityFun | noExplicitVisibilityFun() | public |  |
 | methods.kt:5:1:20:1 | Class | methods.kt:19:12:19:29 | inlineFun | inlineFun() | inline, public |  |
 constructors
diff --git a/java/ql/test/kotlin/library-tests/modifiers/modifiers.expected b/java/ql/test/kotlin/library-tests/modifiers/modifiers.expected
@@ -11,7 +11,7 @@
 | modifiers.kt:4:5:4:22 | c | Field | final |
 | modifiers.kt:4:5:4:22 | c | Field | private |
 | modifiers.kt:4:5:4:22 | c | Property | internal |
-| modifiers.kt:4:14:4:22 | getC | Method | internal |
+| modifiers.kt:4:14:4:22 | getC$main | Method | internal |
 | modifiers.kt:5:5:5:34 | d | Field | final |
 | modifiers.kt:5:5:5:34 | d | Field | private |
 | modifiers.kt:5:5:5:34 | d | Property | public |
diff --git a/java/ql/test/kotlin/library-tests/properties/properties.expected b/java/ql/test/kotlin/library-tests/properties/properties.expected
@@ -45,7 +45,7 @@ fieldDeclarations
 | properties.kt:35:5:35:32 | privateProp | properties.kt:35:13:35:32 | getPrivateProp$private | file://:0:0:0:0 | <none> | properties.kt:35:5:35:32 | privateProp | private |
 | properties.kt:36:5:36:36 | protectedProp | properties.kt:36:15:36:36 | getProtectedProp | file://:0:0:0:0 | <none> | properties.kt:36:5:36:36 | protectedProp | protected |
 | properties.kt:37:5:37:30 | publicProp | properties.kt:37:12:37:30 | getPublicProp | file://:0:0:0:0 | <none> | properties.kt:37:5:37:30 | publicProp | public |
-| properties.kt:38:5:38:34 | internalProp | properties.kt:38:14:38:34 | getInternalProp | file://:0:0:0:0 | <none> | properties.kt:38:5:38:34 | internalProp | internal |
+| properties.kt:38:5:38:34 | internalProp | properties.kt:38:14:38:34 | getInternalProp$main | file://:0:0:0:0 | <none> | properties.kt:38:5:38:34 | internalProp | internal |
 | properties.kt:67:1:67:23 | constVal | properties.kt:67:7:67:23 | getConstVal | file://:0:0:0:0 | <none> | properties.kt:67:1:67:23 | constVal | public |
 | properties.kt:70:5:70:16 | prop | properties.kt:70:5:70:16 | getProp | file://:0:0:0:0 | <none> | properties.kt:70:5:70:16 | prop | public |
 | properties.kt:78:1:79:13 | x | properties.kt:79:5:79:13 | getX | file://:0:0:0:0 | <none> | file://:0:0:0:0 | <none> | public |

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+from create_database_utils import *`
	`2`	`+`
	`3`	`+run_codeql_database_create(["kotlinc test1.kt", "kotlinc test2.kt -module-name mymodule", "kotlinc test3.kt -module-name reservedchars\\\"${}/", "javac User.java -cp ." ], lang="java")`
-Original file line number
+Diff line change
@@ @@ -0,0 +1,5 @@ @@
 +public class Test1 {
++
 +  internal fun f() = 1
++
 +}