Skip to content

[TrimmableTypeMap] Fix trimmable primitive and alias arrays#11985

Merged
jonathanpeppers merged 1 commit into
mainfrom
android-trimmable-typemap-fix-array2
Jul 6, 2026
Merged

[TrimmableTypeMap] Fix trimmable primitive and alias arrays#11985
jonathanpeppers merged 1 commit into
mainfrom
android-trimmable-typemap-fix-array2

Conversation

@simonrozsival

Copy link
Copy Markdown
Member

Fix trimmable typemap array proxy emission for NativeAOT array lookups.

Changes:

  • Emit primitive array proxies for Mono.Android primitive aliases: byte, ushort, uint, and ulong.
  • Emit array entries for alias-group peers such as Java.Lang.Object instead of skipping the whole group.
  • Allow primitive proxy metadata to model zero or more Java.Interop concrete array wrappers.
  • Improve the NativeAOT missing-array-proxy diagnostic with leaf element type and rank.

Validation:

  • dotnet test tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests.csproj --filter 'FullyQualifiedName~ModelBuilderTests+ArrayEntries' --verbosity normal

Emit trimmable array proxy entries for primitive aliases used by Mono.Android JNIEnv APIs and for alias-group peers such as Java.Lang.Object. Update array proxy metadata to support primitive proxies without Java.Interop concrete wrapper types and improve the NativeAOT missing-proxy diagnostic.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@simonrozsival simonrozsival changed the title [typemap] Fix trimmable primitive and alias arrays [TrimmableTypeMap] Fix trimmable primitive and alias arrays Jul 4, 2026
@simonrozsival simonrozsival marked this pull request as ready for review July 5, 2026 10:28
Copilot AI review requested due to automatic review settings July 5, 2026 10:28
@simonrozsival

Copy link
Copy Markdown
Member Author

/review

@github-actions

github-actions Bot commented Jul 5, 2026

Copy link
Copy Markdown

Android PR Reviewer completed successfully!

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

This PR fixes TrimmableTypeMap array-proxy emission and diagnostics, primarily to ensure NativeAOT can resolve array proxies for primitive aliases and for alias-group peers (multiple managed peers sharing a JNI name) without skipping needed entries.

Changes:

  • Emit per-managed-type array entries for alias groups (instead of skipping the entire group).
  • Extend primitive array proxy synthesis to include Mono.Android primitive aliases (byte/ushort/uint/ulong) and allow “zero concrete wrapper types” for alias proxies.
  • Improve the NativeAOT “missing array proxy” exception by including leaf element type and computed rank.
Show a summary per file
File Description
tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests/Generator/TypeMapModelBuilderTests.cs Updates/extends tests to cover alias-group array entries and primitive-alias array proxy emission (including “no concrete wrapper” cases).
src/Mono.Android/Android.Runtime/JNIEnv.cs Improves NativeAOT missing-array-proxy diagnostic to include leaf element type + rank.
src/Microsoft.Android.Sdk.TrimmableTypeMap/Generator/TypeMapAssemblyEmitter.cs Adjusts emitted array-proxy type references to support 0..N concrete wrapper array types.
src/Microsoft.Android.Sdk.TrimmableTypeMap/Generator/ModelBuilder.cs Changes array entry emission to operate per peer within alias groups; expands primitive proxy modeling to include aliases and 0..N concrete wrappers.
src/Microsoft.Android.Sdk.TrimmableTypeMap/Generator/Model/TypeMapAssemblyData.cs Updates primitive array proxy metadata model to a list of concrete array wrapper types (default empty).

Copilot's findings

  • Files reviewed: 5/5 changed files
  • Comments generated: 0

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🤖 Android PR Review — ⚠️ Needs Changes (pending CI only)

Independent read of the change. This closes two NativeAOT array-proxy gaps in the trimmable typemap:

  1. Unsigned primitive aliases — adds Byte, UInt16, UInt32, UInt64 proxies that reuse the JNI keyword (B/S/I/J) of their signed counterparts, so byte[]/ushort[]/uint[]/ulong[] can be created at runtime instead of throwing NotSupportedException.
  2. Alias-group arraysEmitArrayEntries no longer skips whole alias groups; it emits per managed type. Because array map keys are managed-type-qualified (not JNI-keyed), each peer gets a distinct key, so the original "duplicate JNI array keys" deferral is genuinely resolved.
  3. Model shapePrimitiveArrayProxyData.ConcreteArrayTypeConcreteArrayTypes (0..N), letting unsigned aliases associate zero concrete wrappers without stealing JavaSByteArray/JavaInt16Array/... from the signed variant.
  4. Diagnostic — the "no array proxy entry" message now reports the leaf element type and rank.

