Add an `ISet<T>.AsReadOnly()` extension method

Question

Add an `ISet<T>.AsReadOnly()` extension method

vanillajonathan opened this issue 7 years ago · 28 comments

The List<> class have a AsReadOnly method to turn the collection into a ReadOnlyCollection, but HashSet<> does not have any such method.

Rationale and usage

Allows Entity Framework Core to expose a collection navigation property as a IReadOnlyCollection when backing it with a private backing field declared as a HashSet.

[Table("Blogs")]
public class Blog
{
    private HashSet<Post> _posts;
 
    public int BlogId { get; set; }
 
    public IReadOnlyCollection<Post> Posts => _posts?.AsReadOnly();
 
    public void AddPost(Post post)
    {
        // Do some business logic here...
        _posts.Add(post);
    }
}

Proposed API

namespace System.Collections.Generic;

public class CollectionExtensions
{
      public ReadOnlyCollection<T> AsReadOnly(this IList<T> list);
      public ReadOnlyDictionary<T> AsReadOnly(this IDictionary<T> dictionary);
+     public ReadOnlySet<T> AsReadOnly(this ISet<T> set);
}

Answer 1 · 2019-05-23T21:30:31.000Z

Thanks @vanillajonathan I think this makes sense. I've marked the issue as api ready for review.

Answer 2 · 2019-09-10T19:22:17.000Z

Returning ReadOnlyCollection means AsReadOnly will have to make a full copy of the HashSet.

What about a new type ReadOnlySet than can wrap any ISet<T>?
Then we will have readonly wrappers for all "ObjectModel" collections: IList, IDictionary, ObservableCollection and ISet.

Answer 3 · 2020-01-07T19:22:28.000Z

Feedback - what are you actually trying to do here?

HashSet<T> already implements IReadOnlyCollection<T>, so you can cast it directly: IReadOnlyCollection<T> collection = myHashSet;.

If you want to create a ReadOnlyCollection<T>, it's one line: ReadOnlyCollection<T> collection = new ReadOnlyCollection<T>(myHashSet.ToArray());.

Answer 4 · 2020-01-07T23:21:03.000Z

I want to create a entity in Entity Framework Core. EF Core internally treats collections as hash sets but they are commonly exposed as ICollection properties in the DbSet.

I intend to employ domain-driven design (DDD), so I do not want to expose it as a ICollection since that would allow the collection to be modified.

Since HashSet (as you pointed out) implements IReadOnlyCollection<T> I can expose it as a IReadOnlyCollection (instead of a ICollection) but since that is just exposing it as a different interface it is still possible to typecast it back to a ICollection and modify it.

Answer 5 · 2020-01-08T01:43:13.000Z

In your scenario, accessing the Posts property will return a brand new collection instance every single time. I'd instead recommend using the existing List<T> and ReadOnlyCollection<T> types, especially since it seems HashSet<T> might not be a good fit for the Post type.

Example:

public class Blog
{
    private List<Post> _posts = new List<Post>();
    private ReadOnlyCollection<Post> _postsAsReadOnlyCollection = new ReadOnlyCollection<Post>(_posts);

    public IReadOnlyCollection<Post> Posts => _postsAsReadOnlyCollection;

    public void AddPost(Post post)
    {
        // business logic here
        _posts.Add(post);
    }
}

Or, if you really want to use HashSet<Post>:

public class Blog
{
    private HashSet<Post> _posts = new HashSet<Post>();
    private ReadOnlyCollection<Post> _cachedROC = null;

    public IReadOnlyCollection<Post> Posts => _cachedROC ?? (_cachedROC = new ReadOnlyCollection<T>(_posts.ToArray()));

    public void AddPost(Post post)
    {
        // business logic
        _posts.Add(post);
        _cachedROC = null; // clear cached value if it exists
    }
}

Answer 6 · 2020-01-08T15:45:21.000Z

I was under the impression that ToList() would create a brand new collection instance every single time, and that AsReadOnly would not do so.

In the first of your examples I would need two private and one public instead of just one public or one private setter. Also it uses List while EF Core internally uses HashSet so the performance may differ.

