Merge pull request #9876 from smowton/smowton/feature/interface-forwarding

Kotlin: implement default interface forwarding

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
@@ -845,10 +846,68 @@ open class KotlinFileExtractor(
         }
     }
 
+    private fun isKotlinDefinedInterface(cls: IrClass?) =
+        cls != null && cls.isInterface && cls.origin != IrDeclarationOrigin.IR_EXTERNAL_JAVA_DECLARATION_STUB
+
+    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 Kotlin-defined method inheriting a Kotlin-defined 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.parentClassOrNull.let { it != null && it.origin != IrDeclarationOrigin.IR_EXTERNAL_JAVA_DECLARATION_STUB && it.modality != Modality.ABSTRACT } &&
+                f.realOverrideTarget.let { it != f && (it as? IrSimpleFunction)?.modality != Modality.ABSTRACT && isKotlinDefinedInterface(it.parentClassOrNull) }
+
+    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, overriddenAttributes = OverriddenFunctionAttributes(visibility = DescriptorVisibilities.PUBLIC)).also { functionId ->
+            tw.writeCompiler_generated(functionId, CompilerGeneratedKinds.INTERFACE_FORWARDER.kind)
+            if (extractBody) {
+                val realFunctionLocId = tw.getLocation(f)
+                val inheritedDefaultFunction = f.realOverrideTarget
+                val directlyInheritedSymbol =
+                    when(f) {
+                        is IrSimpleFunction ->
+                            f.overriddenSymbols.find { it.owner === inheritedDefaultFunction }
+                                ?: f.overriddenSymbols.find { it.owner.realOverrideTarget === inheritedDefaultFunction }
+                                ?: inheritedDefaultFunction.symbol
+                        else -> inheritedDefaultFunction.symbol // This is strictly invalid, since we shouldn't use A.super.f(...) where A may not be a direct supertype, but this path should also be unreachable.
+                    }
+                val defaultDefiningInterfaceType = (directlyInheritedSymbol.owner.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())
@@ -1163,7 +1222,7 @@ open class KotlinFileExtractor(
                     extractBody(body, id)
                 }
 
-                extractVisibility(f, id, f.visibility)
+                extractVisibility(f, id, overriddenAttributes?.visibility ?: f.visibility)
 
                 if (f.isInline) {
                     addModifiers(id, "inline")
@@ -1227,7 +1286,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 {
 
@@ -5375,7 +5436,9 @@ open class KotlinFileExtractor(
         val sourceLoc: Label<DbLocation>? = null,
         val valueParameters: List<IrValueParameter>? = null,
         val typeParameters: List<IrTypeParameter>? = null,
-        val isStatic: Boolean? = null)
+        val isStatic: Boolean? = null,
+        val visibility: DescriptorVisibility? = null,
+    )
 
     private fun peekDeclStackAsDeclarationParent(elementToReportOn: IrElement): IrDeclarationParent? {
         val trapWriter = tw
@@ -5400,6 +5463,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/gradle_kotlinx_serialization/PrintAst.expected

@@ -425,6 +425,15 @@ app/src/main/kotlin/testProject/App.kt:
 #    7|                   -1: [ThisAccess] $serializer.this
 #    7|                     0: [TypeAccess] $serializer
 #    7|                 1: [VarAccess] tmp0_serialDesc
+#    7|       7: [Method] typeParametersSerializers
+#    7|         3: [TypeAccess] KSerializer<?>[]
+#    7|           0: [TypeAccess] KSerializer<?>
+#    7|             0: [WildcardTypeAccess] ? ...
+#    7|         5: [BlockStmt] { ... }
+#    7|           0: [ReturnStmt] return ...
+#    7|             0: [MethodAccess] typeParametersSerializers(...)
+#    7|               -1: [SuperAccess] GeneratedSerializer.super
+#    7|                 0: [TypeAccess] GeneratedSerializer
 #    7|     11: [Class] Companion
 #    0|       1: [Method] serializer
 #    0|         3: [TypeAccess] KSerializer<Project>

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

@@ -0,0 +1,25 @@
+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.
+  // RealIndirect is similar to Real, except it inherits its methods indirectly via MiddleInterface.
+  public static void test(Real r1, RealNoForwards r2, RealIndirect r3) {
+
+    sink(r1.f());
+    sink(r1.g(2));
+    sink(r1.getX());
+
+    sink(r2.f());
+    sink(r2.g(5));
+    sink(r2.getX());
+
+    sink(r3.f());
+    sink(r3.g(8));
+    sink(r3.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,36 @@
+callables
+| User.java:1:14:1:17 | User | User.java:1:14:1:17 | User | from source |
+| User.java:3:22:3:25 | sink | User.java:1:14:1:17 | User | from source |
+| User.java:9:22:9:25 | test | User.java:1:14:1:17 | User | from source |
+| noforwards.kt:3:3:3:13 | f | noforwards.kt:1:1:10:1 | NoForwards | from source |
+| noforwards.kt:5:3:5:19 | g | noforwards.kt:1:1:10:1 | NoForwards | from source |
+| noforwards.kt:8:5:8:13 | getX | noforwards.kt:1:1:10:1 | NoForwards | from source |
+| noforwards.kt:12:1:12:37 | RealNoForwards | noforwards.kt:12:1:12:37 | RealNoForwards | from source |
+| test.kt:3:3:3:13 | f | test.kt:1:1:10:1 | Test | from source |
+| test.kt:5:3:5:19 | g | test.kt:1:1:10:1 | Test | from source |
+| test.kt:8:5:8:13 | getX | test.kt:1:1:10:1 | Test | from source |
+| test.kt:12:1:12:21 | Real | test.kt:12:1:12:21 | Real | from source |
+| test.kt:12:1:12:21 | f | test.kt:12:1:12:21 | Real | Forwarder for a Kotlin class inheriting an interface default method |
+| test.kt:12:1:12:21 | g | test.kt:12:1:12:21 | Real | Forwarder for a Kotlin class inheriting an interface default method |
+| test.kt:12:1:12:21 | getX | test.kt:12:1:12:21 | Real | Forwarder for a Kotlin class inheriting an interface default method |
+| test.kt:16:1:16:40 | RealIndirect | test.kt:16:1:16:40 | RealIndirect | from source |
+| test.kt:16:1:16:40 | f | test.kt:16:1:16:40 | RealIndirect | Forwarder for a Kotlin class inheriting an interface default method |
+| test.kt:16:1:16:40 | g | test.kt:16:1:16:40 | RealIndirect | Forwarder for a Kotlin class inheriting an interface default method |
+| test.kt:16:1:16:40 | getX | test.kt:16:1:16:40 | RealIndirect | Forwarder for a Kotlin class inheriting an interface default method |
+superAccesses
+| test.kt:12:1:12:21 | Test.super | test.kt:12:1:12:21 | Real | test.kt:12:1:12:21 | f | test.kt:12:1:12:21 | Test |
+| test.kt:12:1:12:21 | Test.super | test.kt:12:1:12:21 | Real | test.kt:12:1:12:21 | g | test.kt:12:1:12:21 | Test |
+| test.kt:12:1:12:21 | Test.super | test.kt:12:1:12:21 | Real | test.kt:12:1:12:21 | getX | test.kt:12:1:12:21 | Test |
+| test.kt:16:1:16:40 | MiddleInterface.super | test.kt:16:1:16:40 | RealIndirect | test.kt:16:1:16:40 | f | test.kt:16:1:16:40 | MiddleInterface |
+| test.kt:16:1:16:40 | MiddleInterface.super | test.kt:16:1:16:40 | RealIndirect | test.kt:16:1:16:40 | g | test.kt:16:1:16:40 | MiddleInterface |
+| test.kt:16:1:16:40 | MiddleInterface.super | test.kt:16:1:16:40 | RealIndirect | test.kt:16:1:16:40 | getX | test.kt:16:1:16:40 | MiddleInterface |
+#select
+| User.java:12:15:12:15 | 2 | User.java:12:10:12:16 | g(...) |
+| User.java:16:15:16:15 | 5 | User.java:16:10:16:16 | g(...) |
+| User.java:20:15:20:15 | 8 | User.java:20:10:20:16 | g(...) |
+| noforwards.kt:3:13:3:13 | 4 | User.java:15:10:15:15 | f(...) |
+| noforwards.kt:8:13:8:13 | 6 | User.java:17:10:17:18 | getX(...) |
+| test.kt:3:13:3:13 | 1 | User.java:11:10:11:15 | f(...) |
+| test.kt:3:13:3:13 | 1 | User.java:19:10:19:15 | f(...) |
+| test.kt:8:13:8:13 | 3 | User.java:13:10:13:18 | getX(...) |
+| test.kt:8:13:8:13 | 3 | User.java:21:10:21:18 | getX(...) |

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

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

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,36 @@
+import java
+import semmle.code.java.dataflow.DataFlow
+
+query predicate callables(Callable c, RefType declType, string kind) {
+  c.fromSource() and
+  declType = c.getDeclaringType() and
+  (
+    kind = c.compilerGeneratedReason()
+    or
+    not exists(c.compilerGeneratedReason()) and kind = "from source"
+  )
+}
+
+query predicate superAccesses(
+  SuperAccess sa, RefType enclosingType, Callable enclosingCallable, Expr qualifier
+) {
+  sa.getQualifier() = qualifier and
+  enclosingCallable = sa.getEnclosingCallable() and
+  enclosingType = enclosingCallable.getDeclaringType()
+}
+
+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"
     )
   }
 }
@@ -77,7 +79,18 @@ class Element extends @element, Top {
 private predicate hasChildElement(Element parent, Element e) {
   cupackage(e, parent)
   or
-  enclInReftype(e, parent)
+  enclInReftype(e, parent) and
+  not e instanceof LocalClassOrInterface
+  or
+  // Reasoning: any specialised instance of a local class is supposed to belong to the general
+  // case of its enclosing method because we don't instantiate specialised variants of either generic
+  // functions or function bodies, and therefore the local class cannot be specialised with respect
+  // to its enclosing reftypes.
+  e.(LocalClassOrInterface)
+      .getSourceDeclaration()
+      .(LocalClassOrInterface)
+      .getLocalTypeDeclStmt()
+      .getEnclosingCallable() = parent
   or
   not enclInReftype(e, _) and
   e.(Class).getCompilationUnit() = parent

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.