Correctness — verified end-to-end:

  • ✅ The diagnostic is now accurate: TryGetArrayProxy keys on the leaf type with rankIndex = additionalRank-1 + nesting, and SingleUniverseTypeMap indexes arrayMapsByRank[rankIndex] (0-based → __ArrayMapRank{N} 1-based). The new GetArrayRank returns rankIndex+1, so both the leaf type and the printed rank line up with the actual lookup.
  • ✅ The empty-ConcreteArrayTypes design correctly avoids double-associating a concrete wrapper to both the signed and unsigned proxy (the tests explicitly assert JavaSByteArray maps to SByte, not Byte).
  • ✅ Alias-group keys are distinct per managed type → no duplicate TypeMap / TypeMapAssociation entries; the GenerateArrayEntries/IsGenericDefinition guards are preserved in the extracted EmitArrayEntriesForPeer.
  • ✅ Both parallel emitters (ModelBuilder.GetArrayTypeReferences and TypeMapAssemblyEmitter.GetArrayProxyTypes) were updated identically, and JNIEnv.GetArrayRank's new out leafElementType is purely additive.

Tests: Strong coverage — 36 = 12 × 3 primitive count, per-managed-type alias emission (java/lang/Object group), and a full-pipeline round-trip proving the unsigned aliases emit proxies without a concrete wrapper association. 👍

CI: ⚠️ One leg — dotnet-android (Java.Interop Tests Windows - .NET) — failed on the latest run (buildId 1494399) at external\Java.Interop\tools\java-source-utils\java-source-utils.targets(8,5): error MSB3073 — a Gradle jar build failure. This PR touches neither Java source-utils nor any .gradle/.targets in that tool, and the previous full pipeline run (buildId 1494242) was entirely green, so this is almost certainly a transient/infrastructure failure. A re-run should clear it — but per review policy the PR isn't mergeable until CI is green.

Inline comments: 2 💡 documentation-only suggestions — nothing blocking on the code itself.

Clean, well-scoped fix with excellent test coverage. The only actionable item is getting the (unrelated) Windows leg green. 👏

Generated by Android PR Reviewer for #11985 · 252.8 AIC · ⌖ 28.5 AIC · ⊞ 6.8K
Comment /review to run again

.Where (e => e.MapKey.StartsWith ("System.", StringComparison.Ordinal) && e.AnchorRank is not null)
.ToList ();
Assert.Equal (24, primitiveEntries.Count); // 8 primitive keywords × 3 ranks
Assert.Equal (36, primitiveEntries.Count);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🤖 💡 Testing — The prior assertion self-documented its count (// 8 primitive keywords × 3 ranks); the bump to 36 dropped that note. Restoring // 12 primitive keywords × 3 ranks lets a reader validate the expected total at a glance instead of reverse-engineering where 36 comes from.

Rule: Test assertions should be self-documenting

new ("D", "Double", "System.Double", "Java.Interop.JavaDoubleArray"),
new ("Z", "Boolean", "System.Boolean", ["Java.Interop.JavaBooleanArray"]),
new ("B", "SByte", "System.SByte", ["Java.Interop.JavaSByteArray"]),
new ("B", "Byte", "System.Byte", []),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🤖 💡 Documentation — The empty concrete-array-type list on the four unsigned aliases (Byte, UInt16, UInt32, UInt64) is deliberate but non-obvious: each shares its JNI keyword (B/S/I/J) with the signed variant, which owns the sole Java.Interop.Java*Array wrapper (e.g. JavaSByteArray), so the alias must not re-associate that concrete wrapper. A one-line comment on the table would keep the [] from reading like an oversight to a future maintainer.

Rule: Comments explain "why", not "what"

@simonrozsival simonrozsival added the ready-to-review This PR is ready to review/merge, I think any CI failures are just flaky (ignorable). label Jul 5, 2026
@jonathanpeppers jonathanpeppers merged commit 3ca5659 into main Jul 6, 2026
43 checks passed
@jonathanpeppers jonathanpeppers deleted the android-trimmable-typemap-fix-array2 branch July 6, 2026 13:00
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