[TrimmableTypeMap] SafeJavaCollectionFactory per-container construction

[simonrozsival]

Note

Draft. Independent of #12126 (the JavaListTest fix); this PR targets main directly.

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

[simonrozsival]

/review

[github-actions]

✅ Android PR Reviewer completed successfully!

[Copilot]

Pull request overview

Updates the trimmable-typemap Java collection construction path in src/Mono.Android/Java.Interop by refactoring how JavaList<> / JavaCollection<> / JavaDictionary<,> wrappers are instantiated from JNI handles, including new reflection/AOT-trimming suppressions and a reworked mixed reference/value dictionary creation helper.

Changes:

• Refactors SafeJavaCollectionFactory to use explicit per-container creation methods (TryCreate*FromJniHandle) with stricter support checks and clearer error behavior for unsupported value-type generic arguments.
• Reworks ValueTypeFactory<T> mixed dictionary creation to root canonical reference/value shapes via exemplar instantiations and centralized Activator.CreateInstance logic (with detailed suppression justifications).
• Adds/updates documentation and supporting imports for the new construction paths.

Show a summary per file

File	Description
src/Mono.Android/Java.Interop/SafeJavaCollectionFactory.cs	Refactors converter selection and per-container wrapper creation; introduces additional reflection usage that needs trim-analyzer suppressions.
src/Mono.Android/Java.Interop/ValueTypeFactory.cs	Reworks mixed reference/value JavaDictionary<,> creation via exemplar-rooted canonical templates and adds supporting reflection helpers/suppressions.

Copilot's findings

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

[github-actions]

🤖 Android PR Review — ⚠️ Needs Changes (minor)

Nicely done refactor. Splitting the container construction into explicit per-container TryCreate...FromJniHandle methods with concrete-literal IJavaPeerable rooting branches is far easier to reason about than the previous shape/enum indirection, and the XML docs + suppression justifications are excellent. Rejecting open/partially-open constructed types up front (ContainsGenericParameters) is a good hardening.

Findings

• ⚠️ 1 behavioral change to confirm (inline): unsupported value-type arguments on a known container now throw NotSupportedException at marshal time instead of returning false and falling through to the non-generic FromJniHandle fallback in JavaConvert. Likely intended and arguably better, but it's runtime-observable — please confirm and consider a regression test.
• 💡 1 minor readability nit (inline) on the Debug.Assert in ValueTypeFactory.cs.

Notes

• CI: no status checks reported on the head commit yet (state: pending, total_count: 0). Not mergeable until CI is green.
• Validation gap: the PR description itself flags that a NativeAOT publish + runtime round-trip hasn't been run to confirm the concrete-literal anchor actually roots the __Canon activation constructor. That's the load-bearing assumption of this change — recommend confirming via device/AOT CI before merge rather than static reasoning alone.

Positives: consistent formatting, tabs/Mono style, no ! null-forgiving operators, thorough suppression justifications, and existing JavaConvertTest coverage for the primitive/byte/nullable paths.

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

[simonrozsival]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).