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 common ancestor for ICollection<T> and IReadOnlyCollection<T> #105180

Closed
CodeSetting opened this issue Jul 20, 2024 · 7 comments
Closed

Add common ancestor for ICollection<T> and IReadOnlyCollection<T> #105180

CodeSetting opened this issue Jul 20, 2024 · 7 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Collections

Comments

@CodeSetting
Copy link

Background and motivation

Often, I find myself designing methods or classes that can act on any readable and countable collection. When doing so, it would be advantageous to add a constraint that allows all such collections to be consumed. Ideally, this constraint would encompass both ICollection<T> and IReadOnlyCollection<T>.

Unfortunately, this is not currently possible. These two interfaces both share the requisite Count property and GetEnumerator() method. Unfortunately, they do not share a common ancestor interface that advertises these capabilities.

Admittedly, the common IEnumerable<T> interface comes close. However, there are often advantages to knowing the size of the collection (Count) prior to its iteration. This is the specific use case considered by this issue.

I propose adding a common ancestor interface,, perhaps IReadableCollection<T>.

While the current IReadOnlyCollection<T> and IReadableCollection<T> would be nearly identical, their purpose would be significantly different. The former asserts that the collection cannot be written. The latter does not make this same assertion.

Regrettably, the historical lack of this interface has also led to a proliferation of ReadOnlyXXX<T> classes that must implement both ICollection<T> and IReadOnlyCollection<T>. It is a shame that these classes must implement, hide, and throw exceptions for the properties and methods related to writing. However, I do understand, given .NET's evolutionary history, why this was necessary.

While adding this new common ancestor cannot fix the past, and it can help mitigate the continuing proliferation of this issue.

API Proposal

namespace System.Collections.Generic;

public interface IReadableCollection<out T> : IEnumerable<T>, IEnumerable
{
    int Count { get; }
}

public interface IReadOnlyCollection<out T> : IReadableCollection<T>
{
}

public interface ICollection<out T> : IReadableCollection<T>
{
  bool IsReadOnly { get; }
  void Add(T item);
  bool Contains(T item);
  void CopyTo(T[] array, int arrayIndex);
  bool Remove(T item);
}

API Usage

public class MyFancyCollection<T> where T : IReadableCollection<T>
{
  .
  .
  .
}

public class MyFancyClass
{
  public void DoSomethingGood(IReadableCollection<T> countable)
  {
    .
    .
    .
  }

  public void DoSomething(IEnumerable<T> collection)
  {
    if (collection is IReadableCollection<T> countable)
      DoSomethingGood(countable);

    .
    .
    .
  }
}

Alternative Designs

Originally, a cleaner design might have also moved the IsReadOnly property to IReadableCollection<T> and eliminated the need for the IReadOnlyCollection<T> interface. Unfortunately, at this point, that would likely involve breaking changes.

Risks

Since both of the derived interfaces already implement the necessary property Count and method GetEnumerator() for the proposed ancestor IReadableCollection<T>, the risks should be minimal.

The risk in not implementing it is that developers will continue to "creatively" work around this issue.

@CodeSetting CodeSetting added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jul 20, 2024
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Jul 20, 2024
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-collections
See info in area-owners.md if you want to be subscribed.

@huoyaoyuan
Copy link
Member

Duplicate of #31001.

Moving interface member into another interface is massive breaking change - we can only deal it carefully with default interface methods.

The previous attempt of #31001 ends up broke C++/CLI. We plan to revisit it in .NET 10.

@huoyaoyuan huoyaoyuan closed this as not planned Won't fix, can't repro, duplicate, stale Jul 20, 2024
@dotnet-policy-service dotnet-policy-service bot removed the untriaged New issue has not been triaged by the area owner label Jul 20, 2024
@CodeSetting
Copy link
Author

CodeSetting commented Jul 20, 2024

@huoyaoyuan No need to be quite so dismissive. I did search for similar issues. I simply missed #31001, because it proposed a very different solution and wasn't well-indexed.

