[Xamarin.Android.Tools.Bytecode] Improve Kotlin metadata matching.

Author: jpobst

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

@@ -358,9 +358,9 @@ public class KotlinType
 			};
 		}
 
-		public string GetSignature ()
+		public string GetSignature (bool convertUnsignedToPrimitive = true)
 		{
-			return KotlinUtilities.ConvertKotlinTypeSignature (this);
+			return KotlinUtilities.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;
 			}
 
-			var java_parameters = method.GetFilteredParameters ();
+			(var start, var end) = CreateParameterMap (method, metadata, kotlinClass);
 
-			for (var i = 0; i < java_parameters.Length; i++) {
-				var java_p = java_parameters [i];
+			var java_parameters = method.GetParameters ();
+
+			for (var i = 0; i < end - start; i++) {
+				var java_p = java_parameters [start + i];
 				var kotlin_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);
 		}
 
+		public static (int start, int end) CreateParameterMap (MethodInfo method, KotlinFunction function, KotlinClass? kotlinClass)
+		{
+			var parameters = method.GetParameters ();
+			var start = 0;
+			var end = 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);
+
+			var last_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);
+		}
+
+		static bool IsValidParameterMap (MethodInfo method, KotlinFunction function, int start, int end) => function.ValueParameters?.Count == end - start;
+
 		static void FixupExtensionMethod (MethodInfo method)
 		{
 			// Kotlin "extension" methods give the first parameter an ugly name
@@ -271,10 +313,23 @@ static void FixupField (FieldInfo? field, KotlinProperty metadata)
 
 		static MethodInfo? FindJavaMethod (KotlinFile kotlinFile, KotlinFunction function, ClassFile klass)
 		{
-			var possible_methods = klass.Methods.Where (method => method.Name == function.JvmName &&
-									      method.GetFilteredParameters ().Length == function.ValueParameters?.Count);
+			var possible_methods = klass.Methods.Where (method => method.Name == function.JvmName).ToArray ();
+			var signature = 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) is MethodInfo m)
+					return m;
+
+			// Sometimes JvmSignature is null (or unhelpful), so we're going to construct one ourselves and see if they match
+			signature = function.ConstructJvmSignature ();
+
+			if (possible_methods.SingleOrDefault (method => method.Descriptor == signature) is MethodInfo m2)
+				return m2;
 
-			foreach (var method in possible_methods) {
+			// 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 (var method in possible_methods.Where (method => method.GetFilteredParameters ().Length == function.ValueParameters?.Count)) {
 				if (function.ReturnType == null)
 					continue;
 				if (!TypesMatch (method.ReturnType, function.ReturnType, kotlinFile))
@@ -286,6 +341,8 @@ static void FixupField (FieldInfo? field, KotlinProperty metadata)
 				return method;
 			}
 
+			// Theoretically this should never be hit, but who knows. At worst, it just means
+			// Kotlin niceties won't be applied to the method.
 			return null;
 		}
 

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

@@ -8,7 +8,7 @@ namespace Xamarin.Android.Tools.Bytecode
 {
 	public static class KotlinUtilities
 	{
-		public static string ConvertKotlinTypeSignature (KotlinType? type, KotlinFile? metadata = null)
+		public static string ConvertKotlinTypeSignature (KotlinType? type, KotlinFile? metadata = null, bool convertUnsignedToPrimitive = true)
 		{
 			if (type is null)
 				return string.Empty;
@@ -27,15 +27,15 @@ public static string ConvertKotlinTypeSignature (KotlinType? type, KotlinFile? m
 				return "Ljava/lang/Object;";
 			}
 
-			var result = ConvertKotlinClassToJava (class_name);
+			var result = ConvertKotlinClassToJava (class_name, convertUnsignedToPrimitive);
 
 			if (result == "[")
-				result += ConvertKotlinTypeSignature (type.Arguments?.FirstOrDefault ()?.Type);
+				result += ConvertKotlinTypeSignature (type.Arguments?.FirstOrDefault ()?.Type, null, convertUnsignedToPrimitive);
 
 			return result;
 		}
 
-		public static string ConvertKotlinClassToJava (string? className)
+		public static string ConvertKotlinClassToJava (string? className, bool convertUnsignedToPrimitive = true)
 		{
 			if (className == null || string.IsNullOrWhiteSpace (className))
 				return string.Empty;
@@ -45,6 +45,9 @@ public static string ConvertKotlinClassToJava (string? className)
 			if (type_map.TryGetValue (className.TrimEnd (';'), out var result))
 				return result;
 
+			if (convertUnsignedToPrimitive && unsigned_type_map.TryGetValue (className.TrimEnd (';'), out var result2))
+				return result2;
+
 			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
+		public static string ConstructJvmSignature (this KotlinFunction function)
+		{
+			// 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)}";
+		}
+
 		internal static List<TResult>? ToList<TSource, TResult> (this IEnumerable<TSource>? self, JvmNameResolver resolver, Func<TSource, JvmNameResolver, TResult?> creator)
 			where TResult: class
 		{
@@ -114,37 +125,42 @@ public static bool IsDefaultConstructorMarker (this MethodInfo method)
 
 		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 _);
 
+		public static bool IsCompilerNamed (this ParameterInfo parameter) => parameter.Name.Length > 0 && parameter.Name.StartsWith ("$", StringComparison.Ordinal);
+
 		public static bool IsUnnamedParameter (this KotlinValueParameter parameter) => parameter.Name?.Length > 1 &&
 			parameter.Name.StartsWith ("p", StringComparison.Ordinal) &&
 			int.TryParse (parameter.Name.Substring (1), out var _);
 
+		static Dictionary<string, string> unsigned_type_map = new Dictionary<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" },
+		};
+
 		static Dictionary<string, string> type_map = new Dictionary<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 ());
 		}
 
