Skip to content

[NativeAOT] Preserve nested Java class names in trimmable ACW maps - #12187

Merged
jonathanpeppers merged 3 commits into
mainfrom
jonathanpeppers-fix-nested-r8-rules
Jul 21, 2026
Merged

[NativeAOT] Preserve nested Java class names in trimmable ACW maps#12187
jonathanpeppers merged 3 commits into
mainfrom
jonathanpeppers-fix-nested-r8-rules

Conversation

@jonathanpeppers

Copy link
Copy Markdown
Member

Description

NativeAOT trimmable typemaps wrote nested JNI class names to acw-map.txt using Java source notation. For example:

androidx/activity/result/contract/ActivityResultContracts$TakePicture

became:

androidx.activity.result.contract.ActivityResultContracts.TakePicture

GenerateNativeAotProguardConfiguration copied that value into an R8 keep rule, but R8 requires the Java binary name with $. The resulting rules matched no classes, and runtime lookup of ActivityResultContracts$TakePicture failed with ClassNotFoundException.

This was exposed downstream by dotnet/maui#36121 while consuming a dotnet/android build with trimmable typemaps and R8 enabled.

Preserve $ in ACW-map Java keys by adding JniNameToJavaBinaryName(), which replaces JNI package / separators with . without changing nested-class separators. Use binary names consistently for ACW maps and Android manifest class names, while retaining Java source-name conversion for generated Java source and JNI descriptor rendering.

Testing

  • Added helper unit tests for top-level and nested JNI names.
  • Added an acw-map.txt regression for ActivityResultContracts$TakePicture.
  • Added a generated-manifest nested-class regression.
  • Added a NativeAOT ProGuard assertion for the exact $ keep rule.
  • Microsoft.Android.Sdk.TrimmableTypeMap.Tests: 585 passed.
  • GenerateTrimmableTypeMapTests: 13 passed.

  • Useful description of why the change is necessary.
  • Links to downstream failure.
  • Unit tests.

jonathanpeppers and others added 3 commits July 20, 2026 12:05
Use ACW map normalization that preserves JNI nested-class separators so NativeAOT ProGuard rules match R8 binary names.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 6e5f7e9f-ac6c-4c8c-b887-c562959e721b
Centralize JNI-to-Java binary name conversion and use it for ACW maps and Android manifest class names while retaining source-name conversion for generated Java.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 6e5f7e9f-ac6c-4c8c-b887-c562959e721b
Cover the same top-level JNI names as the Java source-name helper while asserting that binary conversion preserves nested-class separators.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 6e5f7e9f-ac6c-4c8c-b887-c562959e721b
Copilot AI review requested due to automatic review settings July 20, 2026 18:37

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Fixes NativeAOT trimmable typemap output so nested Java class names are preserved in Java binary form (using $), which is required by R8 keep rules and runtime class lookup. This prevents ClassNotFoundException scenarios when nested types are referenced through acw-map.txt and then translated into ProGuard/R8 rules.

Changes:

  • Added JniSignatureHelper.JniNameToJavaBinaryName() to convert JNI names to Java binary names (/., preserve $).
  • Updated typemap generators (ACW map + manifest generation + lookup name mapping) to use Java binary names consistently where runtime/R8 expects them.
  • Added regression tests covering nested-class behavior for manifest generation, ACW-map output, and NativeAOT ProGuard rule emission.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.

Show a summary per file
File Description
tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests/Generator/ManifestGeneratorTests.cs Adds regression ensuring manifest android:name preserves $ for nested activity types.
tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests/Generator/JcwJavaSourceGeneratorTests.cs Adds unit coverage for new JNI→Java binary name conversion helper.
tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests/Generator/AcwMapWriterTests.cs Adds regression ensuring acw-map.txt preserves $ in Java keys for nested classes.
src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Tasks/GenerateTrimmableTypeMapTests.cs Extends NativeAOT ProGuard configuration test to assert exact $ keep rule derived from acw-map.txt.
src/Microsoft.Android.Sdk.TrimmableTypeMap/TrimmableTypeMapGenerator.cs Switches manifest lookup name population to Java binary naming while retaining source-name handling where needed.
src/Microsoft.Android.Sdk.TrimmableTypeMap/Generator/ManifestGenerator.cs Uses Java binary names for component names and compat-name rewriting to match runtime expectations.
src/Microsoft.Android.Sdk.TrimmableTypeMap/Generator/JniSignatureHelper.cs Introduces JniNameToJavaBinaryName() helper with documentation and examples.
src/Microsoft.Android.Sdk.TrimmableTypeMap/Generator/ComponentElementBuilder.cs Ensures application/instrumentation android:name values use Java binary names.
src/Microsoft.Android.Sdk.TrimmableTypeMap/Generator/AssemblyLevelElementBuilder.cs Ensures assembly-level Application properties that reference types write Java binary names.
src/Microsoft.Android.Sdk.TrimmableTypeMap/Generator/AcwMapWriter.cs Writes ACW map Java keys using Java binary names, preserving $ for nested types.

