[TrimmableTypeMap] SafeJavaCollectionFactory per-container construction

[simonrozsival]

Note

Replacement for #12131 because its fork head repository was deleted. This branch now lives in dotnet/android.

What

Reworks the trimmable-typemap Java-collection converter (SafeJavaCollectionFactory) so the AOT-safe construction of JavaList<T> / JavaCollection<T> / JavaDictionary<K,V> is explicit and easy to reason about, and hardens a couple of edge cases. Net change touches the two implementation files, SafeJavaCollectionFactory.cs and ValueTypeFactory.cs, plus a targeted JavaConvertTest regression test (the factory name, public entry point name, and JavaConvert.cs are unchanged vs main).

Construction split into explicit, non-overlapping paths

Container kind is resolved once when the converter is selected, producing a container-specific closure (CreateListFromJniHandle / CreateCollectionFromJniHandle / CreateDictionaryFromJniHandle) that captures only its exact generic arguments. Each method handles exactly one wrapper:

• reference element/key/value args → direct new for the concrete IJavaPeerable rooting anchors; other reference types use MakeGenericType(...) + Activator.CreateInstance, riding NativeAOT's __Canon canonical templates;
• mapped primitive/nullable args → ValueTypeFactory, rooting the exact instantiation with a direct new (no reflection);
• any other value type (e.g. a custom struct) → this factory declines the conversion so JavaConvert preserves its existing untyped collection fallback, covered for list, collection, and dictionary interface targets by a trimmable-path regression test.

Concrete-literal rooting (replaces [DynamicallyAccessedMembers] for reference wrappers)

Each reference path has an if (elementType == typeof(IJavaPeerable)) branch that constructs the concrete JavaList<IJavaPeerable> / JavaCollection<IJavaPeerable> / JavaDictionary<IJavaPeerable,IJavaPeerable> literal. That branch is effectively unreachable at runtime, but being a statically reachable reference to the concrete instantiation it forces the trimmer/ILC to keep the (IntPtr, JniHandleOwnership) activation constructor of the shared __Canon template. The else branch reuses exactly that canonical constructor for every other reference-argument instantiation. This roots the constructor through real code rather than relying on [DAM] flowing through reflection.

ValueTypeFactory's mixed reference/value dictionaries (JavaDictionary<string,int>, …) use the analogous CreateReferenceMixedDictionary<JavaDictionary<IJavaPeerable,T>> / <JavaDictionary<T,IJavaPeerable>> exemplars to root the JavaDictionary<__Canon,T> / JavaDictionary<T,__Canon> templates, replacing the previous static rooting-token fields. Value/value dictionaries continue to be rooted by the statically-emitted GVM cross-product (new JavaDictionary<TKey,T>()).

Hardening

• Reject open / partially-open constructed targets (IList<>, IDictionary<T,int>, …) via a ContainsGenericParameters check up front, so they fail cleanly instead of throwing ArgumentException during activation.

Suppressions

The reflective paths carry the minimum verified suppressions: IL3050 (RequiresDynamicCode) and IL2071 on MakeGenericType, plus IL2072 on the Activator.CreateInstance calls whose constructed types ride the rooted canonical templates. The IL2071 justification states the real cause — MakeGenericType cannot prove the individual runtime type arguments satisfy the [DAM(Constructors)] requirements on the wrapper's element parameters, requirements only exercised by the dynamic-code path (the trimmable path never activates elements through the wrapper).

Validation

• ✅ Builds locally (Mono.Android, Debug, net11.0) with no new warnings and no IL trim/AOT analysis errors.
• ✅ Added NativeAOT reference-only round trips for list, collection, and dictionary targets; expected wrapper types remain open generic definitions so the closed wrappers must be activated through the rooted canonical templates.

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

[Copilot]

Pull request overview

Refactors the trimmable typemap’s Java-collection conversion path to make NativeAOT-safe construction of JavaList<T>, JavaCollection<T>, and JavaDictionary<K,V> explicit (reference-template vs mapped-primitive vs unsupported value-type), and adds a regression test to ensure unsupported value-type container arguments throw NotSupportedException on the trimmable path.

Changes:

• Reworks SafeJavaCollectionFactory into explicit per-container construction paths with early rejection of open/partially-open generic targets.
• Updates ValueTypeFactory mixed reference/value dictionary construction to root the canonical templates via exemplar instantiations rather than static “token” types.
• Adds a JavaConvertTest regression test and unwraps TargetInvocationException so assertions observe the inner exception type.

