[class-parse] Loosely match parameter names, backwards

Context: 8ccb837
Context: dotnet/android-libraries#413
Context: https://discord.com/channels/732297728826277939/732297837953679412/902301741159182346
Context: https://repo1.maven.org/maven2/org/jetbrains/kotlin/kotlin-stdlib/1.5.31/kotlin-stdlib-1.5.31.jar
Context: https://discord.com/channels/732297728826277939/732297837953679412/902554256035426315

dotnet/android-libraries#413 ran into an issue:

	D:\a\1\s\generated\org.jetbrains.kotlin.kotlin-stdlib\obj\Release\net6.0-android\generated\src\Kotlin.Coroutines.AbstractCoroutineContextElement.cs(100,8):
	error CS1002: ; expected

The offending line:

	var this = Java.Lang.Object.GetObject<Java.Lang.Object> (native_this, JniHandleOwnership.DoNotTransfer);

(Assigning to `this` makes for a very weird error message.)

This was eventually tracked down to commit 8ccb837; @jpobst wrote:

> previously it produced:
>
>     <parameter name="initial" type="R" jni-type="TR;" />
>     <parameter name="operation" type="kotlin.jvm.functions.Function2&lt;? super R, ? super kotlin.coroutines.CoroutineContext.Element, ? extends R&gt;" />
>
> now it produces:
>
>     <parameter name="this" type="R" jni-type="TR;" />
>     <parameter name="initial" type="kotlin.jvm.functions.Function2&lt;? super R, ? super kotlin.coroutines.CoroutineContext.Element, ? extends R&gt;" />

The (a?) "source" of the problem is that Kotlin is "weird": it emits
a Java method with signature:

	/* partial */ class AbstractCoroutineContextElement {
	    public Object fold(Object initial, Function2 operation);
	}

However, the local variables table declares *three* local variables:

 1. `this` of type `kotlin.coroutines.CoroutineContext.Element`
 2. `initial` of type `java.lang.Object`
 3. `operation` of type `Function2`

This is an instance method, so normally we would skip the first
local variable, as "normally" the first local variable of an instance
method has the same type as the declaring type.

The "weirdness" with Kotlin is that the first local parameter type
is *not* the same as the declaring type, it's of the implemented
interface type!

	% mono class-parse.exe --dump kotlin/coroutines/AbstractCoroutineContextElement.class
	…
	ThisClass: Utf8("kotlin/coroutines/AbstractCoroutineContextElement")
	…
	    3: fold (Ljava/lang/Object;Lkotlin/jvm/functions/Function2;)Ljava/lang/Object; Public
	        Code(13, Unknown[LineNumberTable](6),
	         LocalVariableTableAttribute(
	          LocalVariableTableEntry(Name='this', Descriptor='Lkotlin/coroutines/CoroutineContext$Element;', StartPC=0, Index=0),
	          LocalVariableTableEntry(Name='initial', Descriptor='Ljava/lang/Object;', StartPC=0, Index=1),
	          LocalVariableTableEntry(Name='operation', Descriptor='Lkotlin/jvm/functions/Function2;', StartPC=0, Index=2)))
	        Signature(<R:Ljava/lang/Object;>(TR;Lkotlin/jvm/functions/Function2<-TR;-Lkotlin/coroutines/CoroutineContext$Element;+TR;>;)TR;)
	        RuntimeInvisibleParameterAnnotationsAttribute(Parameter0(), Parameter1(Annotation('Lorg/jetbrains/annotations/NotNull;', {})))
	…

Here, we "expect" the `this` local variable to be of type
`kotlin.coroutines.AbstractCoroutineContextElement`, but it is
instead of type `kotlin.coroutines.CoroutineContext.Element`.

This "type mismatch" means that our logic to skip the first local
variable doesn't actually skip the first local variable.

But wait, Kotlin can throw differently weird stuff at us, too.
See e.g. [inline and reified type parameters][0], which can result in
local parameter names such as `$i$f$get`, or see e.g.
[`CoroutinesRoom.execute()`][1], in which the local variable table
*lacks* a name for one of the JNI signature parameter types.

To better address these scenarios, relax and rework the logic in
`MethodInfo.UpdateParametersFromLocalVariables()`: instead of
requiring that we know the "start" offset between the local variable
names and the parameters (previous world order), instead:

 1. Given `names` from local variables table, and `parameters` from
    the JNI signature,

 2. For each element in `parameters`, going *backards*, from the end
    of `parameters` to the front,

 3. Compare the `parameters` element type to each item in `names`,
    traversing `names` backwards as well.

 4. When the parameter types match, set the `parameters` element name
    to the `names` element name, then *remove* the `names` element
	from `names`.  This prevents us from reusing the local variable
	for other parameters.

