Skip to content
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

Add Invariant IReadOnlyCollection<T> and IReadOnlyList<T> interfaces #30661

Closed
TylerBrinkley opened this issue Aug 22, 2019 · 2 comments
Closed
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Collections
Milestone

Comments

@TylerBrinkley
Copy link
Contributor

TylerBrinkley commented Aug 22, 2019

If #31001 gets implemented through default interface implementation the next logical addition would be to add invariant versions of the IReadOnlyCollection<T> and IReadOnlyList<T> interfaces as some of the read-only methods on ICollection<T> and IList<T> were not included in the read-only interfaces because that would prevent the generic type argument from being declared covariant. ICollection<T> and IList<T> would then inherit these new interfaces and use default interface implementation to forward their calls to their own versions.

Proposed API

 namespace System.Collections.Generic {
+    public interface IReadOnlyCollectionInvariant<T> : IReadOnlyCollection<T> {
+        bool Contains(T item);
+    }
-    public interface ICollection<T> : IReadOnlyCollection<T> {
+    public interface ICollection<T> : IReadOnlyCollectionInvariant<T> {
-        bool Contains(T item);
+        new bool Contains(T item);
+        bool IReadOnlyCollectionInvariant<T>.Contains(T item) => Contains(item);
     }
+    public interface IReadOnlyListInvariant<T> : IReadOnlyList<T>, IReadOnlyCollectionInvariant<T> {
+        int IndexOf(T item);
+    }
-    public interface IList<T> : ICollection<T>, IReadOnlyList<T> {
+    public interface IList<T> : ICollection<T>, IReadOnlyListInvariant<T> {
-        int IndexOf(T item);
+        new int IndexOf(T item);
+        int IReadOnlyListInvariant<T>.IndexOf(T item) => IndexOf(item);
     }
-    public interface IReadOnlySet<T> : IReadOnlyCollection<T> {
+    public interface IReadOnlySet<T> : IReadOnlyCollectionInvariant<T> {
-        bool Contains(T value);
+        new bool Contains(T value);
+        bool IReadOnlyCollectionInvariant<T>.Contains(T value) => Contains(value);
     }
-    public interface IReadOnlyDictionary<TKey, TValue> : IReadOnlyCollection<KeyValuePair<TKey, TValue>> {
+    public interface IReadOnlyDictionary<TKey, TValue> : IReadOnlyCollectionInvariant<KeyValuePair<TKey, TValue>> {
+        bool IReadOnlyCollectionInvariant<KeyValuePair<TKey, TValue>>.Contains(KeyValuePair<TKey, TValue> item) => TryGetValue(item.Key, out var value) && EqualityComparer<TValue>.Default.Equals(value, item.Value);
     }
     public interface IDictionary<TKey, TValue> : ICollection<KeyValuePair<TKey, TValue>>, IReadOnlyDictionary<TKey, TValue> {
+        bool IReadOnlyCollectionInvariant<KeyValuePair<TKey, TValue>>.Contains(KeyValuePair<TKey, TValue> item) => ((ICollection<KeyValuePair<TKey, TValue>>)this).Contains(item); // Avoids the diamond problem
     }
 }

Open Questions

  • Will adding these interfaces and the generic interface implementations cause significant code-bloat?
  • Is using an Invariant suffix a good naming pattern?
  • Should the CopyTo method be included as it seems to be a deprecated API or at least from my perspective?

Updates

  • Removed CopyTo method as it appears to be a deprecated API.
@msftgits msftgits transferred this issue from dotnet/corefx Feb 1, 2020
@msftgits msftgits added this to the Future milestone Feb 1, 2020
@maryamariyan maryamariyan added the untriaged New issue has not been triaged by the area owner label Feb 23, 2020
@layomia layomia removed the untriaged New issue has not been triaged by the area owner label Jun 24, 2020
@eiriktsarpalis
Copy link
Member

Are Contains and IndexOf the only methods to be added to the invariant subtypes? If so it feels like a lot of added complexity in order to accommodate a scenario that could in principle be obtained using enumeration or indexers.

Is using an Invariant suffix a good naming pattern?

Not sure I like, but I can't think of anything better either.

@eiriktsarpalis eiriktsarpalis added the api-needs-work API needs work before it is approved, it is NOT ready for implementation label Dec 14, 2020
@TylerBrinkley
Copy link
Contributor Author

At this point I don't really advocate for making this change due to the added complexity. If someone else would like to keep this issue open please let me know and I can re-open.

@ghost ghost locked as resolved and limited conversation to collaborators Jan 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Collections
Projects
None yet
Development

No branches or pull requests

5 participants