[TrimmableTypeMap] JavaCast/JavaAs + container support#11225
[TrimmableTypeMap] JavaCast/JavaAs + container support#11225simonrozsival wants to merge 4 commits intomainfrom
Conversation
4a2262f to
cd6b318
Compare
Improve CreatePeer under the trimmable typemap to match legacy JavaCast/JavaAs contracts: - Bad-cast disambiguation: distinguish incompatible Java types (return null → InvalidCastException) from missing typemap entries (ArgumentException) and generator gaps (NotSupportedException). - Closed-generic activation: when the proxy targets an open generic (e.g. JavaList<>), activate the closed targetType (e.g. JavaList<int>) via reflection using the (IntPtr, JniHandleOwnership) ctor. The [DynamicallyAccessedMembers(Constructors)] annotation on targetType guarantees the trimmer preserves the ctor metadata. - Type resolution: map IJavaPeerable/object/Exception to concrete peer types before proxy lookup, mirroring the legacy GetPeerType behavior. - TargetTypeMatches: restructure so open-generic proxies match only closed instantiations of their definition. - FindClass safety: catch ClassNotFoundException in TryGetProxyFromTargetType for types not present in the APK. - Don't synthesize activation from inherited ctors: match legacy GetConstructor() which doesn't find inherited constructors. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ranks - Resolve activation ctor on invoker types for interface peers, so the generator picks the correct ctor signature (XA vs JI style). - Skip JNI keyword types (Z, B, C, S, I, J, F, D) in the scanner. These single-letter JNI names collided with primitive type handling in JniRuntime.JniTypeManager. - Skip all ArrayRank>0 types (JavaBooleanArray, JavaArray<>, etc.) — they were incorrectly added as aliases for java/lang/Object, causing alias resolution to select the open-generic JavaArray<> proxy. - Don't synthesize activation from inherited ctors in the generator, matching legacy Type.GetConstructor() behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
32fae2e to
f4cba0b
Compare
Add trimmable-typemap variant of GetThis.java that uses mono.android.Runtime.register instead of ManagedPeer.registerNativeMembers. Java.Interop-Tests.targets swaps variants based on $(_AndroidTypeMapImplementation). Re-enabled tests: - JavaCast_BadInterfaceCast (bad-cast disambiguation) - JavaCast_BaseToGenericWrapper (closed-generic activation) - JavaCast_CheckForManagedSubclasses (bad-cast disambiguation) - JavaCast_InvalidTypeCastThrows (bad-cast disambiguation) - JavaAs_Exceptions (inherited ctor fix) - DisposeAccessesThis (trimmable GetThis.java) - CreateGenericValue (ArrayRank scanner fix) Updated remaining exclusion comments with accurate root-cause descriptions. Removed resolved TODO comments from test files. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
f4cba0b to
c5062a6
Compare
There was a problem hiding this comment.
Pull request overview
Improves the trimmable typemap path so JavaCast/JavaAs follow their expected contracts again (null vs InvalidCastException vs ArgumentException vs NotSupportedException), adds support for activating closed generic peer types, and updates the scanner/generator to avoid typemap collisions from JNI array wrapper types. This enables re-running previously excluded device tests in the trimmable lane.
Changes:
- Refines trimmable
CreatePeerfailure classification (incompatible cast vs missing mapping vs missing proxy) and adds reflection-based activation for closed generics. - Updates typemap scanner/generator to (a) skip
[JniTypeSignature(ArrayRank>0)]array wrapper types and (b) handle interface invoker activation ctors + avoid inherited-ctor synthesis. - Adjusts test infrastructure: re-enables exclusions, adds scanner coverage, and conditionally swaps an Android-trimmable
GetThis.javaimplementation into the test JAR.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Mono.Android-Tests/Mono.Android-Tests/Xamarin.Android.RuntimeTests/NUnitInstrumentation.cs | Updates trimmable-lane test exclusions/comments; removes exclusions for fixed cast/as behaviors. |
| tests/Mono.Android-Tests/Mono.Android-Tests/Java.Interop/JavaObjectExtensionsTests.cs | Removes resolved TODO comments around trimmable typemap casting behavior. |
| tests/Mono.Android-Tests/Java.Interop-Tests/java-trimmable/net/dot/jni/test/GetThis.java | Adds Android-trimmable GetThis Java fixture variant without desktop-only native registration. |
| tests/Mono.Android-Tests/Java.Interop-Tests/Java.Interop-Tests.targets | Conditionally includes the trimmable GetThis.java and excludes the desktop variant when needed. |
| tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests/TestFixtures/TestTypes.cs | Adds array-rank fixture types to validate scanner exclusion behavior. |
| tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests/Scanner/JavaPeerScannerTests.cs | Adds test ensuring [JniTypeSignature(ArrayRank>0)] types are excluded by the scanner. |
| src/Mono.Android/Microsoft.Android.Runtime/TrimmableTypeMap.cs | Improves proxy matching for open generics and disambiguates missing Java classes during FindClass. |
| src/Mono.Android/Microsoft.Android.Runtime/JavaMarshalValueManager.cs | Adds ResolvePeerType, reflection activation for closed generics, and cast failure disambiguation under trimmable typemap. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/Scanner/JavaPeerScanner.cs | Skips array-rank [JniTypeSignature] wrappers; resolves activation ctors via invoker for interface peers. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/Scanner/AssemblyIndex.cs | Records ArrayRank>0 as IsArrayType on parsed [JniTypeSignature] attributes. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/Generator/TypeMapAssemblyEmitter.cs | Stops synthesizing activation when ctor is only on a base type; emits “no activation” to match legacy behavior. |
|
/review |
|
✅ Android PR Reviewer completed successfully! |
There was a problem hiding this comment.
✅ LGTM — solid work
High-quality PR that fixes real JavaCast/JavaAs contract violations under the trimmable typemap. The three-way error disambiguation (incompatible cast → null, missing typemap → ArgumentException, compatible but no proxy → NotSupportedException) correctly matches the legacy JniValueManager.CreatePeer contract. The TargetTypeMatches refactoring properly prevents open-generic proxies from over-matching, and the scanner ArrayRank filtering closes the primitive-array keyword collision.
Issue summary
| Severity | Count |
|---|---|
| 1 |
What's good
- Well-documented error paths with clear comments linking to the cast contract
ResolvePeerTypecorrectly mirrors legacyGetPeerTypefor universal interfacesTargetTypeMatchesrestructure fixes theJavaArray<>→JavaObjectover-match bug- Scanner test coverage with both keyword and non-keyword array fixtures
- Clean trimmable
GetThis.javavariant with thorough MSBuild target conditioning - 10 previously-excluded device tests re-enabled with passing results
Minor note
The PR description lists 7 re-enabled tests, but the diff actually re-enables 10 (also CreateValue, JavaAs, and JavaAs_InstanceThatDoesNotImplementInterfaceReturnsNull). Worth updating the description for traceability.
CI
All 6 checks pass ✅ (including internal dotnet-android pipeline).
Generated by Android PR Reviewer for issue #11225 · ● 4.6M
| try { | ||
| try { | ||
| targetClass = JniEnvironment.Types.FindClass (targetJniName); | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
🤖 Exception but the analogous FindClass call in TrimmableTypeMap.cs:243 catches only Java.Lang.ClassNotFoundException. The broad catch here could mask unexpected JNI failures (e.g., NoClassDefFoundError from a class whose static initializer failed, or OutOfMemoryError) by wrapping them as ArgumentException, making the root cause harder to diagnose.
Consider narrowing to catch (Java.Lang.ClassNotFoundException e) for consistency with TrimmableTypeMap.TryGetProxyFromTargetType, or if intentionally broader, add a comment explaining why.
Rule: Challenge exception swallowing / Differentiate similar error messages (Postmortem #7)
There was a problem hiding this comment.
@copilot address this comment - change the code so that we catch Java.Lang.ClassNotFoundException
Reverts the runtime swap and factory-method removal from the previous
commit. Keeps the new `ITypeMapWithAliasing.TryGetType` raw lookup and
`TrimmableTypeMap.TryGetArrayType` helper as scaffolding for the
eventual ILLink-fixed approach.
The speculative `[L<jni>;` / `[[L<jni>;` / `[[[L<jni>;` TypeMap entries
crash ILLink with:
System.NotSupportedException: TypeDefinition cannot be resolved from
'Mono.Cecil.ArrayType' type
at Mono.Linker.LinkContext.Resolve(TypeReference typeReference)
at Mono.Linker.TypeMapHandler.RecordTypeMapEntry(...) (3-arg form)
at Mono.Linker.TypeMapHandler.MarkTypeMapAttribute(...) (2-arg form)
at Mono.Linker.TypeMapHandler.ProcessExternalTypeMapGroupSeen(...)
Both 2-arg and 3-arg TypeMap forms are affected — `MarkTypeMapAttribute`
calls `LinkContext.Resolve` on the `TargetType` slot (constructor arg
index 1) for any TypeMap, and Cecil's `ArrayType` is not a
`TypeDefinition`. There is no shape of `TypeMapAttribute` today that
accepts a closed array `Type`.
`EmitArrayEntries` is now a documented no-op.
`JavaPeerContainerFactory<T>.CreateArray` and
`CreateHigherRankArray` are restored.
`JNIEnv.ArrayCreateInstance` falls back to the legacy per-T factory
under trimmable.
Trimmable + CoreCLR lane after revert: 917 total, 0 errors, 3 failures
(pre-existing `TryGetJniNameForManagedType_*`, out of scope — same
baseline as #11225).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
`JNIEnv.ArrayCreateInstance` now branches on `RuntimeFeature.IsDynamicCodeSupported`: * CoreCLR / Mono (true) — `Array.CreateInstance(elementType, length)`. No typemap roundtrip; supports unlimited array rank. * NativeAOT (false) — typemap lookup → AOT-safe `Array.CreateInstanceFromArrayType`. Capped at the emitted ranks (1–3); miss throws `NotSupportedException` with diagnostic. The runtime fork lets us avoid emitting (and paying for) speculative array TypeMap entries on CoreCLR-only builds, where the runtime type loader can construct any `T[]` dynamically anyway. * `ITypeMapWithAliasing.TryGetArrayType(string jniElementTypeName, int rank, out Type? arrayType)` — new abstraction for the per-rank array dictionary lookup. `SingleUniverseTypeMap` carries three nullable `IReadOnlyDictionary<string, Type>?` fields (rank 1, 2, 3) populated at `TrimmableTypeMap.Initialize` time; `AggregateTypeMap` does first-wins iteration. * `TrimmableTypeMap.TryGetArrayType(Type elementType, out Type?)` — walks down `elementType.IsArray` / `GetElementType()` to find the leaf type and array depth, resolves the leaf JNI element name (primitive static dict OR `TryGetJniNameForManagedType` wrapped), and delegates the (jni, rank+1) lookup to the interface. * `TrimmableTypeMap.Initialize` gains 5-arg overloads (single + aggregate) accepting the per-rank dicts. Existing 2-arg overloads stay as wrappers passing null per-rank dicts so older generated assemblies keep working. * `RootTypeMapAssemblyGenerator`: the generated `TypeMapLoader.Initialize` IL now branches on the new `emitArrayEntries` flag. When true, it collects per-assembly `__ArrayMapRank{1,2,3}` sentinels via `TypeMapping.GetOrCreateExternalTypeMapping<__ArrayMapRank{N}>()` and passes the resulting dicts to the 5-arg `TrimmableTypeMap.Initialize`. Aggregate (Debug) path is fully implemented; merged-universe (Release) path throws at generation time with a clear message — wiring the shared-universe array sentinels is a small follow-up. * `GenerateTrimmableTypeMap` MSBuild task: new `EmitArrayEntries` property forwarded through `TrimmableTypeMapGenerator.Execute` and `RootTypeMapAssemblyGenerator.Generate`. SDK target sets it to `$(PublishAot)`. * `JavaPeerContainerFactory<T>.CreateArray` and `CreateHigherRankArray` deleted. Container methods (`CreateList`, `CreateCollection`, `CreateDictionary*`) stay untouched — those are tracked separately in #11234. Validation: * 445 / 445 generator unit tests pass. * Trimmable + CoreCLR `RunTestApp` lane on emulator: **917 total / 0 errors / 3 failures** (pre-existing `TryGetJniNameForManagedType_*`, called out as out-of-scope in #11225). No regression. * The NativeAOT branch path is gated on dotnet/runtime#126380 (ships in .NET 11 nightly preview.5+); validated with the playground repro separately. Tracking: #11234 Phase 2 (arrays only). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Allow exact type matches before applying open-generic target matching so alias lookup resolves open generic peers such as JavaList<>. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
Fix
JavaCast/JavaAsbehavior under the trimmable typemap and re-enable 7 device tests that were previously excluded. Consolidates the JavaCast/JavaAs work and the trimmable container support that was previously split across two PRs.Cast error disambiguation
When a trimmable typemap proxy lookup fails,
CreatePeernow distinguishes between:null(JavaCast wraps toInvalidCastException, JavaAs returnsnull)ArgumentExceptionNotSupportedExceptionPreviously all failures threw
NotSupportedException, breakingJavaCast/JavaAscontracts.Also adds
ResolvePeerTypeto map universal interfaces (IJavaPeerable,object,Exception) to concrete peer types before proxy lookup, and catchesClassNotFoundExceptionfromFindClassso missing Java classes surface asArgumentException.Closed-generic peer activation
Open-generic proxies (e.g.
JavaList<>) cannotnewobjclosed instantiations (e.g.JavaList<int>) in generated IL. AddsActivateUsingReflectionwhich usesType.GetConstructor+ConstructorInfo.Invokeon the closedtargetType— safe because[DynamicallyAccessedMembers(Constructors)]annotations on the parameter ensure the trimmer preserves the ctor metadata.TargetTypeMatchesis restructured so open-generic proxies match only closed instantiations of their definition — preventing incorrect matches likeJavaArray<>matching anyJavaObject-targeted cast.Interface invoker activation ctor
Interface peers have no constructors.
TryResolveActivationCtorOnInvokerresolves the activation ctor from the invoker type so the generator picks the correct ctor signature (XA-style vs JI-style).Don't synthesize activation from inherited ctors
The generator no longer emits
GetUninitializedObject+ base-ctor IL when the activation ctor is on a base type (IsOnLeafType=false) and there's no invoker type. This matches legacyType.GetConstructor()behavior, which doesn't find inherited constructors. Without this, types likeMyJavaInterfaceImplthat lack their own activation ctor would unexpectedly succeed in the trimmable path.Scanner: skip JNI keyword and array types
The scanner was incorrectly adding primitive array wrapper types (
JavaBooleanArrayetc.) to the typemap under single-letter keyword keys (Z, B, C, S, I, J, F, D), colliding with the built-in primitive type handling inJniRuntime.JniTypeManager.GetPrimitiveArrayTypesForSimpleReference. Skip all[JniTypeSignature]types withArrayRank > 0(which covers both primitive array wrappers andJavaArray<>/JavaObjectArray<>/JavaPrimitiveArray<>).Trimmable GetThis.java
The
net.dot.jni.test.GetThisJava class (in the Java.Interop submodule) registers native methods viaManagedPeer.registerNativeMembers, which the trimmable path rejects. Adds an Android-trimmable variant that omits the desktop static block.Java.Interop-Tests.targetsswaps variants based on$(_AndroidTypeMapImplementation).Test exclusion updates
Re-enabled (7 tests):
JavaObjectExtensionsTests.JavaCast_BadInterfaceCastJavaObjectExtensionsTests.JavaCast_BaseToGenericWrapper(closed-generic activation)JavaObjectExtensionsTests.JavaCast_CheckForManagedSubclassesJavaObjectExtensionsTests.JavaCast_InvalidTypeCastThrowsJavaPeerableExtensionsTests.JavaAs_Exceptions(inherited-ctor fix)JavaObjectTest.DisposeAccessesThis(trimmable GetThis.java)JniValueMarshaler_IJavaPeerable_ContractTests.CreateGenericValue(ArrayRank scanner fix)Updated remaining exclusion comments with accurate root-cause descriptions. Removed resolved TODO comments from
JavaObjectExtensionsTests.The remaining
JavaProxyObject/JavaProxyThrowableexclusions are tracked at #11170.Test results
Trimmable + CoreCLR lane: 917 total, 0 errors, 3 failures (pre-existing
TryGetJniNameForManagedType_*, out of scope).Follow-up
JavaPeerContainerFactory<T>is still used and causes per-type bloat under NativeAOT (~2000 instantiations for a typical MAUI app). Tracked at #11234 for a separate refactor PR.