[API Proposal]: ImmutableArray.CreateUnsafe<T>(T[] array)

[Sergio0694]

Background and motivation

There are several scenarios where it's useful to create an ImmutableArray<T> value over an existing array that's been pre-populated, which the caller owns. But, there is no way of creating such an ImmutableArray<T> value without doing an extra copy and/or allocating a new array. This is not ideal in cases where callers do own the existing array and just want to wrap it and transfer ownership. A common approach is to rely on the internals of ImmutableArray<T> and do something like this:

Foo[] items = new Foo[numberOfItems];

PopulateItems(items);

ImmutableArray<Foo> array = Unsafe.As<Foo[], ImmutableArray<Foo>>(ref items);

This works fine today, as the layout of ImmutableArray<T> does match that of T[], but it's far from ideal and generally undefined behavior. This proposal is about adding a reliable API to just allow advanced users to do this in a safe and reliable way.

API Proposal

namespace System.Runtime.InteropServices;

public static class ImmutableCollectionsMarshal
{
    public static ImmutableArray<T> AsImmutableArray<T>(T[]? array);
    public static T[]? AsArray<T>(ImmutableArray<T> array);
}

API Usage

Here's the previous example, rewritten using this API:

Foo[] items = new Foo[numberOfItems];

PopulateItems(items);

ImmutableArray<Foo> array = ImmutableCollectionsMarshal.AsImmutableArray(items);

Here's some current uses of this (some of them are in 1st party libraries as well):

• microsoft/clrmd
• stronginject
• ComputeSharp (and also here)

Risks

No risk, as people are already doing this using Unsafe.As, which is undefined behavior. The new API would allow them to keep the same performance characteristics while making the code safe and reliable. The method would have the "Unsafe" suffix which is now an established pattern for these kind of APIs, so non advanced users would clearly see it as not something they should use.

[ghost]

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

Issue Details

---

Background and motivation

There are several scenarios where it's useful to create an ImmutableArray<T> value over an existing array that's been pre-populated, which the caller owns. But, there is no way of creating such an ImmutableArray<T> value without doing an extra copy and/or allocating a new array. This is not ideal in cases where callers do own the existing array and just want to wrap it and transfer ownership. A common approach is to rely on the internals of ImmutableArray<T> and do something like this:

Foo[] items = new Foo[numberOfItems];

PopulateItems(items);

ImmutableArray<Foo> array = Unsafe.As<Foo[], ImmutableArray<Foo>>(ref items);

This works fine today, as the layout of ImmutableArray<T> does match that of T[], but it's far from ideal and generally undefined behavior. This proposal is about adding a reliable API to just allow advanced users to do this in a safe and reliable way.

API Proposal

namespace System.Collections.Immutable;

public static class ImmutableArray
{
    public static ImmutableArray<T> CreateUnsafe<T>(T[] array);
}

API Usage

Here's the previous example, rewritten using this API:

Foo[] items = new Foo[numberOfItems];

PopulateItems(items);

ImmutableArray<Foo> array = Unsafe.As<Foo[], ImmutableArray<Foo>>(ref items);

Here's some current uses of this (some of them are in 1st party libraries as well):

• microsoft/clrmd
• stronginject
• ComputeSharp (and also here)

Risks

No risk, as people are already doing this using Unsafe.As, which is undefined behavior. The new API would allow them to keep the same performance characteristics while making the code safe and reliable. The method would have the "Unsafe" suffix which is now an established pattern for these kind of APIs, so non advanced users would clearly see it as not something they should use.

Author:	Sergio0694
Assignees:	-
Labels:	api-suggestion, area-System.Collections
Milestone:	-

[stephentoub]

#25461

[Sergio0694]

@stephentoub forgot about that one, thank you! The issue doesn't seem to be resolved though. The API that was added since that issue was created is the ReadOnlySpan<T> overload mentioned in #25461 (comment), which does help but doesn't cover all scenarios. I still do agree with what @jkotas said in #25461 (comment):

"I think it is a much worse situation than having officially santioned unsafe interop methods. Maybe we should reconsider?"

It seems to me that point hasn't been fully settled just yet. Yes, that hack currently works and everyone's using it, but:

• Does this mean the BCL guarantees the internal layout of ImmutableArray<T> will never change?
  • In other words, does that mean that Unsafe.As<T[], ImmutableArray<T>> and back is officially well defined?
• Even in that case, wouldn't it still be better to have a proper method for this somewhere (à la CollectionMarshal) that people can use so they can stop relying and having to know about internal implementation details at all?

[jkotas]

Does this mean the BCL guarantees the internal layout of ImmutableArray will never change?

Yes. We told people to depend on internal layout of ImmutableArray<T> when we refused to add the proper API 9 years ago. Many popular libraries took this dependency. There is no reasonable way to roll this back.

[Sergio0694]

I see, fair enough, thank you! I guess we can keep this open to track updating the docs to make this clearer then 🙂

[jkotas]

I think we will have hard time to find a good place in the docs to add this information. The API docs are organized by APIs. There is no good API to attach this to and including it in the top level ImmutableArray topic would steer people towards the unsafe pattern.

If we want to make the situation better, adding the proper unsafe API seems to be like the best option.

[Sergio0694]

If we did go with that (which I personally like better than documenting the hack), what API shape would you recommend? As in, does the one being proposed here look fine, or would you rather have it be somewhere else and/or using a different name?

[jkotas]

The original approved API shape that was later withdrawn looked fine to me: dotnet/corefx#196 . We would probably want to call it ImmutableCollectionsMarshal and omit Dangerous to follow the patten that have established since then. It should have API for both directions (from array to immutable array and from immutable array to array).

[eiriktsarpalis]

I like the idea of an ImmutableCollectionsMarshal. @Sergio0694 would it be possible to update your API proposal accordingly?

[Sergio0694]

Sure! I've updated the proposal as suggested and also added nullability annotations 🙂

Couple questions:

• Do we want to keep the original API shape, or should we use the "As___" pattern as the other marshal APIs?
  • That'd essentially just be AsImmutableArray and AsArray, always taking the input by value
• Follow up from the previous point: are we ok with the fact the two APIs are not "equivalent"? As in, the first also resets the input reference, whereas the second one only returns the underlying array without resetting the input immutable array.

[eiriktsarpalis]

Do we want to keep the original API shape, or should we use the "As___" pattern as the other marshal APIs?

I think "As___" is a good naming convention, although we can debate the final name in API review.

As in, the first also resets the input reference

Why would the first method need to accept the array by reference in the first place? It is a reference type anyways.

You might want to consider updating the namespace to something prefixed by System.Collections.Immutable since it's a separate assembly from collections.

[Sergio0694]

"I think "As___" is a good naming convention, although we can debate the final name in API review."

Great! I'll update the proposal then 🙂

"Why would the first method need to accept the array by reference in the first place? It is a reference type anyways."

I would assume the original proposal did that to try to reduce risk? As in, if the method forces you to pass the array reference by reference and resets it, it can help avoid that array being accidentally accessed again after that method is called. I personally don't think that's really needed, since these APIs are already for advanced users who would know not to do that 🤔

"You might want to consider updating the namespace"

That'd cause the type to be much more accessible to all devs using that namespace just to access the other collection types, is that something we'd want? I guess the idea of putting it into System.Runtime.InteropServices is that the namespace as a whole is already considered "unsafe equivalent", and only used by more advanced devs that are specifically looking for the special APIs that it contains. Happy to move it if you feel strongly about this though.

[eiriktsarpalis]

I personally don't think that's really needed, since these APIs are already for advanced users who would know not to do that 🤔

Same here. I don't see much value in it given that it can be bypassed trivially.

[bartonjs]

Video

Looks good as proposed

namespace System.Runtime.InteropServices;

public static class ImmutableCollectionsMarshal
{
    public static ImmutableArray<T> AsImmutableArray<T>(T[]? array);
    public static T[]? AsArray<T>(ImmutableArray<T> array);
}

[Sergio0694]

@eiriktsarpalis I see this is assigned to you, but I'm also happy to contribute this. Feel free to assign this to me if you want 😄

[eiriktsarpalis]

Happy to hand it over, thanks :-)