Align API surface of immutable collections and their corresponding builder types

[reflectronic]

Rationale

There is a slight mismatch between the API surface of each immutable collection type and its corresponding builder. To improve the usability of these APIs, we should consider more closely aligning the APIs exposed in each immutable collection type and its corresponding builder. (I automatically generated this list, but of course some differences should be left be; for example, Union vs UnionWith, or equality operators, so I left them out.)

Related to: #31242 #28160 #31208 Probably some others

Proposed API

ImmutableArray

public class ImmutableArray<T>.Builder
{ 
	public void CopyTo(T[] destination);
	public void CopyTo(int sourceIndex, T[] destination, int destinationIndex, int length);
 
	public int IndexOf(T item, int startIndex, IEqualityComparer<T>? equalityComparer);
	public void InsertRange(int index, IEnumerable<T> items);
	public void InsertRange(int index, ImmutableArray<T> items);
	public void Replace(T oldValue, T newValue);
	public void Replace(T oldValue, T newValue, IEqualityComparer<T>? equalityComparer);
	public void Remove(T item, IEqualityComparer<T>? equalityComparer);
	public void RemoveRange(int index, int length);
	public void RemoveRange(IEnumerable<T> items);
	public void RemoveRange(IEnumerable<T> items, IEqualityComparer<T>? equalityComparer);
	public void RemoveAll(Predicate<T> match);
}

public struct ImmutableArray<T>
{
	public ImmutableArray<T> AddRange(params T[] items);
	public ImmutableArray<T> AddRange(T[] items, int length);
	public ImmutableArray<T> AddRange<TDerived>(TDerived[] items) where TDerived : T;
	public ImmutableArray<T> AddRange(ImmutableArray<T> items, int length);
	public ImmutableArray<T> AddRange<TDerived>(ImmutableArray<TDerived> items) where TDerived : T;
}

ImmutableList

public class ImmutableList<T>.Builder
{ 
	public void Remove(T value, IEqualityComparer<T>? equalityComparer);
	public void RemoveRange(int index, int count);
	public void RemoveRange(IEnumerable<T> items);
	public void RemoveRange(IEnumerable<T> items, IEqualityComparer<T>? equalityComparer);
	public void Replace(T oldValue, T newValue);
	public void Replace(T oldValue, T newValue, IEqualityComparer<T>? equalityComparer);
}

ImmutableSortedSet

public class ImmutableSortedSet<T>.Builder
{ 
	public int IndexOf(T item);
}

---

ImmutableSortedDictionary<TKey, TValue> has a member SetItems which isn't available on the builder. However, because one can just use the indexer to do the same thing, it isn't really appropriate.

[eiriktsarpalis]

Thank you for the proposal @reflectronic. I think the changes make sense, given that we are committed to API parity between immutable types and their builder counterparts.

Would you be able to make a couple of updates to the proposal before I mark this ready for review?

• The ImmutableArray.Builder.CopyTo method signatures are incorrect: the destination arrays should be generic.
• Would you be able to update the signatures so that nullability annotations are reflected?

Thanks!

[reflectronic]

Thanks for taking a look @eiriktsarpalis. I made the requested changes and additionally removed a couple more APIs from the proposal which didn't make sense.

[eiriktsarpalis]

A few follow-up remarks:

• Would it make sense to substitute the ImmutableArray<T>.Builder.CopyTo overloads with just one accepting Span<T>?
• How common would a ImmutableArray.Builder.Replace call be? For mutable collections it makes more sense to just use the indexer to make such types of changes. The same applies for the ImmutableList.Builder.Replace proposal.
• It seems that the only reason the ImmutableArray<T>.RemoveRange(ImmutableArray<T> items) overloads exist is to optimize removal for inputs of size zero or one. This makes sense for immutable collections but not for mutable ones. I think we can remove them. I think we could also remove the InsertRange(ImmutableArray<T>) overload.
• Perhaps public ImmutableArray<T> AddRange(T[] items, int length); and public ImmutableArray<T> AddRange(ImmutableArray<T> items, int length); could be changed to accept ReadOnlySpan<T>?
• Could you remove the ImmutableHashSet addition that has already been approved?
• I think you are right to point out that adding ImmutableArray.Reverse would shadow the existing LINQ overload. I think we should probably remove that addition for this reason.

