Collection initialisers on immutable collections don't work

[louthy]

Collection initialisers look for the Add method on a type so that they can repeatedly call Add to initialise a collection. This is problematic for immutable collections that have a default constructor (or are structs, which is what I use in language-ext).

Example:

    public class Seq<A> : IEnumerable<A>
    {
        A[] values;

        public Seq() =>
            values = new A[0];

        Seq(A[] values) =>
            this.values = values;
        
        public Seq<A> Add(A x)
        {
            var nvalues = new A[values.Length + 1];
            Array.Copy(values, nvalues, values.Length);
            nvalues[values.Length] = x;
            return new Seq<A>(nvalues);
        }

        public IEnumerator<A> GetEnumerator() =>
            values.AsEnumerable().GetEnumerator();

        IEnumerator IEnumerable.GetEnumerator() =>
            values.GetEnumerator();
    }

The following will result in an empty collection:

var xs = new Seq<int> { 1, 2, 3, 4 };

It seems to me that the collection initialiser functionality could use one of the following strategies:

• Only work for Add functions that return a void
• Only work for Add functions that return a type that isn't the same as the one being constructed
• See that the return type for Add is the same as the type being constructed and fold
  • This should work for both mutable types using a fluent return this and immutable types
• Allow an opt out attribute to be applied, so the compiler doesn't present collection initialisation as an option
• Have a new special function for collection initialisation that takes the whole collection. Maybe even just look for a constructor that takes an IEnumerable<A> and use that instead of repeatedly calling Add (which feels like it would have been a better solution in the first place, because then the type can optimise how it constructs from a multi-item sequence).

Right now the behaviour is unexpected and I've had users of my library reporting it as an issue.

[svick]

Only work for Add functions that return a void

This variant would be a breaking change for HashSet<T>. E.g. the code new HashSet<int> { 1, 2, 3 } works correctly today, but the Add method that's called there returns bool.

Also, there has been previous discussion related to this issue on #463.

[louthy]

Also, there has been previous discussion

@svick Thanks, I searched beforehand but couldn't find anything obvious.

[HaloFour]

Unfortunately all of these options but the fourth option would change the meaning of existing code in potentially breaking ways. Even the last option of the initializer preferring a bulk initialization method could mean that the compiler could start favoring an existing method.

The fourth option sounds interesting, a way to disable collection initializers. That would have to be driven by a new attribute. And maybe that could be combined with the fifth option so that the compiler would still naturally favor the existing Add method unless it was disabled in which it would look for an AddRange method.

[HaloFour]

In the interim it'd probably be better to include a helper method to initialize in bulk and educate users to its use. Something like:

var seq = Seq.Of(1, 2, 3, 4);

[louthy]

In the interim it'd probably be better to include a helper method

That already exists, and that's the documented way of construction:

   using static LanguageExt.Prelude;

   ...

   var xs = Seq(1, 2, 3, 4);  

But, users of popular libraries are going to assume that the C# standard way of initialising collections will work and are surprised when they don't (to be honest, I hadn't given it a second though because I'd never used that feature of C# before).

It shouldn't be the case that the language just co-opts methods without consideration for the scope of the problem. I didn't follow the original proposal, but it seems crazy to me that no-one considered immutable collections.

I realise it's a done deal now and there's no way of going back, but we should have some options here. Option 4 from above would be absolutely fine, and I would hope the least amount of work to implement.

[svick]

I didn't follow the original proposal, but it seems crazy to me that no-one considered immutable collections.

Keep in mind that collection initializers were added to C# in version 3.0 in 2007, while .Net immutable collections only came out in 2013.

(The idea of immutable collections in general is of course much older, but I think they weren't commonly used in C# back then.)

[louthy]

I have started a proposal around the opt-out attribute

[canton7]

Note that .NET's immutable collections avoid this by having a private constructor, and defining builders / factory methods instead

[louthy]

Note that .NET's immutable collections avoid this by having a private constructor, and defining builders / factory methods instead

Which doesn't work for structs. In language-ext all immutable collection types (Arr, HashMap, HashSet, Lst, Map, Que, Seq, Set, and Stck) are structs (with a private field that points to a private class) to avoid null. Whilst that may change in the future, when nullable references are used everywhere, the library has a lot of users that aren't in that position, or may never use nullable refs.

I am in no way suggesting that structs are the biggest problem here. Clearly, my usage of them to protect against null collections isn't usual. It's just an example. But, the idea that immutable collections must compromise their implementations by not having a default constructor (due to an obscure language feature that was poorly implemented), isn't great. A mechanism to opt-out of the poorly designed language feature would surely be the simplest approach.

[CyrusNajmabadi]

That library can provide an analyzer to catch this issue and to gently guide users to use the correct patterns they need

[amis92]

library can provide an analyzer to catch this issue

Which is exactly how Roslyn does it, right? :) Not sure about Immutable collections though.

Yeah - if there's a request for a language solution, which can be solved by Analyzers, it's like an additional -100 points for the request (I'd assume).