Skip to content

Commit f91b077

Browse files
jpobstjpobst
authored
[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
1 parent 32635fd commit f91b077

File tree

11 files changed

+185
-46
lines changed

11 files changed

+185
-46
lines changed

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -358,9 +358,9 @@ public class KotlinType
358358
};
359359
}
360360

361-
public string GetSignature ()
361+
public string GetSignature (bool convertUnsignedToPrimitive = true)
362362
{
363-
return KotlinUtilities.ConvertKotlinTypeSignature (this);
363+
return KotlinUtilities.ConvertKotlinTypeSignature (this, null, convertUnsignedToPrimitive);
364364
}
365365
}
366366

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

Lines changed: 65 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -175,10 +175,12 @@ static void FixupFunction (MethodInfo? method, KotlinFunction metadata, KotlinCl
175175
return;
176176
}
177177

178-
var java_parameters = method.GetFilteredParameters ();
178+
(var start, var end) = CreateParameterMap (method, metadata, kotlinClass);
179179

180-
for (var i = 0; i < java_parameters.Length; i++) {
181-
var java_p = java_parameters [i];
180+
var java_parameters = method.GetParameters ();
181+
182+
for (var i = 0; i < end - start; i++) {
183+
var java_p = java_parameters [start + i];
182184
var kotlin_p = metadata.ValueParameters == null ? null : metadata.ValueParameters [i];
183185
if (kotlin_p == null || kotlin_p.Type == null || kotlin_p.Name == null)
184186
continue;
@@ -197,6 +199,46 @@ static void FixupFunction (MethodInfo? method, KotlinFunction metadata, KotlinCl
197199
method.KotlinReturnType = GetKotlinType (method.ReturnType.TypeSignature, metadata.ReturnType?.ClassName);
198200
}
199201

202+
public static (int start, int end) CreateParameterMap (MethodInfo method, KotlinFunction function, KotlinClass? kotlinClass)
203+
{
204+
var parameters = method.GetParameters ();
205+
var start = 0;
206+
var end = parameters.Length;
207+
208+
// If the parameter counts are the same, that's good enough (because we know signatures matched)
209+
if (IsValidParameterMap (method, function, start, end))
210+
return (start, end);
211+
212+
// Remove the "hidden" receiver type parameter from the start of the parameter list
213+
if (function.ReceiverType != null)
214+
start++;
215+
216+
if (IsValidParameterMap (method, function, start, end))
217+
return (start, end);
218+
219+
var last_p = parameters.Last ();
220+
221+
// Remove the "hidden" coroutine continuation type parameter from the end of the parameter list
222+
// We try to restrict it to compiler generated paramteres because a user might have actually used it as a parameter
223+
if (last_p.Type.BinaryName == "Lkotlin/coroutines/Continuation;" && (last_p.IsUnnamedParameter () || last_p.IsCompilerNamed ()))
224+
end--;
225+
226+
if (IsValidParameterMap (method, function, start, end))
227+
return (start, end);
228+
229+
// Remove the "hidden" "this" type parameter for a static method from the start of the parameter list
230+
// Note we do this last because sometimes it isn't there.
231+
if (method.AccessFlags.HasFlag (MethodAccessFlags.Static))
232+
start++;
233+
234+
if (IsValidParameterMap (method, function, start, end))
235+
return (start, end);
236+
237+
return (0, 0);
238+
}
239+
240+
static bool IsValidParameterMap (MethodInfo method, KotlinFunction function, int start, int end) => function.ValueParameters?.Count == end - start;
241+
200242
static void FixupExtensionMethod (MethodInfo method)
201243
{
202244
// Kotlin "extension" methods give the first parameter an ugly name
@@ -271,10 +313,23 @@ static void FixupField (FieldInfo? field, KotlinProperty metadata)
271313

272314
static MethodInfo? FindJavaMethod (KotlinFile kotlinFile, KotlinFunction function, ClassFile klass)
273315
{
274-
var possible_methods = klass.Methods.Where (method => method.Name == function.JvmName &&
275-
method.GetFilteredParameters ().Length == function.ValueParameters?.Count);
316+
var possible_methods = klass.Methods.Where (method => method.Name == function.JvmName).ToArray ();
317+
var signature = function.JvmSignature;
318+
319+
// If the Descriptor/Signature match, that means all parameters and return type match
320+
if (signature != null && possible_methods.SingleOrDefault (method => method.Descriptor == signature) is MethodInfo m)
321+
return m;
322+
323+
// Sometimes JvmSignature is null (or unhelpful), so we're going to construct one ourselves and see if they match
324+
signature = function.ConstructJvmSignature ();
276325

277-
foreach (var method in possible_methods) {
326+
if (possible_methods.SingleOrDefault (method => method.Descriptor == signature) is MethodInfo m2)
327+
return m2;
328+
329+
// If that didn't work, let's do it the hard way!
330+
// I don't know if this catches anything additional, but it was the original code we shipped, so
331+
// we'll keep it just in case something in the wild requires it.
332+
foreach (var method in possible_methods.Where (method => method.GetFilteredParameters ().Length == function.ValueParameters?.Count)) {
278333
if (function.ReturnType == null)
279334
continue;
280335
if (!TypesMatch (method.ReturnType, function.ReturnType, kotlinFile))
@@ -286,6 +341,10 @@ static void FixupField (FieldInfo? field, KotlinProperty metadata)
286341
return method;
287342
}
288343

344+
// Theoretically this should never be hit, but who knows. At worst, it just means
345+
// Kotlin niceties won't be applied to the method.
346+
Log.Debug ($"Kotlin: Could not find Java method to match '{function.Name} ({function.ConstructJvmSignature ()})'");
347+
289348
return null;
290349
}
291350

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

Lines changed: 28 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ namespace Xamarin.Android.Tools.Bytecode
88
{
99
public static class KotlinUtilities
1010
{
11-
public static string ConvertKotlinTypeSignature (KotlinType? type, KotlinFile? metadata = null)
11+
public static string ConvertKotlinTypeSignature (KotlinType? type, KotlinFile? metadata = null, bool convertUnsignedToPrimitive = true)
1212
{
1313
if (type is null)
1414
return string.Empty;
@@ -27,15 +27,15 @@ public static string ConvertKotlinTypeSignature (KotlinType? type, KotlinFile? m
2727
return "Ljava/lang/Object;";
2828
}
2929

30-
var result = ConvertKotlinClassToJava (class_name);
30+
var result = ConvertKotlinClassToJava (class_name, convertUnsignedToPrimitive);
3131

3232
if (result == "[")
33-
result += ConvertKotlinTypeSignature (type.Arguments?.FirstOrDefault ()?.Type);
33+
result += ConvertKotlinTypeSignature (type.Arguments?.FirstOrDefault ()?.Type, null, convertUnsignedToPrimitive);
3434

3535
return result;
3636
}
3737

38-
public static string ConvertKotlinClassToJava (string? className)
38+
public static string ConvertKotlinClassToJava (string? className, bool convertUnsignedToPrimitive = true)
3939
{
4040
if (className == null || string.IsNullOrWhiteSpace (className))
4141
return string.Empty;
@@ -45,6 +45,9 @@ public static string ConvertKotlinClassToJava (string? className)
4545
if (type_map.TryGetValue (className.TrimEnd (';'), out var result))
4646
return result;
4747

48+
if (convertUnsignedToPrimitive && unsigned_type_map.TryGetValue (className.TrimEnd (';'), out var result2))
49+
return result2;
50+
4851
return "L" + className;
4952
}
5053

@@ -92,6 +95,14 @@ public static bool IsDefaultConstructorMarker (this MethodInfo method)
9295
parameters [parameters.Length - 1].Type.TypeSignature == "Lkotlin/jvm/internal/DefaultConstructorMarker;";
9396
}
9497

98+
// Sometimes the Kotlin provided JvmSignature is null (or unhelpful), so we need to construct one ourselves
99+
public static string ConstructJvmSignature (this KotlinFunction function)
100+
{
101+
// The receiver type (if specified) is a "hidden" parameter passed at the beginning
102+
// of the Java parameter list, so we include it so the Signature/Descriptors match.
103+
return $"({function.ReceiverType?.GetSignature (false)}{string.Concat (function.ValueParameters?.Select (p => p.Type?.GetSignature (false)) ?? Enumerable.Empty<string> ())}){function.ReturnType?.GetSignature (false)}";
104+
}
105+
95106
internal static List<TResult>? ToList<TSource, TResult> (this IEnumerable<TSource>? self, JvmNameResolver resolver, Func<TSource, JvmNameResolver, TResult?> creator)
96107
where TResult: class
97108
{
@@ -114,37 +125,42 @@ public static bool IsDefaultConstructorMarker (this MethodInfo method)
114125

115126
public static bool IsUnnamedParameter (this ParameterInfo parameter) => parameter.Name.Length > 1 && parameter.Name.StartsWith ("p", StringComparison.Ordinal) && int.TryParse (parameter.Name.Substring (1), out var _);
116127

128+
public static bool IsCompilerNamed (this ParameterInfo parameter) => parameter.Name.Length > 0 && parameter.Name.StartsWith ("$", StringComparison.Ordinal);
129+
117130
public static bool IsUnnamedParameter (this KotlinValueParameter parameter) => parameter.Name?.Length > 1 &&
118131
parameter.Name.StartsWith ("p", StringComparison.Ordinal) &&
119132
int.TryParse (parameter.Name.Substring (1), out var _);
120133

134+
static Dictionary<string, string> unsigned_type_map = new Dictionary<string, string> {
135+
{ "kotlin/UInt", "I" },
136+
{ "kotlin/ULong", "J" },
137+
{ "kotlin/UShort", "S" },
138+
{ "kotlin/UByte", "B" },
139+
{ "kotlin/UIntArray", "[I" },
140+
{ "kotlin/ULongArray", "[J" },
141+
{ "kotlin/UShortArray", "[S" },
142+
{ "kotlin/UByteArray", "[B" },
143+
};
144+
121145
static Dictionary<string, string> type_map = new Dictionary<string, string> {
122146
{ "kotlin/Int", "I" },
123-
{ "kotlin/UInt", "I" },
124147
{ "kotlin/Double", "D" },
125148
{ "kotlin/Char", "C" },
126149
{ "kotlin/Long", "J" },
127-
{ "kotlin/ULong", "J" },
128150
{ "kotlin/Float", "F" },
129151
{ "kotlin/Short", "S" },
130-
{ "kotlin/UShort", "S" },
131152
{ "kotlin/Byte", "B" },
132-
{ "kotlin/UByte", "B" },
133153
{ "kotlin/Boolean", "Z" },
134154
{ "kotlin/Unit", "V" },
135155

136156
{ "kotlin/Array", "[" },
137157
{ "kotlin/IntArray", "[I" },
138-
{ "kotlin/UIntArray", "[I" },
139158
{ "kotlin/DoubleArray", "[D" },
140159
{ "kotlin/CharArray", "[C" },
141160
{ "kotlin/LongArray", "[J" },
142-
{ "kotlin/ULongArray", "[J" },
143161
{ "kotlin/FloatArray", "[F" },
144162
{ "kotlin/ShortArray", "[S" },
145-
{ "kotlin/UShortArray", "[S" },
146163
{ "kotlin/ByteArray", "[B" },
147-
{ "kotlin/UByteArray", "[B" },
148164
{ "kotlin/BooleanArray", "[Z" },
149165

150166
{ "kotlin/Any", "Ljava/lang/Object;" },

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

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,46 @@ protected static void AssertXmlDeclaration (string[] classResources, string xmlR
105105
Assert.AreEqual (expected, actual.ToString ());
106106
}
107107

108+
protected static KotlinMetadata GetMetadataForClass (ClassFile klass)
109+
{
110+
var attr = klass.Attributes.OfType<RuntimeVisibleAnnotationsAttribute> ().FirstOrDefault ();
111+
var kotlin = attr?.Annotations.FirstOrDefault (a => a.Type == "Lkotlin/Metadata;");
112+
113+
Assert.NotNull (kotlin);
114+
115+
var meta = KotlinMetadata.FromAnnotation (kotlin);
116+
117+
return meta;
118+
}
119+
120+
protected static KotlinClass GetClassMetadataForClass (ClassFile klass)
121+
{
122+
var meta = GetMetadataForClass (klass);
123+
Assert.AreEqual (KotlinMetadataKind.Class, meta.Kind);
124+
125+
return meta.AsClassMetadata ();
126+
}
127+
128+
protected static KotlinFile GetFileMetadataForClass (ClassFile klass)
129+
{
130+
var meta = GetMetadataForClass (klass);
131+
Assert.AreEqual (KotlinMetadataKind.File, meta.Kind);
132+
133+
return meta.AsFileMetadata ();
134+
}
135+
136+
protected static KotlinMetadata GetMetadataForClassFile (string file)
137+
{
138+
var c = LoadClassFile (file);
139+
return GetMetadataForClass (c);
140+
}
141+
142+
protected static KotlinClass GetClassMetadata (string file)
143+
{
144+
var c = LoadClassFile (file);
145+
return GetClassMetadataForClass (c);
146+
}
147+
108148
static Stream GetResourceStream (string resource)
109149
{
110150
// Look for resources that end with our name, this allows us to

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

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -310,5 +310,53 @@ public void HandleKotlinNameShadowing ()
310310
Assert.True (klass.Methods.Single (m => m.Name == "hitCount").AccessFlags.HasFlag (MethodAccessFlags.Public));
311311
Assert.True (klass.Methods.Single (m => m.Name == "setType").AccessFlags.HasFlag (MethodAccessFlags.Public));
312312
}
313+
314+
[Test]
315+
public void MatchParametersWithReceiver ()
316+
{
317+
var klass = LoadClassFile ("DeepRecursiveKt.class");
318+
var meta = GetFileMetadataForClass (klass);
319+
320+
var java_method = klass.Methods.Single (m => m.Name == "invoke");
321+
var kotlin_function = meta.Functions.Single (m => m.Name == "invoke");
322+
323+
(var start, var end) = KotlinFixups.CreateParameterMap (java_method, kotlin_function, null);
324+
325+
// Start is 1 instead of 0 to skip the receiver
326+
Assert.AreEqual (1, start);
327+
Assert.AreEqual (2, end);
328+
}
329+
330+
[Test]
331+
public void MatchParametersWithContinuation ()
332+
{
333+
var klass = LoadClassFile ("DeepRecursiveScope.class");
334+
var meta = GetClassMetadataForClass (klass);
335+
336+
var java_method = klass.Methods.Single (m => m.Name == "callRecursive" && m.GetParameters ().Count () == 2);
337+
var kotlin_function = meta.Functions.Single (m => m.Name == "callRecursive" && m.JvmSignature.Count (c => c == ';') == 3);
338+
339+
(var start, var end) = KotlinFixups.CreateParameterMap (java_method, kotlin_function, null);
340+
341+
// End is 1 instead of 2 to skip the trailing continuation
342+
Assert.AreEqual (0, start);
343+
Assert.AreEqual (1, end);
344+
}
345+
346+
[Test]
347+
public void MatchParametersWithStaticThis ()
348+
{
349+
var klass = LoadClassFile ("UByteArray.class");
350+
var meta = GetClassMetadataForClass (klass);
351+
352+
var java_method = klass.Methods.Single (m => m.Name == "contains-7apg3OU" && m.GetParameters ().Count () == 2);
353+
var kotlin_function = meta.Functions.Single (m => m.Name == "contains");
354+
355+
(var start, var end) = KotlinFixups.CreateParameterMap (java_method, kotlin_function, null);
356+
357+
// Start is 1 instead of 0 to skip the static 'this'
358+
Assert.AreEqual (1, start);
359+
Assert.AreEqual (2, end);
360+
}
313361
}
314362
}

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

Lines changed: 0 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -201,31 +201,6 @@ public void KotlinExtensionMethods ()
201201
Assert.AreEqual (KotlinClassInheritability.Final, klass_meta.Inheritability);
202202
Assert.AreEqual (KotlinClassVisibility.Public, klass_meta.Visibility);
203203
}
204-
205-
private KotlinMetadata GetMetadataForClassFile (string file)
206-
{
207-
var c = LoadClassFile (file);
208-
var attr = c.Attributes.OfType<RuntimeVisibleAnnotationsAttribute> ().FirstOrDefault ();
209-
var kotlin = attr?.Annotations.FirstOrDefault (a => a.Type == "Lkotlin/Metadata;");
210-
211-
Assert.NotNull (kotlin);
212-
213-
var meta = KotlinMetadata.FromAnnotation (kotlin);
214-
215-
return meta;
216-
}
217-
218-
private KotlinClass GetClassMetadata (string file)
219-
{
220-
var meta = GetMetadataForClassFile (file);
221-
222-
Assert.AreEqual (KotlinMetadataKind.Class, meta.Kind);
223-
224-
Assert.AreEqual ("1.0.3", meta.ByteCodeVersion.ToString ());
225-
Assert.AreEqual ("1.1.15", meta.MetadataVersion.ToString ());
226-
227-
return meta.AsClassMetadata ();
228-
}
229204
}
230205
}
231206

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@
2525

2626
<ItemGroup>
2727
<EmbeddedResource Include="Resources\*" />
28-
<EmbeddedResource Include="kotlin\**\*.class" />
28+
<EmbeddedResource Include="kotlin*\**\*.class" />
2929

3030
<EmbeddedResource Include="$(IntermediateOutputPath)classes\com\xamarin\NotNullClass.class" />
3131
<EmbeddedResource Include="$(IntermediateOutputPath)classes\com\xamarin\IJavaInterface.class" />

‎tests/Xamarin.Android.Tools.Bytecode-Tests/kotlin-ThirdParty/DeepRecursiveKt.class‎

2.32 KB
Binary file not shown.

‎tests/Xamarin.Android.Tools.Bytecode-Tests/kotlin-ThirdParty/DeepRecursiveScope.class‎

2.71 KB
Binary file not shown.

‎tests/Xamarin.Android.Tools.Bytecode-Tests/kotlin-ThirdParty/UByteArray.class‎

7 KB
Binary file not shown.

0 commit comments

Comments
 (0)