New check proposal: use of Enum.compareTo

[PeteGillin]

The EnumOrdinal check wisely says that "You should almost never invoke the Enum.ordinal() method" because it's not guaranteed to be stable. I think there should be a similar EnumCompareTo check since Enum.compareTo() compares by ordinal, and the same considerations about lack of stability apply.

[PeteGillin]

I honestly suspect that a lot of Java devs don't actually know what Enum.compareTo() does. I had to check myself. (I write this having just spotted an issue where I was working with an enum representing versions and had naively assumed that, since it implemented Comparable, its compareTo would do the obvious thing, which it didn't.)

[PeteGillin]

Oh, a final note: Enum.comapreTo() is final, so I don't think we have to worry about the possibility that someone might override it to do something sensible for their enum and then think that calling it is valid.

[cpovirk]

Pete!! Hi :)

EnumOrdinal has the backing of Effective Java (item 35 in the third edition), and while I've always found its DOUBLE_QUARTET example a little, like, "Who would do that?!" (though I'm sure someone has :)) I do think it's good to steer users away from code that might unthinkingly depend on ordinal to be stable across runs, as you mention.

I recall a past discussion or two about compareTo, where presumably the risks from instability are lower (though I assume still not zero, and there's still the risk of a DOUBLE_QUARTET scenario, even if I'm calling it small). See google/guava#909 (comment) and some code in AbstractService.

In an ideal world, maybe enums would be able to opt in or out of comparability? I suppose we could imagine declaring an annotation that you can apply to an enum to opt in or out, as seems more appropriate.

Maybe we should specifically warn about protobuf enums, given that they have their own concept of "value" that does not always align with the ordinal? Perhaps this is even a reason to add to the list of reasons that proto enums shouldn't be Java enums? I found cl/17405778 (not that you can see that :)) in a search of CLs that mention enum comparability. (I also found unrelated cl/60926695, which reorders existing constant to match the desired comparison order, rather than supply a separate compare method (which was probably supposed to be an override of compareTo, oops!).)

I'm going to collect a sample of more calls and see if enough jump out at me as scary like your version example. Of course, looking only for direct compareTo calls will not cover cases like putting enums into a TreeSet, sorting a collection of them, or otherwise treating them as comparable more indirectly.

[cpovirk]

I checked Java Practices and saw that it has advice that "If order means anything, specify it." It also points out that ordering additionally shows up in values() and in EnumSet, even though those don't refer to the Comparable type. (EnumSet: still not a SortedSet or even SequencedSet.)

I also made a failed attempt to deprecate compareTo for a given enum: I introduced public interface BadToCompare<T> extends Comparable<T> with a deprecated compareTo method, and I made an enum implement that. That resulted in a deprecation warning where the enum was defined but not a deprecation warning for usages of the enum's compareTo method.

[PeteGillin]

Hi Chris!

In addition to the stability thing and the DOUBLE_QUARTET thing, there's a readability thing: if you saw myVersion.compareTo(yourVersion) > 0, you'd probably make a very reasonable guess what it meant, but if that was defined as enum Version { V_3, V_2, V_1 } then you'd be completely wrong. I think it would be more readable to have a dedicated method so we could write myVersion.isAfter(yourVersion), and of course we can do that, but there's nothing we can do to stop callers writing the compareTo version. (I also tried and failed to deprecate compareTo for the enum in question, although I didn't think of your smart interface trick — shame it didn't work.)

For what it's worth, the project I'm working on now doesn't actually use ErrorProne, so this is very much a drive-by suggestion as I found myself wondering whether it would have caught my issue or not. I'm happy to let you folks decide on whether it makes sense or not.

[PeteGillin]

The whole thing is made worse by the fact that people find it really easy to mess up the sign when writing a.compareTo(b) > 0 (as I'm sure you know from previous attempts to provide better ergonomics around this). My initial thought when I saw my thing working the opposite way to how I'd expected was that I must have managed to fall into that trap.

[cpovirk]

I looked at 19 files (because I asked for a sample of 20 and the 20th turned out to be some kind of compiler test :)). [edit: cl/705191034, for the record]

