Skip to content

Commit

Permalink
[class-parse] Update parameter names backwards
Browse files Browse the repository at this point in the history
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 a "random"
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.

As a solution, @jpobst [suggested][0]:

> if we took all `StartPC=0` parameters, sorted them by `Index`, and
> then applied them to our parameters in reverse order, would that
> simplify the logic of removing the ones at the beginning like
> `$this$find`?

Do that: instead of trying to figure out which local variables to
skip at the beginning, instead look at the *end* of the parameter and
local variables lists, and move "backwards" towards the front.

This allows us to infer and skip the `this` local variable, as it's
the one that doesn't have a matching parameter position.

[0]: https://discord.com/channels/732297728826277939/732297837953679412/902554256035426315
  • Loading branch information
jonpryor committed Oct 28, 2021
1 parent 79744f6 commit 45c0763
Showing 1 changed file with 10 additions and 31 deletions.
41 changes: 10 additions & 31 deletions src/Xamarin.Android.Tools.Bytecode/Methods.cs
Original file line number Diff line number Diff line change
Expand Up @@ -165,44 +165,23 @@ 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 namesStart = names.Count;
int parametersCount = GetDeclaredParametersCount (parameters);
CheckDescriptorVariablesToLocalVariables (parameters, parametersCount, names, namesStart);

for (int ni = names.Count-1, pi = parametersCount-1; ni >= 0 && pi >= 0; ni--, pi--) {
if (parameters [pi].Type.BinaryName != names [ni].Descriptor) {
break;
}
namesStart = ni;
parameters [pi].Name = names [ni].Name;
}
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);
}

CheckDescriptorVariablesToLocalVariables (parameters, parametersCount, names, namesStart);
}

LocalVariableTableAttribute GetLocalVariables ()
Expand Down

0 comments on commit 45c0763

Please sign in to comment.