✨ Implement ImmutableSegmentedHashSet<T>

[sharwell]

Builds on #54574

[sharwell]

@dotnet/roslyn-compiler this is now ready for review

[333fred]

Are these todo's from the original source?

[sharwell]

No, these are ways the current implementation could theoretically be improved. My hope is that if someone sees allocations of certain types in the future, they are directed to the most straightforward way to eliminate those allocations.

[sharwell]

Here's an example of how I broke out a ValueBuilder nested type to reduce builder allocations in ImmutableSegmentedList<T>:
#54424
https://github.com/dotnet/roslyn/blob/main/src/Dependencies/Collections/ImmutableSegmentedList%601%2BValueBuilder.cs

[sharwell]

@dotnet/roslyn-compiler for second review

[sharwell]

@dotnet/roslyn-compiler for second review

[sharwell]

@dotnet/roslyn-compiler for second review

[sharwell]

@dotnet/roslyn-compiler for second review

[sharwell]

@dotnet/roslyn-compiler for second review

[jaredpar]

How can we see the delta from what the BCL sources were to what you added as a part of moving to Roslyn? I'm less interested in reviewing the BCL sources (battle tested, already reviewed, etc ...). Think it's more interesting for us to look at the delta to see what has changed and evaluate that.

[sharwell]

@jaredpar There is no exact mirror for this type in the BCL. The closest we have is the AVL-backed ImmutableHashSet<T>, so a close review would involve comparing the API defined in 1126d19 against the related original type (to show it's a drop-in replacement). In this case, the sources copied from runtime are the test cases for ImmutableHashSet<T> which were updated in 03c2ae7 to execute against the new data type and followed by a2023ec to get the tests fully passing against the new structure.

Note that I followed the pattern from earlier immutable types in this library and made ImmutableSegmentedHashSet<T> a value type to avoid a significant number of small allocations in common use cases.

[jaredpar]

Unsure what the goal of this pattern is here. Can you elaborate?

I can only see this being of value if you're protecting against mixed read / writes of this value in a location. Don't think we generally protect against incorrect uses like that.

[sharwell]

Good catch. This type appears to be missing the equivalent of this notice which is applied to ImmutableSegmentedList<T>:

roslyn/src/Dependencies/Collections/ImmutableSegmentedList`1.cs

Lines 59 to 69 in 519058e

	/// <para><strong>IMPORTANT NOTICE FOR MAINTAINERS AND REVIEWERS:</strong></para>
	///
	/// <para>This type should be thread-safe. As a struct, it cannot protect its own fields from being changed from one
	/// thread while its members are executing on other threads because structs can change <em>in place</em> simply by
	/// reassigning the field containing this struct. Therefore it is extremely important that <strong>⚠⚠ Every member
	/// should only dereference <c>this</c> ONCE ⚠⚠</strong>. If a member needs to reference the
	/// <see cref="_list"/> field, that counts as a dereference of <c>this</c>. Calling other instance members
	/// (properties or methods) also counts as dereferencing <c>this</c>. Any member that needs to use <c>this</c> more
	/// than once must instead assign <c>this</c> to a local variable and use that for the rest of the code instead.
	/// This effectively copies the one field in the struct to a local variable so that it is insulated from other
	/// threads.</para>

[sharwell]

I'll file a follow-up issue to add this documentation and implement the value builder.