Inherited classes Equals ignored when using OrderedEquality on an array of the base class

[christianrondeau]

This is actually a follow up to #38 but in another situation.

Problem

The ObjectEqualityComparer<T> class is used, which actually calls x.Equals(y) where y is of the expected time, resulting in only the base class Equals implementation being called.

I'm wondering if marking the typed Equals method as public might make things harder? If it was protected instead, it would be much harder to fall in that trap. The method checks for

Reproduction

public class GeneratorBaseClassBugTests
{
  [Equatable]
  public partial class MyContainer
  {
    [OrderedEquality]
    public MyBase[]? Content { get; set; }
  }
  
  [Equatable]
  public abstract partial class MyBase
  {
  }
  
  [Equatable]
  public partial class MyImpl : MyBase
  {
    public int Value { get; set; }
  }

  [Test]
  public void TestEqual()
  {
    var v1 = new MyContainer { Content = new MyBase[] { new MyImpl { Value = 1 } } };
    var v2 = new MyContainer { Content = new MyBase[] { new MyImpl { Value = 1 } } };

    Assert.That(v1.Equals(v2), Is.True);
  }

  [Test]
  public void TestNotEqual()
  {
    var v1 = new MyContainer { Content = new MyBase[] { new MyImpl { Value = 1 } } };
    var v2 = new MyContainer { Content = new MyBase[] { new MyImpl { Value = 2 } } };

    Assert.That(v1.Equals(v2), Is.False);
  }
}

Solution

The code ends up calling SequenceEqual, which again calls GenericEqualityComparer<BaseType>, ignoring inherited Equals implementations. This is actually the behavior of SequenceEqual, which makes me think that even outside of your comparison code, the generated equality checks may fail in other circumstances. Let me know if you believe going the protected route for the typed Equals implementation sounds like the way to go.

Version

.NET 6.0, Generator.Equals 2.7.2

Sorry to be a pain, hopefully that'll avoid some nasty surprises for other people :D Thanks again for your consideration and the great library!

[diegofrata]

Thanks for pointing it out. There were a bunch of other places relying on default equality rather than using the specified comparer. I have revised all comparers and tweaked the implementation to ensure we are doing the right thing.

The caveat is that for DictionaryEquality (for the key comparison) and SetEquality (when either obj is ISet) we respect whatever equality comparer these instances already have. We don't want member comparison and the actual lookup operations on these collections to have different behaviors.

[diegofrata]

This should be sorted on 2.7.3. Let me know if you hit any more issues!

[christianrondeau]

It does work, the remaining problems I have are in the code, anywhere there are things like this a.Equals(b) where a is of a base type, it will silently ignore overrides because C# will call the function with the base type rather than the one with object.

In other words, it works "most of the time".

If I may, what is the reason why the typed Equals overload is public rather than protected? Making it protected would solve all problems and avoid the pitfall, and unless the class is sealed or it is a struct, I don't see any reason for calling the typed overload.

Here are my suggestions, in order of preference:

1. Make the typed Equals protected but keep the typed Equals public for sealed classes and structs (avoiding boxing)
2. Add an option to [Equatable] to make the typed Equals public or not (I'd then add it to every single class)

What do you think?

[diegofrata]

I think the IEquatable<T> on classes was an oversight. Making the method protected would be a breaking change in the interface.

I think we can do two things:

• In version 2.x we keep the IEquatable<T> but make it call the DefaultEqualityComparer if the class is not sealed. We also move the logic into the Equals(object) method.
• In version 3.x I will remove support for IEquatable<T> in classes & records or make it an explicit interface implementation.

[diegofrata]

I am still going to do a bit more investigation about these options.

[christianrondeau]

Well, it would indeed be a breaking change, however only the typed method would be protected, the Equals(object) one would be public. So I'd expect any implementation to be unaffected, except maybe when using reflection. In any case, I did find quite a lot of places in our code where it broke in very subtle ways. We have a very good unit test coverage that surfaced most of them... however there are enough of them (and it simply does the wrong thing rather than crashing) so for now I'll rollback the Generator.Equals integration. I think many users of the library may fall in this trap (both the Dictionary case and the manual call to .Equals()) so it's definitely worth thinking through.

I'll soon have some time, so if I can help, I'll do my best, if you settle on how you want to tackle this.

Again, thanks for your consideration and making the library open source :)

[diegofrata]

The problem with breaking ABI is that I have no idea who is using it, so someone shipping a patch version unaware of this change would have a problem. So changing the interface would definitely require a major version.

However, I think I have an acceptable fix that doesn't break ABI compatibility. In the next major version, this can be cleaned up a bit further, but I think this sorts the remaining issue you observed.

public override bool Equals(object? obj)
{
    var other = obj as global::Generator.Equals.Tests.Classes.BaseEquality.Manager;
    return
        !ReferenceEquals(other, null) && base.Equals(obj)
        && global::Generator.Equals.DefaultEqualityComparer<global::System.String>.Default.Equals(this.Department!, other.Department!)
        ;
}

public bool Equals(global::Generator.Equals.Tests.Classes.BaseEquality.Manager? other) =>
    Equals((object?) other);

