Skip to content

Commit 6898c1c

Browse files
[generator] Drop Kotlin hash-mangled siblings that collide on the C# side (#1432)
Context: step (1) of #1431. When Kotlin compiles a function whose parameter is an `@JvmInline value class`, the JVM-level name is mangled with a 7-char hash suffix, e.g. `tint-Rn_QMJI(J)V`. `KotlinFixups` already strips that suffix so the C# binding sees `Tint(long)` instead of an unspellable name. The problem: two distinct Kotlin overloads that take different inline classes erase to the *same* JVM signature, differing only in the hash. Jetpack Compose hits this constantly (`Color`, `TextUnit`, `Dp` all wrap `ULong`/`Long`/`Float`). For example: @JvmInline value class MyColor(val value: ULong) @JvmInline value class MyAlpha(val value: ULong) object Widgets { fun tint(color: MyColor) { } fun tint(alpha: MyAlpha) { } } `javap` shows two hash-distinct siblings with identical erased shape: public static void tint-Rn_QMJI(long); // MyColor public static void tint-uzYZ1wI(long); // MyAlpha After the existing rename both become `Tint(long)`, and `generator` emits something like: // Before: CS0111, build broken public static void Tint (long color) { ... } public static void Tint (long alpha) { ... } This change detects post-rename collisions inside `KotlinFixups`, keeps the first method deterministically, drops the rest, and emits a new `BG8C02` warning so the user can override the choice via Metadata.xml: // After: one overload kept, warning emitted public static void Tint (long color) { ... } // warning BG8C02: For type 'Widgets', the Kotlin name-mangled method // 'Tint' (originally 'tint-uzYZ1wI') has multiple hash-suffixed // siblings that erase to the same C# signature. Only the first will // be emitted; remove the duplicate via Metadata.xml to suppress this // warning. Non-colliding hash-mangled siblings (different arity, or different underlying primitive) still survive — e.g. `pad(MyDp)` and `pad(MyDp, MyDp)` both bind, and `tint(MyDp)` survives alongside one of the `tint(long)` overloads because they differ in raw type (`F` vs `J`). Step (2) of #1431 — projecting inline-class parameters as their own strongly-typed wrappers so all overloads can coexist — is left for a follow-up. Real Kotlin/Gradle fixture -------------------------- To exercise this against real Kotlin compiler output (not hand-written `.class` files), add a small Gradle project under `tests/Xamarin.Android.Tools.Bytecode-Tests/kotlin-gradle/`: * `build.gradle.kts` pins Kotlin 2.0.21 / JVM 17 and emits `.class` files into `$rootDir/classes`. * `src/main/kotlin/InlineClassCollisions.kt` declares the value classes and `Widgets` object shown above. * A new MSBuild target `BuildKotlinGradleProject` invokes the existing `build-tools/gradle/gradlew(.bat)` (no duplicate wrapper) with `-p kotlin-gradle classes` before `BeforeCompile`. The produced `.class` files are then embedded as test resources. * The `classes/` and `build/` outputs stay out of git via `kotlin-gradle/.gitignore`. A `.gitattributes` entry forces `gradlew`, `*.properties`, `*.kt`, and `*.kts` to LF so the shared wrapper script keeps working on Unix. Tests ----- * `KotlinFixupsTests`: 3 new unit tests cover the collision / no-collision / mixed cases with a `WarningCapture` helper that asserts on BG8C02. * `KotlinInlineClassCollisionTests`: 3 new tests load the freshly Gradle-built `Widgets.class` / `MyColor.class` and assert the expected JVM-level mangling, so we'd notice if Kotlin ever changed the scheme. All 5 `KotlinFixupsTests`, all 78 Bytecode tests, and all 453 generator tests pass. ### Address PR review feedback - KotlinFixups.RemoveCollidingSiblings: collide mangled methods against every other method on the type (not only other mangled siblings), so a mangled tint(MyInlineLong) collapsing to tint(long) collides with a pre-existing non-mangled tint(long) too. Mangled methods are still the only thing ever dropped. - Add MangledMethod_CollidesWithNonMangledOverload test for that case. - Mark the BG8C02-asserting tests [NonParallelizable] because they mutate the global Report.OutputDelegate, matching the convention used by other WarningCapture-style tests in the suite. - Bytecode-Tests.targets: use shared $(GradleWPath) / $(GradleArgs)/ EnvironmentVariables=JAVA_HOME pattern from Directory.Build.props, matching tools/java-source-utils. Removes the ad-hoc OS-switch wrapper selection and makes the Gradle invocation work in environments without java on PATH. - build.gradle.kts: drop jvmToolchain(17) so Gradle uses the JDK the caller already configured via JAVA_HOME (auto-provisioning fails in CI environments without download repositories). ### Keep first colliding sibling, not last The previous fix in 8e8d7a6 walked all of gen.Methods when looking for a conflict and removed the *current* mangled method whenever any other method matched -- including ones that appeared LATER in source. For two mangled siblings A and B, iterating A would find B (later) and remove A, leaving B as the survivor. That contradicted the warning text ('Only the first will be emitted') and was non-deterministic w.r.t. the intended Metadata.xml escape hatch. Fix: only treat the current mangled method as the duplicate when a matching method appears EARLIER in source order (TakeWhile(m => m != method)). This guarantees the first-declared overload always wins, whether the earlier method is mangled or non-mangled. Strengthened CollidingHashSiblings_AreDeduplicated to assert the kept method's JavaName is �dd-AAAAAAA (not just any method named Add). ### Drop mangled when non-mangled overload matches in any order When a mangled method appears BEFORE a non-mangled overload with the same erased signature, the previous TakeWhile-only logic kept both (neither was earlier than itself, and non-mangled methods were never in 'renamed'), producing CS0111. Always prefer dropping the mangled method whenever ANY non-mangled match exists regardless of source order. Fall back to the first-declared-wins rule only when the only collisions are between mangled siblings. Adds MangledMethod_CollidesWithNonMangledOverload_ReversedOrder test covering the previously-broken ordering. ### Compile Kotlin in-process to fix macOS CI failure On macOS the external Kotlin compile daemon fails to start on attempt #1 and prints 'e : The daemon has terminated unexpectedly...' to stderr. Kotlin retries successfully (BUILD SUCCESSFUL), but the stray 'e :' line plus the daemon JVM shutdown causes the gradlew process to exit -1, failing the dotnet build. Setting kotlin.compiler.execution.strategy=in-process makes Kotlin compile inside the Gradle daemon process itself, avoiding the extra JVM and the macOS flake entirely. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent e7c502f commit 6898c1c

