[Xamarin.Android.Tools.Bytecode] Improve Kotlin metadata matching (#946)

Fixes: #945
Context: dotnet/android-libraries#436 (comment)
Consider the following Kotlin function:
fun get (index: Int) : UInt { … }
When `get` is compiled with Kotlin 1.5, `class-parse` would emit:
<method abstract="false" final="true" name="get-pVg5ArA" return="uint" jni-return="I" static="true" visibility="public" jni-signature="([II)I">
<parameter name="$this" type="int[]" jni-type="[I" />
<parameter name="index" type="int" jni-type="I" />
</method>
However, when `get` is compiled with Kotlin 1.6, `class-parse` sees:
<method abstract="false" final="true" name="get-pVg5ArA" return="int" jni-return="I" static="true" visibility="public" jni-signature="([II)I">
<parameter name="arg0" type="int[]" jni-type="[I" />
<parameter name="index" type="int" jni-type="I" />
</method>
Note that the name of the first `int[]` parameter changes from
`$this` to `arg0`, and the return type changed from `uint` to `int`.
The parameter name change is annoying; the return type change is
an ABI break, and makes many package updates impossible.
What happened?
Recall commit 71afce5: Kotlin-compiled `.class` files contain a
`kotlin.Metadata` type-level annotation blob, which contains a
Protobuf stream containing Kotlin metadata for the type.
The `kotlin.Metadata` annotation contains information about Kotlin
functions, and `class-parse` needs to associate the information about
the Kotlin functions to Java methods. This can be tricky because
there is not an explicit way to do this, and one must rely on trying
to match function names and parameter types. Previously, this was
accomplished by looping through the parameters and comparing types.
However, the `kotlin.Metadata` annotation only includes "user created"
method parameters, while the Java method may include "compiler created"
method parameters. We attempted to skip these by ignoring Java
parameters whose names begin with a dollar sign (ex `$this`).
This "worked" most of the time.
This broke in Kotlin 1.6, as the compiler generated method parameters
stopped using `$` for compiler-generated parameter names.
algorithm.
We need something better.
The `kKotlin.Metadata` annotation provides a `JvmSignature` property --
which matches the `JvmName` property which we already use -- which
sounds promising. However, `JvmSignature` is sometimes `null`, and
sometimes it doesn't actually match. But it's closer.
So now we try to match with signatures.
* If `JvmSignature` matches the Java signature we use that.
* If `JvmSignature` is `null`, calculate it from the Kotlin
parameters and try to match with that.
* Note it wants unsigned types as `Lkotlin/UInt;` and not the
primitive encoding (`I`).
* If Kotlin metadata defines a `ReceiverType`, add that as the
first parameter to the signature.
Using `kotlin-stdlib-1.5.31.jar` as a baseline, we get much better
results:
| Version | Matched Functions | Unmatched Functions |
| --------------- | ----------------: | --------------------: |
| main @ 32635fd | 946 | 154 |
| This PR | 1100 | 0 |
Now that we can match Kotlin functions to Java methods, we still have
to match up the parameters so we can apply parameter-level transforms.
Do this by removing Java method parameters from the front and back of
the list to account for cases where the compiler adds "hidden"
parameters:
* Remove Kotlin `ReceiverType` from beginning of list.
* Remove Kotlin `Lkotlin/coroutines/Continuation;` from end of list.
* Note this _could_ be a legitimate user supplied parameter so we
try to restrict it to "unnamed" parameters.
* Remove the `this` parameter added to `static` functions.
Note: the test classes `DeepRecursiveKt.class`,
`DeepRecursiveScope.class`, and `UByteArray.class` are pulled from
[`kotlin-stdlib-1.5.31.jar`][0].
[0]: https://mvnrepository.com/artifact/org.jetbrains.kotlin/kotlin-stdlib/1.5.31

Author: jpobst

src/Xamarin.Android.Tools.Bytecode/Kotlin/KotlinClassMetadata.cs

@@ -358,9 +358,9 @@ public class KotlinType
 };
 }
 
-publicstringGetSignature()
+publicstringGetSignature(boolconvertUnsignedToPrimitive=true)
 {
-returnKotlinUtilities.ConvertKotlinTypeSignature(this);
+returnKotlinUtilities.ConvertKotlinTypeSignature(this,null,convertUnsignedToPrimitive);
 }
 }
 

