[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

Per-container private methods (TryCreateListFromJniHandle / TryCreateCollectionFromJniHandle / TryCreateDictionaryFromJniHandle) chained via ... || ..., each handling exactly one wrapper:

• reference element/key/value args → typeof(JavaList<>).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) → NotSupportedException (no AOT-unsafe reflection fallback), covered for list, collection, and dictionary interface/concrete 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 IL2055 on MakeGenericType, plus IL2072 on the Activator.CreateInstance calls whose constructed types ride the rooted canonical templates. The IL2055 justification is corrected to state the real cause — MakeGenericType cannot prove the runtime element type satisfies the [DAM(Constructors)] requirement on the wrapper's element parameter, a requirement 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.
• ⚠️ A NativeAOT publish + runtime round-trip (e.g. marshaling JavaList<string>, JavaDictionary<string,object>, JavaDictionary<string,int>) is still recommended to confirm the concrete-literal anchor roots the canonical-shared activation constructor as intended; relying on CI/device tests for full confirmation.

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}

[github-actions]

🤖 💡 Resource management — Minor consistency gap: the reference-path Activator.CreateInstance (...) results here (and in the JavaCollection/JavaDictionary variants) are assigned directly to result with no null check, whereas the sibling ValueTypeFactory.CreateReferenceMixedDictionary explicitly throws InvalidOperationException when Activator.CreateInstance returns null. If activation ever returns null, the converter here silently yields null instead of a diagnosable error. Consider mirroring the ValueTypeFactory null-check for symmetry and clearer diagnostics.

(Rule: Consistent error handling across sibling code paths)