Add IEquatable<T> to public Equals-overriding value types to enable CA1066/1067

[stephentoub]

CA1066 validates that value types that override Equals also implement IEquatable<T> (and CA1067 validates that types that implement IEquatable<T> override Equals). We have a bunch of types that don't follow that guidance, which is particularly an issue for structs where the argument will be boxed to call Equals. We've been adding IEquatable<T> to such types as they've been requested, but after a bunch of them have come through API review, we generally agreed to just blanket add them all to all types that violate the rule. This issue outlines those types.

These types already provide a public bool Equals(T other) method and just need to implement the IEquatable<T> interface:

Microsoft.Extensions.Logging.EventId
System.ArraySegment<T>
System.Collections.Specialized.BitVector32.Section
System.Diagnostics.CounterSample
System.Diagnostics.SymbolStore.SymbolToken
System.ModuleHandle
System.Runtime.InteropServices.ArrayWithOffset
System.ServiceProcess.SessionChangeDescription
System.Threading.CancellationToken
System.Threading.LockCookie

These types need both the IEquatable<T> interface implementation and a public bool Equals(T other) method added:

System.Collections.Specialized.BitVector32
System.ComponentModel.Composition.ReflectionModel.LazyMemberInfo
System.ComponentModel.Design.Serialization.MemberRelationship
System.Data.SqlTypes.SqlBinary
System.Data.SqlTypes.SqlBoolean
System.Data.SqlTypes.SqlByte
System.Data.SqlTypes.SqlDateTime
System.Data.SqlTypes.SqlDecimal
System.Data.SqlTypes.SqlDouble
System.Data.SqlTypes.SqlGuid
System.Data.SqlTypes.SqlInt16
System.Data.SqlTypes.SqlInt32
System.Data.SqlTypes.SqlInt64
System.Data.SqlTypes.SqlMoney
System.Data.SqlTypes.SqlSingle
System.Data.SqlTypes.SqlString
System.Drawing.CharacterRange
System.Net.Sockets.IPPacketInformation
System.Reflection.CustomAttributeNamedArgument
System.Reflection.CustomAttributeTypedArgument
System.Runtime.InteropServices.GCHandle
System.Transactions.TransactionOptions

These types implement IEquatable<T> but don't override Equals(object) and so need an override:

System.TimeZoneInfo.AdjustmentRule

These types override Equals but are special in some way (e.g. throwing a NotSupportedException from Equals) and I believe should just have CA1066 suppressed rather than adding the interface implementation:

System.HashCode
System.Nullable<T>

These types are part of libraries that are deprecated or are in the process of being deprecated and so I believe should just have CA1066 suppressed:

System.Runtime.Serialization.StreamingContext

[ghost]

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

Issue Details

---

CA1066 validates that types that override Equals also implement IEquatable<T> (and CA1067 validates that types that implement IEquatable<T> override Equals). We have a bunch of types that don't follow that guidance, which is particularly an issue for structs where the argument will be boxed to call Equals. We've been adding IEquatable<T> to such types as they've been requested, but after a bunch of them have come through API review, we generally agreed to just blanket add them all to all types that violate the rule. This issue outlines those types.

These types already provide a public bool Equals(T other) method and just need to implement the IEquatable<T> interface:

Microsoft.Extensions.Logging.EventId
System.ArraySegment<T>
System.Collections.Specialized.BitVector32.Section
System.Diagnostics.CounterSample
System.Diagnostics.SymbolStore.SymbolToken
System.ModuleHandle
System.Runtime.InteropServices.ArrayWithOffset
System.ServiceProcess.SessionChangeDescription
System.Threading.CancellationToken
System.Threading.LockCookie

These types need both the IEquatable<T> interface implementation and a public bool Equals(T other) method added:

System.Collections.Specialized.BitVector32
System.ComponentModel.Composition.ReflectionModel.LazyMemberInfo
System.ComponentModel.Design.Serialization.MemberRelationship
System.Data.SqlTypes.SqlBinary
System.Data.SqlTypes.SqlBoolean
System.Data.SqlTypes.SqlByte
System.Data.SqlTypes.SqlDateTime
System.Data.SqlTypes.SqlDecimal
System.Data.SqlTypes.SqlDouble
System.Data.SqlTypes.SqlGuid
System.Data.SqlTypes.SqlInt16
System.Data.SqlTypes.SqlInt32
System.Data.SqlTypes.SqlInt64
System.Data.SqlTypes.SqlMoney
System.Data.SqlTypes.SqlSingle
System.Data.SqlTypes.SqlString
System.Drawing.CharacterRange
System.Net.Sockets.IPPacketInformation
System.Reflection.CustomAttributeNamedArgument
System.Reflection.CustomAttributeTypedArgument
System.Runtime.InteropServices.GCHandle
System.Transactions.TransactionOptions

These types implement IEquatable<T> but don't override Equals(object) and so need an override:

System.TimeZoneInfo.AdjustmentRule

These types override Equals but are special in some way (e.g. throwing a NotSupportedException from Equals) and I believe should just have CA1066 suppressed rather than adding the interface implementation:

System.HashCode
System.Nullable<T>

These types are part of libraries that are deprecated or are in the process of being deprecated and so I believe should just have CA1066 suppressed:

System.Runtime.Serialization.StreamingContext

Author:	stephentoub
Assignees:	stephentoub
Labels:	area-System.Runtime, api-ready-for-review
Milestone:	7.0.0

[bartonjs]

Video

Looks good as proposed. (Declare on the duck-typeable ones, provide the implementation on the semantic-providing ones, fix AdjustmentRule, and suppress on the three outliers)

[stephentoub]

In implementing this, I found at least one case that's a problem. This popped with ArraySegment<T> in particular because it implements IEnumerable<T>, which today causes an xunit operation like Assert.Equal(arraySegment1, arraySegment2) to enumerate each segment and compare the individual elements one-by-one. But once ArraySegment<T> implements IEquatable<T>, xunit ends up preferring to use that implementation and so uses ArraySegment's Equals, which doesn't implement a deep comparison (as we discussed in the API review).