src/Xamarin.Android.Tools.Bytecode/Kotlin/KotlinFixups.cs

@@ -175,10 +175,12 @@ static void FixupFunction (MethodInfo? method, KotlinFunction metadata, KotlinCl
 return;
 }
 
-varjava_parameters=method.GetFilteredParameters();
+(varstart,varend)=CreateParameterMap(method,metadata,kotlinClass);
 
-for(vari=0;i<java_parameters.Length;i++){
-varjava_p=java_parameters[i];
+varjava_parameters=method.GetParameters();
+
+for(vari=0;i<end-start;i++){
+varjava_p=java_parameters[start+i];
 varkotlin_p=metadata.ValueParameters==null?null:metadata.ValueParameters[i];
 if(kotlin_p==null||kotlin_p.Type==null||kotlin_p.Name==null)
 continue;
@@ -197,6 +199,46 @@ static void FixupFunction (MethodInfo? method, KotlinFunction metadata, KotlinCl
 method.KotlinReturnType=GetKotlinType(method.ReturnType.TypeSignature,metadata.ReturnType?.ClassName);
 }
 
+publicstatic(intstart,intend)CreateParameterMap(MethodInfomethod,KotlinFunctionfunction,KotlinClass?kotlinClass)
+{
+varparameters=method.GetParameters();
+varstart=0;
+varend=parameters.Length;
+
+// If the parameter counts are the same, that's good enough (because we know signatures matched)
+if(IsValidParameterMap(method,function,start,end))
+return(start,end);
+
+// Remove the "hidden" receiver type parameter from the start of the parameter list
+if(function.ReceiverType!=null)
+start++;
+
+if(IsValidParameterMap(method,function,start,end))
+return(start,end);
+
+varlast_p=parameters.Last();
+
+// Remove the "hidden" coroutine continuation type parameter from the end of the parameter list
+// We try to restrict it to compiler generated paramteres because a user might have actually used it as a parameter
+if(last_p.Type.BinaryName=="Lkotlin/coroutines/Continuation;"&&(last_p.IsUnnamedParameter()||last_p.IsCompilerNamed()))
+end--;
+
+if(IsValidParameterMap(method,function,start,end))
+return(start,end);
+
+// Remove the "hidden" "this" type parameter for a static method from the start of the parameter list
+// Note we do this last because sometimes it isn't there.
+if(method.AccessFlags.HasFlag(MethodAccessFlags.Static))
+start++;
+
+if(IsValidParameterMap(method,function,start,end))
+return(start,end);
+
+return(0,0);
+}
+
+staticboolIsValidParameterMap(MethodInfomethod,KotlinFunctionfunction,intstart,intend)=>function.ValueParameters?.Count==end-start;
+
 staticvoidFixupExtensionMethod(MethodInfomethod)
 {
 // Kotlin "extension" methods give the first parameter an ugly name
@@ -271,10 +313,23 @@ static void FixupField (FieldInfo? field, KotlinProperty metadata)
 
 staticMethodInfo?FindJavaMethod(KotlinFilekotlinFile,KotlinFunctionfunction,ClassFileklass)
 {
-varpossible_methods=klass.Methods.Where(method =>method.Name==function.JvmName&&
-method.GetFilteredParameters().Length==function.ValueParameters?.Count);
+varpossible_methods=klass.Methods.Where(method =>method.Name==function.JvmName).ToArray();
+varsignature=function.JvmSignature;
+
+// If the Descriptor/Signature match, that means all parameters and return type match
+if(signature!=null&&possible_methods.SingleOrDefault(method =>method.Descriptor==signature)isMethodInfom)
+returnm;
+
+// Sometimes JvmSignature is null (or unhelpful), so we're going to construct one ourselves and see if they match
+signature=function.ConstructJvmSignature();
 
-foreach(varmethodinpossible_methods){
+if(possible_methods.SingleOrDefault(method =>method.Descriptor==signature)isMethodInfom2)
+returnm2;
+
+// If that didn't work, let's do it the hard way!
+// I don't know if this catches anything additional, but it was the original code we shipped, so
+// we'll keep it just in case something in the wild requires it.
+foreach(varmethodinpossible_methods.Where(method =>method.GetFilteredParameters().Length==function.ValueParameters?.Count)){
 if(function.ReturnType==null)
 continue;
 if(!TypesMatch(method.ReturnType,function.ReturnType,kotlinFile))
@@ -286,6 +341,10 @@ static void FixupField (FieldInfo? field, KotlinProperty metadata)
 returnmethod;
 }
 
+// Theoretically this should never be hit, but who knows. At worst, it just means
+// Kotlin niceties won't be applied to the method.
+Log.Debug($"Kotlin: Could not find Java method to match '{function.Name} ({function.ConstructJvmSignature()})'");
+
 returnnull;
 }
 

src/Xamarin.Android.Tools.Bytecode/Kotlin/KotlinUtilities.cs

@@ -8,7 +8,7 @@ namespace Xamarin.Android.Tools.Bytecode
 {
 publicstaticclassKotlinUtilities
 {
-publicstaticstringConvertKotlinTypeSignature(KotlinType?type,KotlinFile?metadata=null)
+publicstaticstringConvertKotlinTypeSignature(KotlinType?type,KotlinFile?metadata=null,boolconvertUnsignedToPrimitive=true)
 {
 if(typeisnull)
 returnstring.Empty;
@@ -27,15 +27,15 @@ public static string ConvertKotlinTypeSignature (KotlinType? type, KotlinFile? m
 return"Ljava/lang/Object;";
 }
 
-varresult=ConvertKotlinClassToJava(class_name);
+varresult=ConvertKotlinClassToJava(class_name,convertUnsignedToPrimitive);
 
 if(result=="[")
-result+=ConvertKotlinTypeSignature(type.Arguments?.FirstOrDefault()?.Type);
+result+=ConvertKotlinTypeSignature(type.Arguments?.FirstOrDefault()?.Type,null,convertUnsignedToPrimitive);
 
 returnresult;
 }
 
-publicstaticstringConvertKotlinClassToJava(string?className)
+publicstaticstringConvertKotlinClassToJava(string?className,boolconvertUnsignedToPrimitive=true)
 {
 if(className==null||string.IsNullOrWhiteSpace(className))
 returnstring.Empty;
@@ -45,6 +45,9 @@ public static string ConvertKotlinClassToJava (string? className)
 if(type_map.TryGetValue(className.TrimEnd(';'),outvarresult))
 returnresult;
 
+if(convertUnsignedToPrimitive&&unsigned_type_map.TryGetValue(className.TrimEnd(';'),outvarresult2))
+returnresult2;
+
 return"L"+className;
 }
 
@@ -92,6 +95,14 @@ public static bool IsDefaultConstructorMarker (this MethodInfo method)
 parameters[parameters.Length-1].Type.TypeSignature=="Lkotlin/jvm/internal/DefaultConstructorMarker;";
 }
 
+// Sometimes the Kotlin provided JvmSignature is null (or unhelpful), so we need to construct one ourselves
+publicstaticstringConstructJvmSignature(thisKotlinFunctionfunction)
+{
+// The receiver type (if specified) is a "hidden" parameter passed at the beginning
+// of the Java parameter list, so we include it so the Signature/Descriptors match.
+return$"({function.ReceiverType?.GetSignature(false)}{string.Concat(function.ValueParameters?.Select(p =>p.Type?.GetSignature(false))??Enumerable.Empty<string>())}){function.ReturnType?.GetSignature(false)}";
+}
+
 internalstaticList<TResult>?ToList<TSource,TResult>(thisIEnumerable<TSource>?self,JvmNameResolverresolver,Func<TSource,JvmNameResolver,TResult?>creator)
 whereTResult:class
 {
@@ -114,37 +125,42 @@ public static bool IsDefaultConstructorMarker (this MethodInfo method)
 
 publicstaticboolIsUnnamedParameter(thisParameterInfoparameter)=>parameter.Name.Length>1&&parameter.Name.StartsWith("p",StringComparison.Ordinal)&&int.TryParse(parameter.Name.Substring(1),outvar_);
 
+publicstaticboolIsCompilerNamed(thisParameterInfoparameter)=>parameter.Name.Length>0&&parameter.Name.StartsWith("$",StringComparison.Ordinal);
+
 publicstaticboolIsUnnamedParameter(thisKotlinValueParameterparameter)=>parameter.Name?.Length>1&&
 parameter.Name.StartsWith("p",StringComparison.Ordinal)&&
 int.TryParse(parameter.Name.Substring(1),outvar_);
 
+staticDictionary<string,string>unsigned_type_map=newDictionary<string,string>{
+{"kotlin/UInt","I"},
+{"kotlin/ULong","J"},
+{"kotlin/UShort","S"},
+{"kotlin/UByte","B"},
+{"kotlin/UIntArray","[I"},
+{"kotlin/ULongArray","[J"},
+{"kotlin/UShortArray","[S"},
+{"kotlin/UByteArray","[B"},
+};
+
 staticDictionary<string,string>type_map=newDictionary<string,string>{
 {"kotlin/Int","I"},
-{"kotlin/UInt","I"},
 {"kotlin/Double","D"},
 {"kotlin/Char","C"},
 {"kotlin/Long","J"},
-{"kotlin/ULong","J"},
 {"kotlin/Float","F"},
 {"kotlin/Short","S"},
-{"kotlin/UShort","S"},
 {"kotlin/Byte","B"},
-{"kotlin/UByte","B"},
 {"kotlin/Boolean","Z"},
 {"kotlin/Unit","V"},
 
 {"kotlin/Array","["},
 {"kotlin/IntArray","[I"},
-{"kotlin/UIntArray","[I"},
 {"kotlin/DoubleArray","[D"},
 {"kotlin/CharArray","[C"},
 {"kotlin/LongArray","[J"},
-{"kotlin/ULongArray","[J"},
 {"kotlin/FloatArray","[F"},
 {"kotlin/ShortArray","[S"},
-{"kotlin/UShortArray","[S"},
 {"kotlin/ByteArray","[B"},
-{"kotlin/UByteArray","[B"},
 {"kotlin/BooleanArray","[Z"},
 
 {"kotlin/Any","Ljava/lang/Object;"},

tests/Xamarin.Android.Tools.Bytecode-Tests/ClassFileFixture.cs

@@ -105,6 +105,46 @@ protected static void AssertXmlDeclaration (string[] classResources, string xmlR
 Assert.AreEqual(expected,actual.ToString());
 }
 
+protectedstaticKotlinMetadataGetMetadataForClass(ClassFileklass)
+{
+varattr=klass.Attributes.OfType<RuntimeVisibleAnnotationsAttribute>().FirstOrDefault();
+varkotlin=attr?.Annotations.FirstOrDefault(a =>a.Type=="Lkotlin/Metadata;");
+
+Assert.NotNull(kotlin);
+
+varmeta=KotlinMetadata.FromAnnotation(kotlin);
+
+returnmeta;
+}
+
+protectedstaticKotlinClassGetClassMetadataForClass(ClassFileklass)
+{
+varmeta=GetMetadataForClass(klass);
+Assert.AreEqual(KotlinMetadataKind.Class,meta.Kind);
+
+returnmeta.AsClassMetadata();
+}
+
+protectedstaticKotlinFileGetFileMetadataForClass(ClassFileklass)
+{
+varmeta=GetMetadataForClass(klass);
+Assert.AreEqual(KotlinMetadataKind.File,meta.Kind);
+
+returnmeta.AsFileMetadata();
+}
+
+protectedstaticKotlinMetadataGetMetadataForClassFile(stringfile)
+{
+varc=LoadClassFile(file);
+returnGetMetadataForClass(c);
+}
+
+protectedstaticKotlinClassGetClassMetadata(stringfile)
+{
+varc=LoadClassFile(file);
+returnGetClassMetadataForClass(c);
+}
+
 staticStreamGetResourceStream(stringresource)
 {
 // Look for resources that end with our name, this allows us to

tests/Xamarin.Android.Tools.Bytecode-Tests/KotlinFixupsTests.cs

@@ -310,5 +310,53 @@ public void HandleKotlinNameShadowing ()
 Assert.True(klass.Methods.Single(m =>m.Name=="hitCount").AccessFlags.HasFlag(MethodAccessFlags.Public));
 Assert.True(klass.Methods.Single(m =>m.Name=="setType").AccessFlags.HasFlag(MethodAccessFlags.Public));
 }
+
+[Test]
+publicvoidMatchParametersWithReceiver()
+{
+varklass=LoadClassFile("DeepRecursiveKt.class");
+varmeta=GetFileMetadataForClass(klass);
+
+varjava_method=klass.Methods.Single(m =>m.Name=="invoke");
+varkotlin_function=meta.Functions.Single(m =>m.Name=="invoke");
+
+(varstart,varend)=KotlinFixups.CreateParameterMap(java_method,kotlin_function,null);
+
+// Start is 1 instead of 0 to skip the receiver
+Assert.AreEqual(1,start);
+Assert.AreEqual(2,end);
+}
+
+[Test]
+publicvoidMatchParametersWithContinuation()
+{
+varklass=LoadClassFile("DeepRecursiveScope.class");
+varmeta=GetClassMetadataForClass(klass);
+
+varjava_method=klass.Methods.Single(m =>m.Name=="callRecursive"&&m.GetParameters().Count()==2);
+varkotlin_function=meta.Functions.Single(m =>m.Name=="callRecursive"&&m.JvmSignature.Count(c =>c==';')==3);
+
+(varstart,varend)=KotlinFixups.CreateParameterMap(java_method,kotlin_function,null);
+
+// End is 1 instead of 2 to skip the trailing continuation
+Assert.AreEqual(0,start);
+Assert.AreEqual(1,end);
+}
+
+[Test]
+publicvoidMatchParametersWithStaticThis()
+{
+varklass=LoadClassFile("UByteArray.class");
+varmeta=GetClassMetadataForClass(klass);
+
+varjava_method=klass.Methods.Single(m =>m.Name=="contains-7apg3OU"&&m.GetParameters().Count()==2);
+varkotlin_function=meta.Functions.Single(m =>m.Name=="contains");
+
+(varstart,varend)=KotlinFixups.CreateParameterMap(java_method,kotlin_function,null);
+
+// Start is 1 instead of 0 to skip the static 'this'
+Assert.AreEqual(1,start);
+Assert.AreEqual(2,end);
+}
 }
 }

tests/Xamarin.Android.Tools.Bytecode-Tests/KotlinMetadataTests.cs

@@ -201,31 +201,6 @@ public void KotlinExtensionMethods ()
 Assert.AreEqual(KotlinClassInheritability.Final,klass_meta.Inheritability);
 Assert.AreEqual(KotlinClassVisibility.Public,klass_meta.Visibility);
 }
-
-privateKotlinMetadataGetMetadataForClassFile(stringfile)
-{
-varc=LoadClassFile(file);
-varattr=c.Attributes.OfType<RuntimeVisibleAnnotationsAttribute>().FirstOrDefault();
-varkotlin=attr?.Annotations.FirstOrDefault(a =>a.Type=="Lkotlin/Metadata;");
-
-Assert.NotNull(kotlin);
-
-varmeta=KotlinMetadata.FromAnnotation(kotlin);
-
-returnmeta;
-}
-
-privateKotlinClassGetClassMetadata(stringfile)
-{
-varmeta=GetMetadataForClassFile(file);
-
-Assert.AreEqual(KotlinMetadataKind.Class,meta.Kind);
-
-Assert.AreEqual("1.0.3",meta.ByteCodeVersion.ToString());
-Assert.AreEqual("1.1.15",meta.MetadataVersion.ToString());
-
-returnmeta.AsClassMetadata();
-}
 }
 }
 

tests/Xamarin.Android.Tools.Bytecode-Tests/Xamarin.Android.Tools.Bytecode-Tests.csproj

@@ -25,7 +25,7 @@
 
  <ItemGroup>
  <EmbeddedResourceInclude="Resources\*" />
- <EmbeddedResourceInclude="kotlin\**\*.class" />
+ <EmbeddedResourceInclude="kotlin*\**\*.class" />
 
  <EmbeddedResourceInclude="$(IntermediateOutputPath)classes\com\xamarin\NotNullClass.class" />
  <EmbeddedResourceInclude="$(IntermediateOutputPath)classes\com\xamarin\IJavaInterface.class" />