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

Have HashSet<T> implement ICollection #23605

Closed
weitzhandler opened this issue Sep 19, 2017 · 9 comments
Closed

Have HashSet<T> implement ICollection #23605

weitzhandler opened this issue Sep 19, 2017 · 9 comments
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Collections backlog-cleanup-candidate An inactive issue that has been marked for automated closure.
Milestone

Comments

@weitzhandler
Copy link
Contributor

Motive

The ICollection (non-generic) interface, is a pathway to getting the Count property of a collection in a non-generic way. This is very common in converters or some other types that take in any object and has to deal with it differently if it's a collection.

Most common collections in .NET (List<T>, Dictionary<TKey, TValue>, Collection<T>, also implement ICollection, so it falls through the 'countable' criteria.

However, the HashSet<T> class doesn't.

So as explained in #23337, in order to obtain the Count of a HashSet<T>, we'll have to use reflection if we don't know T at compile time (for example we have an object typed variable that we know it contains a HashSet<T>, but T can be anything, and is irrelevant to be able to get the count.

Suggestion

I'd prefer seeing this issue addressed as it should be in the first place, which is having a non-generic middle interface that signifies the Count property, but meanwhile, having the HashSet<T> implement ICollection will bring the HashSet<T> to this countable group as well, even as a temporary solution.

Things to consider

  • If we do implement ICountable, we have another class to take care of before getting rid of the ICollection part, if we want to.
  • Please comment on

[EDIT] Fixed code link by @karelz

@sharwell
Copy link
Member

⚠️ This would break cases where a user defines an extension method for both ICollection<T> and ICollection, but fails to specialize the operation for HashSet<T>.

@weitzhandler
Copy link
Contributor Author

weitzhandler commented Sep 19, 2017

@sharwell You mean as opposed to List<T> that can contain duplicates? I don't really understood the issue.

@jnm2
Copy link
Contributor

jnm2 commented Sep 21, 2017

@weitzhandler It's the same as the issue with defining an extension method on both IList<> and IReadOnlyList<>– you can no longer use the extension method on any class that implements both.

An example of this is how people define IReadOnlyDictionary<,>.GetValueOrDefault and IDictionary<,>.GetValueOrDefault. They are then forced to also define an extension method on the concrete Dictionary<,>.GetValueOrDefault or trying to use the extension method on a Dictionary<,> causes a compiler error. @sharwell's point is that anyone who defined the same extension method for ICollection and ICollection<> and used the ICollection<> one with any HashSet<> instances will have their code start failing to compile as soon as HashSet<> starts implementing ICollection.

@ianhays
Copy link
Contributor

ianhays commented Oct 23, 2017

This is similar in break-ability to https://github.com/dotnet/corefx/issues/23578, so my concerns there are echoed here:

I don't think that being able to have a shared interface with the Count property is enough of a convenience to justify the breaks that this would incur.

Because of the required breaking changes, it would be very unlikely that we would be able to push a change this large through the API review process without a huge number of people that wanted it.

@weitzhandler
Copy link
Contributor Author

@ianhays
Hence this proposal.

@ianhays
Copy link
Contributor

ianhays commented Oct 30, 2017

The breaks are still there, though. The breaks come from introducing a new parent interface to HashSet; whether that is ICountable from #23337 or ICollection doesn't matter all that much.

@weitzhandler
Copy link
Contributor Author

weitzhandler commented Oct 30, 2017 via email

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 5.0 milestone Jan 31, 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
@layomia layomia modified the milestones: 5.0.0, Future Jun 24, 2020
@ghost
Copy link

ghost commented Oct 26, 2021

Due to lack of recent activity, this issue has been marked as a candidate for backlog cleanup. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will undo this process.

This process is part of the experimental issue cleanup initiative we are currently trialing in a limited number of areas. Please share any feedback you might have in the linked issue.

@ghost
Copy link

ghost commented Nov 9, 2021

This issue will now be closed since it had been marked no recent activity but received no further activity in the past 14 days. It is still possible to reopen or comment on the issue, but please note that the issue will be locked if it remains inactive for another 30 days.

@ghost ghost closed this as completed Nov 9, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2021
@eiriktsarpalis eiriktsarpalis added the backlog-cleanup-candidate An inactive issue that has been marked for automated closure. label Feb 18, 2022
@ghost ghost removed the no-recent-activity label Feb 18, 2022
This issue was closed.
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 backlog-cleanup-candidate An inactive issue that has been marked for automated closure.
Projects
None yet
Development

No branches or pull requests

8 participants