google / auto

A collection of source code generators for Java.
Apache License 2.0
10.43k stars 1.2k forks source link

Annotation for methods(fields) to be excluded from equals/hashcode #131

Closed AlexKorovyansky closed 8 years ago

AlexKorovyansky commented 10 years ago

In many cases there is necessary to exclude some fields of class from comparing in equals and from generation of hashCode.

So it would be good to have annotation to mark methods to be excluded from AutoValue generated equals/hashCode.

For example:

@AutoValue
public abstract class Foo {
    @ExcludeAnnotation public abstract Bar bar();
    public abstract Baz baz();
}
rharter commented 8 years ago

This is currently (in SNAPSHOT) doable via extension. Check out Square's Redacted extension.

Redacted is all about excluding sensitive properties from toString(), but the same sort of thing (or perhaps the same extension with another feature) would work for hashCode() and equals().

kevinb9n commented 8 years ago

The following will appear in a revised version of the User Guide:

How do I... ... ignore certain properties in equals, etc.?

Suppose your value class has an extra field that shouldn't be included in equals or hashCode computations. One common reason is that it is a "cached" or derived value based on other properties. In this case, simply define the field in your abstract class directly; AutoValue will have nothing to do with it:

@AutoValue
abstract class DerivedExample {
  static DerivedExample create(String realProperty) {
    return new AutoValue_DerivedExample(realProperty);
  }

  abstract String realProperty();

  private String derivedProperty;

  String derivedProperty() {
    // non-thread-safe example
    if (derivedProperty == null) {
      derivedProperty = realProperty().toLowerCase();
    }
  }
}

On the other hand, if the value is user-specified, not derived, the solution is slightly more elaborate (but still reasonable):

@AutoValue
abstract class IgnoreExample {
  static IgnoreExample create(String normalProperty, String ignoredProperty) {
    IgnoreExample ie = new AutoValue_IgnoreExample(normalProperty);
    ie.ignoredProperty = ignoredProperty;
    return ie;
  }

  abstract String normalProperty();

  private String ignoredProperty; // sadly, it can't be `final`

  String ignoredProperty() {
    return ignoredProperty;
  }
}

Note that in both cases the field ignored for equals and hashCode is also ignored by toString; to AutoValue it simply doesn't exist.

kevinb9n commented 8 years ago

imho, this is simple and natural and preferable to using an extension.

jbgi commented 8 years ago

another solution would be to wrap the property inside a class (possibly package private) and use the following implementation of hashCode/equals for that class:

int hashCode() { return 0; }
boolean equals(Object o) { return true; }

and then unwrap the property in a derived accessor.

kevinb9n commented 8 years ago

@jbgi Notably, implementing those eq/hc methods is no different from what you'd get by putting @AutoValue on the new auxiliary class (since it wouldn't have any abstract methods, those are approximately the eq/hc implementations you'll get).

So it turns out basically the same as the examples I illustrated above, but with a separation into 2 classes that I'm not sure really accomplishes anything?

jbgi commented 8 years ago

@kevinb9n it address the 'problem' of the field not being final, plus it allows it to be part of the generated toString... but yeah... not much.

acorn1010 commented 8 years ago

@kevinb9n I believe @jbgi's approach with separation into 2 classes also works for AutoValue.Builders, whereas the IgnoreExample above wouldn't. Unless I'm missing something, there's no way to repopulate these ignored fields when using toBuilder() without the 2 class separation or an extension.

eamonnmcmanus commented 8 years ago

This should work for builders:

@AutoValue
public abstract class IgnoreExample {
  public abstract String normalProperty();

  private String ignoredProperty; // sadly, it can't be `final`

  public String ignoredProperty() {
    return ignoredProperty;
  }

  public static Builder builder() {
    return new AutoValue_IgnoreExample.Builder();
  }

  public Builder toBuilder() {
    Builder builder = autoToBuilder();
    builder.ignoredProperty = this.ignoredProperty;
    return builder;
  }

  abstract Builder autoToBuilder();

  @AutoValue.Builder
  public abstract static class Builder {
    private String ignoredProperty;

    public abstract Builder normalProperty(String s);

    public Builder ignoredProperty(String s) {
      this.ignoredProperty = s;
      return this;
    }

    public IgnoreExample build() {
      IgnoreExample x = autoBuild();
      x.ignoredProperty = this.ignoredProperty;
      return x;
    }

    abstract IgnoreExample autoBuild();
  }
}

It's a fair amount of noise, but then the wrapper-class approach has its own share of noise too.

The key property being exploited here is that the toBuilder() method in the @AutoValue class and the build() method in the @AutoValue.Builder class are identified based on being abstract and having the right return type. Their names are only conventional, so we can use this dodge of having a package-private abstract method (autoToBuilder() and autoBuild()) and a public method that wraps it.

ZakTaccardi commented 8 years ago

@rharter yes, something like @IgnoreHashEquals would make for a great AutoValue extension and be an ideal solution to this problem. The current recommended solution is certainly too verbose.

kevinmost commented 8 years ago

Hi,

I stumbled upon this thread a few days ago and thought that this would be a useful extension to have, and a good way for me to get my feet wet with AutoValue extensions. So I created this project. Lots of inspiration taken from the existing AutoValue extension projects out there.

