Add new ReferenceComparison check

[jtraglia]

Opening as a draft for initial feedback, in case this isn't something we want.

There are about two dozen false positives in Teku that are hard to ignore. Thoughts on these? There's a couple that may be actual issues (validatorSyncCommitteeIndices & SimpleBranchNode#toString), but they don't seem that important.

Findings in Teku

/Users/jtraglia/Projects/jtraglia/teku/infrastructure/json/src/main/java/tech/pegasys/teku/infrastructure/json/types/SerializableOneOfTypeDefinition.java:57: Note: [ReferenceComparison] Reference comparison should be value comparison
      if (current == type) {
                  ^
/Users/jtraglia/Projects/jtraglia/teku/infrastructure/async/src/main/java/tech/pegasys/teku/infrastructure/async/SafeFuture.java:237: Note: [ReferenceComparison] Reference comparison should be value comparison
                      if (completionException.getCause() != error) {
                                                         ^
/Users/jtraglia/Projects/jtraglia/teku/infrastructure/ssz/src/main/java/tech/pegasys/teku/infrastructure/ssz/schema/impl/SszUnionSchemaImpl.java:82: Note: [ReferenceComparison] Reference comparison should be value comparison
            .allMatch(schema -> schema != SszPrimitiveSchemas.NONE_SCHEMA),
                                       ^
/Users/jtraglia/Projects/jtraglia/teku/infrastructure/ssz/src/main/java/tech/pegasys/teku/infrastructure/ssz/tree/SimpleBranchNode.java:81: Note: [ReferenceComparison] Reference comparison should be value comparison
    return left == right ? ("(2x " + left + ")") : ("(" + left + ", " + right + ')');
                ^
/Users/jtraglia/Projects/jtraglia/teku/infrastructure/ssz/src/main/java/tech/pegasys/teku/infrastructure/ssz/schema/impl/AbstractSszVectorSchema.java:170: Note: [ReferenceComparison] Reference comparison should be value comparison
    if (getElementSchema() == SszPrimitiveSchemas.BIT_SCHEMA) {
                           ^
/Users/jtraglia/Projects/jtraglia/teku/infrastructure/ssz/src/main/java/tech/pegasys/teku/infrastructure/ssz/schema/impl/AbstractSszListSchema.java:126: Note: [ReferenceComparison] Reference comparison should be value comparison
    if (getElementSchema() == SszPrimitiveSchemas.BIT_SCHEMA) {
                           ^
/Users/jtraglia/Projects/jtraglia/teku/infrastructure/ssz/src/main/java/tech/pegasys/teku/infrastructure/ssz/schema/impl/AbstractSszListSchema.java:137: Note: [ReferenceComparison] Reference comparison should be value comparison
    if (getElementSchema() == SszPrimitiveSchemas.BIT_SCHEMA) {
                           ^
/Users/jtraglia/Projects/jtraglia/teku/infrastructure/ssz/src/main/java/tech/pegasys/teku/infrastructure/ssz/schema/impl/AbstractSszListSchema.java:316: Note: [ReferenceComparison] Reference comparison should be value comparison
        .addBits(elementSchema == SszPrimitiveSchemas.BIT_SCHEMA ? 1 : 0)
                               ^
/Users/jtraglia/Projects/jtraglia/teku/infrastructure/ssz/src/main/java/tech/pegasys/teku/infrastructure/ssz/schema/collections/SszPrimitiveListSchema.java:41: Note: [ReferenceComparison] Reference comparison should be value comparison
    if (elementSchema == SszPrimitiveSchemas.BIT_SCHEMA) {
                      ^
/Users/jtraglia/Projects/jtraglia/teku/infrastructure/ssz/src/main/java/tech/pegasys/teku/infrastructure/ssz/schema/collections/SszPrimitiveListSchema.java:43: Note: [ReferenceComparison] Reference comparison should be value comparison
    } else if (elementSchema == SszPrimitiveSchemas.UINT64_SCHEMA) {
                             ^
/Users/jtraglia/Projects/jtraglia/teku/infrastructure/ssz/src/main/java/tech/pegasys/teku/infrastructure/ssz/schema/collections/SszPrimitiveListSchema.java:45: Note: [ReferenceComparison] Reference comparison should be value comparison
    } else if (elementSchema == SszPrimitiveSchemas.BYTE_SCHEMA) {
                             ^
/Users/jtraglia/Projects/jtraglia/teku/infrastructure/ssz/src/main/java/tech/pegasys/teku/infrastructure/ssz/schema/collections/SszPrimitiveListSchema.java:47: Note: [ReferenceComparison] Reference comparison should be value comparison
    } else if (elementSchema == SszPrimitiveSchemas.UINT8_SCHEMA) {
                             ^
/Users/jtraglia/Projects/jtraglia/teku/infrastructure/ssz/src/main/java/tech/pegasys/teku/infrastructure/ssz/schema/collections/impl/SszByteListSchemaImpl.java:41: Note: [ReferenceComparison] Reference comparison should be value comparison
        elementSchema == SszPrimitiveSchemas.BYTE_SCHEMA
                      ^
/Users/jtraglia/Projects/jtraglia/teku/infrastructure/ssz/src/main/java/tech/pegasys/teku/infrastructure/ssz/schema/collections/SszPrimitiveVectorSchema.java:41: Note: [ReferenceComparison] Reference comparison should be value comparison
    if (elementSchema == SszPrimitiveSchemas.BIT_SCHEMA) {
                      ^
/Users/jtraglia/Projects/jtraglia/teku/infrastructure/ssz/src/main/java/tech/pegasys/teku/infrastructure/ssz/schema/collections/SszPrimitiveVectorSchema.java:43: Note: [ReferenceComparison] Reference comparison should be value comparison
    } else if (elementSchema == SszPrimitiveSchemas.BYTE_SCHEMA) {
                             ^
/Users/jtraglia/Projects/jtraglia/teku/infrastructure/ssz/src/main/java/tech/pegasys/teku/infrastructure/ssz/schema/collections/SszPrimitiveVectorSchema.java:46: Note: [ReferenceComparison] Reference comparison should be value comparison
    } else if (elementSchema == SszPrimitiveSchemas.UINT8_SCHEMA) {
                             ^
/Users/jtraglia/Projects/jtraglia/teku/infrastructure/ssz/src/main/java/tech/pegasys/teku/infrastructure/ssz/schema/collections/SszPrimitiveVectorSchema.java:49: Note: [ReferenceComparison] Reference comparison should be value comparison
    } else if (elementSchema == SszPrimitiveSchemas.BYTES32_SCHEMA) {
                             ^
/Users/jtraglia/Projects/jtraglia/teku/infrastructure/ssz/src/main/java/tech/pegasys/teku/infrastructure/ssz/schema/collections/impl/SszByteVectorSchemaImpl.java:43: Note: [ReferenceComparison] Reference comparison should be value comparison
    return getElementSchema() == SszPrimitiveSchemas.BYTE_SCHEMA
                              ^
/Users/jtraglia/Projects/jtraglia/teku/infrastructure/ssz/src/testFixtures/java/tech/pegasys/teku/infrastructure/ssz/RandomSszDataGenerator.java:78: Note: [ReferenceComparison] Reference comparison should be value comparison
      if (schema == SszPrimitiveSchemas.NONE_SCHEMA) {
                 ^
/Users/jtraglia/Projects/jtraglia/teku/infrastructure/ssz/src/testFixtures/java/tech/pegasys/teku/infrastructure/ssz/RandomSszDataGenerator.java:80: Note: [ReferenceComparison] Reference comparison should be value comparison
      } else if (schema == SszPrimitiveSchemas.BIT_SCHEMA) {
                        ^
/Users/jtraglia/Projects/jtraglia/teku/infrastructure/ssz/src/testFixtures/java/tech/pegasys/teku/infrastructure/ssz/RandomSszDataGenerator.java:82: Note: [ReferenceComparison] Reference comparison should be value comparison
      } else if (schema == SszPrimitiveSchemas.BYTE_SCHEMA) {
                        ^
/Users/jtraglia/Projects/jtraglia/teku/infrastructure/ssz/src/testFixtures/java/tech/pegasys/teku/infrastructure/ssz/RandomSszDataGenerator.java:84: Note: [ReferenceComparison] Reference comparison should be value comparison
      } else if (schema == SszPrimitiveSchemas.UINT64_SCHEMA) {
                        ^
/Users/jtraglia/Projects/jtraglia/teku/infrastructure/ssz/src/testFixtures/java/tech/pegasys/teku/infrastructure/ssz/RandomSszDataGenerator.java:86: Note: [ReferenceComparison] Reference comparison should be value comparison
      } else if (schema == SszPrimitiveSchemas.UINT256_SCHEMA) {
                        ^
/Users/jtraglia/Projects/jtraglia/teku/infrastructure/ssz/src/testFixtures/java/tech/pegasys/teku/infrastructure/ssz/RandomSszDataGenerator.java:88: Note: [ReferenceComparison] Reference comparison should be value comparison
      } else if (schema == SszPrimitiveSchemas.BYTES4_SCHEMA) {
                        ^
/Users/jtraglia/Projects/jtraglia/teku/infrastructure/ssz/src/testFixtures/java/tech/pegasys/teku/infrastructure/ssz/RandomSszDataGenerator.java:90: Note: [ReferenceComparison] Reference comparison should be value comparison
      } else if (schema == SszPrimitiveSchemas.BYTES32_SCHEMA) {
                        ^
/Users/jtraglia/Projects/jtraglia/teku/infrastructure/ssz/src/test/java/tech/pegasys/teku/infrastructure/ssz/SszListTestBase.java:33: Note: [ReferenceComparison] Reference comparison should be value comparison
    Assumptions.assumeTrue(data.getSchema().getElementSchema() != SszPrimitiveSchemas.BIT_SCHEMA);
                                                               ^
/Users/jtraglia/Projects/jtraglia/teku/ethereum/spec/src/main/java/tech/pegasys/teku/spec/datastructures/state/beaconstate/common/BeaconStateInvariants.java:80: Note: [ReferenceComparison] Reference comparison should be value comparison
    if (state == obj) {
              ^
/Users/jtraglia/Projects/jtraglia/teku/validator/api/src/main/java/tech/pegasys/teku/validator/api/SyncCommitteeDuty.java:58: Note: [ReferenceComparison] Reference comparison should be value comparison
        && validatorSyncCommitteeIndices == that.validatorSyncCommitteeIndices
                                         ^
/Users/jtraglia/Projects/jtraglia/teku/ethereum/spec/src/test/java/tech/pegasys/teku/spec/datastructures/state/beaconstate/common/AbstractBeaconStateTest.java:79: Note: [ReferenceComparison] Reference comparison should be value comparison
    assertThat(state == stateCopy).isFalse();
                     ^
/Users/jtraglia/Projects/jtraglia/teku/ethereum/spec/src/test/java/tech/pegasys/teku/spec/datastructures/state/beaconstate/common/AbstractBeaconStateTest.java:90: Note: [ReferenceComparison] Reference comparison should be value comparison
    assertThat(state == stateCopy).isFalse();

This work is associated with this PR:

• Use value comparison for validator sync committee indices teku#6457

[ajsutton]

Yeah I was just looking through these using IntelliJ's inspection. There are definitely a few interesting cases here.

The assertThat(state == stateCopy).isFalse() should just be assertThat(state).isNotSameAs(stateCopy);

The SszPrimitiveSchemas ones are more interesting because == is definitely safe to use because they're all defined, slightly oddly, as anonymous inner classes and only one instance is ever going to exist for them. May need to investigate which benchmarks cover that code to see if there is actually a performance benefit.

The ones in the type definitions like SerializableOneOfTypeDefinition are an interesting case. Originally the plan was to expect that only one instance of the type definition existed and so use reference equality deliberately. That hasn't panned out quite as planned though. That said I'm not sure they have a meaningful .equals method so it's probably always using reference equality in practice anyway.

The SimpleBranchNode.toString case I think is right to use reference equality - we don't want to trigger calculating the hash tree root for the comparison and it's just to make toString() more readable and particularly shorten the case where the child is the zero node which is always the same instance.

[jtraglia]

Thanks for taking to time to look at those. Is there an IntelliJ inspection check for this? I cannot find it. But if there is, I think that would be preferred over an error-prone check. If not, what's your opinion on this new check? There aren't that many false positives, but I think it does provide some value and it's not that hard to suppress false positives.

[ajsutton]

Yeah the IntelliJ inspection is Object comparison using '==' instead of 'equals()'. Interestingly it was disabled in mine so I've reconfigured it to report as an error now.

I actually think the error prone check is the better approach though because we don't control what inspections people have enabled for IntelliJ and you don't always see them anyway, whereas errorprone is enforced in CI. It's not hard to add a suppression in the very limited cases we really do want to use reference equality.

[jtraglia]

Oh neat. Alright, cool. I'll mark this as ready for review then. I can add more tests if you think it's necessary.

[ajsutton]

LGTM. Sorry for the delay - got caught up with some other issues Friday.

[jtraglia]

No worries! It's all good.

[ajsutton]

Pushed out a 1.1.1 release with this btw.