protobuf icon indicating copy to clipboard operation
protobuf copied to clipboard

Extend the ability of customizing equality comparison for C#

Open imaxct opened this issue 1 year ago • 5 comments

User cannot customize the equality comparison logics since they are hard-coded in auto-generated C# objects. This PR puts default logics of GetHashCode and Equals methods into an IEqualityComparer class and calls the methods in comparer in the original methods. This allows users to implement their own comparison logics.

Before

public partial seal class IdentityData : IMessage<IdentityData> {
 public bool Equals(IdentityData) {
  ...
 }
 public int GetHashCode() {
  ...
 }
}

After

public partial seal class IdentityData : IMessage<IdentityData> {
  private IEqualityComparer<IdentityData> Comparer { get; set; } = new DefaultIdentityDataComparer();

  public bool Equals(IdentityData obj) {
   this.Comparer.Equals(this, obj);
  }
  public int GetHashCode() {
   this.Comparer. GetHashCode(this);
  }

  private class DefaultIdentityDataComparer : IEqualityComparer<IdentityData> {
    public bool Equals(IdentityData) {
    ...
    }
    public int GetHashCode() {
    ...
    } 
  }

}

imaxct avatar Aug 03 '24 03:08 imaxct

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

google-cla[bot] avatar Aug 03 '24 03:08 google-cla[bot]

I'm not convinced that the benefit of this outweighs the additional complexity. Rather than changing the default equality comparer via a partial class, I'd personally just implement a separate IEqualityComparer<T>. I realize that when the one proto you want to have special equality behavior is at the bottom of a long chain of other protos, that means more work, but I think it's still a conceptually-simpler approach than this. (And it means we don't need an extra reference in every single message instance ever created. That's quite an overhead for very small messages.)

jskeet avatar Aug 05 '24 07:08 jskeet

Thank you Jon , I see your concerns. The scenario is that our system, which is a long-history system and has tons of data objects, were using WCF service and then migrated to gRPC. The shared data objects between WCF and gRPC have customized Equals/GetHashCode methods to do the comparison. And some of them are used as key of maps, it's really hard to simply replace them with IEqualityComparer<T> because some of them depend on the Equals/GetHashCode of IEquatable<T> which is the base interface of IMessage. So I was thinking add an option in csharp_opt to let user decide whether use the customized comparer, does this make sense to you? @jskeet

imaxct avatar Aug 05 '24 18:08 imaxct

We try to use command-line options very rarely... and even though that would remove the extra code for most customers, it would still keep the complexity in the protoc plugin.

Imagine if we had 50 customers all with a feature that only they wanted - protoc would get harder and harder to maintain.

I'm afraid that unless we see more people need this functionality, I'd personally really prefer not to put it in.

You could have a fork of protoc which includes this - that's not ideal, of course.

If you could work out a way of using EqualityComparer, you could write a protoc plugin to generate equality comparers for messages (with partial classes designed for overriding potentially just bits of it). It's not clear from your description where you're using the messages as keys in maps - most dictionary implementations allow equality comparers to be specified.

Or as a third, grotty-hack approach, you could write code to transform the "vanilla" generated code into your preferred code. With the predictability of the generated code, that wouldn't be too hard - although you'd need to test it with new versions, of course. Definitely a "hold your nose" approach, but it may be the most practical.

jskeet avatar Aug 06 '24 06:08 jskeet

Thanks Jon, I think the third one is feasible, I will try it.

imaxct avatar Aug 06 '24 17:08 imaxct