dotnet / dotNext

Next generation API for .NET
https://dotnet.github.io/dotNext/
MIT License
1.64k stars 123 forks source link

Do not deprecate EqualityComparerBuilder #126

Closed zvrba closed 2 years ago

zvrba commented 2 years ago

Records are not a fully-featured replacement for EqualityComparerBuilder because the auto-generated equality for records is "all or nothing". If I want to use only a subset of the properties for checking equality, I have to write all of the equality boilerplate myself for the record type. For just that reason, I recently added DotNext dependency to a project and converted a record to an ordinary class with equality generated by ECB, which offers an easy way to ignore some properties.

Maybe this could be solved by using records and inheritance, but just thinking about it a bit causes me headache. (Would that be a use-case for the almost-undocumented EqualityContract?)

sakno commented 2 years ago

Hi @zvrba , thanks for your feedback about this change in the latest version. I understand your concerns, but record doesn't behave in a way you described. Let's take a look at the following example:

public record class C {
    private int value3;

    public int Value {get; set;}

    public int Value2 => Value + 10;

    public int Value3
    {
        get => value3;
        set => value3 = value;
    }
}

Equals/GetHashCode are generated only for Value property because it is only auto-implemented property here. We can verify this easily using SharpLab:

[CompilerGenerated]
public virtual bool Equals(C other)
{
    if ((object)this != other)
    {
        if ((object)other != null && EqualityContract == other.EqualityContract && EqualityComparer<int>.Default.Equals(value3, other.value3))
        {
            return EqualityComparer<int>.Default.Equals(<Value>k__BackingField, other.<Value>k__BackingField);
        }
        return false;
    }
    return true;
}

[CompilerGenerated]
public override int GetHashCode()
{
    return (EqualityComparer<Type>.Default.GetHashCode(EqualityContract) * -1521134295 + EqualityComparer<int>.Default.GetHashCode(value3)) * -1521134295 + EqualityComparer<int>.Default.GetHashCode(<Value>k__BackingField);
}

Moreover, EqualityComparerBuilder is not compatible with AOT (or any other variation of .NET Runtime without dynamic code generation), while record is.

zvrba commented 2 years ago

The official documentation about records and generated equality (https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/builtin-types/record#value-equality) is so abysmal that there's no way that the behaviour you demonstrate can be inferred from it.

I simply do not know what I get with records (citation from the link: "For record types, including record struct and readonly record struct, two objects are equal if they are of the same type and store the same values." contradicts your example as Value3 are stored in the record), and therefore I'd prefer keep using ECB despite the limitations you mention.

It's somewhat ironic that docs and implementation for ECB are more exhaustive and useful than the official docs about records and their behavior.

And then, what should I do with a record if I have a property with additional logic, like Value3 in your example and still want to include it in autogenerated equality?

All of these use cases are nicely handled by ECB.

There's simply no officially documented way to make a record type use only a subset of fields for equality.

EDIT: Your observed behaviour also contradicts another page https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/proposals/csharp-9.0/records where it is stated that "For each instance field fieldN in the record type that is not inherited, the value of System.Collections.Generic.EqualityComparer<TN>.Default.Equals(fieldN, other.fieldN) where TN is the field type, and..."

IOW, I consider records to be unusable for anything but the simplest of use-cases, that of pure data holders w/o any behavior. ECB is well documented, easy to use, predictable and useful precisely because records have none of these properties. Please do not deprecate it.

sakno commented 2 years ago

@zvrba , got it. I'll remove ObsoleteAttribute in the next version, or I can offer source generator to achieve the same (this approach is fully compatible with AOT).

zvrba commented 2 years ago

Thank you!! :)

Personally, I do not care about AOT and don't get why "everyone" has suddenly become so obsessed with it. If I wanted AOT and all the limitations it imposes, I could just as well use C++. (Exaggerating a bit, but...)