False positive in `ReferenceEquality` check?

[msridhar]

To reproduce, clone https://github.com/uber/NullAway, check out the ep-2.50.0 branch, and run ./gradlew :nullaway:compileJava. One of the reported warnings is:

nullaway/src/main/java/com/uber/nullaway/NullAway.java:1179: warning: [ReferenceEquality] Comparison using reference equality instead of value equality
    if (description != Description.NO_MATCH) {

The ReferenceEquality docs say the check should only warn for types that override equals() or inherit equals() from a type other than java.lang.Object. But neither seems to be true for Description:

https://github.com/google/error-prone/blob/4525e7919cbb578e92eb81d86e615424524f2efb/check_api/src/main/java/com/google/errorprone/matchers/Description.java

Is this warning intended?

[cushon]

The scope of ReferenceEquality was deliberately expanded in a116aaf, but it's helpful to get feedback on real world examples where the findings weren't valuable.

For Description.NO_MATCH it's now matching because Description is non-final, maybe it should be. There aren't any subclasses of it internally, I'm not sure if any users of Error Prone would be affected by that.

There was also some more discussion about adjusting the heuristics in the check for sentinel comparisons, although using reference equality for sentinel comparisons isn't necessary correct.

[msridhar]

Ah, ok, interesting. Maybe the docs should be updated about the additional case of reference equality on non-final classes? I think with that, the expansion of the check is probably fine. For the case above, I can switch to using equals() to be safe.

[cpovirk]

Yes, thanks for the report. I am planning to:

• Fix the docs to describe the new behavior.
• Propose adding final to Error Prone classes with only private constructors (including Description) wherever I can get away with it.

Incidentally, on that latter front: I got to wondering whether we might be able to recognize automatically that a class with only private constructors doesn't override equals (as long as it's not extended by any classes in the file). In Turbine terms, that would mean "a class that appears to have no constructors," since the private ones get stripped :) But that leads me to another point, which is that Turbine nowadays strips private classes where possible. As a result, we wouldn't be able to see whether there are any private classes that override equals, so my plan doesn't work.

OK, maybe we could fall back to recognizing only cases in which we have source code (or maybe even cases in which we can tell that private members are available in the class file). But I worry about that, since it would result in different behavior if a compilation is run to rebuild only some files or if compilation is otherwise sharded. The safe move would be to apply that heuristic only to classes declared in the current file. I might try that out....

Also, as background: Ironically, our goal with the recent changes is to make == a more viable option: If we can say that Error Prone produces a warning for any == usage that isn't provably equivalent to equals, then we can tell people to use == without worry. Unfortunately, that leads to warnings like this one.