-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make IList<T> inherit from IReadonlyList<T> #16151
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 To illustrate, this compiles: class MyClass : IMyList
{
int IMyReadOnlyList.Count => 5;
}
interface IMyList : IMyReadOnlyList { }
interface IMyReadOnlyList
{
int Count { get; }
} But this (current developers' code) won't: 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 |
@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 Original Interfaces DLL codenamespace InterfaceTest
{
public interface IMyReadOnlyList<T>
{
int Count { get; }
T this[int index] { get; }
}
public interface IMyList<T>
{
int Count { get; }
T this[int index] { get; set; }
}
} New Interfaces DLL codenamespace 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 Codeusing System;
using System.Collections.Generic;
namespace InterfaceTest
{
class Program
{
static void Main()
{
var myList = new MyList<int>();
Console.WriteLine($"MyList<int>.Count: {myList.Count}");
Console.WriteLine($"IMyList<int>.Count: {((IMyList<int>)myList).Count}");
Console.WriteLine($"IMyReadOnlyList<int>.Count: {(myList as IMyReadOnlyList<int>)?.Count}");
Console.WriteLine($"MyList<int>[1]: {myList[1]}");
Console.WriteLine($"IMyList<int>[1]: {((IMyList<int>)myList)[1]}");
Console.WriteLine($"IMyReadOnlyList<int>[1]: {(myList as IMyReadOnlyList<int>)?[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
New Output
|
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. |
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 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. |
The title an initial description seems to be referring to making |
I have opened a new issue to address my proposed changes dotnet/corefx#41409. |
@NickCraver : |
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
The text was updated successfully, but these errors were encountered: