Runtime: StringBuilder Has Different Equals Implementations

[taoyouh]

Description

StringBuilder has two different versions of Equals. Equals(object) uses the version defined in class Object, while Equals(StringBuilder) uses the in-class implementation. These two different behaviors are confusing.

Considering these code. The different results are confusing.

StringBuilder a = new StringBuilder();
StringBuilder b = new StringBuilder();
object c = b;
a.Append("abc");
b.Append("abc");
Console.WriteLine(a.Equals(b)); // True
Console.WriteLine(a.Equals(c)); // False

Configuration

OS Name: Windows
OS Version: 10.0.18363
OS Platform: Windows
RID: win10-x64
Base Path: C:\Program Files\dotnet\sdk\3.1.301\

.NET Core SDK:
Version: 3.1.301
Commit: 7feb845744

Regression?

Not noticed.

Other information

None.

[Dotnet-GitSync-Bot]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[danmosemsft]

Not sure of latest guidance. This suggests it's correct as is
https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/equality-operators#equality-operators-on-reference-types

[danmosemsft]

@terrajobst

[jkotas]

This guidance talks about equality operators (ie operator== and operator!=). This issue is about Equals method.

I think the current behavior violates "DO NOT have overloads with parameters at the same position and similar types yet with different semantics." from https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/member-overloading

[GrabYourPitchforks]

I loaded all _System.*.dll_ and _Microsoft.*.dll_ files from the runtime dirs, called Assembly.GetExportedTypes on each, and looked for any types which implemented Equals(object) but not Equals(T) (or vice versa).

Here's the resulting list, from 3570 total types examined:

System.TimeZoneInfo+AdjustmentRule
System.Text.StringBuilder
System.Data.DataView

[GSPP]

There should only be one notion of equality per type. All of Equals(object), Equals(T), ==, !=, GetHashCode must be unified on the definition or else it's a mess. Depending on the precise user code, different methods and operators will be called. Often, this is not obvious from the source code. The calls can happen inside of library code. To preserve sanity, all these methods must agree.

Reference equality can always be obtained through ReferenceEquals if desired.

[GrabYourPitchforks]

@jkotas @ericstj @terrajobst Say we were to unify Equals(object) and Equals(T) in the types listed here. Are there concerns about this being a breaking change from Full Framework? My own philosophy is that we can take breaks as long as they're for the good of the ecosystem, but I can't quite figure out where on the spectrum this might fall.

[jkotas]

• AdjustmentRule.Equals(object): Adding it should be pretty safe. The type implements IEquatable<AdjustmentRule?> and overrides GetHashCode. Also, it is not too frequently used type that makes the breaking potential even lower.
• StringBuilder.Equals(object) is hard. If we add it, we should also add IEquatable<StringBuilder?> and GetHashCode override to have a consistent equality contract as @gspp pointed out, but then it again does not make sense because the type is mutable and mutations change the hashcode. My call would be to leave it as it is and not try to fix it.
• Adding DataView.Equals(object) may have some breaking potential. I think there is a realistic chance that somebody may be depending on the referential equality for DataView

[taoyouh]

The StringBuilder.Equals has a confusing name. It is actually something like Enumerable.SequenceEqual in System.Linq.

[GrabYourPitchforks]

My call would be to leave it as it is and not try to fix it.

I misread that as "leave it as it is and try not to think about it." Which also seems like sage advice. 🙂