@jonathanpeppers jonathanpeppers added the ready-to-review This PR is ready to review/merge, I think any CI failures are just flaky (ignorable). label Jul 20, 2026
@jonathanpeppers

Copy link
Copy Markdown
Member Author

/review

@github-actions

github-actions Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Android PR Reviewer completed successfully!

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ LGTM

Solid, well-targeted fix. The root cause is correctly identified: JniNameToJavaName collapses both package (/) and nested-class ($) separators to ., which is right for Java source and JNI descriptors but wrong for ACW-map keys, manifest class names, and R8 keep rules, all of which require Java binary names with $ preserved. Introducing JniNameToJavaBinaryName and routing the binary-name consumers through it is the correct separation.

What I verified

  • Correctness — Every switched call site (AcwMapWriter, AssemblyLevelElementBuilder, ComponentElementBuilder, ManifestGenerator, AddJniLookupNames) genuinely needs the binary name. JniNameToJavaName is correctly retained where Java source is emitted (ApplicationRegistration.java generation at TrimmableTypeMapGenerator.cs:91, JCW base/interface names).
  • No behavior regression in the refactorAddJniLookupNames previously built the manifest name as package + "." + GetJavaSimpleName(...), and GetJavaSimpleName already preserved $; so the old code produced the binary name too. The new JniNameToJavaBinaryName(jniName) is equivalent (including the no-package case) but clearer, and it still registers both binary and source forms in the lookup dictionary for backward-compatible resolution.
  • Tests — Good regression coverage across all three layers: helper unit test, acw-map.txt (ActivityResultContracts$TakePicture), nested manifest activity name, and the exact NativeAOT ProGuard -keep rule with $. This maps directly to the downstream dotnet/maui#36121 failure.

Notes

  • CI is currently pending (Azure DevOps dnceng-public) — not green yet. Not merge-blocking from a code standpoint, but confirm the pipeline goes green before merge.
  • One optional 💡 testing suggestion posted inline (doubly-nested class case).

Issue counts: ❌ 0 · ⚠️ 0 · 💡 1

Generated by Android PR Reviewer for #12187 · 81 AIC · ⌖ 13 AIC · ⊞ 6.8K
Comment /review to run again

@jonathanpeppers

Copy link
Copy Markdown
Member Author

The failing CI test is:

Xamarin.Android.Build.Tests.MarshalMethodsGCHangTests.MarshalMethodsAppRuns(CoreCLR)
Output did not contain XXX:OnStart done!

This is an existing flaky emulator/CoreCLR test rather than a regression from this PR:

  • It failed both the initial execution and automatic retry after approximately five minutes.
  • The latest direct main execution passed.
  • Over the last seven days, the test has 13 failures and 125 passes across PR builds (about a 9% failure rate).
  • An unrelated PR build for Remove redundant null-forgiving operator on cts in Program.cs #12190, which only changes a null-forgiving operator, had the exact same assertion and timeout.
  • This PR does not modify the failing test, marshal methods, GC behavior, CoreCLR runtime code, or device-test infrastructure.

The failure is unrelated to the trimmable typemap/ProGuard changes, so it should not block merging this PR.

@jonathanpeppers
jonathanpeppers merged commit 9ba8363 into main Jul 21, 2026
43 of 45 checks passed
@jonathanpeppers
jonathanpeppers deleted the jonathanpeppers-fix-nested-r8-rules branch July 21, 2026 13:21
jonathanpeppers added a commit that referenced this pull request Jul 21, 2026
## Description

PR #12187 exposed a reviewer blind spot: replacing an inline JNI-name conversion with a source-name helper silently changed nested Java binary names. The reviewer guidance did not clearly distinguish the representations or require tests at the consuming boundary.

Teach `android-reviewer` to load interop guidance for trimmable typemap changes, distinguish JNI internal, Java binary, Java source, and managed nested names, and require nested-name assertions on final manifest, map, rule, or generated-source output.

## Evaluation

A blinded replay of the original regression commit using the same model and review prompt detected the bug in 9/10 trials with this concise guidance, compared with 7/10 using the original skill. Blocking findings improved from 7/10 to 8/10. The sample is directional rather than statistically conclusive.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-to-review This PR is ready to review/merge, I think any CI failures are just flaky (ignorable).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants