Skip to content

Conversation

@jpobst
Copy link
Contributor

@jpobst jpobst commented Feb 3, 2022

Fixes: #945

In order to apply Kotlin metadata fixups to Java methods, we must match the Kotlin function to the Java method. 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 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. In Kotlin 1.6, these compiler generated method parameters were given new names ($this -> arg0) that breaks our algorithm. So we need something better.

Kotlin metadata provides a property called JvmSignature (to match JvmName which we already use) which sounds promising, however sometimes it is 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 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.

@jpobst jpobst force-pushed the kotlin-unsigned branch 2 times, most recently from 56af33d to d11ce38 Compare February 3, 2022 16:11
@jpobst jpobst marked this pull request as ready for review February 3, 2022 19:39
@jonpryor
Copy link
Contributor

jonpryor commented Feb 3, 2022

Would it be possible to move DeepRecursiveKt.class, DeepRecursiveScope.class, and UByteArray.class into e.g a src-ThirdParty/kotlin directory? (See also dotnet/android@a452af2)

return method;
}

// Theoretically this should never be hit, but who knows. At worst, it just means
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we emit a debug message or warning here? Pity Action<TraceLevel, string> isn't already used here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a debug message to match the rest of Kotlin debugging messages. A warning probably wouldn't be appropriate because it's not actionable. There's nothing a user can do to fix it.

@jpobst
Copy link
Contributor Author

jpobst commented Feb 4, 2022

Moved the .class files from Kotlin stdlib to tests/Xamarin.Android.Tools.Bytecode-Tests/kotlin-ThirdParty.

@jonpryor
Copy link
Contributor

jonpryor commented Feb 7, 2022

Fixes: https://github.com/xamarin/java.interop/issues/945

Context: https://github.com/xamarin/AndroidX/pull/436#discussion_r793968309

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 71afce55: 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 @ 32635fd6 |               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

@jonpryor jonpryor merged commit f91b077 into main Feb 7, 2022
@jonpryor jonpryor deleted the kotlin-unsigned branch February 7, 2022 20:54
@github-actions github-actions bot locked and limited conversation to collaborators Apr 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Kotlin 1.6 metadata sometimes isn't applied

3 participants