dotnet / dotnet/android

Reflection value manager doesn't marshal generic collection interfaces other than IList<T>

Open
#12,677 0 comments 0 reactions 0 assignees View on GitHub
Area: App Runtime
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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.