OneTrueError - Automated exception handling

Immutable collections should not implement mutable interfaces

I just read the new blog post about .NETs new mutable collections. It mostly looks really nice, but there is a major problem: They implement mutable interfaces like IList<T> which has an Add() method.

The blog post states this:

Implementing the mutable interfaces

Several people raised the issue that our immutable collection types implement the mutable interfaces. For example, this is what the declaration of ImmutableList looks like:

public sealed class ImmutableList<T> :
        IImmutableList<T>,
        IEnumerable,
        IEnumerable<T>,
        IReadOnlyCollection<T>,
        IReadOnlyList<T>,
        ICollection,
        ICollection<T>,
        IList,
        IList<T>
{
   ...
}

It’s worth pointing out that this declaration does not have a correctness issue – you cannot mutate an instance of ImmutableList by casting it to a mutable interface. For example, calling IList.Add would throw NotSupportedException

Now, why is that such a big issue?

shock

Well. It’s not (in a strict sense). I thought so first. When I saw the throw NotSupportedException(); my internal alert system was blinking “LSP violation” in red and the siren was going all crazy.

But then I started to read more carefully about the interfaces in MSDN. The documentation for the Add() method state that NotSupportedException can be thrown if the collection is read only. It’s therefore not a Liskovs Substitution Principle violation.

You could argue that any interface containting an Add() method is incorrectly defined if that Add() method may not be used for all implementations. It would have been better with an interface like IReadOnlyList that exposes an item accessor.

public interface IReadOnlyList<T> : IEnumerable<T>
{
    T this[int index] { get; }
    int Count { get; }
}

public interface IList<T> : IReadOnlyList<T>
{
    T this[int index] { get; set;  }
    void Add(T item);
    void Remove(T item);
}

(I’m not satisfied with the naming as ‘is-a’ relationships also applies to interfaces, but you might understand what I’m going for.)

Ask yourself. If you got a list through a method like IList<User> GetUsers();, would you ever think that you may NOT use the Add/Remove methods? No, you wouldn’t as the defacto standard has been to expose those through the IEnumerable<T> interface. IEnumerable<T> isn’t a great choice, but it’s the best choice that .NET provides for read only collections.

The implication of using the mutable interfaces for the immutable types is that our code would have to be something like this when we want to get a mutable collection:

public void ProcessUsers(IList<T> users)
{
    if (myList.IsReadOnly)
        throw new InvalidOperationException("We require a proper list, one were the Add method works");

    // [...]
}

As of now, there is no interface that we can use to communicate that we need to work with mutable collections. And no, using concrete types is not an option. Understand? You force us to either start using our own collection interfaces or resort to runtime errors.

The only reason to the decision that I could find is the following:

However, we received the feedback that people had a hard time figuring out how to pass an immutable list to en existing method that took, for example, IList.

Ask yourself WHY they used IList<T> when an immutable object is required. They could have simply used IEnumerable<T>. My best bet is that they required something that they can traverse multiple times or wanted to know the length of the list. As in such the real solution would probably have been to create something like IReadOnlyList<T>.

Please .NET team, do not use the mutable interfaces, even if it’s technically valid to do so. Start over now when you got the chance. Give us better interfaces for mutable types (or do not implement the mutable interfaces in the immutable types).

Update

Someone stated that legacy support for the new immutable types are important. That’s imho not a valid reason since we are talking about interfaces. Writing your own adapter (adapter pattern) between the new immutable interfaces and IList takes five minutes.

Hence, adding support for the immutable types in legacy systems is dead simple. That’s not a valid reason for Microsoft to implement the mutable interfaces. Heck, they could even add those adapters in the new nuget package.

Something like:

public ListAdapter<T> : IList<T>
{
    public ListAdapter(IImmutableList<T> inner)
    {
    }
 
    // [... legacy support ..]
}
This entry was posted in Architecture, CodeProject and tagged . Bookmark the permalink.
  • Stephen Greatrex

    If the return type of your method doesn’t/shouldn’t support Add and Remove, why not return IEnumerable?

    • http://blog.gauffin.org/ jgauffin

      Exactly. Everyone thinks that the IList interface is for mutable collections when it technically supports both mutable and immutable implementations.
      Using it for the new pure immutable types would just make everything even worse.

      • willy_duit

        “it technically supports both mutable and immutable implementations”
        Let me correct that. “It technically supports both mutable and read-only implementations.”
        The BCL has IList, IReadonlyList and soon IImmutableList. It covers the bases about as well as it can. It “falls apart” in that IList can be read-only and will throw NotSupportedException from some methods if it is, and from the fact that there’s no way to guarantee that an IImmutableList really is immutable. The interface is only a promise.
        We’ll have to agree to disagree about (explicitly) implementing IList on an ImmutableList. It is important that these new collections work with existing code, and adapters are not sufficient here.

    • James Curran

      because IEnumerable isn’t the equivalent of a read-only List. IList includes indexed access. Even ICollection include a Count property (unlike the Count() method which which require stepping thru the entire ienumerable)

      • Stephen Greatrex

        I’m fairly sure that if the concrete implementation is a List then the .Count() and .ElementAt(int) methods will delegate to .Count and the indexer with no loss in performance.

  • nbevans

    I disagree. IList and ICollection are legacy interfaces in .NET and everybody already knew they badly violate LSP as well as ISP, including Microsoft. That doesn’t mean they can just pretend they don’t exist. They are both a pattern unto themselves, they provides a IsReadOnly property and that is the pattern. Like it or lump it.

    Plenty of existing stuff in .NET works in the exact same way as Immutable Collections. List.AsReadOnly() for instance. This merely returns a ReadOnlyCollection wrapper which throws exceptions on the mutation methods but does ensure IsReadOnly is true.

    Only implementing IEnumerable would be a disastrous idea because it would force any consuming code to walk the data structure (immutable collections are generally binary tree structures) just to determine the count. That’s what LINQ’s Count() method will do if ICollection is not implemented.

    If you think this fairly minor but yes, annoying at times, LSP violation that’s been there since the dawn of .NET is bad… then go try and work with the Java BCL for a year and you’ll quickly forgive .NET ;)

    • http://blog.gauffin.org/ jgauffin

      I’m not saying that IEnumerable is the right choice. It’s the best available choice.

      They got a chance now to introduce new immutable interfaces (IImmutableList/IImuttableCollection). Anyone having legacy code could continue using AsReadOnly while everyone basing their code on the new immutable collections could get a much better foundation to stand on.

      • nbevans

        Maybe I’m missing something but System.Collections.Immutable *does* include new interfaces. Nice, clean and LSP/ISP-compliant interfaces, too!

        Proof: http://i.imgur.com/POZB0wp.png

        ICollection and IList are implemented by BCL Immutable Collections for backward compatibility more than anything.

        • http://blog.gauffin.org/ jgauffin

          Correct, I even showed that in my quote :)
          Then there is no reason to implement the mutable collections. Anyone required to support legacy code could continue using the old way using the ReadOnlyCollections.

          • nbevans

            Read-only Collections are not the same thing. The “producer” of read-only collections can still mutate them itself. This often leads to bugs where consumers are enumerating through supposedly read-only collections but the producer of them has mutated them anyway. Thus screwing up the enumerations that are occurring. It can be said that read-only collections lead many programmers to think they are completely safe to use in multi-threading but they are absolutely not. So when these bugs are uncovered, I can imagine Immutable Collections being the “tool of choice” to fix the bug in the coming years. If Immutable Collections did not implement the legacy interfaces, it would make them less likely to be used on legacy codebases that would so easily otherwise benefit from them.

            PS: The worst side affect I’ve seen of the legacy interfaces is that many serialization libraries (ProtoBuf, JSON.NET etc) get stuck when a collection it’s deserializing indicates IsReadOnly as true. But ProtoBuf has already implemented full support for Immutable Collections, and I’m sure others won’t be far behind. But this can happen to any read-only collection implementation as well so it’s not really limited to BCL Immut’.

          • http://blog.gauffin.org/ jgauffin

            Good points.

            They do fix problems with the legacy code, but they also makes it harder to show that you require mutable collections in your other code as I showed in my blog post.

            One solution would be to [Obsolete] the old methods in librarys (like protobuf) and introduce new overloads taking the new methods. i.e. let all users solve the problem instead of making .NET less usable. Else we’ll eventually get the same situation as Java.

            Another alternative would be to also introduce new mutable and readonly interfaces which can be used for methods that truly require mutable collections, so we do not have to fallback on exceptions if the incorrect type of implementation is passed on to those methods.

          • nbevans

            I feel that Microsoft has struck the right balance here. Not everything in a BCL can be “perfect”; compromises sometimes have to be made. Often in the name of backward compatibility. I feel that the compromise they made here is very small and will rarely hurt anyone except possibly the pride of die-hard SOLID aficionados :)

          • http://blog.gauffin.org/ jgauffin

            Yeah, it’s only “SOLID aficionados” that want to be able to have method contracts that only allow mutable collections

    • http://www.Marisic.Net/ dotnetchris

      I take this side too. Pragmatic is more important than dogmatic here.

      If i have a method that takes IList, i should be able to provide it an IImmutableList, without having to do something dumb like immutableList.ToList().

      I would be open to a middle ground solution of it having support for explicit cast instead.

  • Pingback: The Morning Brew - Chris Alcock » The Morning Brew #1443

  • James Curran

    If you rename your interface from IReadOnlyList to IReadableList it solves your ‘is-a’ problem when implemented by mutable lists.