+		protected static KotlinMetadata GetMetadataForClass (ClassFile klass)
+		{
+			var attr = klass.Attributes.OfType<RuntimeVisibleAnnotationsAttribute> ().FirstOrDefault ();
+			var kotlin = attr?.Annotations.FirstOrDefault (a => a.Type == "Lkotlin/Metadata;");
+
+			Assert.NotNull (kotlin);
+
+			var meta = KotlinMetadata.FromAnnotation (kotlin);
+
+			return meta;
+		}
+
+		protected static KotlinClass GetClassMetadataForClass (ClassFile klass)
+		{
+			var meta = GetMetadataForClass (klass);
+			Assert.AreEqual (KotlinMetadataKind.Class, meta.Kind);
+
+			return meta.AsClassMetadata ();
+		}
+
+		protected static KotlinFile GetFileMetadataForClass (ClassFile klass)
+		{
+			var meta = GetMetadataForClass (klass);
+			Assert.AreEqual (KotlinMetadataKind.File, meta.Kind);
+
+			return meta.AsFileMetadata ();
+		}
+
+		protected static KotlinMetadata GetMetadataForClassFile (string file)
+		{
+			var c = LoadClassFile (file);
+			return GetMetadataForClass (c);
+		}
+
+		protected static KotlinClass GetClassMetadata (string file)
+		{
+			var c = LoadClassFile (file);
+			return GetClassMetadataForClass (c);
+		}
+
 		static Stream GetResourceStream (string resource)
 		{
 			// 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]
+		public void MatchParametersWithReceiver ()
+		{
+			var klass = LoadClassFile ("DeepRecursiveKt.class");
+			var meta = GetFileMetadataForClass (klass);
+
+			var java_method = klass.Methods.Single (m => m.Name == "invoke");
+			var kotlin_function = meta.Functions.Single (m => m.Name == "invoke");
+
+			(var start, var end) = 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]
+		public void MatchParametersWithContinuation ()
+		{
+			var klass = LoadClassFile ("DeepRecursiveScope.class");
+			var meta = GetClassMetadataForClass (klass);
+
+			var java_method = klass.Methods.Single (m => m.Name == "callRecursive" && m.GetParameters ().Count () == 2);
+			var kotlin_function = meta.Functions.Single (m => m.Name == "callRecursive" && m.JvmSignature.Count (c => c == ';') == 3);
+
+			(var start, var end) = 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]
+		public void MatchParametersWithStaticThis ()
+		{
+			var klass = LoadClassFile ("UByteArray.class");
+			var meta = GetClassMetadataForClass (klass);
+
+			var java_method = klass.Methods.Single (m => m.Name == "contains-7apg3OU" && m.GetParameters ().Count () == 2);
+			var kotlin_function = meta.Functions.Single (m => m.Name == "contains");
+
+			(var start, var end) = 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);
 		}
-
-		private KotlinMetadata GetMetadataForClassFile (string file)
-		{
-			var c = LoadClassFile (file);
-			var attr = c.Attributes.OfType<RuntimeVisibleAnnotationsAttribute> ().FirstOrDefault ();
-			var kotlin = attr?.Annotations.FirstOrDefault (a => a.Type == "Lkotlin/Metadata;");
-
-			Assert.NotNull (kotlin);
-
-			var meta = KotlinMetadata.FromAnnotation (kotlin);
-
-			return meta;
-		}
-
-		private KotlinClass GetClassMetadata (string file)
-		{
-			var meta = GetMetadataForClassFile (file);
-
-			Assert.AreEqual (KotlinMetadataKind.Class, meta.Kind);
-
-			Assert.AreEqual ("1.0.3", meta.ByteCodeVersion.ToString ());
-			Assert.AreEqual ("1.1.15", meta.MetadataVersion.ToString ());
-
-			return meta.AsClassMetadata ();
-		}
 	}
 }
 

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

@@ -15,6 +15,7 @@
       "tests\\Xamarin.Android.Tools.Bytecode-Tests\\Xamarin.Android.Tools.Bytecode-Tests.csproj",
       "tests\\Xamarin.SourceWriter-Tests\\Xamarin.SourceWriter-Tests.csproj",
       "tests\\generator-Tests\\generator-Tests.csproj",
+      "tools\\class-parse\\class-parse.csproj",
       "tools\\generator\\generator.csproj"
     ]
   }