[Trimming] Use trimmable type map for non-generic collection CopyTo#11429
[Trimming] Use trimmable type map for non-generic collection CopyTo#11429simonrozsival wants to merge 10 commits into
Conversation
Route non-generic JavaCollection and JavaList CopyTo element conversion through a helper that can use runtime type mapping and trimmable type map peer creation instead of suppressing DAM propagation warnings. Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Pull request overview
This PR updates non-generic JavaCollection/JavaList CopyTo() behavior so array element conversion can use the trimmable type map peer-creation path, removing prior trimming-warning suppressions. It also adds runtime coverage for the updated conversion paths and fixes a malformed XPath in src/Mono.Android/metadata that can block generation.
Changes:
- Route non-generic
CopyTo()element conversion throughJavaConvert.ConvertArrayElement()to enable trimmable type-map peer creation. - Add runtime coverage tests for
JavaCollection/JavaListCopyTo()under trimmable typemap, including primitive/string scenarios. - Fix malformed
starts-with()XPath usage insrc/Mono.Android/metadata.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Mono.Android-Tests/Mono.Android-Tests/Java.Interop/TrimmableTypeMapRuntimeCoverageTests.cs | Adds runtime coverage tests for non-generic CopyTo() conversions under trimmable typemap. |
| src/Mono.Android/metadata | Fixes an invalid XPath starts-with() expression used in metadata. |
| src/Mono.Android/Java.Interop/JavaConvert.cs | Introduces ConvertArrayElement() and typemap-aware conversion helpers for collection-to-array element conversion. |
| src/Mono.Android/Android.Runtime/JavaList.cs | Updates CopyTo() to use ConvertArrayElement() and refactors handle retrieval to support it. |
| src/Mono.Android/Android.Runtime/JavaCollection.cs | Updates CopyTo() to use ConvertArrayElement() for element conversion. |
Comments suppressed due to low confidence (1)
src/Mono.Android/Java.Interop/JavaConvert.cs:176
- ConvertArrayElement() calls array.GetType().GetElementType() on every element conversion. In CopyTo() this is invoked inside a loop, so it adds avoidable reflection overhead for large collections. Consider caching the element type once per CopyTo (e.g., add an overload that accepts the element type, or compute it once in CopyTo and pass it through).
internal static object? ConvertArrayElement (Array array, IntPtr handle, JniHandleOwnership transfer)
{
var elementType = array.GetType ().GetElementType ();
if (elementType is null || elementType == typeof (object))
return FromJniHandleWithRuntimeTypeMapping (handle, transfer);
if (JniHandleConverters.TryGetValue (elementType, out var converter))
return converter (handle, transfer);
if (elementType.IsArray)
return JNIEnv.GetArray (handle, transfer, elementType.GetElementType ());
Hoist array element conversion state out of the CopyTo loops, remove the added trimming suppression, and inline JavaList element retrieval instead of adding a raw-handle helper. Co-authored-by: Copilot <[email protected]>
Return null for zero JNI handles during array element conversion and add CopyTo coverage for null elements in non-generic JavaCollection and JavaList. Co-authored-by: Copilot <[email protected]>
simonrozsival
changed the title
|
/review |
|
✅ Android PR Reviewer completed successfully! |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ LGTM — Clean, well-structured change
CI Status: All green — dotnet-android ✅, Xamarin.Android-PR ✅, license/cla ✅
Summary
This PR correctly routes non-generic JavaCollection.CopyTo and JavaList.CopyTo through a new ArrayElementConverter struct that supports the trimmable type map path. The key improvements:
-
Removes DAM warning suppressions — eliminates
[UnconditionalSuppressMessage]+[DynamicallyAccessedMembers]on localGetElementTypefunctions in bothJavaCollectionandJavaList, replacing them withArrayElementConverterthat pre-resolves the conversion strategy once perCopyTocall. -
ArrayElementConverterdesign — Nice use of areadonly structthat caches the element type, converter delegate, and runtime-mapping flag in the constructor. JNI reference ownership is correctly handled through all code paths (peek → delete, converter → delegate owns, trimmable → finally cleanup, fallback → forwarded). -
Trimmable type map integration —
FromJniHandleWithTrimmableTypeMappingproperly handlesIJavaPeerableelements viaPeekPeer/GetPeerwith correctconsumedtracking for reference cleanup, and throws clearNotSupportedExceptionmessages for unsupported scenarios (generic types, unknown peers). -
Metadata XPath fix —
starts-with(@jni-signature='(I)')→starts-with(@jni-signature, '(I)')is a genuine bug fix — the old syntax passed a boolean comparison as a single argument to a two-argument XPath function. -
Tests — Good coverage across View objects, primitives + null, and strings with the trimmable type map. The existing
JavaListTest.CopyTounder theJavaListcategory validates the llvm-ir backward compatibility path.
Issues
| Severity | Count |
|---|---|
| 💡 Suggestion | 2 |
Positive callouts
- Clean reference management — every path properly handles JNI handle ownership
- The
ArrayElementConverterstruct avoids per-elementGetElementType()+ converter lookup overhead - Good separation between runtime-type-mapping path (for
object[]) and typed-element paths
Generated by Android PR Reviewer for issue #11429 · ● 5M
| static object? FromJniHandleWithTrimmableTypeMapping (IntPtr handle, JniHandleOwnership transfer, Type elementType) | ||
| { | ||
| bool consumed = false; | ||
| try { | ||
| var peeked = Java.Lang.Object.PeekObject (handle, elementType); |
There was a problem hiding this comment.
🤖 💡 Performance — PeekObject(handle, elementType) is called here, but the same call was already made at line 187 in ArrayElementConverter.FromJniHandle and returned null (otherwise we wouldn't have reached this code path). Since FromJniHandleWithTrimmableTypeMapping is a private static method with this single call site, the check is redundant.
That said, the cost is just a managed dictionary lookup (no JNI call), and the redundancy acts as a safety net if a future call site is added. Consider removing it if you want to keep this lean, or keep it for defensive reasons — either way is fine.
Rule: Avoid redundant work on hot paths
| using (var arrayList = new Java.Util.ArrayList ()) { | ||
| arrayList.Add ("alpha"); | ||
| using (var collection = new JavaCollection (arrayList.Handle, JniHandleOwnership.DoNotTransfer)) { | ||
| var values = new string [1]; | ||
| collection.CopyTo (values, 0); | ||
| Assert.AreEqual ("alpha", values [0]); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🤖 💡 Testing — Tests 1 (View) and 2 (int+null) exercise both JavaCollection.CopyTo and JavaList.CopyTo, but this string test only exercises JavaCollection. Consider adding a JavaList + string[] case for symmetry, e.g.:
using (var list = new JavaList (arrayList.Handle, JniHandleOwnership.DoNotTransfer)) {
var values = new string [1];
list.CopyTo (values, 0);
Assert.AreEqual ("alpha", values [0]);
}Rule: Test edge cases and maintain test matrix symmetry
|
@copilot can you address the code review comments: #11429 (review) |
Agent-Logs-Url: https://github.com/dotnet/android/sessions/97298485-55ae-4c3c-a1ea-aa1a0b113cbc Co-authored-by: jonathanpeppers <[email protected]>
Addressed both suggestions from the review in
|
Split InternalGet so CopyTo can reuse the List.get local-reference path while keeping array element conversion separate. Co-authored-by: Copilot <[email protected]>
Break the combined trimmable typemap CopyTo coverage into focused tests while exercising both JavaCollection and JavaList paths. Co-authored-by: Copilot <[email protected]>
|
/review |
|
✅ Android PR Reviewer completed successfully! |
There was a problem hiding this comment.
✅ LGTM (with minor suggestions)
Summary: Well-structured refactoring that introduces ArrayElementConverter to centralize array element conversion in JavaCollection.CopyTo and JavaList.CopyTo, routing through the trimmable type map when available. The removal of the [UnconditionalSuppressMessage] suppressions is a nice cleanup — the new converter struct handles type resolution without needing to suppress trimming warnings.
Positive callouts:
- Clean separation of
InternalGetReferencefromInternalGetinJavaList, enabling reuse without redundant JNI-to-managed conversion ArrayElementConverteras areadonly structavoids allocations and caches the converter delegate upfront — good perf choice- The metadata XPath fix (
starts-with(@jni-signature, '(I)')) is clearly correct — the two-argument form is required by XPath - Good test coverage: typed arrays (
View[]),object[]with nulls, andstring[]— covering the key conversion branches
Issues: 0 ❌ | 0
Generated by Android PR Reviewer for issue #11429 · ● 9M
| static object? FromJniHandleWithTrimmableTypeMapping (IntPtr handle, JniHandleOwnership transfer, Type elementType) | ||
| { | ||
| bool consumed = false; | ||
| try { | ||
| if (elementType.IsGenericType) { | ||
| throw new NotSupportedException ( | ||
| FormattableString.Invariant ($"Cannot convert Java collection elements to closed generic array element type '{elementType}'.")); | ||
| } | ||
|
|
||
| var reference = new JniObjectReference (handle, JniObjectReferenceType.Local); | ||
| var peer = JniEnvironment.Runtime.ValueManager.GetPeer (reference); | ||
| if (peer != null) { | ||
| consumed = true; | ||
| JNIEnv.DeleteRef (handle, transfer); | ||
| return peer; | ||
| } | ||
|
|
||
| throw new NotSupportedException ( |
There was a problem hiding this comment.
🤖 💡 Error handling — When GetPeer returns null (the object isn't already tracked), this throws NotSupportedException. This means CopyTo into a typed array (e.g., View[]) will fail if the Java-side collection contains an element that hasn't been activated as a .NET peer yet — i.e., an object created purely on the Java side. Is that the expected limitation for the trimmable type map path? If so, consider documenting this constraint in the exception message (e.g., mentioning that the object must already have a .NET peer).
Rule: Error messages must have context
| } | ||
|
|
||
| public void CopyTo (Array array, int array_index) | ||
| public unsafe void CopyTo (Array array, int array_index) |
There was a problem hiding this comment.
🤖 💡 Formatting — The unsafe modifier appears unnecessary here. CopyTo doesn't use any unsafe constructs directly (no pointers, stackalloc, etc.). The called method InternalGetReference is itself marked unsafe, but C# doesn't require callers to be in an unsafe context to call unsafe methods.
Rule: Minimal diffs
Move proxy lookup, direct CreateInstance activation, and created-peer state setup into TrimmableTypeMap so JavaConvert can use the trimmable activation path without owning those details. Co-authored-by: Copilot <[email protected]>
Use the no-reflection trimmable typemap creation path for typed array element conversion so CopyTo does not require DAM annotations on array element types. Co-authored-by: Copilot <[email protected]>
…yto-trimmable-typemap
Summary
Validation