From 4f185097fda6f7536b736d01e97ccbf2fe1fe487 Mon Sep 17 00:00:00 2001
From: Eirik Tsarpalis <eirik.tsarpalis@gmail.com>
Date: Wed, 11 Sep 2024 13:58:33 +0100
Subject: [PATCH] Fix HashSet copy constructor handling of instances that have
 fallen back to the randomized hashcode generator. (#107613)

---
 .../HashSet/HashSet.NonGeneric.Tests.cs       | 39 +++++++++++++++++++
 .../tests/System.Collections.Tests.csproj     |  5 +--
 .../src/System/Collections/Generic/HashSet.cs | 19 +++++++--
 3 files changed, 57 insertions(+), 6 deletions(-)
 create mode 100644 src/libraries/System.Collections/tests/Generic/HashSet/HashSet.NonGeneric.Tests.cs

diff --git a/src/libraries/System.Collections/tests/Generic/HashSet/HashSet.NonGeneric.Tests.cs b/src/libraries/System.Collections/tests/Generic/HashSet/HashSet.NonGeneric.Tests.cs
new file mode 100644
index 0000000000000..94705efb6fcc2
--- /dev/null
+++ b/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;
+            }
+        }
+    }
+}
diff --git a/src/libraries/System.Collections/tests/System.Collections.Tests.csproj b/src/libraries/System.Collections/tests/System.Collections.Tests.csproj
index 7c03f82de3934..6445d52cb3d9a 100644
--- a/src/libraries/System.Collections/tests/System.Collections.Tests.csproj
+++ b/src/libraries/System.Collections/tests/System.Collections.Tests.csproj
@@ -6,9 +6,7 @@
   </PropertyGroup>
   <ItemGroup>
     <!-- Reference the `NetCoreAppMinimum` build which has a functional BinaryFormatter and force a private copy to ensure it's not excluded -->
-    <ProjectReference Include="$(LibrariesProjectRoot)System.Runtime.Serialization.Formatters\src\System.Runtime.Serialization.Formatters.csproj"
-                      Private="true"
-                      SetTargetFramework="TargetFramework=$(NetCoreAppMinimum)" />
+    <ProjectReference Include="$(LibrariesProjectRoot)System.Runtime.Serialization.Formatters\src\System.Runtime.Serialization.Formatters.csproj" Private="true" SetTargetFramework="TargetFramework=$(NetCoreAppMinimum)" />
   </ItemGroup>
   <ItemGroup>
     <RdXmlFile Include="default.rd.xml" />
@@ -34,6 +32,7 @@
     <Compile Include="$(CommonTestPath)System\EnumTypes.cs" Link="Common\System\EnumTypes.cs" />
     <Compile Include="$(CommonTestPath)System\ObjectCloner.cs" Link="Common\System\ObjectCloner.cs" />
     <Compile Include="$(CommonTestPath)System\Runtime\Serialization\Formatters\BinaryFormatterHelpers.cs" Link="Common\System\Runtime\Serialization\Formatters\BinaryFormatterHelpers.cs" />
+    <Compile Include="Generic\HashSet\HashSet.NonGeneric.Tests.cs" />
     <Compile Include="Generic\ReadOnlySet\ReadOnlySetTests.cs" />
     <!-- Generic tests -->
     <Compile Include="Generic\SortedSet\SortedSet.TreeSubSet.Tests.cs" />
diff --git a/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/HashSet.cs b/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/HashSet.cs
index 621af08d72a84..f39fe93b13454 100644
--- a/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/HashSet.cs
+++ b/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/HashSet.cs
@@ -69,7 +69,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;
                 }
@@ -92,7 +92,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);
             }
@@ -145,6 +145,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,
@@ -1250,6 +1252,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)
         {
@@ -1768,7 +1775,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
         {