From b6f32f3f1cc85f554c443c677c17323559b8c79a Mon Sep 17 00:00:00 2001 From: EgorBo Date: Mon, 18 Apr 2022 19:02:59 +0300 Subject: [PATCH 01/10] Apply Jan's suggestion --- .../System/Collections/Generic/ComparerHelpers.cs | 15 ++------------- .../Generic/EqualityComparer.CoreCLR.cs | 6 +++--- .../Internal/IntrinsicSupport/ComparerHelpers.cs | 7 ++----- .../IntrinsicSupport/EqualityComparerHelpers.cs | 7 ++----- src/coreclr/vm/jitinterface.cpp | 8 ++------ .../src/System/Collections/Generic/Comparer.cs | 15 +++++++++++++-- .../Collections/Generic/EqualityComparer.cs | 15 +++++++++++++-- .../System/Collections/Generic/Comparer.Mono.cs | 5 ++--- .../Collections/Generic/EqualityComparer.Mono.cs | 5 +---- src/mono/mono/mini/method-to-ir.c | 3 ++- .../opt/Devirtualization/Comparer_get_Default.cs | 5 +++++ 11 files changed, 47 insertions(+), 44 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Collections/Generic/ComparerHelpers.cs b/src/coreclr/System.Private.CoreLib/src/System/Collections/Generic/ComparerHelpers.cs index 6812a6c92faf26..b85fdbdded61dc 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Collections/Generic/ComparerHelpers.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Collections/Generic/ComparerHelpers.cs @@ -66,12 +66,7 @@ internal static object CreateDefaultComparer(Type type) var embeddedType = (RuntimeType)nullableType.GetGenericArguments()[0]; - if (typeof(IComparable<>).MakeGenericType(embeddedType).IsAssignableFrom(embeddedType)) - { - return RuntimeTypeHandle.CreateInstanceForAnotherGenericParameter((RuntimeType)typeof(NullableComparer), embeddedType); - } - - return null; + return RuntimeTypeHandle.CreateInstanceForAnotherGenericParameter((RuntimeType)typeof(NullableComparer), embeddedType); } /// @@ -163,13 +158,7 @@ internal static object CreateDefaultEqualityComparer(Type type) Debug.Assert(nullableType.IsGenericType && nullableType.GetGenericTypeDefinition() == typeof(Nullable<>)); var embeddedType = (RuntimeType)nullableType.GetGenericArguments()[0]; - - if (typeof(IEquatable<>).MakeGenericType(embeddedType).IsAssignableFrom(embeddedType)) - { - return RuntimeTypeHandle.CreateInstanceForAnotherGenericParameter((RuntimeType)typeof(NullableEqualityComparer), embeddedType); - } - - return null; + return RuntimeTypeHandle.CreateInstanceForAnotherGenericParameter((RuntimeType)typeof(NullableEqualityComparer), embeddedType); } /// diff --git a/src/coreclr/System.Private.CoreLib/src/System/Collections/Generic/EqualityComparer.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Collections/Generic/EqualityComparer.CoreCLR.cs index ef5ec59482110a..9d35ae81313b6e 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Collections/Generic/EqualityComparer.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Collections/Generic/EqualityComparer.CoreCLR.cs @@ -57,7 +57,7 @@ internal override int LastIndexOf(T[] array, T value, int startIndex, int count) } } - public sealed partial class NullableEqualityComparer : EqualityComparer where T : struct, IEquatable + public sealed partial class NullableEqualityComparer : EqualityComparer where T : struct { internal override int IndexOf(T?[] array, T? value, int startIndex, int count) { @@ -73,7 +73,7 @@ internal override int IndexOf(T?[] array, T? value, int startIndex, int count) { for (int i = startIndex; i < endIndex; i++) { - if (array[i].HasValue && array[i].value.Equals(value.value)) return i; + if (array[i].HasValue && EqualityComparer.Default.Equals(array[i].value, value.value)) return i; } } return -1; @@ -93,7 +93,7 @@ internal override int LastIndexOf(T?[] array, T? value, int startIndex, int coun { for (int i = startIndex; i >= endIndex; i--) { - if (array[i].HasValue && array[i].value.Equals(value.value)) return i; + if (array[i].HasValue && EqualityComparer.Default.Equals(array[i].value, value.value)) return i; } } return -1; diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/Internal/IntrinsicSupport/ComparerHelpers.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/Internal/IntrinsicSupport/ComparerHelpers.cs index 2a74b9560d4f48..2800fc3735a495 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/Internal/IntrinsicSupport/ComparerHelpers.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/Internal/IntrinsicSupport/ComparerHelpers.cs @@ -59,11 +59,8 @@ internal static object GetComparer(RuntimeTypeHandle t) if (RuntimeAugments.IsNullable(t)) { RuntimeTypeHandle nullableType = RuntimeAugments.GetNullableType(t); - if (ImplementsIComparable(nullableType)) - { - openComparerType = typeof(NullableComparer<>).TypeHandle; - comparerTypeArgument = nullableType; - } + openComparerType = typeof(NullableComparer<>).TypeHandle; + comparerTypeArgument = nullableType; } if (EqualityComparerHelpers.IsEnum(t)) { diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/Internal/IntrinsicSupport/EqualityComparerHelpers.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/Internal/IntrinsicSupport/EqualityComparerHelpers.cs index 1339c917fe9f94..ab77843009c787 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/Internal/IntrinsicSupport/EqualityComparerHelpers.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/Internal/IntrinsicSupport/EqualityComparerHelpers.cs @@ -62,11 +62,8 @@ internal static object GetComparer(RuntimeTypeHandle t) if (RuntimeAugments.IsNullable(t)) { RuntimeTypeHandle nullableType = RuntimeAugments.GetNullableType(t); - if (ImplementsIEquatable(nullableType)) - { - openComparerType = typeof(NullableEqualityComparer<>).TypeHandle; - comparerTypeArgument = nullableType; - } + openComparerType = typeof(NullableEqualityComparer<>).TypeHandle; + comparerTypeArgument = nullableType; } if (IsEnum(t)) { diff --git a/src/coreclr/vm/jitinterface.cpp b/src/coreclr/vm/jitinterface.cpp index d5a3c11e3575c2..33dd175e6b9ecf 100644 --- a/src/coreclr/vm/jitinterface.cpp +++ b/src/coreclr/vm/jitinterface.cpp @@ -8932,12 +8932,8 @@ CORINFO_CLASS_HANDLE CEEInfo::getDefaultEqualityComparerClassHelper(CORINFO_CLAS if (Nullable::IsNullableType(elemTypeHnd)) { Instantiation nullableInst = elemTypeHnd.AsMethodTable()->GetInstantiation(); - TypeHandle iequatable = TypeHandle(CoreLibBinder::GetClass(CLASS__IEQUATABLEGENERIC)).Instantiate(nullableInst); - if (nullableInst[0].CanCastTo(iequatable)) - { - TypeHandle resultTh = ((TypeHandle)CoreLibBinder::GetClass(CLASS__NULLABLE_EQUALITYCOMPARER)).Instantiate(nullableInst); - return CORINFO_CLASS_HANDLE(resultTh.GetMethodTable()); - } + TypeHandle resultTh = ((TypeHandle)CoreLibBinder::GetClass(CLASS__NULLABLE_EQUALITYCOMPARER)).Instantiate(nullableInst); + return CORINFO_CLASS_HANDLE(resultTh.GetMethodTable()); } // Enum diff --git a/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/Comparer.cs b/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/Comparer.cs index 34f1b26c3d2104..cf4eba635bc28c 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/Comparer.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/Comparer.cs @@ -75,13 +75,24 @@ public override int GetHashCode() => [Serializable] [TypeForwardedFrom("mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089")] // Needs to be public to support binary serialization compatibility - public sealed partial class NullableComparer : Comparer where T : struct, IComparable + public sealed class NullableComparer : Comparer where T : struct { + public NullableComparer() { } + private NullableComparer(SerializationInfo info, StreamingContext context) { } + public void GetObjectData(SerializationInfo info, StreamingContext context) + { + if (!typeof(T).IsAssignableTo(typeof(IEquatable))) + { + // We used to use NullableComparer only for types implementing IEquatable + info.SetType(typeof(ObjectComparer)); + } + } + public override int Compare(T? x, T? y) { if (x.HasValue) { - if (y.HasValue) return x.value.CompareTo(y.value); + if (y.HasValue) return Comparer.Default.Compare(x.value, y.value); return 1; } if (y.HasValue) return -1; diff --git a/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/EqualityComparer.cs b/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/EqualityComparer.cs index e7f23a80ab5fae..bcc1b1fa899791 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/EqualityComparer.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/EqualityComparer.cs @@ -97,14 +97,25 @@ public override int GetHashCode() => [Serializable] [TypeForwardedFrom("mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089")] // Needs to be public to support binary serialization compatibility - public sealed partial class NullableEqualityComparer : EqualityComparer where T : struct, IEquatable + public sealed partial class NullableEqualityComparer : EqualityComparer where T : struct { + public NullableEqualityComparer() { } + private NullableEqualityComparer(SerializationInfo info, StreamingContext context) { } + public void GetObjectData(SerializationInfo info, StreamingContext context) + { + if (!typeof(T).IsAssignableTo(typeof(IEquatable))) + { + // We used to use NullableComparer only for types implementing IEquatable + info.SetType(typeof(ObjectEqualityComparer)); + } + } + [MethodImpl(MethodImplOptions.AggressiveInlining)] public override bool Equals(T? x, T? y) { if (x.HasValue) { - if (y.HasValue) return x.value.Equals(y.value); + if (y.HasValue) return EqualityComparer.Default.Equals(x.value, y.value); return false; } if (y.HasValue) return false; diff --git a/src/mono/System.Private.CoreLib/src/System/Collections/Generic/Comparer.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Collections/Generic/Comparer.Mono.cs index 94066bccfc5436..06044427a30e61 100644 --- a/src/mono/System.Private.CoreLib/src/System/Collections/Generic/Comparer.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Collections/Generic/Comparer.Mono.cs @@ -32,12 +32,11 @@ private static Comparer CreateComparer() if (typeof(IComparable).IsAssignableFrom(t)) return (Comparer)RuntimeType.CreateInstanceForAnotherGenericParameter(typeof(GenericComparer<>), t); - // If T is a Nullable where U implements IComparable return a NullableComparer + // If T is a Nullable return a NullableComparer if (t.IsGenericType && t.GetGenericTypeDefinition() == typeof(Nullable<>)) { RuntimeType u = (RuntimeType)t.GetGenericArguments()[0]; - if (typeof(IComparable<>).MakeGenericType(u).IsAssignableFrom(u)) - return (Comparer)RuntimeType.CreateInstanceForAnotherGenericParameter(typeof(NullableComparer<>), u); + return (Comparer)RuntimeType.CreateInstanceForAnotherGenericParameter(typeof(NullableComparer<>), u); } if (t.IsEnum) diff --git a/src/mono/System.Private.CoreLib/src/System/Collections/Generic/EqualityComparer.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Collections/Generic/EqualityComparer.Mono.cs index 73a5edbf74539b..796c946eebb8d8 100644 --- a/src/mono/System.Private.CoreLib/src/System/Collections/Generic/EqualityComparer.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Collections/Generic/EqualityComparer.Mono.cs @@ -48,10 +48,7 @@ private static EqualityComparer CreateComparer() if (t.IsGenericType && t.GetGenericTypeDefinition() == typeof(Nullable<>)) { RuntimeType u = (RuntimeType)t.GetGenericArguments()[0]; - if (typeof(IEquatable<>).MakeGenericType(u).IsAssignableFrom(u)) - { - return (EqualityComparer)RuntimeType.CreateInstanceForAnotherGenericParameter(typeof(NullableEqualityComparer<>), u); - } + return (EqualityComparer)RuntimeType.CreateInstanceForAnotherGenericParameter(typeof(NullableEqualityComparer<>), u); } if (t.IsEnum) diff --git a/src/mono/mono/mini/method-to-ir.c b/src/mono/mono/mini/method-to-ir.c index 688004b83f60ff..3fbc563de5ff04 100644 --- a/src/mono/mono/mini/method-to-ir.c +++ b/src/mono/mono/mini/method-to-ir.c @@ -5422,7 +5422,8 @@ handle_call_res_devirt (MonoCompile *cfg, MonoMethod *cmethod, MonoInst *call_re /* EqualityComparer.Default returns specific types depending on T */ // FIXME: Add more - /* 1. Implements IEquatable */ + /* 1. Implements IEquatable + /* 2. Nullable /* * Can't use this for string/byte as it might use a different comparer: * diff --git a/src/tests/JIT/opt/Devirtualization/Comparer_get_Default.cs b/src/tests/JIT/opt/Devirtualization/Comparer_get_Default.cs index 888c303a59b822..e754e79a072e20 100644 --- a/src/tests/JIT/opt/Devirtualization/Comparer_get_Default.cs +++ b/src/tests/JIT/opt/Devirtualization/Comparer_get_Default.cs @@ -221,6 +221,11 @@ private static void GetTypeTests() AssertEquals("System.Collections.Generic.EnumComparer`1[System.Runtime.CompilerServices.MethodImplOptions]", Comparer.Default.GetType().ToString()); AssertEquals("System.Collections.Generic.NullableComparer`1[System.Byte]", Comparer.Default.GetType().ToString()); AssertEquals("System.Collections.Generic.ObjectComparer`1[Struct1]", Comparer.Default.GetType().ToString()); + + AssertEquals("System.Collections.Generic.NullableComparer`1[System.Runtime.CompilerServices.MethodImplOptions]", Comparer.Default.GetType().ToString()); + AssertEquals("System.Collections.Generic.NullableEqualityComparer`1[System.Runtime.CompilerServices.MethodImplOptions]", EqualityComparer.Default.GetType().ToString()); + AssertEquals("System.Collections.Generic.NullableComparer`1[Struct1]", Comparer.Default.GetType().ToString()); + AssertEquals("System.Collections.Generic.NullableEqualityComparer`1[Struct1]", EqualityComparer.Default.GetType().ToString()); } private static int GetHashCodeTests() { From 0d7ed4589912abed317689ffdeca4930be40abc8 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Mon, 18 Apr 2022 19:06:56 +0300 Subject: [PATCH 02/10] fix comments --- src/mono/mono/mini/method-to-ir.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/mono/mono/mini/method-to-ir.c b/src/mono/mono/mini/method-to-ir.c index 3fbc563de5ff04..e04b71f5016b47 100644 --- a/src/mono/mono/mini/method-to-ir.c +++ b/src/mono/mono/mini/method-to-ir.c @@ -5422,8 +5422,8 @@ handle_call_res_devirt (MonoCompile *cfg, MonoMethod *cmethod, MonoInst *call_re /* EqualityComparer.Default returns specific types depending on T */ // FIXME: Add more - /* 1. Implements IEquatable - /* 2. Nullable + // 1. Implements IEquatable + // 2. Nullable /* * Can't use this for string/byte as it might use a different comparer: * From ecfd50c801de39671010d1fc6ae1361e25532238 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Mon, 18 Apr 2022 19:14:57 +0300 Subject: [PATCH 03/10] fix copy-paste error --- .../src/System/Collections/Generic/Comparer.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/Comparer.cs b/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/Comparer.cs index cf4eba635bc28c..f01495b0fe7259 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/Comparer.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/Comparer.cs @@ -81,9 +81,9 @@ public NullableComparer() { } private NullableComparer(SerializationInfo info, StreamingContext context) { } public void GetObjectData(SerializationInfo info, StreamingContext context) { - if (!typeof(T).IsAssignableTo(typeof(IEquatable))) + if (!typeof(T).IsAssignableTo(typeof(IComparable))) { - // We used to use NullableComparer only for types implementing IEquatable + // We used to use NullableComparer only for types implementing IComparable info.SetType(typeof(ObjectComparer)); } } From 5e90c70f42770f8eefa04f886d1c25c7ca89d4ec Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 19 Apr 2022 02:11:35 +0300 Subject: [PATCH 04/10] Add tests --- .../Comparers/EqualityComparer.Tests.cs | 36 ++++++++++++++++ .../tests/BinaryFormatterTests.cs | 41 +++++++++++++++++++ 2 files changed, 77 insertions(+) diff --git a/src/libraries/System.Collections/tests/Generic/Comparers/EqualityComparer.Tests.cs b/src/libraries/System.Collections/tests/Generic/Comparers/EqualityComparer.Tests.cs index 4d4ce87f02b28d..9f274cdd15ea3c 100644 --- a/src/libraries/System.Collections/tests/Generic/Comparers/EqualityComparer.Tests.cs +++ b/src/libraries/System.Collections/tests/Generic/Comparers/EqualityComparer.Tests.cs @@ -33,8 +33,10 @@ public class HashData : TheoryData { } [MemberData(nameof(Int16EnumData))] [MemberData(nameof(SByteEnumData))] [MemberData(nameof(Int32EnumData))] + [MemberData(nameof(NullableInt32EnumData))] [MemberData(nameof(Int64EnumData))] [MemberData(nameof(NonEquatableValueTypeData))] + [MemberData(nameof(NullableNonEquatableValueTypeData))] [MemberData(nameof(ObjectData))] public void EqualsTest(T left, T right, bool expected) { @@ -253,6 +255,22 @@ public static EqualsData Int32EnumData() }; } + public static EqualsData NullableInt32EnumData() + { + return new EqualsData + { + { (Int32Enum)(-2), (Int32Enum)(-4), false }, + { Int32Enum.Two, Int32Enum.Two, true }, + { Int32Enum.Min, Int32Enum.Max, false }, + { Int32Enum.Min, Int32Enum.Min, true }, + { Int32Enum.One, Int32Enum.Min + 1, false } + { (Int32Enum)(-2), null, false }, + { Int32Enum.Two, null, false }, + { null, Int32Enum.Max, false }, + { null, Int32Enum.Min + 1, false } + }; + } + public static EqualsData Int64EnumData() { return new EqualsData @@ -281,6 +299,24 @@ public static EqualsData NonEquatableValueTypeData() }; } + public static EqualsData NullableNonEquatableValueTypeData() + { + // Comparisons for structs that do not override ValueType.Equals or + // ValueType.GetHashCode should still work as expected. + + var one = new NonEquatableValueType { Value = 1 }; + + return new EqualsData + { + { new NonEquatableValueType(), new NonEquatableValueType(), true }, + { one, one, true }, + { new NonEquatableValueType(-1), new NonEquatableValueType(), false }, + { new NonEquatableValueType(2), new NonEquatableValueType(2), true } + { new NonEquatableValueType(-1), null, false }, + { null, new NonEquatableValueType(2), false } + }; + } + public static EqualsData ObjectData() { var obj = new object(); diff --git a/src/libraries/System.Runtime.Serialization.Formatters/tests/BinaryFormatterTests.cs b/src/libraries/System.Runtime.Serialization.Formatters/tests/BinaryFormatterTests.cs index 5c698d2ec4f46d..6bd4360974f5af 100644 --- a/src/libraries/System.Runtime.Serialization.Formatters/tests/BinaryFormatterTests.cs +++ b/src/libraries/System.Runtime.Serialization.Formatters/tests/BinaryFormatterTests.cs @@ -715,5 +715,46 @@ private class DelegateBinder : SerializationBinder public Func BindToTypeDelegate = null; public override Type BindToType(string assemblyName, string typeName) => BindToTypeDelegate?.Invoke(assemblyName, typeName); } + + public static IEnumerable NullableComparersTestData() + { + yield return new object[] { "AAEAAAD/////AQAAAAAAAAAEAQAAAJEBU3lzdGVtLkNvbGxlY3Rpb25zLkdlbmVyaWMuTnVsbGFibGVFcXVhbGl0eUNvbXBhcmVyYDFbW1N5c3RlbS5CeXRlLCBtc2NvcmxpYiwgVmVyc2lvbj00LjAuMC4wLCBDdWx0dXJlPW5ldXRyYWwsIFB1YmxpY0tleVRva2VuPWI3N2E1YzU2MTkzNGUwODldXQAAAAAL", + EqualityComparer.Default }; + yield return new object[] { "AAEAAAD/////AQAAAAAAAAAEAQAAAJIBU3lzdGVtLkNvbGxlY3Rpb25zLkdlbmVyaWMuTnVsbGFibGVFcXVhbGl0eUNvbXBhcmVyYDFbW1N5c3RlbS5JbnQzMiwgbXNjb3JsaWIsIFZlcnNpb249NC4wLjAuMCwgQ3VsdHVyZT1uZXV0cmFsLCBQdWJsaWNLZXlUb2tlbj1iNzdhNWM1NjE5MzRlMDg5XV0AAAAACw==", + EqualityComparer.Default }; + yield return new object[] { "AAEAAAD/////AQAAAAAAAAAEAQAAAJMBU3lzdGVtLkNvbGxlY3Rpb25zLkdlbmVyaWMuTnVsbGFibGVFcXVhbGl0eUNvbXBhcmVyYDFbW1N5c3RlbS5TaW5nbGUsIG1zY29ybGliLCBWZXJzaW9uPTQuMC4wLjAsIEN1bHR1cmU9bmV1dHJhbCwgUHVibGljS2V5VG9rZW49Yjc3YTVjNTYxOTM0ZTA4OV1dAAAAAAs=", + EqualityComparer.Default }; + yield return new object[] { "AAEAAAD/////AQAAAAAAAAAEAQAAAJMBU3lzdGVtLkNvbGxlY3Rpb25zLkdlbmVyaWMuTnVsbGFibGVFcXVhbGl0eUNvbXBhcmVyYDFbW1N5c3RlbS5JbnRQdHIsIG1zY29ybGliLCBWZXJzaW9uPTQuMC4wLjAsIEN1bHR1cmU9bmV1dHJhbCwgUHVibGljS2V5VG9rZW49Yjc3YTVjNTYxOTM0ZTA4OV1dAAAAAAs=", + EqualityComparer.Default }; + yield return new object[] { "AAEAAAD/////AQAAAAAAAAAEAQAAAJEBU3lzdGVtLkNvbGxlY3Rpb25zLkdlbmVyaWMuTnVsbGFibGVFcXVhbGl0eUNvbXBhcmVyYDFbW1N5c3RlbS5HdWlkLCBtc2NvcmxpYiwgVmVyc2lvbj00LjAuMC4wLCBDdWx0dXJlPW5ldXRyYWwsIFB1YmxpY0tleVRva2VuPWI3N2E1YzU2MTkzNGUwODldXQAAAAAL", + EqualityComparer.Default }; + yield return new object[] { "AAEAAAD/////AQAAAAAAAAAEAQAAAOsBU3lzdGVtLkNvbGxlY3Rpb25zLkdlbmVyaWMuT2JqZWN0RXF1YWxpdHlDb21wYXJlcmAxW1tTeXN0ZW0uTnVsbGFibGVgMVtbUHJvZytNeVN0cnVjdCwgQ29uc29sZUFwcDIyLCBWZXJzaW9uPTEuMC4wLjAsIEN1bHR1cmU9bmV1dHJhbCwgUHVibGljS2V5VG9rZW49bnVsbF1dLCBtc2NvcmxpYiwgVmVyc2lvbj00LjAuMC4wLCBDdWx0dXJlPW5ldXRyYWwsIFB1YmxpY0tleVRva2VuPWI3N2E1YzU2MTkzNGUwODldXQAAAAAL", + EqualityComparer.Default }; + yield return new object[] { "AAEAAAD/////AQAAAAAAAAAEAQAAAIQCU3lzdGVtLkNvbGxlY3Rpb25zLkdlbmVyaWMuT2JqZWN0RXF1YWxpdHlDb21wYXJlcmAxW1tTeXN0ZW0uTnVsbGFibGVgMVtbU3lzdGVtLkRheU9mV2VlaywgU3lzdGVtLlByaXZhdGUuQ29yZUxpYiwgVmVyc2lvbj03LjAuMC4wLCBDdWx0dXJlPW5ldXRyYWwsIFB1YmxpY0tleVRva2VuPTdjZWM4NWQ3YmVhNzc5OGVdXSwgbXNjb3JsaWIsIFZlcnNpb249NC4wLjAuMCwgQ3VsdHVyZT1uZXV0cmFsLCBQdWJsaWNLZXlUb2tlbj1iNzdhNWM1NjE5MzRlMDg5XV0AAAAACw==", + EqualityComparer.Default }; + + yield return new object[] { "AAEAAAD/////AQAAAAAAAAAEAQAAAIkBU3lzdGVtLkNvbGxlY3Rpb25zLkdlbmVyaWMuTnVsbGFibGVDb21wYXJlcmAxW1tTeXN0ZW0uQnl0ZSwgbXNjb3JsaWIsIFZlcnNpb249NC4wLjAuMCwgQ3VsdHVyZT1uZXV0cmFsLCBQdWJsaWNLZXlUb2tlbj1iNzdhNWM1NjE5MzRlMDg5XV0AAAAACw==", + Comparer.Default }; + yield return new object[] { "AAEAAAD/////AQAAAAAAAAAEAQAAAIoBU3lzdGVtLkNvbGxlY3Rpb25zLkdlbmVyaWMuTnVsbGFibGVDb21wYXJlcmAxW1tTeXN0ZW0uSW50MzIsIG1zY29ybGliLCBWZXJzaW9uPTQuMC4wLjAsIEN1bHR1cmU9bmV1dHJhbCwgUHVibGljS2V5VG9rZW49Yjc3YTVjNTYxOTM0ZTA4OV1dAAAAAAs=", + Comparer.Default }; + yield return new object[] { "AAEAAAD/////AQAAAAAAAAAEAQAAAIsBU3lzdGVtLkNvbGxlY3Rpb25zLkdlbmVyaWMuTnVsbGFibGVDb21wYXJlcmAxW1tTeXN0ZW0uU2luZ2xlLCBtc2NvcmxpYiwgVmVyc2lvbj00LjAuMC4wLCBDdWx0dXJlPW5ldXRyYWwsIFB1YmxpY0tleVRva2VuPWI3N2E1YzU2MTkzNGUwODldXQAAAAAL", + Comparer.Default }; + yield return new object[] { "AAEAAAD/////AQAAAAAAAAAEAQAAAIsBU3lzdGVtLkNvbGxlY3Rpb25zLkdlbmVyaWMuTnVsbGFibGVDb21wYXJlcmAxW1tTeXN0ZW0uSW50UHRyLCBtc2NvcmxpYiwgVmVyc2lvbj00LjAuMC4wLCBDdWx0dXJlPW5ldXRyYWwsIFB1YmxpY0tleVRva2VuPWI3N2E1YzU2MTkzNGUwODldXQAAAAAL", + Comparer.Default }; + yield return new object[] { "AAEAAAD/////AQAAAAAAAAAEAQAAAIkBU3lzdGVtLkNvbGxlY3Rpb25zLkdlbmVyaWMuTnVsbGFibGVDb21wYXJlcmAxW1tTeXN0ZW0uR3VpZCwgbXNjb3JsaWIsIFZlcnNpb249NC4wLjAuMCwgQ3VsdHVyZT1uZXV0cmFsLCBQdWJsaWNLZXlUb2tlbj1iNzdhNWM1NjE5MzRlMDg5XV0AAAAACw==", + Comparer.Default }; + yield return new object[] { "AAEAAAD/////AQAAAAAAAAAEAQAAAOMBU3lzdGVtLkNvbGxlY3Rpb25zLkdlbmVyaWMuT2JqZWN0Q29tcGFyZXJgMVtbU3lzdGVtLk51bGxhYmxlYDFbW1Byb2crTXlTdHJ1Y3QsIENvbnNvbGVBcHAyMiwgVmVyc2lvbj0xLjAuMC4wLCBDdWx0dXJlPW5ldXRyYWwsIFB1YmxpY0tleVRva2VuPW51bGxdXSwgbXNjb3JsaWIsIFZlcnNpb249NC4wLjAuMCwgQ3VsdHVyZT1uZXV0cmFsLCBQdWJsaWNLZXlUb2tlbj1iNzdhNWM1NjE5MzRlMDg5XV0AAAAACw==", + Comparer.Default }; + yield return new object[] { "AAEAAAD/////AQAAAAAAAAAEAQAAAPwBU3lzdGVtLkNvbGxlY3Rpb25zLkdlbmVyaWMuT2JqZWN0Q29tcGFyZXJgMVtbU3lzdGVtLk51bGxhYmxlYDFbW1N5c3RlbS5EYXlPZldlZWssIFN5c3RlbS5Qcml2YXRlLkNvcmVMaWIsIFZlcnNpb249Ny4wLjAuMCwgQ3VsdHVyZT1uZXV0cmFsLCBQdWJsaWNLZXlUb2tlbj03Y2VjODVkN2JlYTc3OThlXV0sIG1zY29ybGliLCBWZXJzaW9uPTQuMC4wLjAsIEN1bHR1cmU9bmV1dHJhbCwgUHVibGljS2V5VG9rZW49Yjc3YTVjNTYxOTM0ZTA4OV1dAAAAAAs=", + Comparer.Default }; + } + + [Theory] + [MemberData(nameof(NullableComparersTestData))] + public void NullableComparers(string base64, object expectedObj) + { + var actualObject = BinaryFormatterHelpers.FromBase64String(base64, FormatterAssemblyStyle.Full); + Assert.Equal(expectedObj.GetType(), actualObject.GetType()); + } } } From 878d99a174d483ba7f741e681b942dd0fd7c024a Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 19 Apr 2022 02:17:00 +0300 Subject: [PATCH 05/10] fix failed build --- .../tests/BinaryFormatterTests.cs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/libraries/System.Runtime.Serialization.Formatters/tests/BinaryFormatterTests.cs b/src/libraries/System.Runtime.Serialization.Formatters/tests/BinaryFormatterTests.cs index 6bd4360974f5af..4cb4c58d01d580 100644 --- a/src/libraries/System.Runtime.Serialization.Formatters/tests/BinaryFormatterTests.cs +++ b/src/libraries/System.Runtime.Serialization.Formatters/tests/BinaryFormatterTests.cs @@ -716,6 +716,11 @@ private class DelegateBinder : SerializationBinder public override Type BindToType(string assemblyName, string typeName) => BindToTypeDelegate?.Invoke(assemblyName, typeName); } + public struct MyStruct + { + public int A; + } + public static IEnumerable NullableComparersTestData() { yield return new object[] { "AAEAAAD/////AQAAAAAAAAAEAQAAAJEBU3lzdGVtLkNvbGxlY3Rpb25zLkdlbmVyaWMuTnVsbGFibGVFcXVhbGl0eUNvbXBhcmVyYDFbW1N5c3RlbS5CeXRlLCBtc2NvcmxpYiwgVmVyc2lvbj00LjAuMC4wLCBDdWx0dXJlPW5ldXRyYWwsIFB1YmxpY0tleVRva2VuPWI3N2E1YzU2MTkzNGUwODldXQAAAAAL", From 580db645bdfb120c35f6131c63e0078bed3bf0d8 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 19 Apr 2022 13:11:17 +0300 Subject: [PATCH 06/10] fix tests --- .../tests/Generic/Comparers/EqualityComparer.Tests.cs | 4 ++-- .../tests/BinaryFormatterTests.cs | 5 ++--- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/libraries/System.Collections/tests/Generic/Comparers/EqualityComparer.Tests.cs b/src/libraries/System.Collections/tests/Generic/Comparers/EqualityComparer.Tests.cs index 9f274cdd15ea3c..c8b2581da92571 100644 --- a/src/libraries/System.Collections/tests/Generic/Comparers/EqualityComparer.Tests.cs +++ b/src/libraries/System.Collections/tests/Generic/Comparers/EqualityComparer.Tests.cs @@ -263,7 +263,7 @@ public static EqualsData Int32EnumData() { Int32Enum.Two, Int32Enum.Two, true }, { Int32Enum.Min, Int32Enum.Max, false }, { Int32Enum.Min, Int32Enum.Min, true }, - { Int32Enum.One, Int32Enum.Min + 1, false } + { Int32Enum.One, Int32Enum.Min + 1, false }, { (Int32Enum)(-2), null, false }, { Int32Enum.Two, null, false }, { null, Int32Enum.Max, false }, @@ -311,7 +311,7 @@ public static EqualsData NonEquatableValueTypeData() { new NonEquatableValueType(), new NonEquatableValueType(), true }, { one, one, true }, { new NonEquatableValueType(-1), new NonEquatableValueType(), false }, - { new NonEquatableValueType(2), new NonEquatableValueType(2), true } + { new NonEquatableValueType(2), new NonEquatableValueType(2), true }, { new NonEquatableValueType(-1), null, false }, { null, new NonEquatableValueType(2), false } }; diff --git a/src/libraries/System.Runtime.Serialization.Formatters/tests/BinaryFormatterTests.cs b/src/libraries/System.Runtime.Serialization.Formatters/tests/BinaryFormatterTests.cs index 4cb4c58d01d580..46a15ee80fabf4 100644 --- a/src/libraries/System.Runtime.Serialization.Formatters/tests/BinaryFormatterTests.cs +++ b/src/libraries/System.Runtime.Serialization.Formatters/tests/BinaryFormatterTests.cs @@ -756,10 +756,9 @@ public static IEnumerable NullableComparersTestData() [Theory] [MemberData(nameof(NullableComparersTestData))] - public void NullableComparers(string base64, object expectedObj) + public void NullableComparers(string base64, object obj) { - var actualObject = BinaryFormatterHelpers.FromBase64String(base64, FormatterAssemblyStyle.Full); - Assert.Equal(expectedObj.GetType(), actualObject.GetType()); + Assert.Equal(base64, BinaryFormatterHelpers.ToBase64String(obj, FormatterAssemblyStyle.Full)); } } } From 98d5252bda927169db7af8b9cfd067d30ff0fe2f Mon Sep 17 00:00:00 2001 From: EgorBo Date: Mon, 30 May 2022 22:09:53 +0200 Subject: [PATCH 07/10] Update tests --- .../tests/BinaryFormatterTests.cs | 50 +++++++------------ 1 file changed, 19 insertions(+), 31 deletions(-) diff --git a/src/libraries/System.Runtime.Serialization.Formatters/tests/BinaryFormatterTests.cs b/src/libraries/System.Runtime.Serialization.Formatters/tests/BinaryFormatterTests.cs index 6d315ee2498376..f616f72c4d7635 100644 --- a/src/libraries/System.Runtime.Serialization.Formatters/tests/BinaryFormatterTests.cs +++ b/src/libraries/System.Runtime.Serialization.Formatters/tests/BinaryFormatterTests.cs @@ -735,42 +735,30 @@ public struct MyStruct public static IEnumerable NullableComparersTestData() { - yield return new object[] { "AAEAAAD/////AQAAAAAAAAAEAQAAAJEBU3lzdGVtLkNvbGxlY3Rpb25zLkdlbmVyaWMuTnVsbGFibGVFcXVhbGl0eUNvbXBhcmVyYDFbW1N5c3RlbS5CeXRlLCBtc2NvcmxpYiwgVmVyc2lvbj00LjAuMC4wLCBDdWx0dXJlPW5ldXRyYWwsIFB1YmxpY0tleVRva2VuPWI3N2E1YzU2MTkzNGUwODldXQAAAAAL", - EqualityComparer.Default }; - yield return new object[] { "AAEAAAD/////AQAAAAAAAAAEAQAAAJIBU3lzdGVtLkNvbGxlY3Rpb25zLkdlbmVyaWMuTnVsbGFibGVFcXVhbGl0eUNvbXBhcmVyYDFbW1N5c3RlbS5JbnQzMiwgbXNjb3JsaWIsIFZlcnNpb249NC4wLjAuMCwgQ3VsdHVyZT1uZXV0cmFsLCBQdWJsaWNLZXlUb2tlbj1iNzdhNWM1NjE5MzRlMDg5XV0AAAAACw==", - EqualityComparer.Default }; - yield return new object[] { "AAEAAAD/////AQAAAAAAAAAEAQAAAJMBU3lzdGVtLkNvbGxlY3Rpb25zLkdlbmVyaWMuTnVsbGFibGVFcXVhbGl0eUNvbXBhcmVyYDFbW1N5c3RlbS5TaW5nbGUsIG1zY29ybGliLCBWZXJzaW9uPTQuMC4wLjAsIEN1bHR1cmU9bmV1dHJhbCwgUHVibGljS2V5VG9rZW49Yjc3YTVjNTYxOTM0ZTA4OV1dAAAAAAs=", - EqualityComparer.Default }; - yield return new object[] { "AAEAAAD/////AQAAAAAAAAAEAQAAAJMBU3lzdGVtLkNvbGxlY3Rpb25zLkdlbmVyaWMuTnVsbGFibGVFcXVhbGl0eUNvbXBhcmVyYDFbW1N5c3RlbS5JbnRQdHIsIG1zY29ybGliLCBWZXJzaW9uPTQuMC4wLjAsIEN1bHR1cmU9bmV1dHJhbCwgUHVibGljS2V5VG9rZW49Yjc3YTVjNTYxOTM0ZTA4OV1dAAAAAAs=", - EqualityComparer.Default }; - yield return new object[] { "AAEAAAD/////AQAAAAAAAAAEAQAAAJEBU3lzdGVtLkNvbGxlY3Rpb25zLkdlbmVyaWMuTnVsbGFibGVFcXVhbGl0eUNvbXBhcmVyYDFbW1N5c3RlbS5HdWlkLCBtc2NvcmxpYiwgVmVyc2lvbj00LjAuMC4wLCBDdWx0dXJlPW5ldXRyYWwsIFB1YmxpY0tleVRva2VuPWI3N2E1YzU2MTkzNGUwODldXQAAAAAL", - EqualityComparer.Default }; - yield return new object[] { "AAEAAAD/////AQAAAAAAAAAEAQAAAOsBU3lzdGVtLkNvbGxlY3Rpb25zLkdlbmVyaWMuT2JqZWN0RXF1YWxpdHlDb21wYXJlcmAxW1tTeXN0ZW0uTnVsbGFibGVgMVtbUHJvZytNeVN0cnVjdCwgQ29uc29sZUFwcDIyLCBWZXJzaW9uPTEuMC4wLjAsIEN1bHR1cmU9bmV1dHJhbCwgUHVibGljS2V5VG9rZW49bnVsbF1dLCBtc2NvcmxpYiwgVmVyc2lvbj00LjAuMC4wLCBDdWx0dXJlPW5ldXRyYWwsIFB1YmxpY0tleVRva2VuPWI3N2E1YzU2MTkzNGUwODldXQAAAAAL", - EqualityComparer.Default }; - yield return new object[] { "AAEAAAD/////AQAAAAAAAAAEAQAAAIQCU3lzdGVtLkNvbGxlY3Rpb25zLkdlbmVyaWMuT2JqZWN0RXF1YWxpdHlDb21wYXJlcmAxW1tTeXN0ZW0uTnVsbGFibGVgMVtbU3lzdGVtLkRheU9mV2VlaywgU3lzdGVtLlByaXZhdGUuQ29yZUxpYiwgVmVyc2lvbj03LjAuMC4wLCBDdWx0dXJlPW5ldXRyYWwsIFB1YmxpY0tleVRva2VuPTdjZWM4NWQ3YmVhNzc5OGVdXSwgbXNjb3JsaWIsIFZlcnNpb249NC4wLjAuMCwgQ3VsdHVyZT1uZXV0cmFsLCBQdWJsaWNLZXlUb2tlbj1iNzdhNWM1NjE5MzRlMDg5XV0AAAAACw==", - EqualityComparer.Default }; - - yield return new object[] { "AAEAAAD/////AQAAAAAAAAAEAQAAAIkBU3lzdGVtLkNvbGxlY3Rpb25zLkdlbmVyaWMuTnVsbGFibGVDb21wYXJlcmAxW1tTeXN0ZW0uQnl0ZSwgbXNjb3JsaWIsIFZlcnNpb249NC4wLjAuMCwgQ3VsdHVyZT1uZXV0cmFsLCBQdWJsaWNLZXlUb2tlbj1iNzdhNWM1NjE5MzRlMDg5XV0AAAAACw==", - Comparer.Default }; - yield return new object[] { "AAEAAAD/////AQAAAAAAAAAEAQAAAIoBU3lzdGVtLkNvbGxlY3Rpb25zLkdlbmVyaWMuTnVsbGFibGVDb21wYXJlcmAxW1tTeXN0ZW0uSW50MzIsIG1zY29ybGliLCBWZXJzaW9uPTQuMC4wLjAsIEN1bHR1cmU9bmV1dHJhbCwgUHVibGljS2V5VG9rZW49Yjc3YTVjNTYxOTM0ZTA4OV1dAAAAAAs=", - Comparer.Default }; - yield return new object[] { "AAEAAAD/////AQAAAAAAAAAEAQAAAIsBU3lzdGVtLkNvbGxlY3Rpb25zLkdlbmVyaWMuTnVsbGFibGVDb21wYXJlcmAxW1tTeXN0ZW0uU2luZ2xlLCBtc2NvcmxpYiwgVmVyc2lvbj00LjAuMC4wLCBDdWx0dXJlPW5ldXRyYWwsIFB1YmxpY0tleVRva2VuPWI3N2E1YzU2MTkzNGUwODldXQAAAAAL", - Comparer.Default }; - yield return new object[] { "AAEAAAD/////AQAAAAAAAAAEAQAAAIsBU3lzdGVtLkNvbGxlY3Rpb25zLkdlbmVyaWMuTnVsbGFibGVDb21wYXJlcmAxW1tTeXN0ZW0uSW50UHRyLCBtc2NvcmxpYiwgVmVyc2lvbj00LjAuMC4wLCBDdWx0dXJlPW5ldXRyYWwsIFB1YmxpY0tleVRva2VuPWI3N2E1YzU2MTkzNGUwODldXQAAAAAL", - Comparer.Default }; - yield return new object[] { "AAEAAAD/////AQAAAAAAAAAEAQAAAIkBU3lzdGVtLkNvbGxlY3Rpb25zLkdlbmVyaWMuTnVsbGFibGVDb21wYXJlcmAxW1tTeXN0ZW0uR3VpZCwgbXNjb3JsaWIsIFZlcnNpb249NC4wLjAuMCwgQ3VsdHVyZT1uZXV0cmFsLCBQdWJsaWNLZXlUb2tlbj1iNzdhNWM1NjE5MzRlMDg5XV0AAAAACw==", - Comparer.Default }; - yield return new object[] { "AAEAAAD/////AQAAAAAAAAAEAQAAAOMBU3lzdGVtLkNvbGxlY3Rpb25zLkdlbmVyaWMuT2JqZWN0Q29tcGFyZXJgMVtbU3lzdGVtLk51bGxhYmxlYDFbW1Byb2crTXlTdHJ1Y3QsIENvbnNvbGVBcHAyMiwgVmVyc2lvbj0xLjAuMC4wLCBDdWx0dXJlPW5ldXRyYWwsIFB1YmxpY0tleVRva2VuPW51bGxdXSwgbXNjb3JsaWIsIFZlcnNpb249NC4wLjAuMCwgQ3VsdHVyZT1uZXV0cmFsLCBQdWJsaWNLZXlUb2tlbj1iNzdhNWM1NjE5MzRlMDg5XV0AAAAACw==", - Comparer.Default }; - yield return new object[] { "AAEAAAD/////AQAAAAAAAAAEAQAAAPwBU3lzdGVtLkNvbGxlY3Rpb25zLkdlbmVyaWMuT2JqZWN0Q29tcGFyZXJgMVtbU3lzdGVtLk51bGxhYmxlYDFbW1N5c3RlbS5EYXlPZldlZWssIFN5c3RlbS5Qcml2YXRlLkNvcmVMaWIsIFZlcnNpb249Ny4wLjAuMCwgQ3VsdHVyZT1uZXV0cmFsLCBQdWJsaWNLZXlUb2tlbj03Y2VjODVkN2JlYTc3OThlXV0sIG1zY29ybGliLCBWZXJzaW9uPTQuMC4wLjAsIEN1bHR1cmU9bmV1dHJhbCwgUHVibGljS2V5VG9rZW49Yjc3YTVjNTYxOTM0ZTA4OV1dAAAAAAs=", - Comparer.Default }; + yield return new object[] { "NullableEqualityComparer`1", EqualityComparer.Default }; + yield return new object[] { "NullableEqualityComparer`1", EqualityComparer.Default }; + yield return new object[] { "NullableEqualityComparer`1", EqualityComparer.Default }; + yield return new object[] { "NullableEqualityComparer`1", EqualityComparer.Default }; + yield return new object[] { "NullableEqualityComparer`1", EqualityComparer.Default }; // implements IEquatable<> + + yield return new object[] { "ObjectEqualityComparer`1", EqualityComparer.Default }; // doesn't implement IEquatable<> + yield return new object[] { "ObjectEqualityComparer`1", EqualityComparer.Default }; + + yield return new object[] { "NullableComparer`1", Comparer.Default }; + yield return new object[] { "NullableComparer`1", Comparer.Default }; + yield return new object[] { "NullableComparer`1", Comparer.Default }; + yield return new object[] { "NullableComparer`1", Comparer.Default }; + yield return new object[] { "NullableComparer`1", Comparer.Default }; + + yield return new object[] { "ObjectComparer`1", Comparer.Default }; + yield return new object[] { "ObjectComparer`1", Comparer.Default }; } [Theory] [MemberData(nameof(NullableComparersTestData))] - public void NullableComparers(string base64, object obj) + public void NullableComparersRoundtrip(string expectedType, object obj) { - Assert.Equal(base64, BinaryFormatterHelpers.ToBase64String(obj, FormatterAssemblyStyle.Full)); + Assert.Equal(expectedType, FromBase64String(ToBase64String(obj)).GetType().Name); } } } From 8451f3f03999ac8caded1c88b39448e8a733d592 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Mon, 30 May 2022 23:10:19 +0200 Subject: [PATCH 08/10] Clean up --- .../Collections/Generic/ComparerHelpers.cs | 47 +++---------------- .../System/Collections/Generic/Comparer.cs | 2 +- .../Collections/Generic/EqualityComparer.cs | 2 +- .../tests/BinaryFormatterTests.cs | 7 ++- 4 files changed, 12 insertions(+), 46 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Collections/Generic/ComparerHelpers.cs b/src/coreclr/System.Private.CoreLib/src/System/Collections/Generic/ComparerHelpers.cs index b85fdbdded61dc..b020c127629413 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Collections/Generic/ComparerHelpers.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Collections/Generic/ComparerHelpers.cs @@ -37,14 +37,11 @@ internal static object CreateDefaultComparer(Type type) { result = CreateInstanceForAnotherGenericParameter((RuntimeType)typeof(GenericComparer), runtimeType); } - // Nullable does not implement IComparable directly because that would add an extra interface call per comparison. - // Instead, it relies on Comparer.Default to specialize for nullables and do the lifted comparisons if T implements IComparable. - else if (type.IsGenericType) + else if (type.IsGenericType && type.GetGenericTypeDefinition() == typeof(Nullable<>)) { - if (type.GetGenericTypeDefinition() == typeof(Nullable<>)) - { - result = TryCreateNullableComparer(runtimeType); - } + // Nullable does not implement IComparable directly because that would add an extra interface call per comparison. + var embeddedType = (RuntimeType)type.GetGenericArguments()[0]; + result = RuntimeTypeHandle.CreateInstanceForAnotherGenericParameter((RuntimeType)typeof(NullableComparer), embeddedType); } // The comparer for enums is specialized to avoid boxing. else if (type.IsEnum) @@ -55,20 +52,6 @@ internal static object CreateDefaultComparer(Type type) return result ?? CreateInstanceForAnotherGenericParameter((RuntimeType)typeof(ObjectComparer), runtimeType); } - /// - /// Creates the default for a nullable type. - /// - /// The nullable type to create the default comparer for. - private static object? TryCreateNullableComparer(RuntimeType nullableType) - { - Debug.Assert(nullableType != null); - Debug.Assert(nullableType.IsGenericType && nullableType.GetGenericTypeDefinition() == typeof(Nullable<>)); - - var embeddedType = (RuntimeType)nullableType.GetGenericArguments()[0]; - - return RuntimeTypeHandle.CreateInstanceForAnotherGenericParameter((RuntimeType)typeof(NullableComparer), embeddedType); - } - /// /// Creates the default for an enum type. /// @@ -130,14 +113,11 @@ internal static object CreateDefaultEqualityComparer(Type type) // If T implements IEquatable return a GenericEqualityComparer result = CreateInstanceForAnotherGenericParameter((RuntimeType)typeof(GenericEqualityComparer), runtimeType); } - else if (type.IsGenericType) + else if (type.IsGenericType && type.GetGenericTypeDefinition() == typeof(Nullable<>)) { // Nullable does not implement IEquatable directly because that would add an extra interface call per comparison. - // Instead, it relies on EqualityComparer.Default to specialize for nullables and do the lifted comparisons if T implements IEquatable. - if (type.GetGenericTypeDefinition() == typeof(Nullable<>)) - { - result = TryCreateNullableEqualityComparer(runtimeType); - } + var embeddedType = (RuntimeType)type.GetGenericArguments()[0]; + result = CreateInstanceForAnotherGenericParameter((RuntimeType)typeof(NullableEqualityComparer), embeddedType); } else if (type.IsEnum) { @@ -148,19 +128,6 @@ internal static object CreateDefaultEqualityComparer(Type type) return result ?? CreateInstanceForAnotherGenericParameter((RuntimeType)typeof(ObjectEqualityComparer), runtimeType); } - /// - /// Creates the default for a nullable type. - /// - /// The nullable type to create the default equality comparer for. - private static object? TryCreateNullableEqualityComparer(RuntimeType nullableType) - { - Debug.Assert(nullableType != null); - Debug.Assert(nullableType.IsGenericType && nullableType.GetGenericTypeDefinition() == typeof(Nullable<>)); - - var embeddedType = (RuntimeType)nullableType.GetGenericArguments()[0]; - return RuntimeTypeHandle.CreateInstanceForAnotherGenericParameter((RuntimeType)typeof(NullableEqualityComparer), embeddedType); - } - /// /// Creates the default for an enum type. /// diff --git a/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/Comparer.cs b/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/Comparer.cs index 16bd8b730733b2..1e0ddf0b4b1770 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/Comparer.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/Comparer.cs @@ -77,7 +77,7 @@ public override int GetHashCode() => [Serializable] [TypeForwardedFrom("mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089")] // Needs to be public to support binary serialization compatibility - public sealed class NullableComparer : Comparer where T : struct + public sealed class NullableComparer : Comparer, ISerializable where T : struct { public NullableComparer() { } private NullableComparer(SerializationInfo info, StreamingContext context) { } diff --git a/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/EqualityComparer.cs b/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/EqualityComparer.cs index 923cf3a27e96aa..127f35d150b9c7 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/EqualityComparer.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/EqualityComparer.cs @@ -97,7 +97,7 @@ public override int GetHashCode() => [Serializable] [TypeForwardedFrom("mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089")] // Needs to be public to support binary serialization compatibility - public sealed partial class NullableEqualityComparer : EqualityComparer where T : struct + public sealed partial class NullableEqualityComparer : EqualityComparer, ISerializable where T : struct { public NullableEqualityComparer() { } private NullableEqualityComparer(SerializationInfo info, StreamingContext context) { } diff --git a/src/libraries/System.Runtime.Serialization.Formatters/tests/BinaryFormatterTests.cs b/src/libraries/System.Runtime.Serialization.Formatters/tests/BinaryFormatterTests.cs index f616f72c4d7635..2a04351cfa9eed 100644 --- a/src/libraries/System.Runtime.Serialization.Formatters/tests/BinaryFormatterTests.cs +++ b/src/libraries/System.Runtime.Serialization.Formatters/tests/BinaryFormatterTests.cs @@ -738,7 +738,6 @@ public static IEnumerable NullableComparersTestData() yield return new object[] { "NullableEqualityComparer`1", EqualityComparer.Default }; yield return new object[] { "NullableEqualityComparer`1", EqualityComparer.Default }; yield return new object[] { "NullableEqualityComparer`1", EqualityComparer.Default }; - yield return new object[] { "NullableEqualityComparer`1", EqualityComparer.Default }; yield return new object[] { "NullableEqualityComparer`1", EqualityComparer.Default }; // implements IEquatable<> yield return new object[] { "ObjectEqualityComparer`1", EqualityComparer.Default }; // doesn't implement IEquatable<> @@ -747,10 +746,9 @@ public static IEnumerable NullableComparersTestData() yield return new object[] { "NullableComparer`1", Comparer.Default }; yield return new object[] { "NullableComparer`1", Comparer.Default }; yield return new object[] { "NullableComparer`1", Comparer.Default }; - yield return new object[] { "NullableComparer`1", Comparer.Default }; yield return new object[] { "NullableComparer`1", Comparer.Default }; - yield return new object[] { "ObjectComparer`1", Comparer.Default }; + yield return new object[] { "ObjectComparer`1", Comparer.Default }; yield return new object[] { "ObjectComparer`1", Comparer.Default }; } @@ -758,7 +756,8 @@ public static IEnumerable NullableComparersTestData() [MemberData(nameof(NullableComparersTestData))] public void NullableComparersRoundtrip(string expectedType, object obj) { - Assert.Equal(expectedType, FromBase64String(ToBase64String(obj)).GetType().Name); + string serialized = BinaryFormatterHelpers.ToBase64String(obj); + Assert.Equal(expectedType, BinaryFormatterHelpers.FromBase64String(serialized).GetType().Name); } } } From 8ee6d6777a0bd4e8e685ab15e88808a3cc7199b4 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 31 May 2022 14:04:11 +0200 Subject: [PATCH 09/10] Address Jan's feedback --- .../Common/TypeSystem/IL/Stubs/ComparerIntrinsics.cs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/coreclr/tools/Common/TypeSystem/IL/Stubs/ComparerIntrinsics.cs b/src/coreclr/tools/Common/TypeSystem/IL/Stubs/ComparerIntrinsics.cs index 289556d9e58195..91856de2d880d5 100644 --- a/src/coreclr/tools/Common/TypeSystem/IL/Stubs/ComparerIntrinsics.cs +++ b/src/coreclr/tools/Common/TypeSystem/IL/Stubs/ComparerIntrinsics.cs @@ -113,11 +113,9 @@ private static TypeDesc GetComparerForType(TypeDesc type, string flavor, string // We can't tell at compile time either. return null; } - else if (ImplementsInterfaceOfSelf(nullableType, interfaceName)) - { - return context.SystemModule.GetKnownType("System.Collections.Generic", $"Nullable{flavor}`1") - .MakeInstantiatedType(nullableType); - } + + return context.SystemModule.GetKnownType("System.Collections.Generic", $"Nullable{flavor}`1") + .MakeInstantiatedType(nullableType); } else if (type.IsEnum) { From 481e782cc2ab53285a51b61f124e39de5c5bdcc8 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 31 May 2022 16:29:45 +0200 Subject: [PATCH 10/10] Use correct type for backward compatibility during binary serialization --- .../src/System/Collections/Generic/Comparer.cs | 2 +- .../src/System/Collections/Generic/EqualityComparer.cs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/Comparer.cs b/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/Comparer.cs index 1e0ddf0b4b1770..22cc2f3aac02cd 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/Comparer.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/Comparer.cs @@ -86,7 +86,7 @@ public void GetObjectData(SerializationInfo info, StreamingContext context) if (!typeof(T).IsAssignableTo(typeof(IComparable))) { // We used to use NullableComparer only for types implementing IComparable - info.SetType(typeof(ObjectComparer)); + info.SetType(typeof(ObjectComparer)); } } diff --git a/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/EqualityComparer.cs b/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/EqualityComparer.cs index 127f35d150b9c7..60a65072a41f90 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/EqualityComparer.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/EqualityComparer.cs @@ -106,7 +106,7 @@ public void GetObjectData(SerializationInfo info, StreamingContext context) if (!typeof(T).IsAssignableTo(typeof(IEquatable))) { // We used to use NullableComparer only for types implementing IEquatable - info.SetType(typeof(ObjectEqualityComparer)); + info.SetType(typeof(ObjectEqualityComparer)); } }