In the second of your examples, it introduces caching into the business entity.

Both of these are undesirable. Could HashSet have a AsReadOnly method, and would that solve this in a clean, efficient manner?

Answer 7 · 2020-01-08T18:44:30.000Z

I was under the impression that ToList() would create a brand new collection instance every single time, and that AsReadOnly would not do so.

Yes, it must create a brand new collection every time. The reason for this is that ReadOnlyCollection<T> requires the underlying collection to implement IList<T>, which HashSet<T> does not do on its own. This means that a AsReadOnly() method which returns a ReadOnlyCollection<T> necessarily needs to make a copy of the contents of the HashSet<T> instance.

Answer 8 · 2020-01-09T09:16:04.000Z

The reason for this is that ReadOnlyCollection<T> requires the underlying collection to implement IList<T>, which HashSet<T> does not do on its own.

This raises the question, should HashSet<T> implement IList<T>?

Answer 9 · 2020-01-09T18:36:33.000Z

This raises the question, should HashSet implement IList?

No. HashSet<T> is an unordered no-duplicate collection. IList<T> is an ordered list, potentially with duplicates. They represent two different collection concepts.

Answer 10 · 2020-01-09T22:44:39.000Z

This raises the question, should HashSet implement IList?

No. HashSet<T> is an unordered no-duplicate collection. IList<T> is an ordered list, potentially with duplicates. They represent two different collection concepts.

Good point. Could HashSet<T> have a AsReadOnly method (like List<T> does, but without implementing IList<T>)?

With a List<T> you can cleanly do things like:

public class Blog
{
    private List<Post> _posts;
    public IReadOnlyCollection<Post> Posts => _posts?.AsReadOnly();
}

Which you cannot do as cleanly when using a HashSet<T> instead of a List<T>.

Answer 11 · 2020-01-10T00:01:47.000Z

Which brings us back to the original response on the thread... given your scenario it really would just be easier to have a property that directly casts the HashSet<T> to an IReadOnlyCollection<T>.

Since HashSet (as you pointed out) implements IReadOnlyCollection I can expose it as a IReadOnlyCollection (instead of a ICollection) but since that is just exposing it as a different interface it is still possible to typecast it back to a ICollection and modify it.

So don't cast it back? :)

Answer 12 · 2020-01-10T09:42:11.000Z

Which brings us back to the original response on the thread... given your scenario it really would just be easier to have a property that directly casts the HashSet<T> to an IReadOnlyCollection<T>.

Since HashSet (as you pointed out) implements IReadOnlyCollection I can expose it as a IReadOnlyCollection (instead of a ICollection) but since that is just exposing it as a different interface it is still possible to typecast it back to a ICollection and modify it.

So don't cast it back? :)

I wouldn't typecast it back to a ICollection<T>, but if I exposed a IReadOnlyCollection<T> then I want that to be read-only, but someone using my library could typecast it back a ICollection<T> and modify it.
With a List<T> this is much easier to prevent since it has a AsReadOnly method.

Answer 13 · 2020-01-10T20:21:02.000Z

So what is your concrete proposal then? Are you proposing a new type ReadOnlyCollectionWrapper<T> whose ctor takes an IReadOnlyCollection<T> instead of an IList<T>? Is this a scenario that is common enough and that would be burdensome enough for somebody to do themselves that such a type belongs in the Framework proper? Presumably if you need to be unblocked immediately you've already created such a wrapper.

Answer 14 · 2020-01-10T23:35:02.000Z

Well I don't know how feasible it is, but I expected the HashSet<T> to have a AsReadOnly method, so I would propose the addition of a AsReadOnly method to the HashSet<T>.

Answer 15 · 2020-01-11T03:20:29.000Z

And what would be the return type of HashSet<T>.AsReadOnly()? As discussed it can't be ReadOnlyCollection<T>.

Answer 16 · 2020-01-11T12:01:44.000Z

And what would be the return type of HashSet<T>.AsReadOnly()? As discussed it can't be ReadOnlyCollection<T>.

You're right, it cannot return a ReadOnlyCollection<T> since that class implements IList<T> which HashSet<T> does not.

