SteveDunn / Vogen

A semi-opinionated library which is a source generator and a code analyser. It Source generates Value Objects
Apache License 2.0
782 stars 46 forks source link

Vogen 3.0.21 -> 3.0.22 - Compilation Error on Custom Equality #516

Closed cmeyertons closed 10 months ago

cmeyertons commented 10 months ago

Describe the bug

We have a Value Object that implements its own Equals and GetHashCode and it fails to compile when upgrading from 3.0.21 -> 3.0.22

Steps to reproduce

[ValueObject<string>(Conversions.Default | Conversions.NewtonsoftJson)]
public readonly partial record struct AttributeId
{
    public readonly bool Equals(AttributeId other)
    {
        // custom code
        return Value == other.Value;
    }

    public override int GetHashCode()
    {
        // custom code
        return 0;
    }
}

Error message: image

Expected behaviour

For a patch version release, I would not expect this kind of breakage.

SteveDunn commented 10 months ago

Apologies @cmeyertons - I completely missed this scenario. I'll fix ASAP.

SteveDunn commented 10 months ago

The 2nd beta is there now on NuGet: https://www.nuget.org/packages/Vogen/3.0.23-beta.2

cmeyertons commented 10 months ago

Apologies @cmeyertons - I completely missed this scenario. I'll fix ASAP.

No worries at all! Appreciate the quick fix, no rush on my end. Just like to stay up-to-date

SteveDunn commented 10 months ago

Sorry @cmeyertons - the comment above was meant for another issue.

I'm taking a look at this issue now. Previously, for records, GetHashCode and Equals were not generated because this was auto-generated as part of the record.

But there was a feature request to support case-insensitive comparison and as part of that, I made the change to bring records more in line with classes and structs.

It seems reasonable to allow users to provide their own GetHashCode, so the change I just made skips the generation of this.

Regarding Equals: May I ask why you override Equals? The latest version of Vogen now generates Equals for everything, including records (hence your issue). In addition, it now generates an Equals that takes an IEqualityComparer<T> where T is the underlying type.

This causes a problem for Vogen if users have supplied their own Equals method(s). It could check for user supplied instances, but I originally imagined that equality would be something that wouldn't require an overload from the user (aside from case-insensitivity which has now been done).

I'm wondering, before I start the work on checking for overloaded Equals methods (and probably the == and != operators too (😱 ) - if the generated Equals that takes an IEqualityComparer is enough of an extension point for users.

SteveDunn commented 10 months ago

Thanks for the report @cmeyertons - this is now fixed in 3.0.23 - apologies for the mess up. I've documented what can be overridden here: https://github.com/SteveDunn/Vogen/wiki/Overriding-methods