Show a summary per file

File	Description
tests/Mono.Android-Tests/Mono.Android-Tests/Java.Interop/JavaConvertTest.cs	Adds regression coverage for unsupported value-type generic arguments on the trimmable typemap path; improves reflection-invocation exception unwrapping for accurate assertions.
src/Mono.Android/Java.Interop/ValueTypeFactory.cs	Adjusts mixed dictionary creation to use exemplar-rooted canonical templates and centralizes reflective activation for those mixed shapes.
src/Mono.Android/Java.Interop/SafeJavaCollectionFactory.cs	Splits list/collection/dictionary creation into explicit, non-overlapping paths and hardens behavior for open generic targets and unsupported value-type arguments.

Copilot's findings

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

[simonrozsival]

/review

[github-actions]

✅ Android PR Reviewer completed successfully!

[github-actions]

🤖 Code Review — ⚠️ Minor comments

Verdict: Looks solid. The rework of SafeJavaCollectionFactory into explicit, non-overlapping per-container paths (TryCreateListFromJniHandle / TryCreateCollectionFromJniHandle / TryCreateDictionaryFromJniHandle) is a clear readability win over the previous shape-based dispatch, and the concrete-literal IJavaPeerable rooting branches are a defensible way to keep the __Canon activation constructors alive without leaning on [DAM] reflection flow.

What I verified

• Dictionary argument routing is correct for all four mixed/uniform combinations (<ref,val>, <val,ref>, <val,val>, <ref,ref>), and unsupported value-type args (DateTime) are rejected up front via EnsureReferenceOrPrimitive before any reflection — matching the new FromJniHandle_UnsupportedValueTypeThrows test cases.
• ContainsGenericParameters hardening correctly rejects open/partially-open targets before MakeGenericType.
• Suppression justifications (IL3050/IL2055/IL2072) are specific and accurately describe the __Canon template sharing; the corrected IL2055 rationale reads well.
• Test harness change (ExceptionDispatchInfo unwrapping of TargetInvocationException) is the right way to assert the inner NotSupportedException.

Comments (both 💡, non-blocking)

1. Behavioral change: unsupported value-type containers now throw at conversion time instead of falling through to the non-generic JavaList/JavaCollection/JavaDictionary.FromJniHandle fallback — please confirm no consumer depended on that path. (inline)
2. Null-check inconsistency between SafeJavaCollectionFactory's Activator.CreateInstance results and ValueTypeFactory.CreateReferenceMixedDictionary. (inline)

Notes

• CI checks were still pending at review time — this is not an approval; verify green CI (and ideally the recommended NativeAOT round-trip called out in the PR description) before merge.

Nice, well-documented change overall. 👍

Generated by Android PR Reviewer for #12159 · 112.3 AIC · ⌖ 13.2 AIC · ⊞ 6.8K
_{Comment /review to run again}

[jonathanpeppers]

/review

[github-actions]

✅ Android PR Reviewer completed successfully!

[github-actions]

🤖 Android PR Review — ⚠️ Minor suggestions

Reviewed the reworked trimmable-typemap Java-collection converter (SafeJavaCollectionFactory.cs, ValueTypeFactory.cs) and the new JavaConvertTest regression case.

Assessment

The refactor is clean and the design reasoning is sound. Verified independently:

• Non-overlapping construction paths — TryCreate{List,Collection,Dictionary}FromJniHandle chained via || each guard on genericDefinition and return cleanly on mismatch, so there's no overlap or double-handling.
• Argument filtering is correct — AreSupportedCollectionArguments rejects any non-primitive value-type argument up front, which is why the indexer accesses on PrimitiveTypeFactories [elementType] inside the value paths are safe (no KeyNotFoundException risk). The new test confirms DateTime/UnsupportedValueType containers fall back to the untyped JavaList/JavaDictionary.
• Open/partially-open target rejection — the ContainsGenericParameters check correctly short-circuits IList<>, IDictionary<T,int>, etc.
• Value/value dictionary ordering — traced CreateDictionary → CreateDictionaryWithKey<TKey> and confirmed the resulting JavaDictionary<TKey, T> preserves key/value order.
• Concrete-literal rooting branches — the IJavaPeerable anchors correctly root the __Canon activation constructor while the else branch reuses it.

Findings

• 💡 One inline suggestion: null-guard consistency between the reflective branches in SafeJavaCollectionFactory and ValueTypeFactory.CreateReferenceMixedDictionary.

