Fix legacy generic dictionary conversion - #12116
Open
simonrozsival wants to merge 4 commits into
Open
Conversation
Route requested generic dictionary targets through JavaConvert while preserving assignable cached peers and non-generic inference. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0e043be9-9070-43bc-aea4-a48438690bdf
Member
Author
|
@copilot resolve the merge conflicts in this pull request |
Co-authored-by: simonrozsival <374616+simonrozsival@users.noreply.github.com>
Co-authored-by: simonrozsival <374616+simonrozsival@users.noreply.github.com>
Contributor
Member
Author
|
@copilot resolve the merge conflicts in this pull request |
Co-authored-by: simonrozsival <374616+simonrozsival@users.noreply.github.com>
Contributor
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes legacy reflection-backed JniValueManager dictionary conversions so that explicit closed generic dictionary targets (IDictionary<TKey,TValue> and JavaDictionary<TKey,TValue>) are routed through JavaConvert’s generic-collection conversion, avoiding cached non-generic JavaDictionary reuse that can lead to InvalidCastException.
Changes:
- Route explicit generic dictionary targets through
JavaConvert.FromObjectReference(...)in both legacy reflection-backed value managers (AndroidValueManagerandJavaMarshalValueManager). - Extend the dynamic generic-collection factory path to accept explicit
JavaDictionary<,>targets (in addition toIDictionary<,>). - Add device regression tests covering representative generic/non-generic dictionary conversion shapes and cached-peer identity behavior.
Show a summary per file
| File | Description |
|---|---|
| tests/Mono.Android-Tests/Mono.Android-Tests/Java.Interop/JavaConvertTest.cs | Adds regression coverage for generic dictionary conversions and cached-peer identity behavior. |
| src/Mono.Android/Microsoft.Android.Runtime/JavaMarshalValueManager.cs | Ensures reflection-backed CoreCLR value manager routes explicit generic dictionary targets through JavaConvert. |
| src/Mono.Android/Java.Interop/JavaConvert.cs | Expands generic-collection factory support to include JavaDictionary<,> targets and adds a helper to detect explicit generic dictionary types. |
| src/Mono.Android/Android.Runtime/AndroidRuntime.cs | Ensures legacy reflection-backed Android value manager routes explicit generic dictionary targets through JavaConvert. |
Review details
Suppressed comments (1)
tests/Mono.Android-Tests/Mono.Android-Tests/Java.Interop/JavaConvertTest.cs:163
- 💡 Here
new JniObjectReference (source.Handle)uses the defaultJniObjectReferenceType.Invalid. Usingvar reference = source.PeerReference;makes the reference type accurate (global) and avoids subtle ownership issues if this test ever switches to a disposing/transfer option.
using (var source = new JavaDictionary ()) {
source.Add (key, value);
var reference = new JniObjectReference (source.Handle);
var actual = JniEnvironment.Runtime.ValueManager.GetValue (
ref reference, JniObjectReferenceOptions.Copy, targetType);
- Files reviewed: 4/4 changed files
- Comments generated: 3
- Review effort level: Lite
| { | ||
| using (var source = new JavaDictionary ()) { | ||
| source.Add ("answer", 42); | ||
| var reference = new JniObjectReference (source.Handle); |
Comment on lines
+47
to
+51
| if (JavaConvert.IsGenericDictionary (requestedType)) { | ||
| #pragma warning disable CS8600 // Converting null literal or possible null value to non-nullable type. | ||
| return (T) JavaConvert.FromObjectReference (ref reference, options, requestedType); | ||
| #pragma warning restore CS8600 // Converting null literal or possible null value to non-nullable type. | ||
| } |
Comment on lines
+635
to
+639
| if (JavaConvert.IsGenericDictionary (requestedType)) { | ||
| #pragma warning disable CS8600 // Converting null literal or possible null value to non-nullable type. | ||
| return (T) JavaConvert.FromObjectReference (ref reference, options, requestedType); | ||
| #pragma warning restore CS8600 // Converting null literal or possible null value to non-nullable type. | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Experimental coverage of all 400 key/value combinations across primitive, nullable primitive,
string, and Java peer types showed thatJavaConvert.FromJniHandle(Type)produces the exact requestedJavaDictionary<TKey,TValue>, while reflection-backed value managers reused a cached non-genericJavaDictionaryandGetValue<T>then failed withInvalidCastException.This change makes both legacy reflection-backed Android value managers honor explicit closed
IDictionary<TKey,TValue>andJavaDictionary<TKey,TValue>targets by routing them through the existingJavaConvertgeneric collection factory. It also teaches the dynamic factory path to accept an explicitJavaDictionary<TKey,TValue>target.Compatibility
IDictionaryconversion remains unchanged.JavaConvertreturns an already cached peer when it is assignable to the requested type, preserving peer identity.Tests
Added representative device coverage for:
IDictionary<int, string>JavaDictionary<int?, double?>IDictionary<string, MyIntent>JavaDictionary<MyIntent, long?>Validation:
make all CONFIGURATION=Debugdotnet build src/Mono.Android/Mono.Android.csproj -c Debug --no-restoredotnet test external/Java.Interop/tests/Java.Interop-Tests/Java.Interop-Tests.csproj -c Debug(670 passed, 6 skipped)Mono.Android.NET-Tests(918 passed, 55 skipped)The 400-case result is the experimental motivating matrix; the committed regression suite intentionally uses representative shapes rather than duplicating all 400 combinations.