In the case of the #31001, the proposal is that ICollection<T> should inherit from IReadOnlyCollection<T>. In my solution, I proposed that both of these interfaces should inherit from an entirely new, common ancestor interface IReadableCollection<T>. This was mostly to avoid the naming confusion.

That said, I see that moving the singular Count property would create similar breaking changes. For me, considering what would happen to an explicit interface implementation made the problem more obvious than the explanation in #31001.

I never claimed to be perfect. Though, with due respect, the response to this rather obvious issue has also been less than perfect.

It has been lingering (and snowballing) for about 20 years now, since shortly after generics were introduced in 2005. It might be nice to see a resolution in (maybe?) the next decade. 😄

Until then, I'll continue doing what I have since pattern matching was introduced. I constrain to IEnumerable<T> and match to either ICollection<T> or IReadOnlyCollection<T> to get the Count. When it doesn't match, I throw an exception or slow roll the implementation. Though, honestly, that solution is becoming rather tedious.

@CyrusNajmabadi
Copy link
Member

Though, with due respect, the response to this rather obvious issue has also been less than perfect.

There's no perfect solution. So any response will be in kind.

It has been lingering (and snowballing) for about 20 years now, since shortly after generics were introduced in 2005. It might be nice to see a resolution in (maybe?) the next decade.

Improvements have been made (esp. in hte .net 9 timeframe). These are the best possible steps given the enormous importance of compatibility in the .net ecosystem.

@julealgon
Copy link

@CyrusNajmabadi has there been any discussions around potentially creating an interface that represents just the availability of a Count property? Or would that be too granular?

I know it would similarly introduce breaking changes, but just wondering if the team has considered that (instead of having it be part of ICollection itself).

@CodeSetting
Copy link
Author

CodeSetting commented Jul 22, 2024

@CyrusNajmabadi Thank you for taking the time to read this discussion and offer some feedback.

I'd gladly settle for an imperfect solution. The problem has continued to compound for a very long time now. After all that time, I'd happily settle for a "bandage" that makes the issue less cumbersome to work around.

Perhaps, a well-documented extension method for IEnumerable<T> that returns an IReadOnlyCollection<T> might help? The method might return the original sequence, if it implements IReadOnlyCollection<T>. Otherwise, if the original sequence implements ICollection<T>, it might wrap it in a helper class that implements IReadOnlyCollection<T>. Otherwise, it could throw an exception or (optionally) use ToArray() to convert the sequence and then wrap it.

This would at least lighten the lifting load when I am inevitably forced to use IEnumerable<T>, yet again, as my constraint or parameter type.

Or maybe it would help if a common ancestor interface (for the two collection interfaces) were added? Even one without any members could be useful (providing it is descended from IEnumerable<T>). If it existed, the aforementioned extension method could extend that new interface instead of IEnumerable<T>.

The new interface would at least provide a more restrictive constraint and/or parameter type. The name of the interface would advertise its intent. The extension method would handle the plumbing and enforcement (admittedly at run-time instead of compile-time).

With more commitment, perhaps the compiler could help with some syntactic sugar?

Anyhow, these are just a few ideas, off the top of my head ideas. None of them introduce breaking changes. Any of them would at least slightly improve the situation.

I feel certain smarter people than I could come up with even better ideas.

That said, my original point stands, taking nearly 20 years (and counting) to address the issue is not a good look. At this rate, I'm not sure C# will still be relevant by the time help arrives 😄

@CodeSetting
Copy link
Author

Adding some cross-references (#42254, #101469, and #101644) each of which discusses different aspects of the ICollection<T> versus IReadOnlyCollection<T> issue. From these, it sounds like there might be some hope of a resolution in .NET 10.

@github-actions github-actions bot locked and limited conversation to collaborators Sep 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Collections
Projects
None yet
Development

No branches or pull requests

4 participants