Fix HashSet copy constructor handling of instances that have fallen b…

…ack to the randomized hashcode generator. (#107613) (#107687)

src/libraries/System.Collections/tests/Generic/HashSet/HashSet.NonGeneric.Tests.cs

@@ -0,0 +1,39 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System.Collections.Generic;
+using System.Reflection;
+using System.Runtime.Serialization;
+using Xunit;
+
+namespace System.Collections.Tests
+{
+    public static class HashSet_NonGeneric_Tests
+    {
+        [Fact]
+        public static void HashSet_CopyConstructor_ShouldWorkWithRandomizedEffectiveComparer()
+        {
+            HashSet<string> set = CreateCopyWithRandomizedComparer(new HashSet<string>() { "a", "b" });
+            Assert.True(set.Contains("a"));
+
+            HashSet<string> copiedSet = new(set);
+            Assert.True(copiedSet.Contains("a"));
+
+            static HashSet<string> CreateCopyWithRandomizedComparer(HashSet<string> set)
+            {
+                // To reproduce the bug, we need a HashSet<string> instance that has fallen back to
+                // the randomized comparer. This typically happens when there are many collisions but
+                // it can also happen when the set is serialized and deserialized via ISerializable.
+                // For consistent results and to avoid brute forcing collisions, use the latter approach.
+
+                SerializationInfo info = new(typeof(HashSet<string>), new FormatterConverter());
+                StreamingContext context = new(StreamingContextStates.All);
+                set.GetObjectData(info, context);
+
+                HashSet<string> copiedSet = (HashSet<string>)Activator.CreateInstance(typeof(HashSet<string>), BindingFlags.NonPublic | BindingFlags.Instance, null, [info, context], null);
+                copiedSet.OnDeserialization(null);
+                return copiedSet;
+            }
+        }
+    }
+}

src/libraries/System.Collections/tests/System.Collections.Tests.csproj

@@ -1,4 +1,4 @@
-<Project Sdk="Microsoft.NET.Sdk">
+<Project Sdk="Microsoft.NET.Sdk">
   <PropertyGroup>
     <TargetFramework>$(NetCoreAppCurrent)</TargetFramework>
     <TestRuntime>true</TestRuntime>
@@ -48,6 +48,7 @@
     <Compile Include="$(CommonTestPath)System\Runtime\Serialization\Formatters\BinaryFormatterHelpers.cs"
              Link="Common\System\Runtime\Serialization\Formatters\BinaryFormatterHelpers.cs" />
     <!-- Generic tests -->
+    <Compile Include="Generic\HashSet\HashSet.NonGeneric.Tests.cs" />
     <Compile Include="Generic\SortedSet\SortedSet.TreeSubSet.Tests.cs" />
     <Compile Include="StructuralComparisonsTests.cs" />
     <Compile Include="BitArray\BitArray_CtorTests.cs" />

src/libraries/System.Private.CoreLib/src/System/Collections/Generic/HashSet.cs

@@ -68,7 +68,7 @@ public HashSet(IEqualityComparer<T>? comparer)
                 // We use a non-randomized comparer for improved perf, falling back to a randomized comparer if the
                 // hash buckets become unbalanced.
                 if (typeof(T) == typeof(string) &&
-                    NonRandomizedStringEqualityComparer.GetStringComparer(_comparer!) is IEqualityComparer<string> stringComparer)
+                    NonRandomizedStringEqualityComparer.GetStringComparer(_comparer) is IEqualityComparer<string> stringComparer)
                 {
                     _comparer = (IEqualityComparer<T>)stringComparer;
                 }
@@ -91,7 +91,7 @@ public HashSet(IEnumerable<T> collection, IEqualityComparer<T>? comparer) : this
                 ThrowHelper.ThrowArgumentNullException(ExceptionArgument.collection);
             }
 
-            if (collection is HashSet<T> otherAsHashSet && EqualityComparersAreEqual(this, otherAsHashSet))
+            if (collection is HashSet<T> otherAsHashSet && EffectiveEqualityComparersAreEqual(this, otherAsHashSet))
             {
                 ConstructFrom(otherAsHashSet);
             }
@@ -144,6 +144,8 @@ protected HashSet(SerializationInfo info, StreamingContext context)
         /// <summary>Initializes the HashSet from another HashSet with the same element type and equality comparer.</summary>
         private void ConstructFrom(HashSet<T> source)
         {
+            Debug.Assert(EffectiveEqualityComparersAreEqual(this, source), "must use identical effective comparers.");
+
             if (source.Count == 0)
             {
                 // As well as short-circuiting on the rest of the work done,
@@ -929,6 +931,11 @@ public IEqualityComparer<T> Comparer
             }
         }
 
+        /// <summary>
+        /// Similar to <see cref="Comparer"/> but surfaces the actual comparer being used to hash entries.
+        /// </summary>
+        internal IEqualityComparer<T> EffectiveComparer => _comparer ?? EqualityComparer<T>.Default;
+
         /// <summary>Ensures that this hash set can hold the specified number of elements without growing.</summary>
         public int EnsureCapacity(int capacity)
         {
@@ -1439,7 +1446,13 @@ private unsafe void SymmetricExceptWithEnumerable(IEnumerable<T> other)
         /// </summary>
         internal static bool EqualityComparersAreEqual(HashSet<T> set1, HashSet<T> set2) => set1.Comparer.Equals(set2.Comparer);
 
-#endregion
+        /// <summary>
+        /// Checks if effective equality comparers are equal. This is used for algorithms that
+        /// require that both collections use identical hashing implementations for their entries.
+        /// </summary>
+        internal static bool EffectiveEqualityComparersAreEqual(HashSet<T> set1, HashSet<T> set2) => set1.EffectiveComparer.Equals(set2.EffectiveComparer);
+
+        #endregion
 
         private struct Entry
         {