-
Notifications
You must be signed in to change notification settings - Fork 64
[generator] Drop Kotlin hash-mangled siblings that collide on the C# side #1432
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
Open
jonathanpeppers
wants to merge
3
commits into
main
Choose a base branch
from
jonathanpeppers/kotlin-inline-collision
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
70 changes: 70 additions & 0 deletions
70
tests/Xamarin.Android.Tools.Bytecode-Tests/KotlinInlineClassCollisionTests.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,70 @@ | ||
| using System; | ||
| using System.Linq; | ||
| using NUnit.Framework; | ||
| using Xamarin.Android.Tools.Bytecode; | ||
|
|
||
| namespace Xamarin.Android.Tools.BytecodeTests | ||
| { | ||
| // Exercises the real Kotlin bytecode produced by the Gradle fixture under | ||
| // kotlin-gradle/ to confirm that the JVM-level mangling we expect (and that | ||
| // the generator's KotlinFixups must now de-collide) is actually what kotlinc | ||
| // emits for @JvmInline value-class parameters. See dotnet/java-interop#1431. | ||
| [TestFixture] | ||
| public class KotlinInlineClassCollisionTests : ClassFileFixture | ||
| { | ||
| [Test] | ||
| public void Widgets_HasCollidingHashMangledSiblings () | ||
| { | ||
| var klass = LoadClassFile ("Widgets.class"); | ||
|
|
||
| // Kotlin emits one mangled method per inline-class overload: | ||
| // tint-<hash>(J)V for MyColor (ULong-backed) | ||
| // tint-<hash>(J)V for MyAlpha (ULong-backed) — collides with MyColor | ||
| // tint-<hash>(F)V for MyDp (Float-backed) — unique | ||
| var tints = klass.Methods | ||
| .Where (m => m.Name.StartsWith ("tint-", StringComparison.Ordinal)) | ||
| .ToList (); | ||
|
|
||
| Assert.AreEqual (3, tints.Count, "Expected three `tint-<hash>` overloads from the Gradle fixture."); | ||
|
|
||
| var longTints = tints.Where (m => m.Descriptor == "(J)V").ToList (); | ||
| Assert.AreEqual (2, longTints.Count, | ||
| "Expected two `tint-<hash>(J)V` siblings (MyColor + MyAlpha) — this is the multi-sibling collision case from dotnet/java-interop#1431."); | ||
|
|
||
| Assert.AreEqual (1, tints.Count (m => m.Descriptor == "(F)V"), | ||
| "Expected one unique `tint-<hash>(F)V` (MyDp) that should survive deduplication."); | ||
| } | ||
|
|
||
| [Test] | ||
| public void Widgets_HasNonCollidingHashMangledOverloads () | ||
| { | ||
| var klass = LoadClassFile ("Widgets.class"); | ||
|
|
||
| var pads = klass.Methods | ||
| .Where (m => m.Name.StartsWith ("pad-", StringComparison.Ordinal)) | ||
| .ToList (); | ||
|
|
||
| Assert.AreEqual (2, pads.Count); | ||
| CollectionAssert.AreEquivalent ( | ||
| new [] { "(F)F", "(FF)F" }, | ||
| pads.Select (m => m.Descriptor).ToArray (), | ||
| "`pad` overloads have distinct JVM signatures and should both survive after rename."); | ||
| } | ||
|
|
||
| [Test] | ||
| public void InlineClasses_AreEmittedAsValueClasses () | ||
| { | ||
| // Sanity check that @JvmInline really produced a JvmInline annotation on | ||
| // the inline-class type — this is what step (2) of #1431 will key on. | ||
| var myColor = LoadClassFile ("MyColor.class"); | ||
|
|
||
| var annotations = myColor.Attributes | ||
| .OfType<RuntimeVisibleAnnotationsAttribute> () | ||
| .SelectMany (a => a.Annotations) | ||
| .Select (a => a.Type) | ||
| .ToList (); | ||
|
|
||
| Assert.Contains ("Lkotlin/jvm/JvmInline;", annotations); | ||
| } | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
4 changes: 4 additions & 0 deletions
4
tests/Xamarin.Android.Tools.Bytecode-Tests/kotlin-gradle/.gitignore
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| # Gradle build outputs - rebuilt from source on every test build. | ||
| .gradle/ | ||
| build/ | ||
| classes/ |
21 changes: 21 additions & 0 deletions
21
tests/Xamarin.Android.Tools.Bytecode-Tests/kotlin-gradle/build.gradle.kts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| plugins { | ||
| kotlin("jvm") version "2.0.21" | ||
| } | ||
|
|
||
| repositories { | ||
| mavenCentral() | ||
| } | ||
|
|
||
| // Don't pin a jvmToolchain -- it would force Gradle to auto-provision a | ||
| // matching JDK and fail in CI environments without download repositories | ||
| // configured. Use whatever JDK the caller already set in JAVA_HOME (the | ||
| // .NET build forwards $(JavaSdkDirectory) for consistency with the rest | ||
| // of the repo). Kotlin 2.0.21 targets JVM 11 by default, which is fine | ||
| // for the bytecode the tests inspect. | ||
|
|
||
| // Emit compiled classes into a stable, predictable location so the | ||
| // .NET test harness can load them via ClassFileFixture without needing | ||
| // to know the Gradle build directory layout. | ||
| tasks.named<org.jetbrains.kotlin.gradle.tasks.KotlinCompile>("compileKotlin") { | ||
| destinationDirectory.set(file("$rootDir/classes")) | ||
| } |
1 change: 1 addition & 0 deletions
1
tests/Xamarin.Android.Tools.Bytecode-Tests/kotlin-gradle/settings.gradle.kts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| rootProject.name = "kotlin-inline-class-fixtures" |
34 changes: 34 additions & 0 deletions
34
...marin.Android.Tools.Bytecode-Tests/kotlin-gradle/src/main/kotlin/InlineClassCollisions.kt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| @file:JvmName("InlineClassCollisionsKt") | ||
|
|
||
| package xat.bytecode.tests | ||
|
|
||
| // Two distinct Kotlin inline classes that erase to the same JVM primitive (long). | ||
| // Both `tint(MyColor)` and `tint(MyAlpha)` mangle to `tint-<hash>(J)V`, so they | ||
| // collide once class-parse drops the hash suffix. This is the exact scenario | ||
| // that Jetpack Compose triggers with Color/TextUnit/etc. and is the case | ||
| // step (1) of dotnet/java-interop#1431 must handle. | ||
| @JvmInline | ||
| value class MyColor(val value: ULong) | ||
|
|
||
| @JvmInline | ||
| value class MyAlpha(val value: ULong) | ||
|
|
||
| // A second inline class with a different backing primitive, so we can verify | ||
| // that *non*-colliding hash siblings still survive. | ||
| @JvmInline | ||
| value class MyDp(val value: Float) | ||
|
|
||
| object Widgets { | ||
|
|
||
| // Colliding pair: both erase to `tint-XXXXXXX(J)V`. | ||
| fun tint(color: MyColor) { /* no-op */ } | ||
| fun tint(alpha: MyAlpha) { /* no-op */ } | ||
|
|
||
| // Distinct hash-mangled sibling of the same source name — should survive | ||
| // alongside one of the `tint(long)` overloads. | ||
| fun tint(dp: MyDp) { /* no-op */ } | ||
|
|
||
| // A non-colliding pair: different arity, both hash-mangled. | ||
| fun pad(dp: MyDp): MyDp = dp | ||
| fun pad(dp1: MyDp, dp2: MyDp): MyDp = dp1 | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.