Merge pull request #9123 from smowton/smowton/fix/type-variable-in-scope-consistency

Kotlin: fix cases where type variables were used out of scope

Author: smowton

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

@@ -173,19 +173,7 @@ open class KotlinFileExtractor(
 
     fun extractTypeParameter(tp: IrTypeParameter, apparentIndex: Int): Label<out DbTypevariable>? {
         with("type parameter", tp) {
-            val parentId: Label<out DbClassorinterfaceorcallable>? = when (val parent = tp.parent) {
-                is IrFunction -> useFunction(parent)
-                is IrClass -> useClassSource(parent)
-                else -> {
-                    logger.errorElement("Unexpected type parameter parent", tp)
-                    null
-                }
-            }
-
-            if (parentId == null) {
-                return null
-            }
-
+            val parentId = getTypeParameterParentLabel(tp) ?: return null
             val id = tw.getLabelFor<DbTypevariable>(getTypeParameterLabel(tp))
 
             // Note apparentIndex does not necessarily equal `tp.index`, because at least constructor type parameters
@@ -334,17 +322,7 @@ open class KotlinFileExtractor(
             val typeParamSubstitution =
                 when (argsIncludingOuterClasses) {
                     null -> { x: IrType, _: TypeContext, _: IrPluginContext -> x.toRawType() }
-                    else -> {
-                        makeTypeGenericSubstitutionMap(c, argsIncludingOuterClasses).let {
-                            { x: IrType, useContext: TypeContext, pluginContext: IrPluginContext ->
-                                x.substituteTypeAndArguments(
-                                    it,
-                                    useContext,
-                                    pluginContext
-                                )
-                            }
-                        }
-                    }
+                    else -> makeGenericSubstitutionFunction(c, argsIncludingOuterClasses)
                 }
 
             c.declarations.map {
@@ -511,12 +489,12 @@ open class KotlinFileExtractor(
         return FieldResult(instanceId, instanceName)
     }
 
-    private fun extractValueParameter(vp: IrValueParameter, parent: Label<out DbCallable>, idx: Int, typeSubstitution: TypeSubstitution?, parentSourceDeclaration: Label<out DbCallable>, classTypeArgsIncludingOuterClasses: List<IrTypeArgument>?, extractTypeAccess: Boolean): TypeResults {
+    private fun extractValueParameter(vp: IrValueParameter, parent: Label<out DbCallable>, idx: Int, typeSubstitution: TypeSubstitution?, parentSourceDeclaration: Label<out DbCallable>, classTypeArgsIncludingOuterClasses: List<IrTypeArgument>?, extractTypeAccess: Boolean, locOverride: Label<DbLocation>? = null): TypeResults {
         with("value parameter", vp) {
-            val location = getLocation(vp, classTypeArgsIncludingOuterClasses)
+            val location = locOverride ?: getLocation(vp, classTypeArgsIncludingOuterClasses)
             val id = useValueParameter(vp, parent)
             if (extractTypeAccess) {
-                extractTypeAccessRecursive(vp.type, location, id, -1)
+                extractTypeAccessRecursive(typeSubstitution?.let { it(vp.type, TypeContext.OTHER, pluginContext) } ?: vp.type, location, id, -1)
             }
             return extractValueParameter(id, vp.type, vp.name.asString(), location, parent, idx, typeSubstitution, useValueParameter(vp, parentSourceDeclaration), vp.isVararg)
         }
@@ -676,7 +654,7 @@ open class KotlinFileExtractor(
         }
     }
 
-    fun extractFunction(f: IrFunction, parentId: Label<out DbReftype>, extractBody: Boolean, extractMethodAndParameterTypeAccesses: Boolean, typeSubstitution: TypeSubstitution?, classTypeArgsIncludingOuterClasses: List<IrTypeArgument>?, idOverride: Label<DbMethod>? = null): Label<out DbCallable>? {
+    fun extractFunction(f: IrFunction, parentId: Label<out DbReftype>, extractBody: Boolean, extractMethodAndParameterTypeAccesses: Boolean, typeSubstitution: TypeSubstitution?, classTypeArgsIncludingOuterClasses: List<IrTypeArgument>?, idOverride: Label<DbMethod>? = null, locOverride: Label<DbLocation>? = null): Label<out DbCallable>? {
         if (isFake(f)) return null
 
         with("function", f) {
@@ -694,21 +672,21 @@ open class KotlinFileExtractor(
                             useFunction<DbCallable>(f, parentId, classTypeArgsIncludingOuterClasses, noReplace = true)
 
                 val sourceDeclaration =
-                    if (typeSubstitution != null)
+                    if (typeSubstitution != null && idOverride == null)
                         useFunction(f)
                     else
                         id
 
                 val extReceiver = f.extensionReceiverParameter
                 val idxOffset = if (extReceiver != null) 1 else 0
                 val paramTypes = f.valueParameters.mapIndexed { i, vp ->
-                    extractValueParameter(vp, id, i + idxOffset, typeSubstitution, sourceDeclaration, classTypeArgsIncludingOuterClasses, extractTypeAccess = extractMethodAndParameterTypeAccesses)
+                    extractValueParameter(vp, id, i + idxOffset, typeSubstitution, sourceDeclaration, classTypeArgsIncludingOuterClasses, extractTypeAccess = extractMethodAndParameterTypeAccesses, locOverride)
                 }
                 val allParamTypes = if (extReceiver != null) {
                     val extendedType = useType(extReceiver.type)
                     tw.writeKtExtensionFunctions(id.cast<DbMethod>(), extendedType.javaResult.id, extendedType.kotlinResult.id)
 
-                    val t = extractValueParameter(extReceiver, id, 0, null, sourceDeclaration, classTypeArgsIncludingOuterClasses, extractTypeAccess = extractMethodAndParameterTypeAccesses)
+                    val t = extractValueParameter(extReceiver, id, 0, null, sourceDeclaration, classTypeArgsIncludingOuterClasses, extractTypeAccess = extractMethodAndParameterTypeAccesses, locOverride)
                     listOf(t) + paramTypes
                 } else {
                     paramTypes
@@ -718,7 +696,7 @@ open class KotlinFileExtractor(
 
                 val substReturnType = typeSubstitution?.let { it(f.returnType, TypeContext.RETURN, pluginContext) } ?: f.returnType
 
-                val locId = getLocation(f, classTypeArgsIncludingOuterClasses)
+                val locId = locOverride ?: getLocation(f, classTypeArgsIncludingOuterClasses)
 
                 if (f.symbol is IrConstructorSymbol) {
                     val unitType = useType(pluginContext.irBuiltIns.unitType, TypeContext.RETURN)
@@ -738,7 +716,7 @@ open class KotlinFileExtractor(
                     tw.writeMethodsKotlinType(methodId, returnType.kotlinResult.id)
 
                     if (extractMethodAndParameterTypeAccesses) {
-                        extractTypeAccessRecursive(f.returnType, locId, id, -1)
+                        extractTypeAccessRecursive(substReturnType, locId, id, -1)
                     }
 
                     if (shortName.nameInDB != shortName.kotlinName) {
@@ -4014,7 +3992,12 @@ open class KotlinFileExtractor(
                     helper.extractParameterToFieldAssignmentInConstructor("<fn>", functionType, fieldId, 0, 1)
 
                     // add implementation function
-                    extractFunction(samMember, classId, extractBody = false, extractMethodAndParameterTypeAccesses = true, null, null, ids.function)
+                    val classTypeArgs = (e.type as? IrSimpleType)?.arguments
+                    val typeSub = classTypeArgs?.let { makeGenericSubstitutionFunction(typeOwner, it) }
+
+                    fun trySub(t: IrType, context: TypeContext) = if (typeSub == null) t else typeSub(t, context, pluginContext)
+
+                    extractFunction(samMember, classId, extractBody = false, extractMethodAndParameterTypeAccesses = true, typeSub, classTypeArgs, idOverride = ids.function, locOverride = tw.getLocation(e))
 
                     //body
                     val blockId = tw.getFreshIdLabel<DbBlock>()
@@ -4037,7 +4020,7 @@ open class KotlinFileExtractor(
 
                     // Call to original `invoke`:
                     val callId = tw.getFreshIdLabel<DbMethodaccess>()
-                    val callType = useType(samMember.returnType)
+                    val callType = useType(trySub(samMember.returnType, TypeContext.RETURN))
                     tw.writeExprs_methodaccess(callId, callType.javaResult.id, returnId, 0)
                     tw.writeExprsKotlinType(callId, callType.kotlinResult.id)
                     extractCommonExpr(callId)
@@ -4061,7 +4044,7 @@ open class KotlinFileExtractor(
 
                     fun extractArgument(p: IrValueParameter, idx: Int, parent: Label<out DbExprparent>) {
                         val argsAccessId = tw.getFreshIdLabel<DbVaraccess>()
-                        val paramType = useType(p.type)
+                        val paramType = useType(trySub(p.type, TypeContext.OTHER))
                         tw.writeExprs_varaccess(argsAccessId, paramType.javaResult.id, parent, idx)
                         tw.writeExprsKotlinType(argsAccessId, paramType.kotlinResult.id)
                         extractCommonExpr(argsAccessId)

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

@@ -215,6 +215,17 @@ open class KotlinUsesExtractor(
     fun makeTypeGenericSubstitutionMap(c: IrClass, argsIncludingOuterClasses: List<IrTypeArgument>) =
         getTypeParametersInScope(c).map({ it.symbol }).zip(argsIncludingOuterClasses.map { it.withQuestionMark(true) }).toMap()
 
+    fun makeGenericSubstitutionFunction(c: IrClass, argsIncludingOuterClasses: List<IrTypeArgument>) =
+        makeTypeGenericSubstitutionMap(c, argsIncludingOuterClasses).let {
+            { x: IrType, useContext: TypeContext, pluginContext: IrPluginContext ->
+                x.substituteTypeAndArguments(
+                    it,
+                    useContext,
+                    pluginContext
+                )
+            }
+        }
+
     // The Kotlin compiler internal representation of Outer<A, B>.Inner<C, D>.InnerInner<E, F>.someFunction<G, H>.LocalClass<I, J> is LocalClass<I, J, G, H, E, F, C, D, A, B>. This function returns [A, B, C, D, E, F, G, H, I, J].
     fun orderTypeArgsLeftToRight(c: IrClass, argsIncludingOuterClasses: List<IrTypeArgument>?): List<IrTypeArgument>? {
         if(argsIncludingOuterClasses.isNullOrEmpty())
@@ -242,10 +253,12 @@ open class KotlinUsesExtractor(
         val extractClass = substituteClass ?: c
 
         // `KFunction1<T1,T2>` is substituted by `KFunction<T>`. The last type argument is the return type.
+        // Similarly Function23 and above get replaced by kotlin.jvm.functions.FunctionN with only one type arg, the result type.
         // References to SomeGeneric<T1, T2, ...> where SomeGeneric is declared SomeGeneric<T1, T2, ...> are extracted
         // as if they were references to the unbound type SomeGeneric.
         val extractedTypeArgs = when {
-            c.symbol.isKFunction() && typeArgs != null && typeArgs.isNotEmpty() -> listOf(typeArgs.last())
+            extractClass.symbol.isKFunction() && typeArgs != null && typeArgs.isNotEmpty() -> listOf(typeArgs.last())
+            extractClass.fqNameWhenAvailable == FqName("kotlin.jvm.functions.FunctionN") && typeArgs != null && typeArgs.isNotEmpty() -> listOf(typeArgs.last())
             typeArgs != null && isUnspecialised(c, typeArgs) -> listOf()
             else -> typeArgs
         }
@@ -1083,8 +1096,21 @@ open class KotlinUsesExtractor(
         return classTypeResult.id
     }
 
+    fun getTypeParameterParentLabel(param: IrTypeParameter) =
+        param.parent.let {
+            when (it) {
+                is IrClass -> useClassSource(it)
+                is IrFunction -> useFunction(it, noReplace = true)
+                else -> { logger.error("Unexpected type parameter parent $it"); null }
+            }
+        }
+
     fun getTypeParameterLabel(param: IrTypeParameter): String {
-        val parentLabel = useDeclarationParent(param.parent, false)
+        // Use this instead of `useDeclarationParent` so we can use useFunction with noReplace = true,
+        // ensuring that e.g. a method-scoped type variable declared on kotlin.String.transform <R> gets
+        // a different name to the corresponding java.lang.String.transform <R>, even though useFunction
+        // will usually replace references to one function with the other.
+        val parentLabel = getTypeParameterParentLabel(param)
         return "@\"typevar;{$parentLabel};${param.name}\""
     }
 

java/ql/consistency-queries/typeParametersInScope.ql

@@ -1,11 +1,15 @@
 import java
 
+class InstantiatedType extends ParameterizedType {
+  InstantiatedType() { typeArgs(_, _, this) }
+}
+
 Type getAMentionedType(RefType type) {
   result = type
   or
   result = getAMentionedType(type).(Array).getElementType()
   or
-  result = getAMentionedType(type).(ParameterizedType).getATypeArgument()
+  result = getAMentionedType(type).(InstantiatedType).getATypeArgument()
   or
   result = getAMentionedType(type).(NestedType).getEnclosingType()
 }
@@ -24,17 +28,26 @@ Type getATypeUsedInClass(RefType type) {
   result = getAMentionedType(getATypeUsedInClass(type))
 }
 
-TypeVariable getATypeVariableInScope(RefType type) {
-  result = type.getACallable().(GenericCallable).getATypeParameter()
-  or
-  result = type.(GenericType).getATypeParameter()
+Element getEnclosingElementStar(RefType e) {
+  result = e
   or
-  result = getAMentionedType(type.(ParameterizedType).getATypeArgument())
-  or
-  result = getATypeVariableInScope(type.getEnclosingType())
+  result.contains(e)
+}
+
+TypeVariable getATypeVariableInScope(RefType type) {
+  exists(Element e | e = getEnclosingElementStar(type) |
+    result = e.(RefType).getACallable().(GenericCallable).getATypeParameter()
+    or
+    result = e.(GenericType).getATypeParameter()
+    or
+    result = e.(GenericCallable).getATypeParameter()
+    or
+    result = getAMentionedType(e.(InstantiatedType).getATypeArgument())
+  )
 }
 
 from ClassOrInterface typeUser, TypeVariable outOfScope
 where
-  outOfScope = getAMentionedType(typeUser) and not outOfScope = getATypeVariableInScope(typeUser)
-select "Type " + typeUser + " uses out-of-scope type variable " + outOfScope
+  outOfScope = getATypeUsedInClass(typeUser) and not outOfScope = getATypeVariableInScope(typeUser)
+select "Type " + typeUser + " uses out-of-scope type variable " + outOfScope +
+    ". Note the Java extractor is known to sometimes do this; the Kotlin extractor should not."

java/ql/test/kotlin/library-tests/dataflow/func/util.kt

@@ -1,23 +1,23 @@
 class Processor {
-    fun <R> process(f: () -> R) : R {
+    fun <R1> process(f: () -> R1) : R1 {
         return f()
     }
 
-    fun <T, R> process(f: (T) -> R, arg: T) : R {
+    fun <T, R2> process(f: (T) -> R2, arg: T) : R2 {
         return f(arg)
     }
 
-    fun <T0, T1, R> process(f: (T0, T1) -> R, arg0: T0, arg1: T1) : R {
+    fun <T0, T1, R3> process(f: (T0, T1) -> R3, arg0: T0, arg1: T1) : R3 {
         return f(arg0, arg1)
     }
 
-    fun <T, R> process(
-        f: (T,T,T,T,T,T,T,T,T,T,T,T,T,T,T,T,T,T,T,T,T,T,T,T) -> R,
-        a: T, b: T) : R {
+    fun <T, R4> process(
+        f: (T,T,T,T,T,T,T,T,T,T,T,T,T,T,T,T,T,T,T,T,T,T,T,T) -> R4,
+        a: T, b: T) : R4 {
         return f(a,b,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a)
     }
 
-    fun <T, R> processExt(f: T.(T) -> R, ext: T, arg: T) : R {
+    fun <T, R5> processExt(f: T.(T) -> R5, ext: T, arg: T) : R5 {
         return ext.f(arg)
     }
 }