Background and motivation

Roslyn has several internal algorithms (including as very hot spots in our core analyzer driver) where we may have data flowing in as either an ImmutableArray<T> (when the data is fixed and known ahead of time), or as an ImmutableArray<T>.Builder (when it's more the result of some scratch computation that we still want to operate on).

Our choices when we run into this have not been attractive. We either need to make duplicate copies of the code (increasing redundancy and possibility of increases bugs), or we need to update the core functions to take an IEnumerable<T> (or some other list-like interface). This is not great as this both boxes the ImmutableArray<T> case, and then has costs later on when we iterate the values.

Ideally, we could write the core logic to operate uniformly. Either on an array+length (or just a span). With ImmutableArray it is possible to get it as a span with .AsSpan, or get to the backing array with ImmutableCollectionsMarshal. However, no such facility exists for IA.Builder. This blocks us from having uniform processing logic that operates on a contiguous sequence of values, which might be coming from either source.

Our request is to provide some mechanism to expose thsi data from an IA.B. There are a few concerns here:

1. If the array (or span) is exposed, then it might no longer be valid if the IA.B is mutated. As such, this probably makes to sense to add as an API on ImmutableCollectionsMarshap, not on IA.B itself.

2. This would lock IA.B into being definitely backed by an array. That said, my belief is that that is already a requirement of the IA.B api because it already has a MoveToImmutable method which is guaranteed to work as long as the current capacity matches the count of the builder:

           /// <summary>
           /// Extracts the internal array as an <see cref="ImmutableArray{T}"/> and replaces it
           /// with a zero length array.
           /// </summary>
           /// <exception cref="InvalidOperationException">When <see cref="ImmutableArray{T}.Builder.Count"/> doesn't
           /// equal <see cref="ImmutableArray{T}.Builder.Capacity"/>.</exception>
           public ImmutableArray<T> MoveToImmutable()

This could not be changed (say, to have multiple internal arrays) as then MoveToImmutable would no longer be an O(1) move as promised, and which lots of consumption code depends on.

API Proposal

namespace System.Runtime.InteropServices;

public static class ImmutableCollectionsMarshal
{
        /// <summary>
        /// Gets the underlying <typeparamref name="T"/> array for an input <see cref="ImmutableArray{T}.Builder"/> value.
        /// </summary>
        /// <typeparam name="T">The type of elements in the input <see cref="ImmutableArray{T}.Builder"/> value.</typeparam>
        /// <param name="builder">The input <see cref="ImmutableArray{T}.Builder"/> value to get the underlying <typeparamref name="T"/> array from.</param>
        /// <returns>The underlying <typeparamref name="T"/> array for <paramref name="builder"/>, if present.</returns>
        /// <remarks>
        /// <para>
        /// The contents of the returned array may change as the builder is used.  If the builder decides to move to a new array,
        /// (for example, if a value is added and the builder is currently at max capacity), this array will then point at data no longer
        /// backing the builder.
        /// </para>
        /// </remarks>
        public static T[] AsArray<T>(ImmutableArray<T>.Builder builder)
        {
            return builder._elements;
        }
}

API Usage

void Analyze(ImmutableArray<SyntaxNode> nodes)
    => AnalyzeCore(nodes.AsSpan());

void Analyze(ImmutableArray<SyntaxNode>.Builder nodes)
    => AnalyzeCore(ImmutableCollectionsMarshal.AsArray(nodes).AsSpan(0, nodes.Count));

void AnalyzeCore(ReadOnlySpan<SyntaxNode> nodes);
{
    // ...
}

Alternative Designs

No response

Risks

This forces the internal backing storage of a builder to always be a contiguous array. However, as stated in the proposal above, it seems like this is already a requirement that cannot be violated to begin with. So having one more dependency on this is likely fine.