CI

Azure DevOps pipeline is pending (state: pending) at time of review — not yet green. A NativeAOT publish + runtime round-trip (as the PR description notes) is the key confirmation still outstanding; the added regression test only exercises the unsupported-value-type fallback, not the reference/reference or concrete-wrapper rooting paths on an actual AOT publish.

No blocking issues found in the code itself.

Generated by Android PR Reviewer for #12159 · 88 AIC · ⌖ 13.1 AIC · ⊞ 6.8K
_{Comment /review to run again}

[jonathanpeppers]

/review

[github-actions]

✅ Android PR Reviewer completed successfully!

[github-actions]

⚠️ Review — SafeJavaCollectionFactory per-container construction

Solid, well-reasoned refactor. The split into explicit CreateList/CreateCollection/CreateDictionary paths is much easier to follow than the prior shared reflection helpers, and the concrete-literal IJavaPeerable rooting branches are a legitimate technique to keep the __Canon activation constructor alive without leaning on [DAM] reflection flow. The upfront ContainsGenericParameters rejection is a good hardening addition, and the suppression justifications are unusually thorough and accurate.

I verified the construction logic against the full files:

• AreSupportedCollectionArguments correctly gates value types to the mapped primitive/nullable set, so unsupported value types cleanly decline and fall back to JavaConvert's untyped path (well covered by the new FromJniHandle_UnsupportedValueTypeUsesUntypedFallback cases).
• The value/value, value/reference, and reference/value dictionary dispatch in CreateDictionaryFromJniHandle + CreateReferenceMixedDictionary<TExemplar> all resolve to the correct JavaDictionary<K,V> instantiation and canonical template.
• IntPtr.Zero handles are short-circuited to null before any activation.

No correctness or safety issues found. Two non-blocking 💡 suggestions posted inline:

1. Reuse the ActivationConstructorBinding constant in ValueTypeFactory rather than re-inlining the same BindingFlags.
2. Add a trimmable-path regression test for the pure reference/reference reflective branch (IList<Java.Lang.String> / IDictionary<string,string>), which is the exact rooting path the PR reworks and which the description notes is not yet validated under NativeAOT.

Verdict: ⚠️ Needs Changes is not warranted on correctness — treat this as ✅ with suggestions. Note: CI is still pending on the head commit, so this isn't mergeable until the Azure DevOps pipeline goes green; please confirm no new trim/AOT analysis warnings there, and ideally land the reference/reference test before merge.

Generated by Android PR Reviewer for #12159 · 111.9 AIC · ⌖ 13.2 AIC · ⊞ 6.8K
_{Comment /review to run again}

[jonathanpeppers]

/review

[github-actions]

✅ Android PR Reviewer completed successfully!

[github-actions]

🤖 Review — ⚠️ minor suggestions only

I traced the reworked per-container construction across all key/value combinations (reference/reference, value/reference, reference/value, value/value, and unsupported-value fallback) and the dispatch logic is correct and well-structured. The split into per-container closures plus the concrete-literal IJavaPeerable rooting branches is a clear improvement over the previous reflection-token approach.

Verified

• ✅ No leftover references to the removed ReferenceKeyDictionaryType / ReferenceValueDictionaryType fields or the old SafeJavaCollectionFactory.MakeGenericType / CreateInstance helpers.
• ✅ ContainsGenericParameters early-reject correctly heads off open/partially-open targets before activation.
• ✅ AreSupportedCollectionArguments correctly diverts any unsupported value-type argument (including mixed dictionaries) to the untyped fallback; the new regression test asserts the intended (deliberately non-assignable) fallback shape, matching pre-existing main behavior.
• ✅ Value/value, value/reference, and reference/value dictionary rooting all resolve to the correct JavaDictionary<K,V> instantiation.

Suggestions (non-blocking, posted inline)

• 💡 Performance — resolve the ValueTypeFactory once when the converter closure is built rather than re-looking-it-up on every conversion.
• 💡 Testing — the new reference-argument test only asserts wrapper type identity; adding an element round-trip would harden it against activation/marshaling regressions.

Notes

• CI status is currently pending on the head commit — not green yet. Please confirm the pipeline passes before merge.

Nice work — the suppressions are well-justified and the XML docs make the canonical-template reasoning easy to follow.

Generated by Android PR Reviewer for #12159 · 109.5 AIC · ⌖ 13.1 AIC · ⊞ 6.8K
_{Comment /review to run again}