S3897 is an unsafe suggestion in my opinion

[jabbera]

I posted this in the google groups, but no one responded. Through I might get more traction here.

Hi all,

I wanted to discuss: S3897 (https://sonarcloud.io/organizations/default/rules#rule_key=csharpsquid%3AS3897)

I think it is an unsafe suggestion on it's surface if the class is not sealed. Especially as a major. The reason is fairly well described here: http://blog.mischel.com/2013/01/05/inheritance-and-iequatable-do-not-mix/

But the important quote: A class that implements IEquatable is saying, “I know how to compare two instances of type T or any type derived from T for equality.” After all, if type B derives from A, then B is-a A. That’s what inheritance means! The problem with implementing IEquatable on a non-sealed type is that a base class cannot know how to compare instances of derived types. Trying to implement IEquatable on a non-sealed class ends up producing inconsistent results when comparing instances of derived types.

Here is an example.

class Base : IEquatable<Base> {
  bool Equals(Base b) => return true;
}

class A : Base, IEquatable<A> {
  bool Equals(A a) => return false;
}

class B : Base, IEquatable<B> {
  bool Equals(B b) => return false;
}

A a = new A();
B b = new B();

Console.WriteLine(a.Equals(b)); // This should obviously return false as they are different classes however because of the existance of Base:Equals(Base b) it will return true.

Those classes are obviously not equal but implementing the suggestion would make the runtime behave as such.

Thanks,
Mike

[Evangelink]

Hi @jabbera,

Indeed we have seen your thread on Google Group but we haven't had the opportunity to have an internal discussion meeting on this subject. At first glance your suggestion make sense but we could also allow a virtual implementation of the equal. So we have to have a little talk to see whether we can think of a best solution.

Cheers,
Amaury

[jabbera]

Thanks for getting back to me! Glad to know it's being looked at.

Cheers,
Mike

[Evangelink]

Hi @jabbera!

We have been discussing this subject with the team and we came up with the following solution:

• Keep this rule as it is
• Add a new rule (description here) to recommend sealing the class.

We have decided to keep the rule as it is because it suggests you to implement IEquatable<T> only when you override Equals(object) or have created a Equals(T) method. On the example you gave, overriding Equals(object) will lead to the same problem.

Feel free to discuss the proposed solution if you thing we are missing something.

[jabbera]

Hi @Evangelink
I respectfully disagree. The following standard implementation of equals works perfectly fine for ALL comparisons unlike S3897.

class Base {
  override bool Equals(object base) {
    Base other = base as Base;
    if (other == null) return false;
    // do comparison of Base properties
  }
}

class A : Base {
    override bool Equals(object a) {
    A other = a as A ;
    if (other == null) return false;
    // do comparison of A properties 
   return base.Equals(other);
  }
}

class B : Base {
    override bool Equals(object b) {
    Base other = b as B;
    if (other == null) return false;
    // do comparison of B properties
   return base.Equals(other);
  }
}

A a = new A();
B b = new B();

Console.WriteLine(a.Equals(b)); // This works is calls A::Equals(object) which does a type 
\\ check and validates A's fields

Lets take the working example above and take the advice of S3897.

class Base : IEquatable<Base> {
  bool Equals(Base base) {
    if (base == null) { return false };
    // do comparison of base properties
  }

  override bool Equals(object base)  => Equals(base as Base); // Calls Base::Equals(Base)
}

class A : Base {
  bool Equals(A a) {
    if (a == null) { return false };
    // do comparison of A properties
   return base.Equals(a); // Calls Base::Equals(Base);
  }

  override bool Equals(object a)  => Equals(a as A); // Calls A::Equals(A)
}

class B : Base {
  bool Equals(B b) {
    if (b == null) { return false };
    // do comparison of B properties
   return base.Equals(b); // Calls Base::Equals(Base);
  }

  override bool Equals(object b)  => Equals(b as B); // Calls B::Equals(B)
}

A a = new A();
B b = new B();

Console.WriteLine(a.Equals(b)); // This calls the WRONG equals. This causes Base::Equals(Base)
//  to be called which only compares the properties in Base and ignores the fact that 
// a and b are different types. In the working example A::Equals(Object) would have been 
// called and Equals would return false because it correctly recognizes that a and b are 
// different types. If a and b have the same base properties they will  be returned as equal. 
// Following the advice of S3897 took something that worked and broke it.

object o = b;
Console.WriteLine(a.Equals(o)); // Ironically this works because it calls A::Equals(Object) which
// does the type check! Implementing S3897 means I now have to cast derived types to object 
// to get a proper comparison!!!! It's knowledge the user of my API needs to know and is
// unnecessary ceremony and VERY error prone.

The second you introduce public type specific equals in a base class equality comparison breaks down when you compare classes derived from a common base class. IE: If the base class properties are equal the object will be considered equal even if they are not the same type.

The origins of IEquatable are actually around structs and not class hierarchies. Before CLR 2.0 structs were compared using reflection. This was not only slow but required a ton of boxing\unboxing. See: https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/struct

I won't drag the topic on any more. If you still don't see the potential harm of this rule (or not having the new rule at the same severity level) feel free to close the issue. I'll be disabling the rule in my installation and be recommending the same to all of my users.

Looking forward to working with you on more issues!

Mike

[jabbera]

One last thing. I posted an issue on the roslyn-analysers and got the following from a MSFT employee:

I typically only implement IEquatable on value types.

It provides a notable win over Equals(object) by avoiding boxing
Inheritance issues don't come into play
If I want to make it obvious that something is equatable, I may implement this on reference types but typically only in a sealed type hierarchy.

[Evangelink]

Hi @jabbera!

Thanks for your feedback that is really interesting and I wasn't aware of this tricky part. To have the example working I had to actually make A implements IEquatable<B> which is wrong and cannot scale. I will have another chat with the team tomorrow and will keep you posted with new decisions.

We are also looking forward for more interactions with you (and more generally our community).

[jabbera]

Thanks for continuing to follow up. I wasn't aware of this either until I tracked down a rediculous production bug based on it. I've been scarred:-)

[Evangelink]

Hi @jabbera ,

It took some time to get to a result. We have reduced the scope of S3897 to only suggest to implement IEquatable<T> when a public method Equals(T) is found (with or without the IEquatable<T> the result will be the same for the bug you mentioned). We have also introduced a new rule that will recommend to seal the class if either IEquatable<T> or a public method Equals(T) is found (see S4035).

I have created 2 tickets (one to fix the implementation #462 and one to implement the new rule #461) so I think I am safe closing this ticket.

[jabbera]

Agreed! Thanks for the update.