14 files changed

Lines changed: 382 additions & 3 deletions

File tree

.gitattributes

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,3 +35,9 @@ Makefile eol=lf
3535
*.wixproj eol=crlf
3636
*.wxs eol=crlf
3737
*.rtf eol=crlf
38+
39+
# Gradle wrapper must stay LF on all platforms (executed under Bash on Unix)
40+
gradlew eol=lf
41+
*.properties eol=lf
42+
*.kt eol=lf
43+
*.kts eol=lf

src/Java.Interop.Localization/Resources.Designer.cs

Lines changed: 9 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

src/Java.Interop.Localization/Resources.resx

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -308,6 +308,10 @@ The following terms should not be translated: Metadata.xml, path.</comment>
308308
<value>For type '{0}', base interface '{1}' is invalid.</value>
309309
<comment>{0}, {1} - .NET types.</comment>
310310
</data>
311+
<data name="Generator_BG8C02" xml:space="preserve">
312+
<value>For type '{0}', the Kotlin name-mangled method '{1}' (originally '{2}') has multiple hash-suffixed siblings that erase to the same C# signature. Only the first will be emitted; remove the duplicate via Metadata.xml to suppress this warning.</value>
313+
<comment>{0} - .NET type. {1} - C# method name. {2} - Original (mangled) JVM method name.</comment>
314+
</data>
311315
<data name="JavaCallableWrappers_XA4200" xml:space="preserve">
312316
<value>Cannot generate Java wrapper for type '{0}'. Only 'class' types are supported.</value>
313317
<comment>{0} - Java type.

src/Java.Interop.Tools.Generator/Utilities/Report.cs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,7 @@ public LocalizedMessage (int code, string value)
7474
public static LocalizedMessage WarningUnknownGenericConstraint => new LocalizedMessage (0x8B00, Localization.Resources.Generator_BG8B00);
7575
public static LocalizedMessage WarningBaseInterfaceNotFound => new LocalizedMessage (0x8C00, Localization.Resources.Generator_BG8C00);
7676
public static LocalizedMessage WarningBaseInterfaceInvalid => new LocalizedMessage (0x8C01, Localization.Resources.Generator_BG8C01);
77+
public static LocalizedMessage WarningKotlinNameMangledCollision => new LocalizedMessage (0x8C02, Localization.Resources.Generator_BG8C02);
7778

