Implement Kotlin default interface method forwarding

Kotlin's implementation of defaults depends on the -Xjvm-default setting (or the @JvmDefault deprecated annotation, not implemented here): by default, actual interface class files don't use default method, and any class that would inherit one instead implements the interface calling a static method defined on TheInterface$DefaultImpls. With
-Xjvm-default=all or =all-compatibility, real interface default methods are emitted, with the latter retaining the DefaultImpls methods so that other Kotlin can use it.

Here I adopt a hybrid solution: create a real default method implementation, but also emit a forwarding method like `@override int f(int x) { return super.TheInterface.f(x); }`, because the Java extractor will see `MyClass.f` in the emitted class file and try to dispatch directly to it. The only downside is that we emit a default interface
method body for a prototype that will appear to be `abstract` to the Java extractor and which it will extract as such. I work around this by tolerating the combination `default abstract` in QL. The alternative would be to fully mimic the DefaultImpls approach, giving 100% fidelity to kotlinc's strategy and therefore no clash with the Java
extractor's view of the world.

Author: smowton

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

@@ -9,6 +9,7 @@ import org.jetbrains.kotlin.backend.common.extensions.IrPluginContext
 import org.jetbrains.kotlin.backend.common.lower.parents
 import org.jetbrains.kotlin.backend.common.pop
 import org.jetbrains.kotlin.builtins.functions.BuiltInFunctionArity
+import org.jetbrains.kotlin.config.JvmAnalysisFlags
 import org.jetbrains.kotlin.descriptors.*
 import org.jetbrains.kotlin.descriptors.java.JavaVisibilities
 import org.jetbrains.kotlin.ir.IrElement
@@ -828,10 +829,56 @@ open class KotlinFileExtractor(
         }
     }
 
+    private fun needsInterfaceForwarder(f: IrFunction) =
+        // forAllMethodsWithBody means -Xjvm-default=all or all-compatibility, in which case real Java default interfaces are used, and we don't need to do anything.
+        // Otherwise, for a method inheriting a default, we need to create a synthetic method like `int f(int x) { return super.InterfaceWithDefault.f(x); }`, because
+        // kotlinc will generate a public method and Java callers may directly target it.
+        // (NB. kotlinc's actual implementation strategy is different -- it makes an inner class called InterfaceWithDefault$DefaultImpls and stores the default methods
+        // there to allow default method usage in Java < 8, but this is hopefully niche.
+        !pluginContext.languageVersionSettings.getFlag(JvmAnalysisFlags.jvmDefaultMode).forAllMethodsWithBody &&
+                f.realOverrideTarget.let { it != f && it.parentClassOrNull?.isInterface == true }
+
+    private fun makeInterfaceForwarder(f: IrFunction, parentId: Label<out DbReftype>, extractBody: Boolean, extractMethodAndParameterTypeAccesses: Boolean, typeSubstitution: TypeSubstitution?, classTypeArgsIncludingOuterClasses: List<IrTypeArgument>?) =
+        forceExtractFunction(f, parentId, extractBody = false, extractMethodAndParameterTypeAccesses, typeSubstitution, classTypeArgsIncludingOuterClasses).also { functionId ->
+            tw.writeCompiler_generated(functionId, CompilerGeneratedKinds.INTERFACE_FORWARDER.kind)
+            if (extractBody) {
+                val realFunctionLocId = tw.getLocation(f)
+                val inheritedDefaultFunction = f.realOverrideTarget
+                val defaultDefiningInterfaceType = (inheritedDefaultFunction.parentClassOrNull ?: return functionId).typeWith()
+
+                extractExpressionBody(functionId, realFunctionLocId).also { returnId ->
+                    extractRawMethodAccess(
+                        f,
+                        realFunctionLocId,
+                        f.returnType,
+                        functionId,
+                        returnId,
+                        0,
+                        returnId,
+                        f.valueParameters.size,
+                        { argParentId, idxOffset ->
+                            f.valueParameters.mapIndexed { idx, param ->
+                                val syntheticParamId = useValueParameter(param, functionId)
+                                extractVariableAccess(syntheticParamId, param.type, realFunctionLocId, argParentId, idxOffset + idx, functionId, returnId)
+                            }
+                        },
+                        f.dispatchReceiverParameter?.type,
+                        { callId ->
+                            extractSuperAccess(defaultDefiningInterfaceType, functionId, callId, -1, returnId, realFunctionLocId)
+                        },
+                        null
+                    )
+                }
+            }
+        }
+
     private fun extractFunction(f: IrFunction, parentId: Label<out DbReftype>, extractBody: Boolean, extractMethodAndParameterTypeAccesses: Boolean, typeSubstitution: TypeSubstitution?, classTypeArgsIncludingOuterClasses: List<IrTypeArgument>?) =