There was a "clearly meaningful" ordering (typically documented as such, maybe always?) in 7 cases: 2 protobuf enums and 5 non-protobuf enums. One of the proto cases did seem closer to DOUBLE_QUARTET than I'm comfortable with.... And one of the non-proto cases was... a version enum! It had only three entries, along with a comment that the ordering was significant, but it still scared me because "main" becomes before "beta." But it's been that way since the beginning, so it's probably OK. And, good news / bad news: The author and reviewer discussed making the ordering explicit by adding an int parameter to the constructor, and they even went through with it... but while under the impression that they could always override compareTo to use that number in the future if they wanted!

One other user (of a protobuf enum) was using compareTo(...) == 0 as a long way to write equals. I smell some more static analysis for that, following some similar static analysis we already have for some other types, though probably internal to Google....

The other 11 (5 proto, 6 not) seemed to be OK with an arbitrary order. One had a comment that explicitly mentioned "determinism," which of course we're big fans of :) One was the "type" enum for a protobuf oneof... huh. I suspect that at least that one (and maybe others; I wasn't thinking about it) didn't actually fully discriminate values (i.e., be "consistent with equals"), so maybe that's something less than a victory for determinism? I also saw at least one that had a Map<Field, Comparator<Row>>, which I thought of as if it were mapping to what happens when you click on headers to sort a table (though it might not actually have been). Arbitrary sorting there is OK-ish (since there didn't seem to be a natural order), but that did make me see that ordering by name would likely be better. Of course, ordering by ordinal is fast, so it's got that going for it :)

I am not totally sure what conclusions to draw from all of that (other than the compareTo(...) == 0 static analysis), but it does update me at least somewhat toward seeing this as an antipattern.

[cpovirk]

Oh, and I forgot to also say that, following up on the cl/60926695 that I mentioned earlier, I saw some other gross things:

• an enum class with a compare (rather than compareTo) method. That method is never used, but I'm not sure if compareTo is used, either, and it also offers an isAfter-like method that is used.
• two projects with enum classes (several such classes, in one project) with a compareTo(SomeSupertype) method, overriding a method from a supertype that explicitly declares that signature (with the supertype somehow triggering https://errorprone.info/bugpattern/MissingImplementsComparable in one project but not the other). In both cases, the methods get used in at least one case, but of course it's hard to know if they're used consistently.

[tbroyer]

Fwiw, Gradle has a JavaVersion enum that's meant to be comparable, even though it also has an isCompatibleWith method (that just delegates to compare to). In Kotlin and Groovy it makes things very readable though: version >= JavaVersion.VERSION_11

https://github.com/gradle/gradle/blob/master/platforms/core-runtime/stdlib-java-extensions/src/main/java/org/gradle/api/JavaVersion.java

(not sure how that fit in the discussion though)

[cpovirk]

One other user (of a protobuf enum) was using compareTo(...) == 0 as a long way to write equals. I smell some more static analysis for that, following some similar static analysis we already have for some other types, though probably internal to Google....

That found some matches, though not a ton. In most cases, equals is an improvement, but there are enough case in which the code has other compareTo calls nearby (in which case it arguably makes sense to keep compareTo) that I'm not sure I'll bother enabling the check. (We already have checks for Instant and Duration (but not LocalTime or other time types, nor for BigDecimal and BigInteger, nor for Truth isEquivalentAccordingToCompareTo for essentially any type, though I do see some existing special cases for comparisons to zero. It might also make sense to look at primitive compare calls, though we'd have to be careful with floating-point values. The main type we cover is String, though I notice that we cover == 0 without also covering != 0. I may change that if I don't end up with mixed feelings like those I've ended up with for someEnum.compareTo(otherEnum).) Granted, plenty of callers who use multiple compareTo calls should be using comparing(...).thenComparing(...) nowadays.