Reflection value manager doesn't marshal generic collection interfaces other than IList<T>
- Dominant language
- C#
- Stars
- 2.1k
- Forks
- 579
- Avg merge
- 1d 20h
- Merged PRs (30d)
- 257
Description
## Summary
`JniRuntime.ReflectionJniValueManager` only special-cases `IList<>` when selecting a value marshaler. `IDictionary`, `ICollection`, and `ISet` all fall through to `ProxyValueMarshaler`, which cannot marshal them and ends up calling `CreatePeer` with an interface `targetType`.
This affects the **non-trimmable (llvm-ir) typemap** on **both** runtimes:
| Value manager | Runtime | Selected when | Affected |
|---|---|---|---|
| `AndroidValueManager` | MonoVM | `!TrimmableTypeMap` | ❌ yes |
| `JavaMarshalValueManager` | CoreCLR / NativeAOT | `!TrimmableTypeMap` | ❌ yes |
| `TrimmableTypeMapValueManager` | any | `TrimmableTypeMap` | ✅ no |
This is a long-standing hole, not a regression — it has never worked on the reflection-based path.
## Root cause
`ReflectionJniValueManager.GetValueMarshalerCore` resolves marshalers in this order: `[JniValueMarshaler]` attribute → `IJavaPeerable` → builtins → `GetListType` → `IJavaPeerable`-assignable → `ProxyValueMarshaler`.
The `GetListType` helper matches `IList<>` only:
```csharp
static Type? GetListType (Type type)
{
foreach (var iface in type.GetInterfaces ().Concat (new [] { type })) {
if (typeof (IList<>).IsAssignableFrom (iface.IsGenericType ? iface.GetGenericTypeDefinition () : iface))
return iface;
}
return null;
}
```
`typeof (IList<>).IsAssignableFrom (typeof (IDictionary<,>))` and `typeof (IList<>).IsAssignableFrom (typeof (ICollection<>))` are both `false`, so those types reach `ProxyValueMarshaler.CreateGenericValue`. That method asks the value manager for a marshaler for the same type, gets *itself* back, and falls through to its `// Punt! Hope it's a java.lang.Object` branch.
## Why the trimmable path is unaffected
`TrimmableTypeMapValueManager.CreateValueCore` routes everything through `JavaConvert.FromObjectReference`, and `SafeJavaCollectionFactory.IsKnownContainerDefinition` already covers `IList<>`/`JavaList<>`, `ICollection<>`/`JavaCollection<>`, and `IDictionary<,>`/`JavaDictionary<,>`. The typemap rewrite closed this incidentally.
**The trimmable manager's behavior is the reference for what the reflection path should do.**
## Scope
1. **`IDictionary` / `JavaDictionary`** — confirmed broken; a targeted fix was prototyped in #12116 (closed in favor of this issue).
2. **`ICollection` / `JavaCollection`** — same code path, expected to be broken; needs a test to confirm. `JavaConvert.TryMakeGenericCollectionTypeFactory` already knows how to convert these, so only the marshaler-selection step is missing.
3. **`ISet` / `JavaSet`** — missing from `JavaConvert.GetJniHandleConverter` *and* `SafeJavaCollectionFactory`, so this one is broken on **all** paths, including trimmable. Larger scope than the other two.
## Suggested approach
Rather than overriding `GetValueCore` in each Mono.Android value manager subclass (which is what #12116 did — it jumps ahead of the base class's `EnsureNotDisposed()` / `reference.IsValid` / `PeekValue` / `targetType` validation, re-derives the base contract, and duplicates the same block in two subclasses), follow the precedent set by 7b0b40652 ("Guard primitive array value-manager routing", #12114).
That commit added a `protected virtual object? CreateNonArrayListValue (...)` hook in `ReflectionJniValueManager` at the point of failure, letting Mono.Android supply only the Android-specific conversion while the base class keeps owning the surrounding contract.
The analogous fix here is a hook at the marshaler-selection point, so that:
- the base class continues to own dispose-checking, reference validation, peer-cache peeking, and `targetType`/`T` compatibility validation;
- `Mono.Android` supplies only the `JavaConvert`-backed converter;
- both `AndroidValueManager` and `JavaMarshalValueManager` inherit the fix with no duplicated code, as would any future value manager.
Because `ReflectionJniValueManager` lives in `external/Java.Interop`, this needs a Java.Interop-side change plus a submodule bump.
## Test coverage
`tests/Mono.Android-Tests/Mono.Android-Tests/Java.Interop/JavaConvertTest.cs` is the right home; #12116 has on-device tests for the dictionary cases that can be reused, and should be extended with `ICollection` (and `ISet` if that is taken on). Tests must run under both `-p:AndroidTypeMapImplementation=llvm-ir` and `trimmable`, and under both `-p:UseMonoRuntime=true` and `false`, since only the llvm-ir lanes exercise the broken path.
## Related
- #12116 — closed; contains a working dictionary-only fix and on-device regression tests
- #12114 / 7b0b40652 — precedent for the `protected virtual` hook pattern in `ReflectionJniValueManager`
Contributor guide
No contributing guide indexed for this repository
Research direction
Start in external/Java.Interop at ReflectionJniValueManager.GetValueMarshalerCore and review the protected hook pattern from 7b0b40652. Then inspect Mono.Android value managers and tests/Mono.Android-Tests/Mono.Android-Tests/Java.Interop/JavaConvertTest.cs, including the reusable cases from #12116. Done means collection and dictionary interfaces marshal correctly across llvm-ir and trimmable type maps with both Mono and CoreCLR runtimes.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- android, csharp
- Domain
- mobile
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 68/100