Problem with collection expression, struct, and immutable collection

[tfenise]

Suppose (pre C# 12) I implement my own version of ImmutableArray<> for some reason, such as I need some custom functionality:

readonly struct MyImmutableArray<T> : IEnumerable<T>//possibly also IImmutableList<T>
{
    readonly ImmutableArray<T> _array;//for simplicity
    
    MyImmutableArray(ImmutableArray<T> array)
    {
        _array = array;
    }
    
    public static MyImmutableArray<T> Empty => new(ImmutableArray<T>.Empty);
    
    public MyImmutableArray<T> Add(T item) => new(_array.Add(item));
    
    public int Count => _array.Length;
    
    public T this[int index] => _array[index];
    
    //...
}

I can use the type like

MyImmutableArray<int> array = MyImmutableArray<int>.Empty.Add(123);
Console.WriteLine(array.Count);

Now with C# 12, it is tempting to try the new feature of collection expression, like

MyImmutableArray<int> array = [123];
Console.WriteLine(array.Count);

And it does compile! Except it does not really work. It translates to

MyImmutableArray<int> myImmutableArray = default(MyImmutableArray<int>);
myImmutableArray.Add(123);
MyImmutableArray<int> myImmutableArray2 = myImmutableArray;
Console.WriteLine(myImmutableArray2.Count);

There are two problems with this translation:

1. It incorrectly assumes default(MyImmutableArray<int>), or more precisely, new MyImmutableArray<int>() should create a valid empty collection. It is probably true for a class (but there still could be exceptions?), but it is often false for a struct, as often a struct has a public parameterless constructor only because it has to have one. This problem is probably easy to debug, though, as it probably leads to a fast and straightforward NullReferenceException.
2. Even if new MyImmutableArray<int>() were the empty collection, it still incorrectly assumes MyImmutableArray<int>.Add modifies the collection. This problem can be harder to debug. If [123] surprisingly creates an empty collection, it could lead to subtle errors later as the program runs.

It is acceptable that a custom type does not automatically work with the new language feature, but this consequence of the new feature, that the code could compile smoothly without any warning to a program with completely unintended behaviour, is a pit of failure and should be avoided. Should the BCL ImmutableArray<> not be updated with CollectionBuilderAttribute as soon as the new language feature was introduced, such a popular type in BCL would be completely unusable with the new language feature.

This behaviour is similar to the old feature of collection initializer. new MyImmutableArray<int>() { 123 } does a similar thing. However, there is an important distinction. The syntax of new MyImmutableArray<int>() { 123 } suggests that it uses the parameterless constructor to construct the object, does some modification on it, and then returns the object, so it is easy to figure out that it should be avoided in this case. However, it is not obvious at all from the syntax what [123] does. It just seems that the compiler has magically figured out how to construct the wanted collection, except it does not and it only pretends to have done so.

There are some options of remedy that I can think of:

1. Issue a compiler/analyzer warning if a collection expression translates to a parameterless constructor call that is seemingly unwanted, such as an empty public parameterless constructor of a struct, or a nonpublic parameterless constructor.
2. In debug builds, at runtime, actually enumerate the collection and verify that the parameterless constructor gives an empty collection, and that after the first addition it becomes a collection with one single element.
3. Issue a compiler/analyzer warning if the Add method to be called has a return value (but what about bool HashSet<>.Add()?), or if the return value is convertible to IEnumerable.
4. Issue a compiler/analyzer warning if Add method is to be used, but the type is convertible to IImmutable*.

I don't know if any similar problem has been suggested before. #5354 is too long to scroll through. Sorry if this turns out to be a duplicate.

[HaloFour]

For custom types that can't be built in a normal manner you can define your own builder:

https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/operators/collection-expressions#collection-builder

[jnm2]

However, it is not obvious at all from the syntax what [123] does. It just seems that the compiler has magically figured out how to construct the wanted collection, except it does not and it only pretends to have done so.

It's the downside that comes with a powerful syntax that fits many builder patterns. The thing to remember is that 'collection initializer' is one of the strategies that collection expressions can use for construction. Collection initializers can also be written as new() { 123 } or new ContainingObj { SomeMyImmutableArray = { 123 } }.

We've discussed having compiler warnings for new ImmutableArray<T> { expr } and I really want them (@RikkiGibson). There's an existing discussion thread around here somewhere about this. The approach I'd like to see would also provide the same diagnostics for your custom collection type.

[CyrusNajmabadi]

It is acceptable that a custom type does not automatically work with the new language feature, but this consequence of the new feature, that the code could compile smoothly without any warning to a program with completely unintended behaviour, is a pit of failure and should be avoided.

This is true. Though it has been true for a very long time. Collection-initializers were added to the language, and have all the same problems here (which is why we added the CollectionBuilderAttribute).

There are some options of remedy that I can think of:

One thing was missed here. A collection itself should provide an error when used incorrectly through the use of a custom attribute that comes with it. After all, those collections would still fail if you did new MyCollection { 1, 2, 3 }. Given the number of these collections out there, i think time would likely better be spent solving at that level, versus investing in the compiler/1st-party-analyzer space itself since it would only hit a handful of cases.

[tfenise]

What kind of custom attribute should the collection itself use? What is the meaning of the attribute?

After all, those collections would still fail if you did new MyCollection { 1, 2, 3 }.

The same analyzer could also apply to collection initiallizers.

[tfenise]

Collection-initializers were added to the language, and have all the same problems here (which is why we added the CollectionBuilderAttribute).

I would say the problem with collection expression is more serious.

1. Usually using a collection initializer requires writing down the keyword new, so it is more or less apparent that it is calling the constructor and trying to do modifications on it, which is unwanted for immutable collections.
2. Collection initializers never work with BCL System.Collections.Immutable.Immutable*<>, so it is not tempting to try them on custom immutable collections. However, collection expression does work with BCL Immutable*<>, but sadly it may compile to the wrong code sequence for custom immutable collections without any warning.