Introduce @ReferenceEqualitySafe class annotation to avoid triggering ReferenceEquality check

[tereigo]

Sometimes there are Java classes which are safe to compare using '==' operator, like Java enums. It can be the special custom made Enum classes for instance or some other classes which instances are controlled and managed to ensure there is only one instance for a given value.

Currently errorprone raises ReferenceEquality warning for such classes because they are Java Objects after all.

It would be nice to be able to mark such classes with @ReferenceEqualitySafe annotation so that errorprone could ignore the comparison of such objects via '==' operator

Example:

public final class ObjectId {
    public static final ObjectId ID1 = new ObjectId(1);
    public static final ObjectId ID2 = new ObjectId(2);

    private final int value;

    private ObjectId(int v) {
        this.value = v;
   }

    public static ObjectId valueOf(int v) {
       switch (v) {
        case 1: 
            return ID1;
        case 2: 
            return ID2;
        }
       throw new IllegalArgumentException();      
    }
}

ObjectId id1 = ObjectId.ID1;
boolean b = id1 == ObjectId.ID2;

[graememorgan]

Internally at least, we tend to prefer using equals even for cases where reference equality is safe, because equals is more self-evidently correct.

Is there a reason to actively prefer using reference equality?

[tereigo]

A couple of things I can think of:

1. We migrated some widely used Java enums to this kind of implementation and so now have to update a lot of places where these reference comparisons are done although it doesn't bring any value because the semantics is still the same.
2. Comparing 2 references is not like-for-like with "obj1.equals(obj2)" since it doesn't account for obj1 being null. So to be equivalent it should be smth like: "(obj1 == null && obj2 == null) || (obj1 != null && obj1.equals(obj2)" which looks rather cumbersome.
3. And comparing such classes with == looks simpler and feels more natural than via 'equals'

After all it's up to the code owner to mark the classes with the proposed tag because he/she knows the usage semantics of it

[cushon]

We try to be fairly conservative about adding new annotations to Error Prone. If we don't add this, and you want to get some of the benefits of ReferenceEquality with a way to suppress it for these classes, one alternative would be to define your own annotation and creating a plugin check similar to ReferenceEquality.

We migrated some widely used Java enums to this kind of implementation

Can you share any details about why you needed to migrate away from enums for this? That sounds like it may be a somewhat uncommon use-case.

(obj1 == null && obj2 == null) || (obj1 != null && obj1.equals(obj2)

I'd recommend java.util.Objects.equals for null-tolerant equals tests

[tereigo]

There might be different reasons to introduce custom-made classes which should have 1 instance per value, not necessarily Enums I guess. In our case we need to provide special wire serialization of such enums into binary and allow them to be dynamic enums meaning that enum values can be added at runtime. But the semantics and usage of such objects are still enum-like.

[graememorgan]

I think it's totally reasonable to introduce custom-made classes with exactly 1 instance per value, but is there a strong reason to prefer == over equals (be that a.equals(b) or equals(a, b) for null tolerance) for those?

[tereigo]

no big reason apart from having to update a lot of code but not because the new code will be cleaner/safer but just to satisfy the code validation tool