-        if (isFake(f))
-            null
-        else {
+        if (isFake(f)) {
+            if (needsInterfaceForwarder(f))
+                makeInterfaceForwarder(f, parentId, extractBody, extractMethodAndParameterTypeAccesses, typeSubstitution, classTypeArgsIncludingOuterClasses)
+            else
+                null
+        } else {
             forceExtractFunction(f, parentId, extractBody, extractMethodAndParameterTypeAccesses, typeSubstitution, classTypeArgsIncludingOuterClasses).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())
@@ -1202,7 +1249,9 @@ open class KotlinFileExtractor(
 
     private fun extractProperty(p: IrProperty, parentId: Label<out DbReftype>, extractBackingField: Boolean, extractFunctionBodies: Boolean, extractPrivateMembers: Boolean, typeSubstitution: TypeSubstitution?, classTypeArgsIncludingOuterClasses: List<IrTypeArgument>?) {
         with("property", p) {
-            if (isFake(p)) return
+            fun needsInterfaceForwarderQ(f: IrFunction?) = f?.let { needsInterfaceForwarder(f) } ?: false
+
+            if (isFake(p) && !needsInterfaceForwarderQ(p.getter) && !needsInterfaceForwarderQ(p.setter)) return
 
             DeclarationStackAdjuster(p).use {
 
@@ -5373,6 +5422,7 @@ open class KotlinFileExtractor(
         DELEGATED_PROPERTY_SETTER(7),
         JVMSTATIC_PROXY_METHOD(8),
         JVMOVERLOADS_METHOD(9),
-        DEFAULT_ARGUMENTS_METHOD(10)
+        DEFAULT_ARGUMENTS_METHOD(10),
+        INTERFACE_FORWARDER(11),
     }
 }

java/ql/integration-tests/posix-only/kotlin/kotlin-interface-inherited-default/User.java

@@ -0,0 +1,20 @@
+public class User {
+
+  public static void sink(int x) { }
+
+  // Real is compiled with synthetic interface method forwarders, so it appears from a Java perspective to override the interface Test.
+  // RealNoForwards is compiled with -Xjvm-default=all, meaning real Java 8 default interface methods are used, no synthetic forwarders
+  // are created, and call resolution should go straight to the default.
+  public static void test(Real r1, RealNoForwards r2) {
+
+    sink(r1.f());
+    sink(r1.g(2));
+    sink(r1.getX());
+
+    sink(r2.f());
+    sink(r2.g(5));
+    sink(r2.getX());
+
+  }
+
+}

java/ql/integration-tests/posix-only/kotlin/kotlin-interface-inherited-default/noforwards.kt

@@ -0,0 +1,12 @@
+interface NoForwards {
+
+  fun f() = 4
+
+  fun g(x: Int) = x
+
+  val x : Int
+    get() = 6
+
+}
+
+class RealNoForwards : NoForwards { }

java/ql/integration-tests/posix-only/kotlin/kotlin-interface-inherited-default/test.expected

@@ -0,0 +1,6 @@
+| User.java:11:15:11:15 | 2 | User.java:11:10:11:16 | g(...) |
+| User.java:15:15:15:15 | 5 | User.java:15:10:15:16 | g(...) |
+| noforwards.kt:3:13:3:13 | 4 | User.java:14:10:14:15 | f(...) |
+| noforwards.kt:8:13:8:13 | 6 | User.java:16:10:16:18 | getX(...) |
+| test.kt:3:13:3:13 | 1 | User.java:10:10:10:15 | f(...) |
+| test.kt:8:13:8:13 | 3 | User.java:12:10:12:18 | getX(...) |

java/ql/integration-tests/posix-only/kotlin/kotlin-interface-inherited-default/test.kt

@@ -0,0 +1,12 @@
+interface Test {
+
+  fun f() = 1
+
+  fun g(x: Int) = x
+
+  val x : Int
+    get() = 3
+
+}
+
+class Real : Test { }

java/ql/integration-tests/posix-only/kotlin/kotlin-interface-inherited-default/test.py

@@ -0,0 +1,6 @@
+from create_database_utils import *
+import glob
+
+os.mkdir('build')
+run_codeql_database_create(["kotlinc test.kt -d build", "kotlinc noforwards.kt -d build -Xjvm-default=all", "javac User.java -cp build"], lang="java")
+

java/ql/integration-tests/posix-only/kotlin/kotlin-interface-inherited-default/test.ql

@@ -0,0 +1,18 @@
+import java
+import semmle.code.java.dataflow.DataFlow
+
+class Config extends DataFlow::Configuration {
+  Config() { this = "testconfig" }
+
+  override predicate isSource(DataFlow::Node x) {
+    x.asExpr() instanceof IntegerLiteral and x.getEnclosingCallable().fromSource()
+  }
+
+  override predicate isSink(DataFlow::Node x) {
+    x.asExpr().(Argument).getCall().getCallee().getName() = "sink"
+  }
+}
+
+from Config c, DataFlow::Node source, DataFlow::Node sink
+where c.hasFlow(source, sink)
+select source, sink

java/ql/lib/semmle/code/java/Element.qll

@@ -67,6 +67,8 @@ class Element extends @element, Top {
       i = 9 and result = "Forwarder for a @JvmOverloads-annotated function"
       or
       i = 10 and result = "Forwarder for Kotlin calls that need default arguments filling in"
+      or
+      i = 11 and result = "Forwarder for a Kotlin class inheriting an interface default method"
     )
   }
 }

java/ql/lib/semmle/code/java/Member.qll

@@ -471,7 +471,12 @@ class Method extends Callable, @method {
   }
 
   override predicate isAbstract() {
-    Callable.super.isAbstract()
+    // The combination `abstract default` isn't legal in Java,
+    // but it occurs when the Kotlin extractor records a default
+    // body, but the output class file in fact uses an abstract
+    // method and an associated static helper, which we don't
+    // extract as an implementation detail.
+    Callable.super.isAbstract() and not this.isDefault()
     or
     // JLS 9.4: An interface method lacking a `private`, `default`, or `static` modifier
     // is implicitly abstract.