[reflectronic]

Sorry for taking a while to respond. I addressed the comments, except for the Span/ReadOnlySpan ones. I think I've convinced myself that they are theoretically okay, even though no other collection types expose methods like that. However, I'd think that 1) they should be added as new overloads (otherwise, it's not really 'parity'), and therefore 2) they need to be added to ImmutableArray and ImmutableArray<T>.Builder at the same time. Does that sound right to you?

[eiriktsarpalis]

they need to be added to ImmutableArray and ImmutableArray.Builder at the same time. Does that sound right to you?

Yeah I think that's pretty reasonable. We could reconsider at a future iteration.

[eiriktsarpalis]

public class ImmutableSortedDictionary<TKey, TValue>.Builder
{ 
	public void SetItems(IEnumerable<KeyValuePair<TKey,TValue>> items);
}

This is not needed for a mutable collections, using the indexer should be sufficient.

[reflectronic]

Done. Should be ready for review now, I think.

[terrajobst]

Video

• Looks good as proposed
  • Note: this is just syncing builder and immutable container, no API innovation happened here
• We should consider adding a unit test to make sure we keep API surface in sync between the builder and the immutable container
• We removed Reverse due to a source breaking change concern. We should follow up. We believe the concern is very low.

namespace System.Collections.Immutable
{
    public struct ImmutableArray<T>
    {
        public ImmutableArray<T> AddRange(params T[] items);
        public ImmutableArray<T> AddRange(T[] items, int length);
        public ImmutableArray<T> AddRange<TDerived>(TDerived[] items) where TDerived : T;
        public ImmutableArray<T> AddRange(ImmutableArray<T> items, int length);
        public ImmutableArray<T> AddRange<TDerived>(ImmutableArray<TDerived> items) where TDerived : T;
        public partial class Builder
        { 
            public void CopyTo(T[] destination);
            public void CopyTo(int sourceIndex, T[] destination, int destinationIndex, int length);
        
            public int IndexOf(T item, int startIndex, IEqualityComparer<T>? equalityComparer);
            public void InsertRange(int index, IEnumerable<T> items);
            public void InsertRange(int index, ImmutableArray<T> items);
            public void Replace(T oldValue, T newValue);
            public void Replace(T oldValue, T newValue, IEqualityComparer<T>? equalityComparer);
            public void Remove(T item, IEqualityComparer<T>? equalityComparer);
            public void RemoveRange(int index, int length);
            public void RemoveRange(IEnumerable<T> items);
            public void RemoveRange(IEnumerable<T> items, IEqualityComparer<T>? equalityComparer);
            public void RemoveAll(Predicate<T> match);
        }
    }
    public partial class ImmutableList<T>
    {
        public partial class Builder
        { 
            public void Remove(T value, IEqualityComparer<T>? equalityComparer);
            public void RemoveRange(int index, int count);
            public void RemoveRange(IEnumerable<T> items);
            public void RemoveRange(IEnumerable<T> items, IEqualityComparer<T>? equalityComparer);
            public void Replace(T oldValue, T newValue);
            public void Replace(T oldValue, T newValue, IEqualityComparer<T>? equalityComparer);
        }        
    }
    public partial class ImmutableSortedSet<T>
    {
        public partial class Builder
        { 
            public int IndexOf(T item);
        }
    }
}

[FilipToth]

Is this ready for 7.0?

[eiriktsarpalis]

This is not on our radar for .NET 7 at the moment, but we would consider a community contribution that provides an implementation of the approved API.

[RaymondHuy]

Hi @eiriktsarpalis I have created the PR for this issue, could you help me review it 😀