We need to do this backwards so that we "skip"/"ignore" extra
parameters at the start of the local variable name table (which is
usually the `this` parameter).

*Not* requiring that a "run" of parameter types match also grants
flexibility, as when there is no local variable for a given
parameter, we won't care.

This allows to "cleanly" handle `fold()`: we'll look at `names` for
a match for the `Function2` type, find `operation`, then look at
`names` to match the `Object` type, find `initial`, then finish.

Update `Xamarin.Android.Tools.Bytecode-Tests.targets` so that there
are more `.java` files built *without* `java -parameters`, so that
the "parameter name inference" logic is actually tested.
(When `javac -parameters` is used, the `MethodParametersAttribute`
blob is emitted, which contains proper parameter names.)

[0]: https://medium.com/swlh/inline-and-reified-type-parameters-in-kotlin-c7585490e103
[1]: dotnet#900 (comment)

Author: jonpryor

src/Xamarin.Android.Tools.Bytecode/Methods.cs

@@ -93,6 +93,7 @@ public ParameterInfo[] GetParameters ()
 			UpdateParametersFromMethodParametersAttribute (parameters);
 			return parameters;
 		}
+
 		static IEnumerable<string> ExtractTypesFromSignature (string signature)
 		{
 			if (signature == null || signature.Length < "()V".Length)
@@ -106,6 +107,7 @@ static IEnumerable<string> ExtractTypesFromSignature (string signature)
 				yield return Signature.ExtractType (signature, ref index);
 			}
 		}
+
 		List<ParameterInfo> GetParametersFromDescriptor (out int endParams)
 		{
 			var signature   = Descriptor;
@@ -165,43 +167,18 @@ void UpdateParametersFromLocalVariables (ParameterInfo[] parameters)
 				return;
 
 			var names = locals.LocalVariables.Where (p => p.StartPC == 0).ToList ();
-			int namesStart  = 0;
-			if (!AccessFlags.HasFlag (MethodAccessFlags.Static) &&
-					names.Count > namesStart &&
-					names [namesStart].Descriptor == DeclaringType.FullJniName) {
-				namesStart++;   // skip `this` parameter
-			}
-			if (!DeclaringType.IsStatic &&
-					IsConstructor &&
-					names.Count > namesStart &&
-					DeclaringType.InnerClass != null && DeclaringType.InnerClass.OuterClassName != null &&
-					names [namesStart].Descriptor == "L" + DeclaringType.InnerClass.OuterClassName + ";") {
-				namesStart++;   // "outer `this`", for non-static inner classes
-			}
-			if (!DeclaringType.IsStatic &&
-					IsConstructor &&
-					names.Count > namesStart &&
-					DeclaringType.TryGetEnclosingMethodInfo (out var declaringClass, out var _, out var _) &&
-					names [namesStart].Descriptor == "L" + declaringClass + ";") {
-				namesStart++;   // "outer `this`", for non-static inner classes
-			}
-
-			// For JvmOverloadsConstructor.<init>.(LJvmOverloadsConstructor;IILjava/lang/String;)V
-			if (namesStart > 0 &&
-					names.Count > namesStart &&
-					parameters.Length > 0 &&
-					names [namesStart].Descriptor   != parameters [0].Type.BinaryName &&
-					names [namesStart-1].Descriptor == parameters [0].Type.BinaryName) {
-				namesStart--;
-			}
 
 			int parametersCount = GetDeclaredParametersCount (parameters);
-			CheckDescriptorVariablesToLocalVariables (parameters, parametersCount, names, namesStart);
 
-			int max = Math.Min (parametersCount,    names.Count - namesStart);
-			for (int i = 0; i < max; ++i) {
-				parameters [i].Name = names [namesStart+i].Name;
-				CheckLocalVariableTypeToDescriptorType (i, parameters, names, namesStart);
+			for (int pi = parametersCount-1; pi >= 0; --pi) {
+				for (int ni = names.Count-1; ni >= 0; --ni) {
+					if (parameters [pi].Type.BinaryName != names [ni].Descriptor) {
+						continue;
+					}
+					parameters [pi].Name = names [ni].Name;
+					names.RemoveAt (ni);
+					break;
+				}
 			}
 		}
 
@@ -273,60 +250,6 @@ void UpdateParametersFromSignature (ParameterInfo[] parameters)
 			}
 		}
 