Maybe it is weird that ReadOnlyCollection<T> implements IList<T>, and that IList<T> implements ICollection<T>?
Maybe ReadOnlyCollection<T> should not have implemented IList<T>?

Answer 17 · 2020-01-11T21:42:32.000Z

Yeah, in hindsight it may have made sense to have two types ReadOnlyList<T> and ReadOnlyCollection<T>. That predates my time on this team so I'm not familiar with the history behind why this decision was made. But regardless it's not a decision that can be undone right now. ☹

Answer 18 · 2020-01-11T22:49:37.000Z

Maybe things can be fixed for .NET 5?

This would be nice for developers using Entity Framework Core and who wish to implement domain-driven design (DDD).

Answer 19 · 2020-01-11T22:52:27.000Z

That's not the type of breaking change we would take in .NET 5.

Answer 20 · 2020-08-15T12:11:40.000Z

And what would be the return type of HashSet<T>.AsReadOnly()? As discussed it can't be ReadOnlyCollection<T>.

What about ReadOnlySet which can wrap any ISet ?
Like https://stackoverflow.com/a/36815316/558098

Answer 21 · 2020-11-11T10:04:55.000Z

Assuming we decided to add something like this, I think it should return an IReadOnlySet<T> instance. This begs the question whether we should be implementing a ReadOnlySet<T> class that wraps an ISet<T> instance.

Answer 22 · 2020-12-07T15:36:47.000Z

Wouldn’t it be better for this not to be part of the class but just an extension? Especially since HashSet<T> now implements IReadOnlySet<T>, this could just be like this:

public static class HashSetExtensions {
    public static IReadOnlySet<T> AsReadOnlySet(
        this IReadOnlySet<T> readOnlySet) {
        return readOnlySet;
    }
}

Since a direct cast exists, the rationale for including the above API would be the same as for having Enumerable.AsEnumerable<TSource>()—to support the use of implicit typing and/or method overload resolution, especially if T in IReadOnlySet<T> is big, impractical, or impossible to provide explicitly.

For wrapping ISet<T>, I’d expect a thin wrapper and the return type to be IReadOnlySet<T> instead of doing something like returning an instance of ReadOnlyCollection<T>. Then the reader can see updates made to the ISet<T> by others (remember, read-only and immutable are very different concepts even though immutable implies unwritable). The biggest issue with this approach would be the specificity/overloads for the appropriate extension methods. Since HashSet<T> is both ISet<T> and IReadOnlySet<T>, the following would create an ambiguity error if merged with the above code:

public static class HashSetExtensions {
    public static IReadOnlySet<T> AsReadOnlySet(
        this ISet<T> set) {
        return new ReadOnlySetAdapter<T>(set);
    }

    class ReadOnlySetAdapter<T> : IReadOnlySet<T> {
        …
    }
}

Answer 23 · 2021-10-29T14:01:29.000Z

Closing -- would not support exposing an extension method as proposed. It would need to return IReadOnlySet<T>, but this should only be considered in conjunction with a ReadOnlySet<T> proposal à la ReadOnlyCollection<T>.

Answer 24 · 2024-06-18T01:56:47.000Z

ReadOnlySet<T> now exists in .NET 9. We could choose to have an ReadOnlySet<T> AsReadOnly() if desirable.

Answer 25 · 2024-06-18T09:29:52.000Z

I've updated the API proposal to add an extension method in CollectionExtensions.

Answer 26 · 2024-08-06T18:12:35.000Z

Video

Looks good as proposed

namespace System.Collections.Generic;

public partial class CollectionExtensions
{
    // Existing:
    // public ReadOnlyCollection<T> AsReadOnly(this IList<T> list);
    // public ReadOnlyDictionary<T> AsReadOnly(this IDictionary<T> dictionary);
    public ReadOnlySet<T> AsReadOnly(this ISet<T> set);
}

Answer 27 · 2024-11-18T16:32:49.000Z

Was there any reason this extension didn't make it into .NET 9?

Answer 28 · 2024-11-18T17:46:55.000Z

Read the discussion in the linked PR #106037