[API Proposal]: ReadOnly empty collection singletons

stephentoub commented 2 years ago

Background and motivation

Various pieces of code today allocate new read-only collections (e.g. ReadOnlyCollection<T>) around empty collections. Sometimes code implements its own singleton caching, e.g.

and sometimes it doesn't, e.g.

In other cases, empty could be special-cased but isn't as doing so would have required someone to add their own cache for it to have meaningful benefits).

In any case, we can make this easier and more efficient by exposing our own singletons that we and others can use.

(Replaces https://github.com/dotnet/runtime/issues/41147, which is limited to just ReadOnlyCollection.) (Related to https://github.com/dotnet/runtime/issues/38340, which asked for such Empty methods on mutable collections like List<T> returning empty instances of those same collections. There we said we'd consider read-only variants with a proposal; this is that proposal.)

API Proposal

namespace System.Collections.Generic
{
    public interface IReadOnlyCollection<out T>
    {
+        public static IReadOnlyCollection<T> Empty { get; }
    }

    public interface IReadOnlyList<out T> : IReadOnlyCollection<T>
    {
+        public static new IReadOnlyList<T> Empty { get; }
    }

    public interface IReadOnlyDictionary<TKey, TValue> : IReadOnlyCollection<KeyValuePair<TKey, TValue>>
    {
+        public static new IReadOnlyDictionary<TKey, TValue> Empty { get; }
    }

    public interface IReadOnlySet<T> : IReadOnlyCollection<T>
    {
+        public static new IReadOnlySet<T> Empty { get; }
    }
}

namespace System.Collections.ObjectModel
{
    public class ReadOnlyCollection<T>
    {
+        public static ReadOnlyCollection<T> Empty { get; }
    }

    public class ReadOnlyDictionary<TKey, TValue>
    {
+        public static ReadOnlyDictionary<TKey, TValue> Empty { get; }
    }

    public class ReadOnlyObservableCollection<T>
    {
+        public static ReadOnlyObservableCollection<T> Empty { get; }
    }
}

API Usage

public ReadOnlyCollection<T> GetValues() =>
    _count != 0 ? new ReadOnlyCollection<T>(_values) : ReadOnlyCollection<T>.Empty;

Alternative Designs

No response

Risks

These might be our first non-abstract/virtual static interface methods in the core libraries?

ghost commented 2 years ago

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

Issue Details

### Background and motivation Various pieces of code today allocate new read-only collections (e.g. `ReadOnlyCollection`) around empty collections. Sometimes code implements its own singleton caching, e.g. - https://github.com/dnSpy/dnSpy/blob/2b6dcfaf602fb8ca6462b8b6237fdfc0c74ad994/dnSpy/dnSpy/Documents/Tabs/DocViewer/DebugInfoDocumentViewerCustomDataProvider.cs#L33 - https://github.com/dnSpy/dnSpy/blob/2b6dcfaf602fb8ca6462b8b6237fdfc0c74ad994/Extensions/dnSpy.Debugger/dnSpy.Debugger.DotNet.Metadata/ReadOnlyCollectionHelpers.cs#L26 - https://github.com/dotnet/runtime/blob/58b682803e398fc18311dcc5faf757247c7e83af/src/libraries/System.ComponentModel.Composition/src/System/ComponentModel/Composition/Hosting/CompositionContainer.cs#L30 - https://github.com/dotnet/roslyn/blob/121d9b5cdba4c917c99fbcb6cfa85a5cf10e6e59/src/ExpressionEvaluator/CSharp/Source/ExpressionCompiler/EvaluationContext.cs#L401-L402 - https://github.com/dotnet/maui/blob/dbe44e68802b7585918e4c001f75be9770dd063d/src/Controls/src/Core/Element.cs#L15 - https://github.com/PowerShell/PowerShell/blob/c978d467ab778fe544808227d915408eaba6b22c/src/System.Management.Automation/engine/Utils.cs#L1451-L1452 - https://github.com/dotnet/wpf/blob/a19b3d79d3dac18930f943e146cfce67a48bb0eb/src/Microsoft.DotNet.Wpf/src/System.Xaml/System/Xaml/XamlType.cs#L1823-L1824 - https://github.com/dotnet/SqlClient/blob/7d348eb0c985b34ce5b76a9330c57bc1015c93ff/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlDataReader.cs#L75 - https://github.com/dotnet/runtime/blob/58b682803e398fc18311dcc5faf757247c7e83af/src/libraries/System.ComponentModel.Composition/src/System/ComponentModel/Composition/MetadataServices.cs#L12 - https://github.com/UiPath/CoreWF/blob/725ab544ec199e7d3013bab3740f77d47c10d892/src/UiPath.Workflow.Runtime/Runtime/DurableInstancing/InstanceOwnerQueryResult.cs#L12-L13 - https://github.com/elastic/elasticsearch-net/blob/fc5d848f3441279bc907ae2000a6ae1f9a2c606d/src/Elastic.Clients.Elasticsearch/Common/IsADictionaryBase.cs#L151 - https://github.com/Particular/NServiceBus/blob/56ca994085a487bbd9597ec1e37513cdf11f02a7/src/NServiceBus.Core/Transports/QueueAddress.cs#L12 - https://github.com/UiPath/CoreWF/blob/725ab544ec199e7d3013bab3740f77d47c10d892/src/UiPath.Workflow.Runtime/Runtime/DurableInstancing/InstanceKey.cs#L13 - https://github.com/datastax/csharp-driver/blob/27738b0ddd62c8e523fb2867c48d331257738a6c/src/Cassandra/Exceptions/ReadFailureException.cs#L29-L30 - https://github.com/googleapis/gax-dotnet/blob/01c7e7fd221649f98eb08586017f62d6ee2ffc22/Google.Api.Gax/EmptyDictionary.cs#L23-L24 and sometimes it doesn't, e.g. - https://github.com/dotnet/runtime/blob/58b682803e398fc18311dcc5faf757247c7e83af/src/libraries/System.Security.Cryptography.Pkcs/src/System/Security/Cryptography/Pkcs/Pkcs12Info.cs#L117-L120 - https://github.com/JosefPihrt/Roslynator/blob/8b27c6ac78c17f5abd8151d995bdb4f88f394b0e/src/Tools/Metadata/SourceFile.cs#L17 - https://github.com/dotnet-script/dotnet-script/blob/b5ff49b47134a2aa716d9dc9f08355d3e89d7638/src/Dotnet.Script.Core/ScriptContext.cs#L17 - https://github.com/UiPath/CoreWF/blob/725ab544ec199e7d3013bab3740f77d47c10d892/src/UiPath.Workflow.Runtime/HybridCollection.cs#L116 - https://github.com/dotnet/wcf/blob/41dfb82e834f7f798e091be235925d44469a9cac/src/dotnet-svcutil/lib/src/FrameworkFork/System.Runtime.Serialization/System/Runtime/Serialization/DataContractSerializer.cs#L156 - https://github.com/dotnet/runtime/blob/58b682803e398fc18311dcc5faf757247c7e83af/src/libraries/System.Speech/src/Synthesis/VoiceInfo.cs#L204 - https://github.com/psibr/REstate/blob/0a3ae3b6d3c5dbf368385476ea4eec2aa5defbb2/src/REstate.Remote/GrpcStateMachine.cs#L66 In other cases, empty could be special-cased but isn't as doing so would have required someone to add their own cache for it to have meaningful benefits). In any case, we can make this easier and more efficient by exposing our own singletons that we and others can use. (Replaces https://github.com/dotnet/runtime/issues/41147, which is limited to just ReadOnlyCollection.) (Related to https://github.com/dotnet/runtime/issues/38340, which asked for such Empty methods on _mutable_ collections like `List` returning empty instances of those same collections. There we said we'd consider read-only variants with a proposal; this is that proposal.) ### API Proposal ```diff namespace System.Collections.Generic { public interface IReadOnlyCollection { + public static IReadOnlyCollection Empty { get; } } public interface IReadOnlyList : IReadOnlyCollection { + public static new IReadOnlyList Empty { get; } } public interface IReadOnlyDictionary : IReadOnlyCollection> { + public static new IReadOnlyDictionary Empty { get; } } public interface IReadOnlySet : IReadOnlyCollection { + public static new IReadOnlySet Empty { get; } } } namespace System.Collections.ObjectModel { public class ReadOnlyCollection { + public static ReadOnlyCollection Empty { get; } } public class ReadOnlyDictionary { + public static ReadOnlyDictionary Empty { get; } } } ``` ### API Usage ```csharp public ReadOnlyCollection GetValues() => _count != 0 ? new ReadOnlyCollection(_values) : ReadOnlyCollection.Empty; ``` ### Alternative Designs _No response_ ### Risks - These might be our first non-abstract/virtual static interface methods in the core libraries?

Author:	stephentoub
Assignees:	-
Labels:	`api-suggestion`, `area-System.Collections`
Milestone:	8.0.0

eiriktsarpalis commented 2 years ago

These might be our first non-abstract/virtual static interface methods in the core libraries?

Any potential risks? From a discoverability standpoint they seem pretty nice, and possibly a pattern we could be introducing in other interface types.

stephentoub commented 2 years ago

Any potential risks?

Not that I'm aware of. It just hasn't been expressible until relatively recently (C# 8 if memory serves).

huoyaoyuan commented 2 years ago

For IReadOnlyCollection<T> and IReadOnlyList<T>, what's the benefit comparing to Array.Empty?

stephentoub commented 2 years ago

For IReadOnlyCollection and IReadOnlyList, what's the benefit comparing to Array.Empty?

Discoverability and consistency across the interfaces. There's no performance benefit I can think of.

eiriktsarpalis commented 2 years ago

For IReadOnlyCollection and IReadOnlyList, what's the benefit comparing to Array.Empty?

Discoverability and consistency across the interfaces. There's no performance benefit I can think of.

Should we be using private implementations for each these interfaces or does returning Array.Empty suffice? Assuming we do the latter, Hyrum's law dictates that users will take a dependency on the underlying type being an array, but I don't foresee this needing to change.

stephentoub commented 2 years ago

Should we be using private implementations for each these interfaces or does returning Array.Empty suffice?

Unless we can optimize something better than what array does, I think we should just use Array.Empty.

omariom commented 2 years ago

ReadOnlyObservableCollection seems to be a good candidate too

GSPP commented 2 years ago

Unless we can optimize something better than what array does, I think we should just use Array.Empty.

This will invariably lead to people taking a dependency on these values being actual arrays. Of course, nobody should do that but it's going to happen. This could make future changes to these properties difficult from a compatibility standpoint. Maybe it's safer to have a private implementation.

For example, a reflection-based serializer might take a different path based on runtime type.

eiriktsarpalis commented 2 years ago

For example, a reflection-based serializer might take a different path based on runtime type.

That's a good point, actually. Any reflection-based serializer using polymorphism might provide divergent serialization contracts for empty instances.

danmoseley commented 2 years ago

What does using the interface members look like -- var empty = IReadOnlyList<Foo>.Empty; ?

List<Foo>.Empty would be more discoverable, presumably. (or create a new ReadOnlyList<T> and put it on there, similar for ReadOnlyHashSet<T> perhaps). what am I missing?

stephentoub commented 2 years ago

what am I missing?

That List<Foo>.Empty would be expected to return a List<Foo> (which we can't do), not IReadOnlyList<Foo>. So it would discoverably return the wrong thing.

danmoseley commented 2 years ago

Right, I figured the return type is something that could be navigated, once you've found the thing. But I defer to API experts.

danmoseley commented 2 years ago

And is there value in considering adding ReadOnlyList<T> and ReadOnlyHashSet<T>, including Empty following the pattern above?

stephentoub commented 2 years ago

And is there value in considering adding ReadOnlyList<T> and ReadOnlyHashSet<T>, including Empty following the pattern above?

ReadOnlyCollection<T> is an IReadOnlyList<T>. I don't think adding a whole new type that's a duplicate of ReadOnlyCollection<T> adds enough values.

There might be value in a ReadOnlySet<T> ala ReadOnlyDictionary<TKey, TValue>, but I wouldn't add that purely for Empty. Rather, we could choose to add such a thing on its own merit, and if we ever did, it's surface area would include an Empty property.

tfenise commented 2 years ago

Would this cause confusions, as the proposed static properties would be inherited? For example, if IReadOnlyList<T>.Empty were introduced, IImmutableList<T>, which inherits from IReadOnlyList<T>, would also have an inherited static property Empty, which unfortunately would be of type IReadOnlyList<T>, not IImmutableList<T>.

For other languages like VB or F#, would static interface members be difficult to consume?

stephentoub commented 2 years ago

add IReadOnlySet<T>.Empty

That's already there in the list.

terrajobst commented 2 years ago

Video

We're undecided on the statics on the interface
- We haven't used non-virtual statics on interface
- Unclear what this means for language interop
- Unclear how it impacts IntelliSense on interfaces extending these
- It's new pattern
We are in favor of exposing them on the concrete types
We currently don't have a ReadOnlySet<T>; if we had this, we should also a static Empty there

namespace System.Collections.ObjectModel
{
    public partial class ReadOnlyCollection<T>
    {
        public static ReadOnlyCollection<T> Empty { get; }
    }

    public partial class ReadOnlyDictionary<TKey, TValue>
    {
        public static ReadOnlyDictionary<TKey, TValue> Empty { get; }
    }

    public partial class ReadOnlyObservableCollection<T>
    {
        public static ReadOnlyObservableCollection<T> Empty { get; }
    }
}

alexrp commented 2 years ago

FWIW, I just ran into a situation today where I would have liked to have IReadOnlySet<T>.Empty and had to roll my own. Would be a bit unfortunate if that doesn't get included here in some form.

dotnet / runtime