[diegofrata]

I released this change in 2.7.4. Can you give it a go?

[christianrondeau]

I tried quickly, and got a few seemingly random StackOverflowException :D I think I have some Equals overrides myself that actually call the typed implementation (this is how ReSharper / Rider generates the Equals implementation). So this is definitely a breaking change :)

I'll report back later when I can test some more and confirm what I just said.

[christianrondeau]

So, I confirm the problem is indeed this kind of code on a class that inherits from one that uses [Equatable]

	public bool Equals(MyClass other)
	{
		return base.Equals(other) && Equals(MyProperty, other.MyProperty);
	}

	public override bool Equals(object obj)
	{
		if (ReferenceEquals(null, obj)) return false;
		if (ReferenceEquals(this, obj)) return true;
		if (obj.GetType() != GetType()) return false;
		return Equals((MyClass)obj);
	}

This is code generated by ReSharper originally, so I expect this to be fairly common, either by the library users directly or by users of the classes made by the library users (if they release themselves NuGet packages or a DLL).

While I can fix my code to make it work, your goal was to avoid a breaking change so I'm not sure you want to keep this out there without good release notes :)

since ReSharper uses the typed Equals to call the inheritance chain, and the object Equals to start the chain (since it is overridden), I feel like this may be the safe way to go. Here's a complete example of what JetBrains generates:

public class Hello
{
	public string Hi { get; set; }

	protected bool Equals(Hello other)
	{
		return Hi == other.Hi;
	}

	public override bool Equals(object obj)
	{
		if (ReferenceEquals(null, obj)) return false;
		if (ReferenceEquals(this, obj)) return true;
		if (obj.GetType() != this.GetType()) return false;
		return Equals((Hello)obj);
	}

	public override int GetHashCode()
	{
		return (Hi != null ? Hi.GetHashCode() : 0);
	}
}

This uses the protected typed Equals, which ensures only the object one can be called publicly.

For what it's worth, I'd definitely wait for a 3.0 if you wanted to bundle such a change with others to justify the major version (though I'd be comfortable with this breaking change personally :) ), but I think this is the implementation you should go for, since it's been used a lot in the wild successfully.

And for performance, you can definitely keep the typed version public for structs and sealed classes, since they will always be safe to call directly.

I hope this is all helpful :)

[diegofrata]

It's very helpful thanks for the feedback!

I really want to get this truly fixed, so I'll release another patch today that fixes the stack overflow you observed.

You're right that it's a common pattern and it's likely someone will hit it, so it needs fixing! But I still need to abide by the public interface.

I think my play here is to leave this as fixed as possible in 2.x and immediately release a version 3.x with the implementation outlined in #43.

This way people can choose or not to migrate.

[diegofrata]

I reverted my changes and release a new version 2.7.5 to ensure there are no stack overflows.

This is basically unfixable as a patch. I am closing this issue and any further work will go on #43.

[christianrondeau]

Hi! I ran, and it doesn't seem to deadlock, which is good. I still have a few cases not working, such as a readonly struct with members rather than properties returning an empty HashCode and not comparing anything in the Equals implementation. I'll get back with a new test case later and confirm :)

[diegofrata]

I have an open PR which will be the 3.0 release, I tagged you on it if you want to have a look at the changes.

I need to fix the GitHub action to allow me to publish pre-release packages, it should allow you to test without making an official release. I'll try to get it sorted in the weekend.

I'd appreciate an example of the readonly struct that's not working for you. Don't worry about writing test cases, just the struct definition will do.

Thanks for the help in uncovering these issues!

[christianrondeau]

Here's the test case (I always prefer to make an actual test first, sometimes it's my fault and it's always good to catch those before using any of your own time!)

[Equatable]
public partial struct MyStruct
{
	public int MyValue;
}

[Test]
public void StructsWithMembersAreCompared()
{
  var v1 = new MyStruct { MyValue = 1 };
  var v2 = new MyStruct { MyValue = 2 };

  Assert.That(v1, Is.Not.EqualTo(v2));
  Assert.That(v1.GetHashCode(), Is.Not.EqualTo(v2.GetHashCode()));
}

But to be clear, this case is actually more of a feature than a bug :) I can add [DefaultEquality] OR I can add { get; } and both will work. So I guess this is more about documenting what is supported (see https://github.com/diegofrata/Generator.Equals#usage "supports properties (and fields)")

For now, as far as I can tell, everything works, if there's an edge case I missed I'll discover it later I guess :D Replacing all the existing Equals and GetHashCode implementations everywhere wasn't necessary, but it removes tons of clutter and it feels very good to see model classes become so much smaller, so thank you! I'll take a look at the 3.0 (hopefully this week) too, see if I can bring anything useful to the table!

[diegofrata]

There is a section in the README saying:

Fields are not used in comparison unless explicitly annotated. To enable the default comparison for a field, annotate it with the DefaultEquality attribute.

I will try to make it more obvious. But we can't pick up fields implicitly due to a lot of issues -- it has to be opt-in.

I will try to get a pre-release of 3.0 this weekend! Thanks!