I definitely need to write more robust tests, but it seems to be working well for me where I needed it. I would like to do more with it in the future (for example, maybe add a @UseForHashCodeEquals annotation so that people can create whitelists instead of blacklists, and maybe a @CustomHashCodeEquals annotation that gets applied to the class itself and takes a String[] of the types to blacklist or whitelist).

I'm currently just building it with Jitpack, which is working well enough for me.

I'd appreciate any contributions or feedback!

REggar commented 8 years ago

@kevinmost Annoyingly I spent some time this week creating an extension for similar reasons to you. I've published it anyway to: https://github.com/REggar/auto-value-ignore-hash-equals

Perhaps, there's something that can be learnt from both libraries...

kevinmost commented 8 years ago

@REggar haha, our libraries are really similar... I used Kotlin for mine, but aside from language idioms, they're pretty much doing the same thing!

PhilippWendler commented 7 years ago

May I ask for reconsidering the inclusion of this feature in AutoValue (regardless of whether as direct inclusion or as a bundled extension, because as a user I won't notice any difference)?

@Memoized has been added to AutoValue, and from my point of view both features are pretty similar. In our (large) project, we certainly have more need of ignoring specific fields in equals() instead of memoizing them, and I would even assume this to be more common in general.

If the decision stays as it is, could you maybe explain why @Memoized was included and this not?

In my case, I have a large hierarchy of data classes, which I would very much like to implement using AutoValue. However, all of these classes share one field that must not be part of equals(). Any solution that requires code to be added to every @AutoValue class is too much effort for me because of the large number of involved classes. I could maybe implement something using the wrapper-class approach, but this doubles the number of involved objects, which would be nice to avoid, and I am not sure whether this can be done in an user-invisible way including builders etc.

Of course I could also use one of the two presented third-party extensions, however, I am skeptical whether I should do so. These extensions need to duplicate the logic for generating hashCode() and equals() that AutoValue has, so I would not benefit from any potential improvements in that regard in AutoValue (e.g., if AutoValue implements #93, would this also work if I use on the extensions?). Furthermore, I don't see any guarantee that the hashCode value is calculated the same way by the code from the extensions than by the code from AutoValue, and I don't know if the extensions could even guarantee this (because this would require a guarantee to keep the calculation constant in the future from AutoValue). While I do not need the guarantee that the hashCode calculation stays constant across AutoValue versions, it would certainly be nice to have the hashCode not needlessly change when extending @AutoValue class A { String s1(); } to @AutoValue class A { String s1(); @IgnoreHashEquals String s2(); }.

Or maybe one could even say that the fact that the extensions need to duplicate code present in AutoValue (for generating hashCode and equals) is a sign that the current extension API is not a good fit for this feature, and implementing it directly in AutoValue or in a bundled extension that can reuse code from AutoValue without duplicating it is worth it? (I would still think that implementing this feature directly in AutoValue requires much less code than if it is in an extension, but of course you could still use an extension.)

eamonnmcmanus commented 7 years ago

Well, right now there is a solution for your problem thanks to @kevinmost and @REggar. I agree that it is not as simple or as reliable as a native support within AutoValue itself. But I'm not seeing an actual problem other than bigger generated code. We don't have any plans to change the semantics of the equals and hashCode methods. In #93, we rejected the idea of using xor for Map.Entry classes and what's suggested there is that we would simply refuse to generate a hashCode for them.

The downside of implementing this is that it would make the specification for @AutoValue classes that bit more complicated. It's pretty simple at the moment (though the same can't be said for builders).

You do have a point about @Memoized. Since its extension always applies, @Memoized is always available, so it is effectively part of the core specification.

rharter commented 7 years ago

While I understand some of the concerns here, this is the type of thing the extensions spec was created for. If you check out auto-value-redacted it implements this exact thing for toString(), so it should be quite easy to do the same for equals() and hashcode().

IMHO, it would be preferable to keep the core of AutoValue as simple as possible and move all optional, customizable functionality to extensions (even builders?). In that regard, it does seem weird to me that things like @Memoized are bundled, as aside from that being a first party extension, there's really no reason to bundle it.

eamonnmcmanus commented 7 years ago

@PhilippWendler could you say a bit more about the extra field you have with every class? It might help to think about the problem with a more concrete example.

kevinmost commented 7 years ago

It might help us, as extension writers, for the AutoValue API to expose some kind of way to get back what the stock hashCode and stock equals calculations for given fields would be? Right now, my extension recreates the entire hashCode and equals implementations based on how I know AutoValue already does it, but if this changes in the future, I'll have to update my extension.

rharter commented 7 years ago

@kevinmost I don't know that that's really important. AFAIK there's no current guarantee that the implementation will remain constant between versions, and it shouldn't matter, as that's an implementation detail.

As long as your extension generates a valid hashCode and equals that meets the requirements of the Java APIs, then it shouldn't matter what the implementation is, as implementation isn't guaranteed in any case.

PhilippWendler commented 7 years ago

@eamonnmcmanus Sure, though I don't think it will help much in this discussion. I have a tree of classes that are used to represent abstract syntax trees (different operators etc.), and in addition to their typical field each has a field with meta information (file name, line number) that is not relevant when comparing two ASTs for equality.