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

Kotlin unsigned type metadata bug #984

Closed
jpobst opened this issue May 12, 2022 · 1 comment · Fixed by #1009
Closed

Kotlin unsigned type metadata bug #984

jpobst opened this issue May 12, 2022 · 1 comment · Fixed by #1009
Labels
bug Component does not function as intended generator Issues binding a Java library (generator, class-parse, etc.)
Milestone

Comments

@jpobst
Copy link
Contributor

jpobst commented May 12, 2022

Context: #946
Context: dotnet/android-libraries#547

While updating AndroidX to XA 17.2, we found that the previous Kotlin issues around unsigned types have been fixed. However, there are some new issues that have been exposed:

#### Type Changed: Kotlin.UIntArray

Removed methods:

public bool Contains (uint element);
public static bool Contains (int[] arg0, int element);

Added methods:

public bool Contains (int element);
public static bool Contains (int[] arg0, uint element);

This regression should be investigated. Perhaps it is related to method overloads?

@jpobst jpobst added bug Component does not function as intended generator Issues binding a Java library (generator, class-parse, etc.) labels May 12, 2022
@jpobst jpobst added this to the 7.0.0 milestone May 12, 2022
@jpobst
Copy link
Contributor Author

jpobst commented Jul 7, 2022

Notes for future me:

fun contains(element: UInt): Boolean
operator fun <T> Array<out T>.contains(element: T): Boolean

In these cases, the Kotlin signature is the same, thus the mangled Java method name is the same:

  public boolean contains-WZ4Q5Ns(int element) {
    return contains-WZ4Q5Ns(this.storage, element);
  }
  
  public static boolean contains-WZ4Q5Ns(int[] arg0, int element) {
    Intrinsics.checkNotNullParameter(arg0, "arg0");
    return ArraysKt.contains(arg0, element);
  }

The mangled part seems to be a hash of the Kotlin parameter types of the method. Because they "match", Kotlin does not write its metadata for each method, it only writes one copy which is correct-ish for both methods:

image

(Note that the JvmSignature is ([II)Z even though there is only one ValueParameter.

Thus, when more than one Java method matches the JvmName and the JvmName is "mangled" (contains a hyphen), we should apply the parameter fixups to all matching methods, even though the JvmSignature doesn't match.

jonpryor pushed a commit that referenced this issue Jul 14, 2022
Fixes: #984

There exists a scenario with Kotlin metadata where an instance method
and an extension method can have the same *Kotlin* signature:

	// Kotlin
	class UByteArray {
	    fun contains(element: UByte): Boolean
	}

	// Extension method
	operator fun <T> Array<out T>.contains(element: T): Boolean

This results in 2 different Java methods:

	// "Java"
	class UByteArray {
	    public boolean contains-7apg3OU(byte element) {
	        return contains-WZ4Q5Ns(this.storage, element);
	    }
	
	    public static boolean contains-7apg3OU(byte[] arg0, byte element) {
	        Intrinsics.checkNotNullParameter(arg0, "arg0");
	        return ArraysKt.contains(arg0, element);
	    }
	}

Kotlin only generates 1 metadata entry, for JvmSignature=`([BB)Z`:

![image](https://user-images.githubusercontent.com/179295/178528374-a6e9a568-dc68-4b88-af6b-1c51946d7997.png)

Previously (439bd83) we assumed that a piece of Kotlin metadata
would apply to only a single Java method.  In this case, we would
find `contains-7apg3OU.([BB)Z`, then apply it to only the `static`
method, resulting in a binding of:

	// C#; previous binding
	partial class UByteArray {
	    public bool Contains(int element);
	    public static bool Contains(int[] arg0, uint element);
	}

Note: `element` is of type `int` for the instance method, `uint` for
the static method.  They *should* be consistent, and are not.

If we don't exclusively look at `JvmSignature`, and also check
`ValueParameters`, then the Kotlin metadata will match the *instance*
`contains-7apg3OU` method.

We surmise that this is intentional on Kotlin's part, in order to
save bytes by omitting the second metadata entry, and that we should
apply this metadata to both Java methods.

*Note*: the "hash" appended to the Java method name (eg: `7apg3OU`)
is a short hash of the Kotlin method parameter types.

Fix this scenario by "inverting" our lookup logic: instead of
starting with Kotlin metadata and looking for an applicable Java
method, start with the Java method and look for applicable Kotlin
metadata.  This way, each Java method can consume a matching Kotlin
metadata entry, even if another Java method already matched it.

This allows us to emit a binding of:

	// C#; new binding
	partial class UByteArray {
	    public bool Contains(uint element);
	    public static bool Contains(int[] arg0, uint element);
	}

This fixes the 4 `contains` methods for `UIntArray`, `UByteArray`,
`ULongArray`, and `UShortArray`.
jonpryor pushed a commit to dotnet/android that referenced this issue Jul 19, 2022
Fixes: dotnet/java-interop#969
Fixes: dotnet/java-interop#982
Fixes: dotnet/java-interop#984

Changes: dotnet/java-interop@fadbb82...032f1e7

  * dotnet/java-interop@032f1e71: [Xamarin.Android.Tools.Bytecode-Tests] Fix BuildClasses (#1013)
  * dotnet/java-interop@0eaa47e1: [Java.Interop.Tools.JavaCallableWrappers] NRT Support (#1012)
  * dotnet/java-interop@45fe3928: [generator] BG8403 when type name matches enclosing namespace name (#1010)
  * dotnet/java-interop@fe60483b: [generator] Mark abstract methods as [Obsolete] if needed (#1011)
  * dotnet/java-interop@275fa755: [generator] Kotlin metadata can apply to multiple Java method (#1009)
  * dotnet/java-interop@3ff6f8fb: [Java.Base] Fix CS0108 Warnings (#1008)

Of note is dotnet/java-interop@fe60483b, which can cause public
members to become marked as `[Obsolete]`.  These members *always*
should have been `[Obsolete]`, but weren't.  (Oops.)  This also
required changes to the `Mono.Android` API-compat checks.

Also of note is dotnet/java-interop@45fe3928, which introduces a new
BG8403 binding warning when a bound type has the same name as its
enclosing namespace name-part, e.g. `Example.View.View`.
@github-actions github-actions bot locked and limited conversation to collaborators Apr 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Component does not function as intended generator Issues binding a Java library (generator, class-parse, etc.)
Projects
None yet
1 participant