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 FrozenSet.Create collection builder #102223

Merged
merged 1 commit into from
May 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,11 @@ void System.IDisposable.Dispose() { }
}
public static partial class FrozenSet
{
public static System.Collections.Frozen.FrozenSet<T> Create<T>(params System.ReadOnlySpan<T> source) { throw null; }
public static System.Collections.Frozen.FrozenSet<T> Create<T>(System.Collections.Generic.IEqualityComparer<T>? equalityComparer, params System.ReadOnlySpan<T> source) { throw null; }
public static System.Collections.Frozen.FrozenSet<T> ToFrozenSet<T>(this System.Collections.Generic.IEnumerable<T> source, System.Collections.Generic.IEqualityComparer<T>? comparer = null) { throw null; }
}
[System.Runtime.CompilerServices.CollectionBuilder(typeof(System.Collections.Frozen.FrozenSet), nameof(System.Collections.Frozen.FrozenSet.Create))]
public abstract partial class FrozenSet<T> : System.Collections.Generic.ICollection<T>, System.Collections.Generic.IEnumerable<T>, System.Collections.Generic.IReadOnlyCollection<T>, System.Collections.Generic.ISet<T>, System.Collections.ICollection, System.Collections.IEnumerable
{
internal FrozenSet() { }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System.Collections.Immutable;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;

namespace System.Collections.Frozen
Expand All @@ -14,6 +15,40 @@ namespace System.Collections.Frozen
/// </summary>
public static class FrozenSet
{
/// <summary>Creates a <see cref="FrozenSet{T}"/> with the specified values.</summary>
/// <param name="source">The values to use to populate the set.</param>
/// <typeparam name="T">The type of the values in the set.</typeparam>
/// <returns>A frozen set.</returns>
public static FrozenSet<T> Create<T>(params ReadOnlySpan<T> source) => Create(null, source);

/// <summary>Creates a <see cref="FrozenSet{T}"/> with the specified values.</summary>
/// <param name="source">The values to use to populate the set.</param>
/// <param name="equalityComparer">The comparer implementation to use to compare values for equality. If null, <see cref="EqualityComparer{T}.Default"/> is used.</param>
/// <typeparam name="T">The type of the values in the set.</typeparam>
/// <returns>A frozen set.</returns>
public static FrozenSet<T> Create<T>(IEqualityComparer<T>? equalityComparer, params ReadOnlySpan<T> source)
{
if (source.Length == 0)
{
return equalityComparer is null || ReferenceEquals(equalityComparer, FrozenSet<T>.Empty.Comparer) ?
FrozenSet<T>.Empty :
new EmptyFrozenSet<T>(equalityComparer);
}

HashSet<T> set =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really related to this PR, but with all the improvements around ROSpan<T> (params, the new language support that makes ROSpan<T> the preferred overload, collection expression, etc) do we have any plans to expose constructor overloads on the various collection types so this can just be new(source), much like we could if it had been an IEnumerable<T>?

Seems like a generally good thing that can simplify the patterns for common user scenarios. -- Notable doing HashSet<T> set = [..source]; works as well, its just not super efficient today and looks to go through the enumerator not the special for (...) iteration that span normally gets.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'd like to, but we can't do that well yet. Even with how the current first-class span support is defined, if HashSet gained a HashSet<T>(ReadOnlySpan<T>) overload and you tried to call that with an array, it would fail to compile due to ambiguity, since array can implicitly convert to both IEnumerable<T> and ReadOnlySpan<T>.

This should however resolve itself if/when we make ReadOnlySpan<T> : IEnumerable<T>, as that will result in ReadOnlySpan<T> being considered better for overload resolution. The ref struct work just landed in Roslyn main today. Once that lands in runtime and I can more easily vet it, my plan is to create a proposal for the resulting improvements we should make in .NET 9, including where we want to annotate with allow ref struct (e.g. delegates, enumerable) and what new APIs we can then expose that we've been wanting to (e.g. that ctor on HashSet).

#if NET
new(source.Length, equalityComparer); // we assume there are few-to-no duplicates when using this API
#else
new(equalityComparer);
#endif
foreach (T item in source)
{
set.Add(item);
}

return ToFrozenSet(set, equalityComparer);
}

/// <summary>Creates a <see cref="FrozenSet{T}"/> with the specified values.</summary>
/// <param name="source">The values to use to populate the set.</param>
/// <param name="comparer">The comparer implementation to use to compare values for equality. If null, <see cref="EqualityComparer{T}.Default"/> is used.</param>
Expand Down Expand Up @@ -199,6 +234,7 @@ private static FrozenSet<T> CreateFromSet<T>(HashSet<T> source)
/// the remainder of the life of the application. <see cref="FrozenSet{T}"/> should only be initialized
/// with trusted elements, as the details of the elements impacts construction time.
/// </remarks>
[CollectionBuilder(typeof(FrozenSet), nameof(FrozenSet.Create))]
[DebuggerTypeProxy(typeof(ImmutableEnumerableDebuggerProxy<>))]
[DebuggerDisplay("Count = {Count}")]
public abstract class FrozenSet<T> : ISet<T>,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,16 @@ public void EmptySource_ProducedFrozenSetEmpty()

Assert.NotSame(FrozenSet<T>.Empty, source.ToFrozenSet(NonDefaultEqualityComparer<T>.Instance));
}

Assert.Same(FrozenSet<T>.Empty, FrozenSet.Create(ReadOnlySpan<T>.Empty));
Assert.Same(FrozenSet<T>.Empty, FrozenSet.Create<T>());

foreach (IEqualityComparer<T> comparer in new IEqualityComparer<T>[] { null, EqualityComparer<T>.Default })
{
Assert.Same(FrozenSet<T>.Empty, FrozenSet.Create(comparer));
}

Assert.NotSame(FrozenSet<T>.Empty, FrozenSet.Create(NonDefaultEqualityComparer<T>.Instance));
}

[Fact]
Expand Down Expand Up @@ -387,7 +397,7 @@ public class FrozenSet_Generic_Tests_string_OrdinalIgnoreCase : FrozenSet_Generi
[Fact]
public void TryGetValue_FindsExpectedResult()
{
FrozenSet<string> frozen = new[] { "abc" }.ToFrozenSet(StringComparer.OrdinalIgnoreCase);
FrozenSet<string> frozen = FrozenSet.Create(StringComparer.OrdinalIgnoreCase, "abc");

Assert.False(frozen.TryGetValue("ab", out string actualValue));
Assert.Null(actualValue);
Expand Down Expand Up @@ -498,7 +508,7 @@ public void Sparse_LookupItems_AlltemsFoundAsExpected()
foreach (int skip in new[] { 2, 3, 5 })
{
var original = new HashSet<int>(Enumerable.Range(-3, size).Where(i => i % skip == 0));
FrozenSet<int> frozen = original.ToFrozenSet();
FrozenSet<int> frozen = [.. original];

for (int i = -10; i <= size + 66; i++)
{
Expand Down Expand Up @@ -539,7 +549,7 @@ public void ClosedRange_Lookup_AllItemsFoundAsExpected()
original.Add(i);
}

FrozenSet<long> frozen = original.ToFrozenSet();
FrozenSet<long> frozen = [.. original];

min = start > long.MinValue ? start - 10 : start;
max = start + size - 1 < long.MaxValue ? start + size + 9 : start + size - 1;
Expand Down
Loading