-		void CheckDescriptorVariablesToLocalVariables (ParameterInfo[] parameters, int parametersCount, List<LocalVariableTableEntry> names, int namesStart)
-		{
-			if (AccessFlags.HasFlag (MethodAccessFlags.Synthetic))
-				return;
-			if ((names.Count - namesStart) == parametersCount)
-				return;
-			if (IsEnumCtor)
-				return;
-
-			var paramsDesc  = CreateParametersList (parameters, (v, i) => $"`{v.Type.BinaryName}` {v.Name}{(i >= parametersCount ? " /* abi; ignored */" : "")}");
-			var localsDesc  = CreateParametersList (names,      (v, i) => $"`{v.Descriptor}` {v.Name}{(i < namesStart ? " /* abi; skipped */" : "")}");
-
-			Log.Debug ($"class-parse: method {DeclaringType.ThisClass.Name.Value}.{Name}.{Descriptor}: namesStart={namesStart}; " +
-					$"Local variables array has {names.Count - namesStart} entries {localsDesc}; " +
-					$"descriptor has {parametersCount} entries {paramsDesc}!");
-		}
-
-		static string CreateParametersList<T>(IEnumerable<T> values, Func<T, int, string> createElement)
-		{
-			var description = new StringBuilder ()
-				.Append ("(");
-
-			int index   = 0;
-			var first   = true;
-			foreach (var v in values) {
-				if (!first) {
-					description.Append (", ");
-				}
-				first   = false;
-				description.Append (createElement (v, index));
-				index++;
-			}
-			description.Append (")");
-
-			return description.ToString ();
-		}
-
-		void CheckLocalVariableTypeToDescriptorType (int index, ParameterInfo[] parameters, List<LocalVariableTableEntry> names, int namesStart)
-		{
-			if (AccessFlags.HasFlag (MethodAccessFlags.Synthetic))
-				return;
-
-			var parameterType   = parameters [index].Type.BinaryName;
-			var descriptorType  = names [index + namesStart].Descriptor;
-			if (parameterType == descriptorType)
-				return;
-
-			var paramsDesc  = CreateParametersList (parameters, (v, i) => $"`{v.Type.BinaryName}` {v.Name}");
-			var localsDesc  = CreateParametersList (names,      (v, i) => $"`{v.Descriptor}` {v.Name}{(i < namesStart ? " /* abi; skipped */" : "")}");
-
-			Log.Debug ($"class-parse: method {DeclaringType.ThisClass.Name.Value}.{Name}.{Descriptor}: " +
-					$"Local variables array {localsDesc} element {index+namesStart} with type `{descriptorType}` doesn't match expected descriptor list {paramsDesc} element {index} with type `{parameterType}`.");
-		}
-
 		void CheckDescriptorVariablesToSignatureParameters (ParameterInfo[] parameters, int parametersCount, MethodTypeSignature sig)
 		{
 			if (IsEnumCtor)

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

@@ -0,0 +1,45 @@
+using System;
+
+using Xamarin.Android.Tools.Bytecode;
+
+using NUnit.Framework;
+
+namespace Xamarin.Android.Tools.BytecodeTests {
+
+	[TestFixture]
+	public class JavaTypeNoParametersTests : ClassFileFixture {
+
+		const string JavaType = "JavaTypeNoParameters";
+
+		[Test]
+		public void ClassFile_WithNonGenericGlobalType_class ()
+		{
+			var c   = LoadClassFile (JavaType + ".class");
+			new ExpectedTypeDeclaration {
+				MajorVersion        = 0x34,
+				MinorVersion        = 0,
+				ConstantPoolCount   = 18,
+				AccessFlags         = ClassAccessFlags.Public | ClassAccessFlags.Super,
+				FullName            = "com/xamarin/JavaTypeNoParameters",
+				Superclass          = new TypeInfo ("java/lang/Object", "Ljava/lang/Object;"),
+				Methods = {
+					new ExpectedMethodDeclaration {
+						Name                    = "<init>",
+						AccessFlags             = MethodAccessFlags.Public,
+						ReturnDescriptor        = "V",
+						Parameters = {
+							new ParameterInfo ("copy", "Lcom/xamarin/JavaTypeNoParameters;", "Lcom/xamarin/JavaTypeNoParameters;"),
+						},
+					},
+				}
+			}.Assert (c);
+		}
+
+		[Test]
+		public void XmlDeclaration_WithNonGenericGlobalType_class ()
+		{
+			AssertXmlDeclaration (JavaType + ".class", JavaType + ".xml");
+		}
+	}
+}
+

tests/Xamarin.Android.Tools.Bytecode-Tests/Resources/JavaTypeNoParameters.xml

@@ -0,0 +1,34 @@
+<api
+  api-source="class-parse">
+  <package
+    name="com.xamarin"
+    jni-name="com/xamarin">
+    <class
+      abstract="false"
+      deprecated="not deprecated"
+      jni-extends="Ljava/lang/Object;"
+      extends="java.lang.Object"
+      extends-generic-aware="java.lang.Object"
+      final="false"
+      name="JavaTypeNoParameters"
+      jni-signature="Lcom/xamarin/JavaTypeNoParameters;"
+      source-file-name="JavaTypeNoParameters.java"
+      static="false"
+      visibility="public">
+      <constructor
+        deprecated="not deprecated"
+        final="false"
+        name="JavaTypeNoParameters"
+        static="false"
+        visibility="public"
+        bridge="false"
+        synthetic="false"
+        jni-signature="(Lcom/xamarin/JavaTypeNoParameters;)V">
+        <parameter
+          name="copy"
+          type="com.xamarin.JavaTypeNoParameters"
+          jni-type="Lcom/xamarin/JavaTypeNoParameters;" />
+      </constructor>
+    </class>
+  </package>
+</api>

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

@@ -38,6 +38,7 @@
     <EmbeddedResource Include="$(IntermediateOutputPath)classes\com\xamarin\JavaType%24RNC%24RPNC.class" />
     <EmbeddedResource Include="$(IntermediateOutputPath)classes\com\xamarin\JavaType%24RNC.class" />
     <EmbeddedResource Include="$(IntermediateOutputPath)classes\com\xamarin\JavaType.class" />
+    <EmbeddedResource Include="$(IntermediateOutputPath)classes\com\xamarin\JavaTypeNoParameters.class" />
     <EmbeddedResource Include="$(IntermediateOutputPath)classes\com\xamarin\NestedInterface%24DnsSdTxtRecordListener.class" />
     <EmbeddedResource Include="$(IntermediateOutputPath)classes\com\xamarin\NestedInterface.class" />
     <EmbeddedResource Include="$(IntermediateOutputPath)classes\com\xamarin\ParameterAbstractClass.class" />

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

@@ -1,15 +1,21 @@
 <Project>
 
   <ItemGroup>
-    <TestJar Include="java\**\*.java" Exclude="java\java\util\Collection.java,java\android\annotation\NonNull.java" />
-    <TestJarNoParameters Include="java\java\util\Collection.java" />
+    <TestJarNoParameters Include="java/java/util/Collection.java" />
+    <TestJarNoParameters Include="java/**/*NoParameters.java" />
+    <TestJar Include="java\**\*.java" Exclude="@(TestJarNoParameters);java\android\annotation\NonNull.java" />
     <TestKotlinJar Include="kotlin\**\*.kt" />
   </ItemGroup>
 
+  <ItemGroup>
+    <_BuildClassOutputs Include="@(TestJar->'$(IntermediateOutputPath)classes\%(RecursiveDir)%(Filename).class')" />
+    <_BuildClassOutputs Include="@(TestJarNoParameters->'$(IntermediateOutputPath)classes\%(RecursiveDir)%(Filename).class')" />
+  </ItemGroup>
+
   <Target Name="BuildClasses"
         BeforeTargets="BeforeBuild"
-        Inputs="@(TestJar)"
-        Outputs="@(TestJar->'$(IntermediateOutputPath)classes\%(RecursiveDir)%(Filename).class')">
+        Inputs="@(TestJar);@(TestJarNoParameters)"
+        Outputs="@(_BuildClassOutputs)">
     <MakeDir Directories="$(IntermediateOutputPath)classes" />
     <Exec Command="&quot;$(JavaCPath)&quot; -parameters $(_JavacSourceOptions) -g -d &quot;$(IntermediateOutputPath)classes&quot; java/android/annotation/NonNull.java @(TestJar->'%(Identity)', ' ')" />
     <Exec Command="&quot;$(JavaCPath)&quot; $(_JavacSourceOptions) -g -d &quot;$(IntermediateOutputPath)classes&quot; @(TestJarNoParameters->'%(Identity)', ' ')" />

tests/Xamarin.Android.Tools.Bytecode-Tests/java/com/xamarin/JavaTypeNoParameters.java

@@ -0,0 +1,9 @@
+package com.xamarin;
+
+public class JavaTypeNoParameters {
+	/**
+	 * JNI sig: (Lcom/xamarin/JavaTypeNoParameters;)V
+	 */
+	public JavaTypeNoParameters (JavaTypeNoParameters copy) {
+	}
+}