Runtime: Make mutable generic collection interfaces implement read-only collection interfaces

[TylerBrinkley]

Rationale

It's long been a source of confusion that the mutable generic collection interfaces don't implement their respective read-only collection interfaces. This was of course due to the read-only collection interfaces being added after the fact and thus would cause breaking changes by changing a published interface API.

With the addition of default interface implementations in C#8/.NET Core 3.0 I think the mutable generic collection interfaces, ICollection<T>, IList<T>, IDictionary<K, V>, and ISet<T> should now implicitly inherit their respective read-only collection interfaces. This can now be done without causing breaking changes.

While it would have been nice for these interfaces to share members, I think the proposed API below is the best we can possibly do with the read-only interfaces being added after the fact.

As an added bonus, this should allow some simplification of the type checking in LINQ code to check for the read-only interfaces instead of the mutable interfaces.

Proposed API

 namespace System.Collections.Generic {
-    public interface ICollection<T> : IEnumerable<T> {
+    public interface ICollection<T> : IReadOnlyCollection<T> {
-        int Count { get; }
+        new int Count { get; }
+        int IReadOnlyCollection<T>.Count => Count;
     }
-    public interface IList<T> : ICollection<T> {
+    public interface IList<T> : ICollection<T>, IReadOnlyList<T> {
-        T this[int index] { get; set; }
+        new T this[int index] { get; set; }
+        T IReadOnlyList<T>.this[int index] => this[index];
     }
-    public interface IDictionary<TKey, TValue> : ICollection<KeyValuePair<TKey, TValue>> {
+    public interface IDictionary<TKey, TValue> : ICollection<KeyValuePair<TKey, TValue>>, IReadOnlyDictionary<TKey, TValue> {
-        TValue this[TKey key] { get; set; }
+        new TValue this[TKey key] { get; set; }
-        ICollection<TKey> Keys { get; }
+        new ICollection<TKey> Keys { get; }
-        ICollection<TValue> Values { get; }
+        new ICollection<TValue> Values { get; }
-        bool ContainsKey(TKey key);
+        new bool ContainsKey(TKey key);
-        bool TryGetValue(TKey key, out TValue value);
+        new bool TryGetValue(TKey key, out TValue value);
+        TValue IReadOnlyDictionary<TKey, TValue>.this[TKey key] => this[key];
+        IEnumerable<TKey> IReadOnlyDictionary<TKey, TValue>.Keys => Keys;
+        IEnumerable<TValue> IReadOnlyDictionary<TKey, TValue>.Values => Values;
+        bool IReadOnlyDictionary<TKey, TValue>.ContainsKey(TKey key) => ContainsKey(key);
+        bool IReadOnlyDictionary<TKey, TValue>.TryGetValue(TKey key, out TValue value) => TryGetValue(key, out value);
     }
-    public interface ISet<T> : ICollection<T> {
+    public interface ISet<T> : ICollection<T>, IReadOnlySet<T> {
-        bool IsProperSubsetOf(IEnumerable<T> other);
+        new bool IsProperSubsetOf(IEnumerable<T> other);
-        bool IsProperSupersetOf(IEnumerable<T> other);
+        new bool IsProperSupersetOf(IEnumerable<T> other);
-        bool IsSubsetOf(IEnumerable<T> other);
+        new bool IsSubsetOf(IEnumerable<T> other);
-        bool IsSupersetOf(IEnumerable<T> other);
+        new bool IsSupersetOf(IEnumerable<T> other);
-        bool Overlaps(IEnumerable<T> other);
+        new bool Overlaps(IEnumerable<T> other);
-        bool SetEquals(IEnumerable<T> other);
+        new bool SetEquals(IEnumerable<T> other);
+        new bool Contains(T value) => ((ICollection<T>)this).Contains(value);
+        bool IReadOnlySet<T>.Contains(T value) => ((ICollection<T>)this).Contains(value);
+        bool IReadOnlySet<T>.IsProperSubsetOf(IEnumerable<T> other) => IsProperSubsetOf(other);
+        bool IReadOnlySet<T>.IsProperSupersetOf(IEnumerable<T> other) => IsProperSupersetOf(other);
+        bool IReadOnlySet<T>.IsSubsetOf(IEnumerable<T> other) => IsSubsetOf(other);
+        bool IReadOnlySet<T>.IsSupersetOf(IEnumerable<T> other) => IsSupersetOf(other);
+        bool IReadOnlySet<T>.Overlaps(IEnumerable<T> other) => Overlaps(other);
+        bool IReadOnlySet<T>.SetEquals(IEnumerable<T> other) => SetEquals(other);
+    }
 }

Binary Compatibility Test

I was able to test that this change doesn't break existing implementers with the following custom interfaces and by simply dropping the new interfaces dll to the publish folder without recompiling the consuming code, the IMyReadOnlyList<T> interface was automatically supported without breaking the code.

Original Interfaces DLL code

```c#
namespace InterfaceTest
{
public interface IMyReadOnlyList
{
int Count { get; }
T this[int index] { get; }
}

public interface IMyList<T>
{
    int Count { get; }
    T this[int index] { get; set; }
}

}

### New Interfaces DLL code
```c#
namespace InterfaceTest
{
    public interface IMyReadOnlyList<T>
    {
        int Count { get; }
        T this[int index] { get; }
    }

    public interface IMyList<T> : IMyReadOnlyList<T>
    {
        new int Count { get; }
        new T this[int index] { get; set; }
        int IMyReadOnlyList<T>.Count => Count;
        T IMyReadOnlyList<T>.this[int index] => this[index];
    }
}

Consuming Code

```c#
using System;
using System.Collections.Generic;

namespace InterfaceTest
{
class Program
{
static void Main()
{
var myList = new MyList();
Console.WriteLine($"MyList.Count: {myList.Count}");
Console.WriteLine($"IMyList.Count: {((IMyList)myList).Count}");
Console.WriteLine($"IMyReadOnlyList.Count: {(myList as IMyReadOnlyList)?.Count}");
Console.WriteLine($"MyList[1]: {myList[1]}");
Console.WriteLine($"IMyList[1]: {((IMyList)myList)[1]}");
Console.WriteLine($"IMyReadOnlyList[1]: {(myList as IMyReadOnlyList)?[1]}");
}
}

public class MyList<T> : IMyList<T>
{
    private readonly List<T> _list = new List<T> { default, default };

    public T this[int index] { get => _list[index]; set => _list[index] = value; }

    public int Count => _list.Count;
}

}

### Original Output

MyList.Count: 2
IMyList.Count: 2
IMyReadOnlyList.Count:
MyList[1]: 0
IMyList[1]: 0
IMyReadOnlyList[1]:

### New Output

MyList.Count: 2
IMyList.Count: 2
IMyReadOnlyList.Count: 2
MyList[1]: 0
IMyList[1]: 0
IMyReadOnlyList[1]: 0
```

Moved from dotnet/runtime#16151

Updates

• Added ISet<T> implementing IReadOnlySet<T> since https://github.com/dotnet/runtime/issues/2293 has been implemented.

[safern]

@terrajobst I would like to have your input on these w.r.t new default implementations and adding new interfaces or new members to interfaces.

[GrabYourPitchforks]

For folks confused as to why this would be a breaking change without default interface implementations, consider three separate assemblies defined as such:

namespace MyClassLib
{
    public interface IFoo
    {
        void PrintHello();
    }
}

namespace MyOtherClassLib
{
    public interface IBar
    {
        void PrintHello();
    }
}

namespace MyApplication
{
    class Program
    {
        static void Main(string[] args)
        {
            object myFoo = MakeNewMyFoo();

            IFoo foo = myFoo as IFoo;
            if (foo is null)
            {
                Console.WriteLine("IFoo not implemented.");
            }
            foo?.PrintHello();

            IBar bar = myFoo as IBar;
            if (bar is null)
            {
                Console.WriteLine("IBar not implemented.");
            }
            bar?.PrintHello();
        }

        [MethodImpl(MethodImplOptions.NoInlining)]
        private static object MakeNewMyFoo()
        {
            return new MyFoo();
        }

        class MyFoo : IFoo
        {
            void IFoo.PrintHello()
            {
                Console.WriteLine("Hello from MyFoo.IFoo.PrintHello!");
            }
        }
    }
}

Compile and run the main application, and you'll see the following output.

Hello from MyFoo.IFoo.PrintHello!
IBar not implemented.

Now change the assembly which contains IFoo to have the following code, and recompile and redeploy that assembly _while not recompiling the assembly which contains MyApplication_.

namespace MyClassLib
{
    public interface IFoo : IBar
    {
    }
}

The main application will crash upon launch with the following exception.

Unhandled exception. System.MissingMethodException: Method not found: 'Void MyClassLib.IFoo.PrintHello()'.
   at MyApplication.Program.Main(String[] args)

The reason for this is that the method that would normally be at the slot IFoo.PrintHello was removed and instead there's a new method at the slot IBar.PrintHello. However, this now prevents the type loader from loading the previously-compiled MyFoo type, as it's trying to implement an interface method for which there's no longer a method entry on the original interface slot. See https://stackoverflow.com/a/35940240 for further discussion.

As OP points out, adding default interface implementations works around the issue by ensuring that an appropriate method exists in the expected slot.

[TylerBrinkley]

@terrajobst any input on this collection interface change as well?

[TylerBrinkley]

@safern @GrabYourPitchforks @terrajobst Is this something we could get triaged soon? I'd love for this to make it into .NET 5.

[safern]

@eiriktsarpalis and @layomia are the new System.Collections owners so I'll defer to them.

[TylerBrinkley]

@terrajobst

We can't make ISet<T> extend IReadOnlySet<T> (for the same reason that we couldn't do for the other mutable interfaces).

Is this still true even with default interface methods? Does that mean dotnet/corefx#41409 should be closed?

We discussed this. We used to think that that DIMs _would_ work, but when we walked the solution we concluded that it would result commonly in a shard diamond which would result in an ambiguous match. However, this was recently challenged so I think I have to write it down and make sure it's actually working or not working.

Here's a comment @terrajobst had about this in a separate issue, https://github.com/dotnet/runtime/issues/2293#issuecomment-579524652

Hoping for some more insight.

[TylerBrinkley]

I updated the proposal to add a Contains DIM to ISet<T> as the current Contains method is defined on ICollection<T> and so if you tried to call Contains on an ISet<T> the call would be ambiguous with the ICollection<T> and IReadOnlySet<T> versions.

The new method would take precedence in this case and it's implementation if not overridden would delegate to ICollection<T>'s implementation as IReadOnlySet<T>'s does. It's not ideal but this is the only incidence of such a solution where this is required for the proposal.

[TylerBrinkley]

As an alternative since IReadOnlySet<T> hasn't released yet, instead of adding another Contains method to the list including ICollection<T>, IReadOnlySet<T>, and now ISet<T> we could instead add an invariant read-only collection interface as proposed in https://github.com/dotnet/runtime/issues/30661 and have it implement a Contains method and then have IReadOnlySet<T> implement this new invariant interface and remove it's Contains method.

With that change then ICollection<T> could implement this new invariant read-only collection interface.

So instead of this.

```c#
namespace System.Collections.Generic {
public interface ICollection : IReadOnlyCollection {
bool Contains(T value);
...
}
public interface IReadOnlySet : IReadOnlyCollection {
bool Contains(T value);
...
}
public interface ISet : IReadOnlySet {
new bool Contains(T value) => ((ICollection)this).Contains(value);
bool ICollection.Contains(T value);
bool IReadOnlySet.Contains(T value) => ((ICollection)this).Contains(value);
...
}
}

it would be this.

```c#
namespace System.Collections.Generic {
    public interface IReadOnlyCollectionInvariant<T> : IReadOnlyCollection<T> {
        bool Contains(T value);
        ...
    }
    public interface ICollection<T> : IReadOnlyCollectionInvariant<T> {
        new bool Contains(T value);
        bool IReadOnlyCollectionInvariant<T>.Contains(T value) => Contains(value);
        ...
    }
    public interface IReadOnlySet<T> : IReadOnlyCollectionInvariant<T> {
        ...
    }
}

[mikernet]

As a follow-up to this:

Perhaps LINQ methods that have optimized execution paths on interfaces like IList<T> could be updated to optimize on IReadOnlyList<T> instead since all IList<T> will now implement IReadOnlyList<T> and then new code can begin moving to a sane development model where custom read-only collections don't need to extraneously implement the writable interfaces as well. I recall the justifications for optimizing LINQ on writable interfaces instead of read-only interfaces were:

1. IReadOnlyList<T> is covariant and thus slower.
2. IReadOnlyList<T> hasn't been around as long as List<T> so some code may not implement it and thus not get the optimizations.
3. It is "standard" to implement both IList<T> and IReadOnlyList<T> on all list-like collections anyway, I believe mostly because of the perf/optimization issues above?

I could be wrong but I believe there have been runtime updates to address point 1. Point 2 would be addressed by this issue. Point 3 is something that we could move away from with these changes to make read-only collection interfaces real first-class citizens in the framework that are actually widely usable.

A remaining sore point would be the annoying lack of support for read-only interfaces in many places where they should be supported as inputs and adding overloads that accept read-only interfaces is problematic because many collections currently implement both IList<T> and IReadOnlyList<T> directly so overload resolution would fail. This could be addressed in the BCL by updating writable collections to only directly implement the writable interface but might be confusing if people's code no longer compiles after updating to a new runtime version when using third-party collections that haven't been updated as such.

The above issues combined with the inability to pass an IList<T> into a method that accepts an IReadOnlyList<T> almost aways prevents me from using read-only collection interfaces in my code. Collections are easily one of the most poorly designed areas of the BCL and so fundamental to development that cleaning this up as much as possible with a bit of help from DIMs would be really nice.

[vpenades]

I would like to propose an alternative solution:

For example, for a List we have this:

```c#
IList
IReadOnlyList
List : IList , IReadOnlyList

What I propose is this:

```c#
IList<T>
IReadOnlyList<T>
IWriteableList<T> : IList<T>, IReadOnlyList<T>
List<T> : IWriteableList<T>

So, from the point of view of the runtime, it would just require introducing a new interface, so I don't think it would break any compatibility. And developers would progressively move from using IList to IWriteableList