Runtime: Make IList<T> inherit from IReadonlyList<T>

Created on 17 Jan 2016  路  14Comments  路  Source: dotnet/runtime

List implements both interfaces, IReadOnlyList does not have any extra feature and it is quite annoying, that they are not compatible. I think the same is with ICollection and IReadOnlyCollection

Most helpful comment

I agree this should now be re-opened.

I just demonstrated above that this could be done without breaking changes, unless you consider collections that implement only the mutable interfaces now also implicitly implement the read-only interfaces a breaking change, seems to me to be _Just Doing The Right Thing (TM)_. And it is simply using default interface implementation so no new fancy clr work.

While it would have been nice for these interfaces to share members, I think this 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.

I know it's too late to make .NET Core 3.0 but I think a target for .NET 5 to go along with dotnet/corefx#37590 would be great.

Thoughts? @karelz @safern

All 14 comments

I agree this _might_ have been a good choice from day 1, but it's a breaking change now because of explicit implementations. I say might because it's still a bit confusing to _imply_ it's _only_ read-only, though that's not at all what any interface means.

For example, since IList<T> has it's own Count member, explicitly implementing IList.Count wouldn't satisfy IReadonlyList.Count if they inherited - so it'd break anyone doing such.

To illustrate, this compiles:

``` C#
class MyClass : IMyList
{
int IMyReadOnlyList.Count => 5;
}
interface IMyList : IMyReadOnlyList { }
interface IMyReadOnlyList
{
int Count { get; }
}

But this (current developers' code) won't:

``` C#
class MyClass : IMyList
{
    int IMyList.Count => 5;
}
interface IMyList : IMyReadOnlyList { }
interface IMyReadOnlyList
{
    int Count { get; }
}

I agree that it would have been better this way. I'll go further than @NickCraver's "might", though add that it _might_ have done with a better name to reflect it being a read-only interface on what might not be a read-only collection. Indeed, that might have been better anyway, if anyone could have thought of a suitable concise name to suggest that.

But as Nick shows, it's not possible to make an existing interface inherit from another interface without breaking.

@NickCraver Sorry, I haven't thought about that. Alternatively, what about to make another interface, that will correspond with List API? It will inherit from IReadOnlyList and IList. Moreover it could contain List specific methods like AddRange and so on.

What would that other interface cater for, that isn't already catered for by IList<T>, by just using List<T> directly, or by using IList<T> with a few optimisations for cases where List<T> gives you something you might want, but can work-around not having (such as AddRange() where you can use it if it's there, and do foreach(T item in source) list.Add(item); if it isn't)?

@JonHanna It would implement IReadOnlyList and IList, but it probably does not matter too much and it is too late to solve it.

With the addition of default interface implementations in C#8/.NET Core 3.0 this be could be done without causing breaking changes.

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);
     }
 }

I was able to test out this theory 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
```

I too think it's time to reopen this issue. The fact that IList does not implement IReadOnlyList, ICollection does not implement IReadOnlyCollection, IDictionary does not implement IReadOnlyDictionary and there is no IReadOnlySet and if there were it would have the same problem is what is currently imposing the largest development cost on my .NET core development.

I actually think this can be done without breaking backwards compatibility to any significant degree, although the implementation is far beyond me.

When the loader encounters a class that implements an interface but doesn't implement a base interface of the interface, try to impute the base interface onto the class by matching members of the derived interface. Obviously this could only be done if we left all the members declared on the derived interface.

I agree this should now be re-opened.

I just demonstrated above that this could be done without breaking changes, unless you consider collections that implement only the mutable interfaces now also implicitly implement the read-only interfaces a breaking change, seems to me to be _Just Doing The Right Thing (TM)_. And it is simply using default interface implementation so no new fancy clr work.

While it would have been nice for these interfaces to share members, I think this 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.

I know it's too late to make .NET Core 3.0 but I think a target for .NET 5 to go along with dotnet/corefx#37590 would be great.

Thoughts? @karelz @safern

@karelz @safern @stephentoub

Could any of you re-open this issue or would you prefer for a new issue to be created?

FWIW, I still don't see this happening because enough people are doing is IReadOnlyList` or some variant to check for the read-only collections that this will still break their code, even if we can be binary compatible. I just looked in a few of our codebases and came across a few examples. It was a safe check for the past 7 years so it's not very surprising.

More than happy to be proven wrong/educated as always...but this would break assumptions and I'd wager that's the biggest factor against it, _even if it can be technically done_.

Could any of you re-open this issue would you prefer for a new issue to be created?

The title an initial description seems to be referring to making IList<T> inherit from IReadOnlyList<T>, however in your latest API proposal you're adding IReadOnlyCollection<T> to other collections, would you mind opening a new issue and we can have a fresh discussion there?

I have opened a new issue to address my proposed changes dotnet/corefx#41409.

@NickCraver :List<T> implements IList<T> and IReadOnlyList<T>.

Was this page helpful?
0 / 5 - 0 ratings

Related issues

jzabroski picture jzabroski  路  3Comments

omajid picture omajid  路  3Comments

matty-hall picture matty-hall  路  3Comments

yahorsi picture yahorsi  路  3Comments

omariom picture omariom  路  3Comments