7879
public static void LogCodedErrorAndExit (LocalizedMessage message, params string [] args)
7980
=> LogCodedErrorAndExit (message, null, null, args);
Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,70 @@
1+
using System;
2+
using System.Linq;
3+
using NUnit.Framework;
4+
using Xamarin.Android.Tools.Bytecode;
5+
6+
namespace Xamarin.Android.Tools.BytecodeTests
7+
{
8+
// Exercises the real Kotlin bytecode produced by the Gradle fixture under
9+
// kotlin-gradle/ to confirm that the JVM-level mangling we expect (and that
10+
// the generator's KotlinFixups must now de-collide) is actually what kotlinc
11+
// emits for @JvmInline value-class parameters. See dotnet/java-interop#1431.
12+
[TestFixture]
13+
public class KotlinInlineClassCollisionTests : ClassFileFixture
14+
{
15+
[Test]
16+
public void Widgets_HasCollidingHashMangledSiblings ()
17+
{
18+
var klass = LoadClassFile ("Widgets.class");
19+
20+
// Kotlin emits one mangled method per inline-class overload:
21+
// tint-<hash>(J)V for MyColor (ULong-backed)
22+
// tint-<hash>(J)V for MyAlpha (ULong-backed) — collides with MyColor
23+
// tint-<hash>(F)V for MyDp (Float-backed) — unique
24+
var tints = klass.Methods
25+
.Where (m => m.Name.StartsWith ("tint-", StringComparison.Ordinal))
26+
.ToList ();
27+
28+
Assert.AreEqual (3, tints.Count, "Expected three `tint-<hash>` overloads from the Gradle fixture.");
29+
30+
var longTints = tints.Where (m => m.Descriptor == "(J)V").ToList ();
31+
Assert.AreEqual (2, longTints.Count,
32+
"Expected two `tint-<hash>(J)V` siblings (MyColor + MyAlpha) — this is the multi-sibling collision case from dotnet/java-interop#1431.");
33+
34+
Assert.AreEqual (1, tints.Count (m => m.Descriptor == "(F)V"),
35+
"Expected one unique `tint-<hash>(F)V` (MyDp) that should survive deduplication.");
36+
}
37+
38+
[Test]
39+
public void Widgets_HasNonCollidingHashMangledOverloads ()
40+
{
41+
var klass = LoadClassFile ("Widgets.class");
42+
43+
var pads = klass.Methods
44+
.Where (m => m.Name.StartsWith ("pad-", StringComparison.Ordinal))
45+
.ToList ();
46+
47+
Assert.AreEqual (2, pads.Count);
48+
CollectionAssert.AreEquivalent (
49+
new [] { "(F)F", "(FF)F" },
50+
pads.Select (m => m.Descriptor).ToArray (),
51+
"`pad` overloads have distinct JVM signatures and should both survive after rename.");
52+
}
53+
54+
[Test]
55+
public void InlineClasses_AreEmittedAsValueClasses ()
56+
{
57+
// Sanity check that @JvmInline really produced a JvmInline annotation on
58+
// the inline-class type — this is what step (2) of #1431 will key on.
59+
var myColor = LoadClassFile ("MyColor.class");
60+
61+
var annotations = myColor.Attributes
62+
.OfType<RuntimeVisibleAnnotationsAttribute> ()
63+
.SelectMany (a => a.Annotations)
64+
.Select (a => a.Type)
65+
.ToList ();
66+
67+
Assert.Contains ("Lkotlin/jvm/JvmInline;", annotations);
68+
}
69+
}
70+
}

tests/Xamarin.Android.Tools.Bytecode-Tests/Xamarin.Android.Tools.Bytecode-Tests.csproj

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,13 @@
2525

2626
<ItemGroup>
2727
<EmbeddedResource Include="Resources\*" />
28-
<EmbeddedResource Include="kotlin*\**\*.class" />
28+
<EmbeddedResource Include="kotlin\**\*.class" />
29+
<EmbeddedResource Include="kotlin-ThirdParty\**\*.class" />
30+
31+
<EmbeddedResource Include="kotlin-gradle\classes\xat\bytecode\tests\Widgets.class" LogicalName="Xamarin.Android.Tools.BytecodeTests.kotlin-gradle.Widgets.class" />
32+
<EmbeddedResource Include="kotlin-gradle\classes\xat\bytecode\tests\MyColor.class" LogicalName="Xamarin.Android.Tools.BytecodeTests.kotlin-gradle.MyColor.class" />
33+
<EmbeddedResource Include="kotlin-gradle\classes\xat\bytecode\tests\MyAlpha.class" LogicalName="Xamarin.Android.Tools.BytecodeTests.kotlin-gradle.MyAlpha.class" />
34+
<EmbeddedResource Include="kotlin-gradle\classes\xat\bytecode\tests\MyDp.class" LogicalName="Xamarin.Android.Tools.BytecodeTests.kotlin-gradle.MyDp.class" />
2935

