Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inconsistency between InternetAddress.Equals(InternetAddress) and InternetAddress.Equals(object) #481

Closed
tgiddings opened this issue Apr 30, 2019 · 2 comments
Labels
bug Something isn't working

Comments

@tgiddings
Copy link

When two instances (which will be referred to as x and y) of InternetAddress are created using InternetAddress.Parse with identical input, x.Equals(y) returns true when using InternetAddress.Equals(InternetAddress) and false when using InternetAddress.Equals(object).

In the NUnit test below, the first assertion succeeds while the second fails.

[Test]
public void Test1()
{
  string address = "[email protected]";
  var x = InternetAddress.Parse(address);
  var y = InternetAddress.Parse(address);

  Assert.True(x.Equals(y));

  var xObj = (object)x;
  var yObj = (object)y;

  Assert.True(xObj.Equals(yObj));
}

The same behavior occurs when using MailboxAddress instead of InternetAddress.

Affects versions 2.13 and 2.14 on NuGet.

@jstedfast
Copy link
Owner

I'm honestly not sure that overriding Equals(object) is the right thing to do here.

Normally, Equals(object) is for reference equality which is why that Equals fails for you.

The Equals(object) method is used in conjunction with GetHashCode() when storing objects in a dictionary or hash table, for example, and in general, I don't think you'd want 2 different object instances to be considered to be the same.

Just cast to an InternetAddress and you get your comparison.

@jstedfast jstedfast added the invalid This doesn't seem right label May 1, 2019
@tgiddings
Copy link
Author

From the documentation of object.Equals(object):

YourIEquatable<T>.Equals implementation should return results that are consistent with Equals.

From the documentation of IEquatable<T>.Equals(T):

If you implement Equals(T), you should also override the base class implementations of Equals(Object) and GetHashCode() so that their behavior is consistent with that of the Equals(T) method.

The vast majority of generic data structures and many algorithms rely on Equals(T), Equals(object), and GetHashCode() being mutually consistent.

Reference-equality is merely a default (and even then only for reference types), and usually not an appropriate one.

There is also no "just" to getting at the Equals(T) method in some circumstances. For example (this is the example that caused me to discover this bug), I have a generic function that takes an IEqualityComparer<T> and an overload which instead requires that T:IEquatable<U> and T:U. To deduplicate code, this overload calls the other with the default IEqualityComparer for T (EqualityComparer<T>.Default). However, the default comparer uses Equals(object) since it's supposed to be the given type's own idea of equality. Your type does not adhere to the .Net framework's expectations, so this does not work correctly.

Working around this requires making the following redundant-looking class

public class EquatableEqualityComparer<T,U>
    :IEqualityComparer<T>
    where T:U:IEquatable<U>
{
    public bool Equals(T x, T y)
    {
        return x.Equals(y);
    }

    public int GetHashCode(T obj)
    {
      return obj.GetHashCode();
    }
}

The Equals(object) method is used in conjunction with GetHashCode() when storing objects in a dictionary or hash table, for example, and in general, I don't think you'd want 2 different object instances to be considered to be the same.

If I have a map from strings to ints, put ("cat",1) in the map, and try to get the value for "cat", I should always get 1 rather than only getting a result if the string "cat" happens to be interned.

jstedfast added a commit that referenced this issue May 2, 2019
@jstedfast jstedfast added bug Something isn't working and removed invalid This doesn't seem right labels May 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants