2

I just stumbled upon the following issue:

class Settings
{
    // Let's set some default value: { 1 }
    public ICollection<int> AllowedIds = new List<int>() { 1 };
}

static void Main(string[] args)
{
    var s = new Settings
    {
        AllowedIds = { 1, 2 }
    };

    Console.WriteLine(string.Join(", ", s.AllowedIds)); // prints 1, 1, 2
}

I understand why this happens: AllowedIds = { 1, 2 } is not an assignment but a collection initializer inside an object initializer, i.e., it's an implicit call of AllowedIds.Add(1); AllowedIds.Add(2).

Still, for me it was a gotcha, since it looks like an assignment (since it uses =).

As an API/library developer (let's say I'm the one developing the Settings class) who wants to adhere to the principle of least surprise, is there anything I can do to prevent the consumers of my library from falling into that trap?


Footnotes:

  • In that particular case, I could use an ISet/HashSet<int> instead of ICollection/List (since duplicates do not make sense for AllowedIds), which would yield the expected result of 1, 2. Still, initializing AllowedIds = { 2 } would yield the counter-intuitive result of 1, 2.

  • I found a related discussion on the C# github repo, which basically concluded that, yes, this syntax is confusing, but it's an old feature (introduced in 2006), and we can't change it without breaking backwards compatibility.

Heinzi
  • 151,145
  • 51
  • 326
  • 481
  • Wow. That is a surprise. I suspect other than mention it in the documentation, there's nothing much you can do about it, but then again, I might be wrong here. – Zohar Peled Jun 02 '20 at 14:53
  • 1
    That is surprising. I've never noticed this before. – Flydog57 Jun 02 '20 at 14:54

1 Answers1

0

If you are not expecting the user of the Settings class to add to AllowedIds, why expose it as an ICollection<int> (which contains an Add method and signifies the intention to be added into)?

The reason why AllowedIds = { 1, 2 } works in your code is because C# uses duck typing to call the Add method on the collection. If you eliminate the possibility of the compiler finding an Add method, there will be a compile error on the line AllowedIds = { 1, 2 }, thus preventing the trap.

You can do something like:

class Settings
{
    // Let's set some default value: { 1 }
    public IEnumerable<int> AllowedIds { get; set; } = new List<int> { 1 };
}

static void Main(string[] args)
{
    var s = new Settings
    {
        AllowedIds = new List<int> { 1, 2 }
    };

    Console.WriteLine(string.Join(", ", s.AllowedIds)); // prints 1, 2
}

This way you are still allowing the caller to set a new collection using the setter, while preventing the trap you mentioned.

rexcfnghk
  • 11,625
  • 1
  • 30
  • 55
  • The problem with this solution is that the Settings class no longer really controls the AllowedIds property very well. For example, Settings may want to implement an `AddAllowedId` method. The initial value is a list, but down in `Main`, someone could have set it to an array of int (which also implements `IEnumerable `, but doesn't allow Adding. I think Settings will need to have an IEnumerable property getter and then some code to allow adding IDs – Flydog57 Jun 02 '20 at 16:29
  • @Flydog57 Yes. what you are describing is to make the `Settings` class immutable or have an `AddAllowedId` method for controlled mutation. However, since OP's code is already exposing `AllowedIds` as an `ICollection` to begin with, i.e. they should be well aware that class is mutable, my answer is the easiest way to prevent callers from falling into the trap OP was describing. – rexcfnghk Jun 03 '20 at 01:06