3036
<EmbeddedResource Include="$(IntermediateOutputPath)classes\com\xamarin\NotNullClass.class" />
3137
<EmbeddedResource Include="$(IntermediateOutputPath)classes\com\xamarin\IJavaInterface.class" />

tests/Xamarin.Android.Tools.Bytecode-Tests/Xamarin.Android.Tools.Bytecode-Tests.targets

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,12 @@
55
<TestJarNoParameters Include="java/**/*NoParameters.java" />
66
<TestJar Include="java\**\*.java" Exclude="@(TestJarNoParameters);java\android\annotation\NonNull.java;" />
77
<TestKotlinJar Include="kotlin\**\*.kt" />
8+
<TestKotlinGradleSource Include="kotlin-gradle\src\**\*.kt;kotlin-gradle\*.gradle.kts" />
9+
<TestKotlinGradleOutput Include="kotlin-gradle\classes\xat\bytecode\tests\Widgets.class" />
10+
<TestKotlinGradleOutput Include="kotlin-gradle\classes\xat\bytecode\tests\MyColor.class" />
11+
<TestKotlinGradleOutput Include="kotlin-gradle\classes\xat\bytecode\tests\MyAlpha.class" />
12+
<TestKotlinGradleOutput Include="kotlin-gradle\classes\xat\bytecode\tests\MyDp.class" />
13+
<TestKotlinGradleOutput Include="kotlin-gradle\classes\xat\bytecode\tests\InlineClassCollisionsKt.class" />
814
</ItemGroup>
915

1016
<ItemGroup>
@@ -34,6 +40,25 @@
3440
<Exec Command="&quot;$(KotlinCPath)&quot; @(TestKotlinJar->'%(Identity)', ' ') -d &quot;kotlin&quot;" />
3541
</Target>
3642

43+
<!--
44+
Build the real Kotlin/Gradle fixture in kotlin-gradle\ using the shared
45+
Gradle wrapper from build-tools/gradle. Unlike BuildKotlinClasses above,
46+
this is run unconditionally (a JDK is already required for BuildClasses),
47+
so the .class files do not need to be committed. The wrapper downloads
48+
Gradle + Kotlin on first run.
49+
-->
50+
<PropertyGroup>
51+
<_KotlinGradleProjectDir>$(MSBuildThisFileDirectory)kotlin-gradle</_KotlinGradleProjectDir>
52+
</PropertyGroup>
53+
54+
<Target Name="BuildKotlinGradleProject"
55+
BeforeTargets="BeforeCompile"
56+
Inputs="@(TestKotlinGradleSource)"
57+
Outputs="@(TestKotlinGradleOutput)">
58+
<Exec Command="&quot;$(GradleWPath)&quot; $(GradleArgs) -p &quot;$(_KotlinGradleProjectDir)&quot; classes"
59+
EnvironmentVariables="JAVA_HOME=$(JavaSdkDirectory);APP_HOME=$(GradleHome)" />
60+
</Target>
61+
3762
<Target Name="BuildJar"
3863
AfterTargets="BuildClasses"
3964
Inputs="@(_BuildClassOutputs)"
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
# Gradle build outputs - rebuilt from source on every test build.
2+
.gradle/
3+
build/
4+
classes/
Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
plugins {
2+
kotlin("jvm") version "2.0.21"
3+
}
4+
5+
repositories {
6+
mavenCentral()
7+
}
8+
9+
// Don't pin a jvmToolchain -- it would force Gradle to auto-provision a
10+
// matching JDK and fail in CI environments without download repositories
11+
// configured. Use whatever JDK the caller already set in JAVA_HOME (the
12+
// .NET build forwards $(JavaSdkDirectory) for consistency with the rest
13+
// of the repo). Kotlin 2.0.21 targets JVM 11 by default, which is fine
14+
// for the bytecode the tests inspect.
15+
16+
// Emit compiled classes into a stable, predictable location so the
17+
// .NET test harness can load them via ClassFileFixture without needing
18+
// to know the Gradle build directory layout.
19+
tasks.named<org.jetbrains.kotlin.gradle.tasks.KotlinCompile>("compileKotlin") {
20+
destinationDirectory.set(file("$rootDir/classes"))
21+
}
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
# Compile Kotlin in-process to avoid the macOS CI flake where the
2+
# external Kotlin compile daemon fails to start on attempt #1, emits an
3+
# `e :` line to stderr (which dotnet/Exec treats as an error and exit -1)
4+
# even though gradle reports BUILD SUCCESSFUL after the retry.
5+
kotlin.compiler.execution.strategy=in-process

0 commit comments

Comments
 (0)