Generate IEquatable.Equals, object.Equals, object.GetHashCode implementations

[fl3pp]

Implementation of Issue #19

Work in progress

[fl3pp]

Equals (IEquotable & object) implemented, not tested yet
GetHashCode implemented, not tested yet

The whole branch needs some refactoring.
All implementations used Visual Studio generations as template.

@amis92
Im not sure wether I should also implement the ToString() method. I think at this point we really require some flags.
I am also not sure wether using the System.Collections.Generics.EqualityComparer is enough for the GetHashCode and Equals implementations. I'm working against Visual Studio here because VS sometimes uses operators (I didn't yet figure out when exactly)

[amis92]

Ad. ToString - Let's leave it out. It's a convenience and it's independent.

---

So, in regards to EqualityComparer vs == - I think it's mostly to do with whether the type is a well-known type or not.

Honestly, I've spent a few hours today researching equality subject (again!).

My findings:

• Champion "Support for == and != on tuple types" (15.7)
• How to: define value equality for a type (C# Programming Guide)
• Champion "Records"

A random summary of my thoughts:

• Equals is damn complicated in a generic I-don't-know-what-it-is case
• For Equals, we should probably only use == for a couple of well-known types, like integers, bool, string (and their nullables) that are known to have the same behavior of Equals and == (e.g. double doesn't: double.NaN == double.NaN is false, while the same with Equals is true - I'd expect us to use the Equals/EqualityComparer).
• For the first iteration we should probably require the class to be sealed and not derived in order to have equalities generated. Inheritance is crazy.
• For the ==/!= operators, we could use the ValueTuple compiler trick, e.g. return the result of the (left.a, left.b, left.c) == (right.a, right.b, right.c) (see the first link).
• For the Equals(object), Equals(T) and GetHashCode the current approach is good enough.
  • Optimizing EqualityComparer call into == could be done later.
  • Introducing support for inheritance could be done later, and even then probably with some restrictions.
• We definitely need a switch/flag to turn this feature on/off/customize.

[fl3pp]

Equals implementations performance test

I did a small performance test comparing different Equals implementations with a string and an integer. You can lookup the results in this gist.

I interpret the results like this: All implementations take about the same time to complete, with the exception of the EqualityComparer which is significantly slower.

My suggestion is that we head for the tuple implementation.

Inheritance in equals

Implementing Equals while respecting inheritance is almost impossible. Nevertheless, I think sealing the class is a too big restriction. Even while I know that I will seal all my classes, that doesn't mean the users will.

IMO we have two ways of finishing up the implementation:

• We leave the implementation as it is. If a class B inherits from A, it should behave in the same way as A does in regard to the inherited functionality. I think of the Liskov substitution principle here. So a.Equals(b) should return true, because b is seen as a implementation of A in this case. What is bad, is that b.Equals(a) will likely return false which can lead to confusion and unexpected behavior.
• We add a type equality check. So even though B inherits from A and has the same values on inherited properties, a.Equals(b) would fail because b is of a different type.

What are your thoughts on this?

[atifaziz]

Implementing Equals while respecting inheritance is almost impossible.

I think this says it all; better to not do it than do it wrong.

Nevertheless, I think sealing the class is a too big restriction. Even while I know that I will seal all my classes, that doesn't mean the users will.

These are records after all, which are tuples with named elements, and as such, it's understandable if they are sealed by default. One can always provide an option to not have them sealed so it's not a restriction (for hacking) but it shouldn't be encouraged by default. I would even go as far as not providing that option initially just in case YAGNI.

From a design perspective (less technical argumentation), a sealed record is not surprising but an open record that doesn't deliver on some expected behaviour across an inheritance hierarchy will lead to disappointment and pain.

My suggestion is that we head for the tuple implementation.

I think this is a good starting point that can always be improved on.

---

My 2¢ even though you never asked for them. 😉

[amis92]

First of all - I agree with @atifaziz. We really shouldn't ignore the possible implications of inheritance on Equals. First and foremost, because if we do it wrong, it'll be inexplicably hard to find what's wrong when someone uses them in dictionaries, or simply List.Contains and it doesn't work.

So, either we generate it by default but only if the record is sealed, or we require an opt-in, for which we can raise a warning/error if the class is not sealed.

It's much better to start with a highly restricted feature and widen the possible applications, than produce code which doesn't adhere to .NET's equality rules.

---

And for that reason, tuple-equality-shortcut should be used only for implementations of == operator - and not for Equals. The simplest case being a record with a double property. Two instances of the same record, which have double.NaN as value, must be Equal to each other. But, if we provide == operator, it should behave similarly to == operators of components - and as such, we can use tuple-equality form.

[amis92]

Regarding the benchmark, actually the int, string and bool (and all the integer types) together with their nullable counterparts, can always use == comparison (and probably should).

All the other types should use EqualityComparer or directly implement it's logic. Performance hit is not that considerable, although I'd be interested in similar benchmark with more value and reference types included.

[amis92]

Regarding parametrizing the RecordAttribute, I think the best way forward would be to use a [Flags] enum that'll switch on/off various features.

I'd imagine something like that:

public sealed class RecordAttribute : Attribute
{
    public RecordAttribute()
    {

    }

    public RecordAttribute(Parts parts)
    {
        Parts = parts;
    }

    public Parts Parts { get; } = Parts.Common;
}

[Flags]
public enum Parts
{
    None = 0,
    Constructor = 0b1,
    Withers = 0b10,
    Builder = 0b100,
    Deconstructor = 0b1000,
    Common = Constructor | Withers | Builder | Deconstructor,

    Equals = 0b10000,
    EqualityOperator = 0b100000,
    GetHashCode = 0b1000000,
    Equality = Equals | EqualityOperator | GetHashCode
}

What do you think?

[atifaziz]

Regarding parametrizing the RecordAttribute, I think the best way forward would be to use a [Flags] enum that'll switch on/off various features.

👍

I'd imagine something like that:

[Flags]
public enum Parts
{
    None = 0,
    Constructor = 0b1,
    Withers = 0b10,
    Builder = 0b100,
    Deconstructor = 0b1000,
    Common = Constructor | Withers | Builder | Deconstructor,

    Equals = 0b10000,
    EqualityOperator = 0b100000,
    GetHashCode = 0b1000000,
    Equality = Equals | EqualityOperator | GetHashCode
}

I would call it Features or Options rather than Parts because parts is confusing with partial. If Features/Options is too esoteric then RecordImplementation would be equally good and explicit.

Nice idea with Common but I would just have Constructor and Withers (which I'd like to suggest renaming to simply With although I am all up for creative names) in there unless you would consider adding Minimal = Constructor | Withers.

In C#, a ~ member is really called a finalizer but the documentation does say that they are also called deconstructors:

Finalizers (which are also called destructors) are used to perform any necessary final clean-up when a class instance is being collected by the garbage collector.

Consequently, I would rename Deconstructor to Deconstruct.

The equality members (Equals and GetHashCode) have to work in tandem so I wouldn't give the option to have one implemented and not the other.

I would also recommend inverting some of the flags as an overall design. You want the generator to produce the most sensible and commonly expected features (albeit opinionated?) of a record and opting out some and opting in other nice-to-haves or esoteric cases. In short, I'm thinking this:

[Flags]
public enum Features
{
    Basic = 0, // all !excluded + !included
    
    // opt-out
    
    ExcludeConstructor      = 0b0001,
    ExcludeWithers          = 0b0010,
    ExcludeEquality         = 0b0100,
    ExcludeEqualityOperator = 0b1000,
    
    // opt-in

    IncludeBuilder          = 0b0001_0000,
    IncludeDeconstruct      = 0b0010_0000,
    IncludeUpdate           = 0b0100_0000,
}

As an aside, shouldn't this design be discussed and iterated in a separate issue as opposed to as part of this PR?

[amis92]

Yes, there's actually an issue for that since long time: #45

Let's move over that discussion there.

[fl3pp]

I am not sure wether or not we should leave the equality features as an opt-in. In the end they can make a siginicant difference when working with collections and most people will probably be using the types in collections.

I can agree that the class should be sealed when implementing equality. It is a more restricted approach but it closes a risky area.

[amis92]

Let's implement this feature semi-agnostically of that [opt-in/out]. Let's pop a warning into IProgress when the class is not sealed (and just skip the generation of Equality), and implement the attribute Flags without any aggregates like Basic/Common. We'll discuss the default settings in #56.

[fl3pp]

Done some refactorings. More to come.

I would like to write some UnitTests. Can I create a UnitTest project, would you like to create a UnitTest project or would you like to stick to the IntegrationTests?

A quick summary of the things discussed above regarding the equality implementation in this first iteration:

• We will implement operator equality operations for well-known types.
• We will stick to the EqualityComparer for all other types.
• We will seal the class if equality operations are being used by the user.
• We will not merge this PR before flags have been implemented.

Please correct me if I misunderstood something.

[amis92]

I would like to write some UnitTests. Can I create a UnitTest project, would you like to create a UnitTest project or would you like to stick to the IntegrationTests?

I'll create a UnitTest project.

We will implement operator equality operations for well-known types.

We'll use == operators for welll known types. I guess that's what you meant.

We will stick to the EqualityComparer for all other types.

Yes. Although I've been reading though https://github.com/dotnet/csharplang/blob/master/proposals/records.md and below the record struct example there's another implementation that makes sense:

return other != null && Equals(Id, other.Id) && Equals(Name, other.Name)

This uses object.Equals(object, object) method

I don't know if it'll be faster than EqualityComparer, and this'll box structs could be used only for reference classes. Just a thought.

We will seal the class if equality operations are being used by the user.

I don't agree, we shouldn't seal classes ourselves when needed. Rather, if the customer requests equality (maybe by default), on a non-sealed class, we'll warn that the given class must be sealed for equality to be generated.

We will not merge this PR before flags have been implemented.

Probably. I'll do the unit tests and flags today or tomorrow.

[fl3pp]

Note: this PR is still a work in progress. I will do more refactorings, write tests, increase functionality etc.

Currently I'm implementing the sealed analyzer, altough I won't be able to finish it before options are available.

best regards

[amis92]

Sealed-analysis can be done not as an Analyzer per se, but rather as a Warning Diagnostic returned via IProgress passed to the GenerateAsync method in the main RecordGenerator. We just need to define the DiagnosticDescriptor and if Equality is requested to be generated, we send the warning into the progress instance. Should work.

[amis92]

And to keep your motivation up, I'm really glad you took up this challenge. It's a complicated feature with a lot of pesky details to work out. I'm very happy we have this discussion and you're still on track to break that wall! 🚀

[amis92]

Thanks, looking good.

[amis92]

We need to finish off this PR ASAP, I feel it's getting too long.

Let's touch up cosmetics, test it manually and merge it. Then we can work on fixing up the smaller stuff, because the longer I browse through the more issues I see. We'll also be able to parallelize the work on those additional issues and/or writing tests. Can you complete this PR soon?

[fl3pp]

Hi @amis92,

I agree with you that the PR should be finished soon.

Unfortunately there are, even when ignoring refactorings and tests, 2 important changes left:

• Generate standalone ObjectEquals if EquatableEquals is not selected
• Generate operator equality

I think these two points must certainly be in a working state before releasing.

Unfortunately, today is the swiss national day so I'll be busy and I'm flying into vacation next week.

I will put the PR out of Draft, so everybody is welcome to help in order to finish this asap.

Another option would be to wait another 2 weeks before releasing, until then I'd be able to finish everything in a certain quality.

best regards

[fl3pp]

Implemented standalone Equals(object) generation

[fl3pp]

Implemented operator equals

[fl3pp]

The operator equals implementation uses (as VS does) the EqualityComparer which relies on the Equals implementations of the objects. Thus, it is recommended to implement Equals when using equality operators.

Fortunately, Roslyn already throws a warning CS0661 so we don't have to.

[fl3pp]

@amis92,
It's a bit of a mess, but if you want to push for this feature I think it's in a state ready for reviewing / manually testing now

[fl3pp]

How do you feel about creating more interfaces?

I get the feeling that I'm not able to create clean UnitTests because everything is coupled so tightly.

A starting point could be a IRecordDescriptor interface. This way we could created easier, cleaner UnitTests.