Expand "Reflection visible array" constructed promotion to generic interfaces implemented by arrays

[NinoFloris]

MetadataEETypeNode always promotes reflection-visible array types to MaximallyConstructableType, the rationale being that Array.CreateInstanceFromArrayType might allocate them. This is what makes that API usable without [RequiresDynamicCode].

This rule however doesn't fire for the generic interfaces that arrays implement (IList<T>, ICollection<T>, IEnumerable<T>, IReadOnlyList<T>, IReadOnlyCollection<T>). A library that wants to produce T[] in response to a typeof(IList<T>) entry point can't, because T[]'s MethodTable isn't guaranteed to be emitted.

We're intending to add AOT-friendly support in Npgsql for reading and writing enums without requiring explicit user registrations. Since Postgres pairs every 'base' type with a corresponding array type, we also want to support enum array reading and writing.

Handling these through their underlying type is the recommended AOT-friendly approach for working with unknown enum types. This support can be extended structurally to arrays via Array.CreateInstanceFromArrayType(typeof(TEnum[]), n), using the array cast compatibility to let the underlying type code handle the rest.

Today this works for reader.GetFieldValue<MyEnum[]>() as internally these generic paths do typeof(T) for mapping resolution, which brings the array type up to constructed level through the metadata tier promotion. If a user calls reader.GetFieldValue<IList<MyEnum>>() however, this fails because MyEnum[] was never rooted. None of the workarounds we can apply on our end are satisfying. MakeArrayType requires dynamic code. Per-enum generic registration defeats what convenience the underlying type handling pattern can already give us. And restricting the mappings for this particular case to T[] only loses natural call shapes we support for all other mappings.

The idea would be to root the array type when a reflection-visible closed generic is one of the interfaces arrays implement. With those roots in place Npgsql can safely pair a suppressed MakeArrayType with CreateInstanceFromArrayType to produce an array instance to back the requested interface.

Rooting T[] for every reachable IList<T> (ICollection<T>, and so on) isn't free, but the cost is smaller than the surface suggests. Such reachability in real apps overwhelmingly coexists with T[] already being rooted indirectly via List<T>, ImmutableArray<T>, {ReadOnly}Memory<T>, etc. The rule could also be limited to enum element types, if the unconditional variant would have too wide of an impact. In which case it would just exist to make the underlying type pattern work more uniformly, as if the user explicitly rooted or registered TEnum[].

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/area-system-collections
See info in area-owners.md if you want to be subscribed.

[dotnet-policy-service]

Tagging subscribers to this area: @agocke, @dotnet/ilc-contrib
See info in area-owners.md if you want to be subscribed.

[MichalStrehovsky]

When we do this kind of overfit, it's because there's a trim unsafe API that we can make trim safe by doing this. Is there a trim unsafe API we can make safe or would taking advantage of this require suppressing trimming warnings. We don't encourage doing that, the suppression is a bug pretty much any time i look at suppressed warnings outside this repo (and often in this repo too).

[NinoFloris]

Well effectively the api that could become trim safe would be some form of Array.CreateInstanceFromCollectionInterfaceType, but obviously the chances that gets added are not very realistic.

There just exists a tension in the advice to handle enums in an erased fashion, while at the same time doing that means you can't guarantee some, any, concrete enum collection type is available to back common collection interface requests with.

You can't guarantee a concrete collection type is rooted precisely because you are handling these enum types in an erased fashion, following the advice. And the runtime isn't helping you by making just one concrete collection type available based on the 'heuristic' I laid out.

Since you have no way to create an instance to back these requests with, handling enums in an erased fashion is effectively half usable advice.

We'll have to tell people the following is all that works (and hope that there isn't an ORM library in between the user and us):
IList<TEnum> list = reader.GetFieldValue<TEnum[]>()

Which is kind of absurd because it just shows how non uniformly this support is implemented in the runtime.
Users will be asking us "Why do I have to cast to IList myself if you have the array type available", to which we'll say: *reasons*.

[MichalStrehovsky]

We can measure this part and see if discussing this is even realistic:

Rooting T[] for every reachable IList (ICollection, and so on) isn't free, but the cost is smaller than the surface suggests.

I made the change in #127278 and kicked off a size measurement run at MichalStrehovsky/rt-sz#218. The results should be available in half an hour or so.

[MichalStrehovsky]

This has two measurements, one is for any of the array interfaces, the other is only for enum element types. The enum element types regression would be acceptable (we would need a new API: Array.CreateInstanceFromCollectionInterfaceTypeButOnlyHowNpgsqlNeedsIt). The other one is close to 1%. It takes a lot of effort to save 1%. It's easy to regress by that much.

We can keep this open if this becomes a recurring theme, however because there's no API we can make safe with this, it doesn't look attractive. We don't overapproximate for so that AOT-unsafe code can work, only for AOT-safe code.

[NinoFloris]

Yeah the size is not too bad for enums indeed.

I did think a bit more about the api that could work. We could maybe tap into the dataflow analysis to discharge the RDC warning on Array.CreateInstance(elementType, ...) if it was preceded by an elementType.IsEnum check?

Similar things are already done for MakeGenericType and such so it's not entirely without precedent?