Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bump to xamarin/xamarin-android/release/6.0.1xx-preview10@27295e27 #413

Merged
merged 4 commits into from
Oct 27, 2021

Conversation

jonathanpeppers
Copy link
Member

@jonathanpeppers jonathanpeppers commented Oct 25, 2021

Context: https://github.com/xamarin/xamarin-android/commits/release/6.0.1xx-preview10
Context: https://github.com/dotnet/maui/wiki/Installing-.NET-6

We need to build with a version of the .NET 6 android workload that
contains the fix:

dotnet/java-interop@2d5431f

For .NET 6, we should:

  1. Install the latest public .NET 6 RC 2 release.
  2. Use a workload.json file to pin to a specific build of the
    android workload.
  3. Use the dotnet6 feed when installing the workload.

I used the workload.json file currently reported by running:

dotnet workload update --print-rollback

I adjusted the version number to match the appropriate build of:

"microsoft.net.sdk.android": "31.0.101-preview.10.56",

Context: https://github.com/xamarin/xamarin-android/commits/release/6.0.1xx-preview10
Context: https://github.com/dotnet/maui/wiki/Installing-.NET-6

We need to build with a version of the .NET 6 `android` workload that
contains the fix:

dotnet/java-interop@2d5431f

For .NET 6, we should:

1. Install the latest public .NET 6 RC 2 release.
2. Use a `workload.json` file to pin to a specific build of the
   `android` workload.
3. Use the `dotnet6` feed when installing the workload.

I used the `workload.json` file currently reported by running:

   dotnet workload update --print-rollback

I adjusted the version number to match the appropriate build of:

   "microsoft.net.sdk.android": "31.0.101-preview.10.56",
@jonathanpeppers jonathanpeppers force-pushed the xamarin-android-27295e27 branch from 4f83491 to f95b189 Compare October 25, 2021 15:47
@jonathanpeppers
Copy link
Member Author

So far seems to work on Windows:

Updated advertising manifest microsoft.net.sdk.android.
...
Installing workload manifest microsoft.net.sdk.android version 31.0.101-preview.10.56…

Hitting an Xcode error on Mac, though.

@jonathanpeppers
Copy link
Member Author

jonathanpeppers commented Oct 25, 2021

Then the bump hit this error:

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 [D:\a\1\s\generated\org.jetbrains.kotlin.kotlin-stdlib\org.jetbrains.kotlin.kotlin-stdlib.csproj]

Fixes the error:

    xcode-select: error: invalid developer directory '/Applications/Xcode_12.3.app/Contents/Developer'
@moljac
Copy link
Contributor

moljac commented Oct 25, 2021

Then the bump hit this error:

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 [D:\a\1\s\generated\org.jetbrains.kotlin.kotlin-stdlib\org.jetbrains.kotlin.kotlin-stdlib.csproj]

OK I will check this one tomorrow. OK?

jonpryor added a commit to jonpryor/java.interop that referenced this pull request Oct 25, 2021
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

TODO: unit tests!

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

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

> 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. kotlin/coroutines/CoroutineContext.Element this
 2. Object initial
 3. Function2 operation

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!  Which means that our logic to skip the
first local variable doesn't actually skip the first local variable.

As a workaround, explicitly ignore the first local variable if:

 1. The method is an instance method, and
 2. Type of first local variable matches the declaring type, *or*
    the name of the first local variable is `this`.
jonpryor added a commit to jonpryor/java.interop that referenced this pull request Oct 26, 2021
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
@jonathanpeppers jonathanpeppers marked this pull request as ready for review October 26, 2021 20:54
@jonathanpeppers
Copy link
Member Author

Downloaded a random package to spot check, and lib\net6.0-android31.0\*.dll looks right:

// Java.Interop, Version=0.1.0.0, Culture=neutral, PublicKeyToken=84e04ff9cfb79065

Copy link
Contributor

@moljac moljac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jonathanpeppers jonathanpeppers merged commit 9d09838 into dotnet:main Oct 27, 2021
@jonathanpeppers jonathanpeppers deleted the xamarin-android-27295e27 branch October 27, 2021 13:41
jonpryor added a commit to jonpryor/java.interop that referenced this pull request Oct 28, 2021
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
jonpryor added a commit to jonpryor/java.interop that referenced this pull request Oct 29, 2021
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.

But wait, Kotlin can throw differently weird stuff at us, too.
Enter [inline and reified type parameters][0], TODO EXPL.

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 look for a
"run" of local variables which have the same types, in the same
order, as the descriptor parameters.  If there are extra local
variables, *ignore them*.

This allows to "cleanly" handle `fold()`: when checking for a
matching "run" of `{ Object, Function2 }`, we'll skip the `this`
parameter and nicely align on the `initial` parameter.

This does mean that if a method has no descriptor parameters, it'll
match *everything*, we we arguably have less validation occurring,
but @jonpryor isn't convinced we were gaining anything from that.

[0]: https://medium.com/swlh/inline-and-reified-type-parameters-in-kotlin-c7585490e103
jonpryor added a commit to jonpryor/java.interop that referenced this pull request Oct 29, 2021
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.

But wait, Kotlin can throw differently weird stuff at us, too.
Enter [inline and reified type parameters][0], TODO EXPL.

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 look for a
"run" of local variables which have the same types, in the same
order, as the descriptor parameters.  If there are extra local
variables, *ignore them*.

This allows to "cleanly" handle `fold()`: when checking for a
matching "run" of `{ Object, Function2 }`, we'll skip the `this`
parameter and nicely align on the `initial` parameter.

This does mean that if a method has no descriptor parameters, it'll
match *everything*, we we arguably have less validation occurring,
but @jonpryor isn't convinced we were gaining anything from that.

[0]: https://medium.com/swlh/inline-and-reified-type-parameters-in-kotlin-c7585490e103
jonpryor added a commit to jonpryor/java.interop that referenced this pull request Nov 1, 2021
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.

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`.

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 look for a
"run" of local variables which have the same types, in the same
order, as the descriptor parameters.  If there are extra local
variables, *ignore them*.  The search for the "run" needs to start at
the *end* of the local variables table, so that it doesn't
inadvertently match the `this` parameter, e.g. as seen with a "copy"
constructor `DeclaringType(DeclaringType)`.

This allows to "cleanly" handle `fold()`: when checking for a
matching "run" of `{ Object, Function2 }`, we'll skip the `this`
parameter and nicely align on the `initial` parameter.

This does mean that if a method has no descriptor parameters, it'll
match *everything*, we arguably have less validation occurring,
but @jonpryor isn't convinced we were gaining anything from that.

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
jonpryor added a commit to jonpryor/java.interop that referenced this pull request Nov 2, 2021
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)
jonpryor added a commit to dotnet/java-interop that referenced this pull request Nov 8, 2021
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]: #900 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants