Fixes and feedback

Author: stephentoub

src/coreclr/tools/Common/TypeSystem/IL/Stubs/ComparerIntrinsics.cs

@@ -75,7 +75,7 @@ private static MethodIL EmitComparerAndEqualityComparerCreateCommon(MethodDesc m
         /// Gets the comparer type that is suitable to compare instances of <paramref name="type"/>
         /// or null if such comparer cannot be determined at compile time.
         /// </summary>
-        private static InstantiatedType GetComparerForType(TypeDesc type, string flavor, string interfaceName)
+        private static TypeDesc GetComparerForType(TypeDesc type, string flavor, string interfaceName)
         {
             TypeSystemContext context = type.Context;
 
@@ -92,6 +92,11 @@ private static InstantiatedType GetComparerForType(TypeDesc type, string flavor,
                     .MakeInstantiatedType(type.Instantiation[0]);
             }
 
+            if (type.IsString && flavor == "EqualityComparer")
+            {
+                return context.SystemModule.GetKnownType("System.Collections.Generic", "StringEqualityComparer");
+            }
+
             if (type.IsEnum)
             {
                 // Enums have a specialized comparer that avoids boxing

src/coreclr/vm/jitinterface.cpp

@@ -8922,8 +8922,7 @@ CORINFO_CLASS_HANDLE CEEInfo::getDefaultEqualityComparerClassHelper(CORINFO_CLAS
     // string
     if (elemTypeHnd.IsString())
     {
-        TypeHandle resultTh = ((TypeHandle)CoreLibBinder::GetClass(CLASS__STRING_EQUALITYCOMPARER)).Instantiate(inst);
-        return CORINFO_CLASS_HANDLE(resultTh.GetMethodTable());
+        return CORINFO_CLASS_HANDLE(CoreLibBinder::GetClass(CLASS__STRING_EQUALITYCOMPARER));
     }
 
     // Nullable<T>

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

@@ -265,7 +265,7 @@ public override int GetHashCode() =>
 
     // This class exists to be EqualityComparer<string>.Default. It can't just use the GenericEqualityComparer<string>,
     // as it needs to also implement IAlternateEqualityComparer<ReadOnlySpan<char>, string>, and it can't be
-    // StringComparer.Ordinal, as that doesn't derive from the abstract EqualityComparer<T>.
+    // StringComparer.Ordinal, as that doesn't derive from the required abstract EqualityComparer<T> base class.
     [Serializable]
     internal sealed partial class StringEqualityComparer :
         EqualityComparer<string>,
@@ -274,12 +274,24 @@ internal sealed partial class StringEqualityComparer :
     {
         void ISerializable.GetObjectData(SerializationInfo info, StreamingContext context)
         {
-            // This type was introduced after BinaryFormatter was deprecated. BinaryFormatter serializes
-            // types from System.Private.CoreLib without an assembly name, and then on deserialization assumes
-            // "mscorlib". To support such roundtripping, this type would need to be public and type-forwarded from
-            // the mscorlib shim. Instead, we serialize this instead as a GenericEqualityComparer<string>. The
-            // resulting behavior is the same, except it won't implement IAlternateEqualityComparer, and so functionality
-            // that relies on that interface (which was also introduced after BinaryFormatter was deprecated) won't work.
+            // This type is added as an internal implementation detail in .NET 9. Even though as of .NET 9 BinaryFormatter has been
+            // deprecated, for back compat we still need to support serializing this type, especially when EqualityComparer<string>.Default
+            // is used as part of a collection, like Dictionary<string, TValue>.
+            //
+            // BinaryFormatter treats types in the core library as being special, in that it doesn't include the assembly as part of the
+            // serialized data, and then on deserialization it assumes the type is in mscorlib. We could make the type public and type forward
+            // it from the mscorlib shim, which would enable roundtripping on .NET 9+, but because this type doesn't exist downlevel, it would
+            // break serializing on .NET 9+ and deserializing downlevel. Therefore, we need to serialize as something that exists downlevel.
+
+            // We could serialize as OrdinalComparer, which does exist downlevel, and which has the nice property that it also implements
+            // IAlternateEqualityComparer<ReadOnlySpan<char>, string>, which means serializing an instance on .NET 9+ and deserializing it
+            // on .NET 9+ would continue to support span-based lookups. However, OrdinalComparer is not an EqualityComparer<string>, which
+            // means the type's public ancestry would not be retained, which could lead to strange casting-related errors, including downlevel.
+
+            // Instead, we can serialize as a GenericEqualityComparer<string>. This exists downlevel and also derives from EqualityComparer<string>,
+            // but doesn't implement IAlternateEqualityComparer<ReadOnlySpan<char>, string>. This means that upon deserializing on .NET 9+,
+            // the comparer loses its ability to handle span-based lookups. As BinaryFormatter is deprecated on .NET 9+, this is a readonable tradeoff.
+
             info.SetType(typeof(GenericEqualityComparer<string>));
         }
 

src/libraries/System.Private.CoreLib/src/System/StringComparer.cs

@@ -88,10 +88,12 @@ public static bool IsWellKnownOrdinalComparer(IEqualityComparer<string?>? compar
             {
                 case StringComparer stringComparer:
                     return stringComparer.IsWellKnownOrdinalComparerCore(out ignoreCase);
-                case StringEqualityComparer:
-                    // special-case EqualityComparer<string>.Default, which is Ordinal-equivalent
+
+                case StringEqualityComparer: // EqualityComparer<string>.Default
+                case GenericEqualityComparer<string>: // EqualityComparer<string>.Default serialized
                     ignoreCase = false;
                     return true;
+
                 default:
                     // unknown comparer
                     ignoreCase = default;

src/libraries/System.Resources.Extensions/tests/BinaryFormatTests/Legacy/BinaryFormatterTestData.cs

@@ -78,6 +78,7 @@ public partial class BinaryFormatterTests
         yield return (EqualityComparer<UInt64Enum>.Default, new TypeSerializableValue[] { new("AAEAAAD/////AQAAAAAAAAAEAQAAANUBU3lzdGVtLkNvbGxlY3Rpb25zLkdlbmVyaWMuT2JqZWN0RXF1YWxpdHlDb21wYXJlcmAxW1tCaW5hcnlGb3JtYXRUZXN0cy5Gb3JtYXR0ZXJUZXN0cy5VSW50NjRFbnVtLCBTeXN0ZW0uUmVzb3VyY2VzLkV4dGVuc2lvbnMuQmluYXJ5Rm9ybWF0LlRlc3RzLCBWZXJzaW9uPTkuMC4wLjAsIEN1bHR1cmU9bmV1dHJhbCwgUHVibGljS2V5VG9rZW49Y2M3YjEzZmZjZDJkZGQ1MV1dAAAAAAs=", TargetFrameworkMoniker.netcoreapp20), new("AAEAAAD/////AQAAAAAAAAAEAQAAANUBU3lzdGVtLkNvbGxlY3Rpb25zLkdlbmVyaWMuT2JqZWN0RXF1YWxpdHlDb21wYXJlcmAxW1tCaW5hcnlGb3JtYXRUZXN0cy5Gb3JtYXR0ZXJUZXN0cy5VSW50NjRFbnVtLCBTeXN0ZW0uUmVzb3VyY2VzLkV4dGVuc2lvbnMuQmluYXJ5Rm9ybWF0LlRlc3RzLCBWZXJzaW9uPTkuMC4wLjAsIEN1bHR1cmU9bmV1dHJhbCwgUHVibGljS2V5VG9rZW49Y2M3YjEzZmZjZDJkZGQ1MV1dAAAAAAs=", TargetFrameworkMoniker.netfx461) });
         yield return (EqualityComparer<SByteEnum>.Default, new TypeSerializableValue[] { new("AAEAAAD/////AQAAAAAAAAAEAQAAANQBU3lzdGVtLkNvbGxlY3Rpb25zLkdlbmVyaWMuT2JqZWN0RXF1YWxpdHlDb21wYXJlcmAxW1tCaW5hcnlGb3JtYXRUZXN0cy5Gb3JtYXR0ZXJUZXN0cy5TQnl0ZUVudW0sIFN5c3RlbS5SZXNvdXJjZXMuRXh0ZW5zaW9ucy5CaW5hcnlGb3JtYXQuVGVzdHMsIFZlcnNpb249OS4wLjAuMCwgQ3VsdHVyZT1uZXV0cmFsLCBQdWJsaWNLZXlUb2tlbj1jYzdiMTNmZmNkMmRkZDUxXV0AAAAACw==", TargetFrameworkMoniker.netcoreapp20), new("AAEAAAD/////AQAAAAAAAAAEAQAAANQBU3lzdGVtLkNvbGxlY3Rpb25zLkdlbmVyaWMuT2JqZWN0RXF1YWxpdHlDb21wYXJlcmAxW1tCaW5hcnlGb3JtYXRUZXN0cy5Gb3JtYXR0ZXJUZXN0cy5TQnl0ZUVudW0sIFN5c3RlbS5SZXNvdXJjZXMuRXh0ZW5zaW9ucy5CaW5hcnlGb3JtYXQuVGVzdHMsIFZlcnNpb249OS4wLjAuMCwgQ3VsdHVyZT1uZXV0cmFsLCBQdWJsaWNLZXlUb2tlbj1jYzdiMTNmZmNkMmRkZDUxXV0AAAAACw==", TargetFrameworkMoniker.netfx461) });
         yield return (EqualityComparer<Int16Enum>.Default, new TypeSerializableValue[] { new("AAEAAAD/////AQAAAAAAAAAEAQAAANQBU3lzdGVtLkNvbGxlY3Rpb25zLkdlbmVyaWMuT2JqZWN0RXF1YWxpdHlDb21wYXJlcmAxW1tCaW5hcnlGb3JtYXRUZXN0cy5Gb3JtYXR0ZXJUZXN0cy5JbnQxNkVudW0sIFN5c3RlbS5SZXNvdXJjZXMuRXh0ZW5zaW9ucy5CaW5hcnlGb3JtYXQuVGVzdHMsIFZlcnNpb249OS4wLjAuMCwgQ3VsdHVyZT1uZXV0cmFsLCBQdWJsaWNLZXlUb2tlbj1jYzdiMTNmZmNkMmRkZDUxXV0AAAAACw==", TargetFrameworkMoniker.netcoreapp20), new("AAEAAAD/////AQAAAAAAAAAEAQAAANQBU3lzdGVtLkNvbGxlY3Rpb25zLkdlbmVyaWMuT2JqZWN0RXF1YWxpdHlDb21wYXJlcmAxW1tCaW5hcnlGb3JtYXRUZXN0cy5Gb3JtYXR0ZXJUZXN0cy5JbnQxNkVudW0sIFN5c3RlbS5SZXNvdXJjZXMuRXh0ZW5zaW9ucy5CaW5hcnlGb3JtYXQuVGVzdHMsIFZlcnNpb249OS4wLjAuMCwgQ3VsdHVyZT1uZXV0cmFsLCBQdWJsaWNLZXlUb2tlbj1jYzdiMTNmZmNkMmRkZDUxXV0AAAAACw==", TargetFrameworkMoniker.netfx461) });
+        yield return (EqualityComparer<string>.Default, new TypeSerializableValue[] { new("AAEAAAD/////AQAAAAAAAAAEAQAAAJIBU3lzdGVtLkNvbGxlY3Rpb25zLkdlbmVyaWMuR2VuZXJpY0VxdWFsaXR5Q29tcGFyZXJgMVtbU3lzdGVtLlN0cmluZywgbXNjb3JsaWIsIFZlcnNpb249NC4wLjAuMCwgQ3VsdHVyZT1uZXV0cmFsLCBQdWJsaWNLZXlUb2tlbj1iNzdhNWM1NjE5MzRlMDg5XV0AAAAACw==", TargetFrameworkMoniker.netcoreapp20), new("AAEAAAD/////AQAAAAAAAAAEAQAAAJIBU3lzdGVtLkNvbGxlY3Rpb25zLkdlbmVyaWMuR2VuZXJpY0VxdWFsaXR5Q29tcGFyZXJgMVtbU3lzdGVtLlN0cmluZywgbXNjb3JsaWIsIFZlcnNpb249NC4wLjAuMCwgQ3VsdHVyZT1uZXV0cmFsLCBQdWJsaWNLZXlUb2tlbj1iNzdhNWM1NjE5MzRlMDg5XV0AAAAACw==", TargetFrameworkMoniker.netfx461) });
     }
 
     /// <summary>
@@ -667,7 +668,6 @@ public partial class BinaryFormatterTests
         // Nullable equality comparer roundtrips as opposed to other equality comparers which serialize to ObjectEqualityComparer
         yield return (EqualityComparer<int>.Default, new TypeSerializableValue[] { new("AAEAAAD/////AQAAAAAAAAAEAQAAAJEBU3lzdGVtLkNvbGxlY3Rpb25zLkdlbmVyaWMuR2VuZXJpY0VxdWFsaXR5Q29tcGFyZXJgMVtbU3lzdGVtLkludDMyLCBtc2NvcmxpYiwgVmVyc2lvbj00LjAuMC4wLCBDdWx0dXJlPW5ldXRyYWwsIFB1YmxpY0tleVRva2VuPWI3N2E1YzU2MTkzNGUwODldXQAAAAAL", TargetFrameworkMoniker.netcoreapp20), new("AAEAAAD/////AQAAAAAAAAAEAQAAAJEBU3lzdGVtLkNvbGxlY3Rpb25zLkdlbmVyaWMuR2VuZXJpY0VxdWFsaXR5Q29tcGFyZXJgMVtbU3lzdGVtLkludDMyLCBtc2NvcmxpYiwgVmVyc2lvbj00LjAuMC4wLCBDdWx0dXJlPW5ldXRyYWwsIFB1YmxpY0tleVRva2VuPWI3N2E1YzU2MTkzNGUwODldXQAAAAAL", TargetFrameworkMoniker.netfx461) });
         yield return (EqualityComparer<long>.Default, new TypeSerializableValue[] { new("AAEAAAD/////AQAAAAAAAAAEAQAAAJEBU3lzdGVtLkNvbGxlY3Rpb25zLkdlbmVyaWMuR2VuZXJpY0VxdWFsaXR5Q29tcGFyZXJgMVtbU3lzdGVtLkludDY0LCBtc2NvcmxpYiwgVmVyc2lvbj00LjAuMC4wLCBDdWx0dXJlPW5ldXRyYWwsIFB1YmxpY0tleVRva2VuPWI3N2E1YzU2MTkzNGUwODldXQAAAAAL", TargetFrameworkMoniker.netcoreapp20), new("AAEAAAD/////AQAAAAAAAAAEAQAAAJEBU3lzdGVtLkNvbGxlY3Rpb25zLkdlbmVyaWMuR2VuZXJpY0VxdWFsaXR5Q29tcGFyZXJgMVtbU3lzdGVtLkludDY0LCBtc2NvcmxpYiwgVmVyc2lvbj00LjAuMC4wLCBDdWx0dXJlPW5ldXRyYWwsIFB1YmxpY0tleVRva2VuPWI3N2E1YzU2MTkzNGUwODldXQAAAAAL", TargetFrameworkMoniker.netfx461) });
-        yield return (EqualityComparer<string>.Default, new TypeSerializableValue[] { new("AAEAAAD/////AQAAAAAAAAAEAQAAAJIBU3lzdGVtLkNvbGxlY3Rpb25zLkdlbmVyaWMuR2VuZXJpY0VxdWFsaXR5Q29tcGFyZXJgMVtbU3lzdGVtLlN0cmluZywgbXNjb3JsaWIsIFZlcnNpb249NC4wLjAuMCwgQ3VsdHVyZT1uZXV0cmFsLCBQdWJsaWNLZXlUb2tlbj1iNzdhNWM1NjE5MzRlMDg5XV0AAAAACw==", TargetFrameworkMoniker.netcoreapp20), new("AAEAAAD/////AQAAAAAAAAAEAQAAAJIBU3lzdGVtLkNvbGxlY3Rpb25zLkdlbmVyaWMuR2VuZXJpY0VxdWFsaXR5Q29tcGFyZXJgMVtbU3lzdGVtLlN0cmluZywgbXNjb3JsaWIsIFZlcnNpb249NC4wLjAuMCwgQ3VsdHVyZT1uZXV0cmFsLCBQdWJsaWNLZXlUb2tlbj1iNzdhNWM1NjE5MzRlMDg5XV0AAAAACw==", TargetFrameworkMoniker.netfx461) });
         yield return (EqualityComparer<object>.Default, new TypeSerializableValue[] { new("AAEAAAD/////AQAAAAAAAAAEAQAAAJEBU3lzdGVtLkNvbGxlY3Rpb25zLkdlbmVyaWMuT2JqZWN0RXF1YWxpdHlDb21wYXJlcmAxW1tTeXN0ZW0uT2JqZWN0LCBtc2NvcmxpYiwgVmVyc2lvbj00LjAuMC4wLCBDdWx0dXJlPW5ldXRyYWwsIFB1YmxpY0tleVRva2VuPWI3N2E1YzU2MTkzNGUwODldXQAAAAAL", TargetFrameworkMoniker.netcoreapp20), new("AAEAAAD/////AQAAAAAAAAAEAQAAAJEBU3lzdGVtLkNvbGxlY3Rpb25zLkdlbmVyaWMuT2JqZWN0RXF1YWxpdHlDb21wYXJlcmAxW1tTeXN0ZW0uT2JqZWN0LCBtc2NvcmxpYiwgVmVyc2lvbj00LjAuMC4wLCBDdWx0dXJlPW5ldXRyYWwsIFB1YmxpY0tleVRva2VuPWI3N2E1YzU2MTkzNGUwODldXQAAAAAL", TargetFrameworkMoniker.netfx461) });
         yield return (EqualityComparer<int?>.Default, new TypeSerializableValue[] { new("AAEAAAD/////AQAAAAAAAAAEAQAAAJIBU3lzdGVtLkNvbGxlY3Rpb25zLkdlbmVyaWMuTnVsbGFibGVFcXVhbGl0eUNvbXBhcmVyYDFbW1N5c3RlbS5JbnQzMiwgbXNjb3JsaWIsIFZlcnNpb249NC4wLjAuMCwgQ3VsdHVyZT1uZXV0cmFsLCBQdWJsaWNLZXlUb2tlbj1iNzdhNWM1NjE5MzRlMDg5XV0AAAAACw==", TargetFrameworkMoniker.netcoreapp20), new("AAEAAAD/////AQAAAAAAAAAEAQAAAJIBU3lzdGVtLkNvbGxlY3Rpb25zLkdlbmVyaWMuTnVsbGFibGVFcXVhbGl0eUNvbXBhcmVyYDFbW1N5c3RlbS5JbnQzMiwgbXNjb3JsaWIsIFZlcnNpb249NC4wLjAuMCwgQ3VsdHVyZT1uZXV0cmFsLCBQdWJsaWNLZXlUb2tlbj1iNzdhNWM1NjE5MzRlMDg5XV0AAAAACw==", TargetFrameworkMoniker.netfx461) });
         yield return (EqualityComparer<double?>.Default, new TypeSerializableValue[] { new("AAEAAAD/////AQAAAAAAAAAEAQAAAJMBU3lzdGVtLkNvbGxlY3Rpb25zLkdlbmVyaWMuTnVsbGFibGVFcXVhbGl0eUNvbXBhcmVyYDFbW1N5c3RlbS5Eb3VibGUsIG1zY29ybGliLCBWZXJzaW9uPTQuMC4wLjAsIEN1bHR1cmU9bmV1dHJhbCwgUHVibGljS2V5VG9rZW49Yjc3YTVjNTYxOTM0ZTA4OV1dAAAAAAs=", TargetFrameworkMoniker.netcoreapp20), new("AAEAAAD/////AQAAAAAAAAAEAQAAAJMBU3lzdGVtLkNvbGxlY3Rpb25zLkdlbmVyaWMuTnVsbGFibGVFcXVhbGl0eUNvbXBhcmVyYDFbW1N5c3RlbS5Eb3VibGUsIG1zY29ybGliLCBWZXJzaW9uPTQuMC4wLjAsIEN1bHR1cmU9bmV1dHJhbCwgUHVibGljS2V5VG9rZW49Yjc3YTVjNTYxOTM0ZTA4OV1dAAAAAAs=", TargetFrameworkMoniker.netfx461) });

src/libraries/System.Resources.Extensions/tests/BinaryFormatTests/Legacy/BinaryFormatterTests.cs

@@ -182,7 +182,7 @@ private static void ValidateEqualityComparer(object obj)
     {
         Type objType = obj.GetType();
         Assert.True(objType.IsGenericType, $"Type `{objType.FullName}` must be generic.");
-        Assert.Equal("System.Collections.Generic.ObjectEqualityComparer`1", objType.GetGenericTypeDefinition().FullName);
+        Assert.True(objType.GetGenericTypeDefinition().FullName is "System.Collections.Generic.ObjectEqualityComparer`1" or "System.Collections.Generic.GenericEqualityComparer`1");
         Assert.Equal(obj.GetType().GetGenericArguments()[0], objType.GetGenericArguments()[0]);
     }