TypeCompatibilityUtils: regard a type as incompatible with _itself_ i…

…f it ends with "Builder" and doesn't override equals.

It's tempting to just say "types that don't override equals are not self-compatible", but that obviously doesn't work because of, for example, Enums. The heuristic of "ends with Builder" is to narrow down to types that someone might reasonably think have value semantics.

Flume: unknown commit
PiperOrigin-RevId: 424309369

core/src/main/java/com/google/errorprone/bugpatterns/EqualsIncompatibleType.java

@@ -29,6 +29,7 @@
 import static com.google.errorprone.util.ASTHelpers.getType;
 
 import com.google.errorprone.BugPattern;
+import com.google.errorprone.ErrorProneFlags;
 import com.google.errorprone.VisitorState;
 import com.google.errorprone.bugpatterns.BugChecker.MemberReferenceTreeMatcher;
 import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher;
@@ -60,6 +61,12 @@ public class EqualsIncompatibleType extends BugChecker
               instanceMethod().anyClass().named("assertFalse"),
               staticMethod().anyClass().named("assertFalse")));
 
+  private final TypeCompatibilityUtils typeCompatibilityUtils;
+
+  public EqualsIncompatibleType(ErrorProneFlags flags) {
+    this.typeCompatibilityUtils = TypeCompatibilityUtils.fromFlags(flags);
+  }
+
   @Override
   public Description matchMethodInvocation(
       MethodInvocationTree invocationTree, final VisitorState state) {
@@ -112,7 +119,7 @@ public Description matchMemberReference(MemberReferenceTree tree, VisitorState s
   private Description handle(
       ExpressionTree invocationTree, Type receiverType, Type argumentType, VisitorState state) {
     TypeCompatibilityReport compatibilityReport =
-        TypeCompatibilityUtils.compatibilityOfTypes(receiverType, argumentType, state);
+        typeCompatibilityUtils.compatibilityOfTypes(receiverType, argumentType, state);
     if (compatibilityReport.isCompatible()) {
       return NO_MATCH;
     }
@@ -134,12 +141,13 @@ private Description handle(
     return buildDescription(invocationTree)
         .setMessage(
             getMessage(
-                invocationTree,
-                receiverType,
-                argumentType,
-                compatibilityReport.lhs(),
-                compatibilityReport.rhs(),
-                state))
+                    invocationTree,
+                    receiverType,
+                    argumentType,
+                    compatibilityReport.lhs(),
+                    compatibilityReport.rhs(),
+                    state)
+                + compatibilityReport.extraReason())
         .build();
   }
 

core/src/main/java/com/google/errorprone/bugpatterns/TypeCompatibilityUtils.java

@@ -16,14 +16,17 @@
 
 package com.google.errorprone.bugpatterns;
 
+import static com.google.common.collect.Iterables.isEmpty;
 import static com.google.errorprone.util.ASTHelpers.findMatchingMethods;
 import static com.google.errorprone.util.ASTHelpers.getUpperBound;
 import static com.google.errorprone.util.ASTHelpers.isCastable;
 import static com.google.errorprone.util.ASTHelpers.isSameType;
 import static com.google.errorprone.util.ASTHelpers.isSubtype;
+import static com.google.errorprone.util.ASTHelpers.scope;
 
 import com.google.auto.value.AutoValue;
 import com.google.common.collect.Streams;
+import com.google.errorprone.ErrorProneFlags;
 import com.google.errorprone.VisitorState;
 import com.google.errorprone.suppliers.Supplier;
 import com.sun.tools.javac.code.Flags;
@@ -34,6 +37,7 @@
 import com.sun.tools.javac.code.TypeTag;
 import com.sun.tools.javac.code.Types;
 import com.sun.tools.javac.util.Name;
+import com.sun.tools.javac.util.Names;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Set;
@@ -48,12 +52,29 @@
  * <p>i.e.: It is possible that an object of one type could be equal to an object of the other type.
  */
 public final class TypeCompatibilityUtils {
-  public static TypeCompatibilityReport compatibilityOfTypes(
+  private static final String WITHOUT_EQUALS_REASON =
+      " Though these types are the same, the type doesn't implement equals.";
+  private final boolean treatBuildersAsIncomparable;
+
+  public static TypeCompatibilityUtils fromFlags(ErrorProneFlags flags) {
+    return new TypeCompatibilityUtils(
+        flags.getBoolean("TypeCompatibility:TreatBuildersAsIncomparable").orElse(true));
+  }
+
+  public static TypeCompatibilityUtils allOn() {
+    return new TypeCompatibilityUtils(/* treatBuildersAsIncomparable= */ true);
+  }
+
+  private TypeCompatibilityUtils(boolean treatBuildersAsIncomparable) {
+    this.treatBuildersAsIncomparable = treatBuildersAsIncomparable;
+  }
+
+  public TypeCompatibilityReport compatibilityOfTypes(
       Type receiverType, Type argumentType, VisitorState state) {
     return compatibilityOfTypes(receiverType, argumentType, typeSet(state), typeSet(state), state);
   }
 
-  private static TypeCompatibilityReport compatibilityOfTypes(
+  private TypeCompatibilityReport compatibilityOfTypes(
       Type leftType,
       Type rightType,
       Set<Type> previouslySeenComponentsOfLeftType,
@@ -67,11 +88,29 @@ private static TypeCompatibilityReport compatibilityOfTypes(
       return TypeCompatibilityReport.compatible();
     }
 
-    // If they're the exact same type, they are definitely compatible.
     Types types = state.getTypes();
     Type leftUpperBound = getUpperBound(leftType, types);
     Type rightUpperBound = getUpperBound(rightType, types);
-    if (types.isSameType(leftUpperBound, rightUpperBound)) {
+    if (treatBuildersAsIncomparable && types.isSameType(leftUpperBound, rightUpperBound)) {
+      // As a special case, consider Builder classes without an equals implementation to not be
+      // comparable to themselves. It would be reasonable to mistake Builders as having value
+      // semantics, which may be misleading.
+      if (leftUpperBound.isFinal() && leftUpperBound.tsym.name.toString().endsWith("Builder")) {
+        Names names = state.getNames();
+        MethodSymbol equals =
+            (MethodSymbol)
+                state.getSymtab().objectType.tsym.members().findFirst(state.getNames().equals);
+        return isEmpty(
+                scope(types.membersClosure(leftUpperBound, /* skipInterface= */ false))
+                    .getSymbolsByName(
+                        names.toString,
+                        m ->
+                            m != equals
+                                && m.overrides(
+                                    equals, leftUpperBound.tsym, types, /* checkResult= */ false)))
+            ? TypeCompatibilityReport.incompatible(leftType, rightType, WITHOUT_EQUALS_REASON)
+            : TypeCompatibilityReport.compatible();
+      }
       return TypeCompatibilityReport.compatible();
     }
 
@@ -184,7 +223,7 @@ private static boolean areTypesIncompatibleCollections(
         && !isSameType(rightType, collectionType, state);
   }
 
-  private static boolean areIncompatibleProtoTypes(
+  private boolean areIncompatibleProtoTypes(
       Type leftType, Type rightType, Type nearestCommonSupertype, VisitorState state) {
     // See discussion in b/152428396 - Proto equality is defined as having the "same message type",
     // with the same corresponding field values. However - there are 3 flavors of Java Proto API
@@ -266,7 +305,7 @@ private static String classNamePart(Type type) {
    * report showing that a specific generic type in the projection of {@code leftType} is
    * incompatible with the specific generic type in the projection of {@code rightType}.
    */
-  private static TypeCompatibilityReport checkForGenericsMismatch(
+  private TypeCompatibilityReport checkForGenericsMismatch(
       Type leftType,
       Type rightType,
       Type superType,
@@ -321,7 +360,7 @@ private static TreeSet<Type> typeSet(VisitorState state) {
   @AutoValue
   public abstract static class TypeCompatibilityReport {
     private static final TypeCompatibilityReport COMPATIBLE =
-        new AutoValue_TypeCompatibilityUtils_TypeCompatibilityReport(true, null, null);
+        new AutoValue_TypeCompatibilityUtils_TypeCompatibilityReport(true, null, null, null);
 
     public abstract boolean isCompatible();
 
@@ -331,12 +370,20 @@ public abstract static class TypeCompatibilityReport {
     @Nullable
     public abstract Type rhs();
 
+    @Nullable
+    public abstract String extraReason();
+
     static TypeCompatibilityReport compatible() {
       return COMPATIBLE;
     }
 
     static TypeCompatibilityReport incompatible(Type lhs, Type rhs) {
-      return new AutoValue_TypeCompatibilityUtils_TypeCompatibilityReport(false, lhs, rhs);
+      return incompatible(lhs, rhs, "");
+    }
+
+    static TypeCompatibilityReport incompatible(Type lhs, Type rhs, String extraReason) {
+      return new AutoValue_TypeCompatibilityUtils_TypeCompatibilityReport(
+          false, lhs, rhs, extraReason);
     }
   }
 

core/src/main/java/com/google/errorprone/bugpatterns/collectionincompatibletype/CollectionIncompatibleType.java

@@ -72,10 +72,12 @@ private enum FixType {
   }
 
   private final FixType fixType;
+  private final TypeCompatibilityUtils typeCompatibilityUtils;
 
   public CollectionIncompatibleType(ErrorProneFlags flags) {
     this.fixType =
         flags.getEnum("CollectionIncompatibleType:FixType", FixType.class).orElse(FixType.NONE);
+    this.typeCompatibilityUtils = TypeCompatibilityUtils.fromFlags(flags);
   }
 
   @Override
@@ -96,14 +98,11 @@ public Description match(ExpressionTree tree, VisitorState state) {
 
     Types types = state.getTypes();
     TypeCompatibilityReport compatibilityReport =
-        TypeCompatibilityUtils.compatibilityOfTypes(
+        typeCompatibilityUtils.compatibilityOfTypes(
             result.targetType(), result.sourceType(), state);
     if (compatibilityReport.isCompatible()) {
       return NO_MATCH;
     }
-
-    // For error message, use simple names instead of fully qualified names unless they are
-    // identical.
     String sourceType = Signatures.prettyType(result.sourceType());
     String targetType = Signatures.prettyType(result.targetType());
     if (sourceType.equals(targetType)) {

core/src/main/java/com/google/errorprone/bugpatterns/collectionincompatibletype/IncompatibleArgumentType.java

@@ -22,6 +22,7 @@
 import com.google.auto.value.AutoValue;
 import com.google.common.collect.ImmutableListMultimap;
 import com.google.errorprone.BugPattern;
+import com.google.errorprone.ErrorProneFlags;
 import com.google.errorprone.VisitorState;
 import com.google.errorprone.annotations.CheckReturnValue;
 import com.google.errorprone.annotations.CompatibleWith;
@@ -55,6 +56,12 @@
     severity = ERROR)
 public class IncompatibleArgumentType extends BugChecker implements MethodInvocationTreeMatcher {
 
+  private final TypeCompatibilityUtils typeCompatibilityUtils;
+
+  public IncompatibleArgumentType(ErrorProneFlags flags) {
+    this.typeCompatibilityUtils = TypeCompatibilityUtils.fromFlags(flags);
+  }
+
   // Nonnull requiredType: The type I need is bound, in requiredType
   // null requiredType: I found the type variable, but I can't bind it to any type
   @AutoValue
@@ -124,7 +131,7 @@ private void reportAnyViolations(
       if (requiredType.type() != null) {
         // Report a violation for this type
         TypeCompatibilityReport report =
-            TypeCompatibilityUtils.compatibilityOfTypes(requiredType.type(), argType, state);
+            typeCompatibilityUtils.compatibilityOfTypes(requiredType.type(), argType, state);
         if (!report.isCompatible()) {
           state.reportMatch(
               describeViolation(argument, argType, requiredType.type(), types, state));

core/src/main/java/com/google/errorprone/bugpatterns/collectionincompatibletype/TruthIncompatibleType.java

@@ -35,6 +35,7 @@
 
 import com.google.common.collect.Streams;
 import com.google.errorprone.BugPattern;
+import com.google.errorprone.ErrorProneFlags;
 import com.google.errorprone.VisitorState;
 import com.google.errorprone.bugpatterns.BugChecker;
 import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher;
@@ -145,6 +146,12 @@ public class TruthIncompatibleType extends BugChecker implements MethodInvocatio
   private static final Supplier<Type> CORRESPONDENCE =
       typeFromString("com.google.common.truth.Correspondence");
 
+  private final TypeCompatibilityUtils typeCompatibilityUtils;
+
+  public TruthIncompatibleType(ErrorProneFlags flags) {
+    this.typeCompatibilityUtils = TypeCompatibilityUtils.fromFlags(flags);
+  }
+
   @Override
   public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
     Streams.concat(
@@ -413,7 +420,7 @@ private static boolean isNonVarargsCall(
   private Stream<Description> checkCompatibility(
       ExpressionTree tree, Type targetType, Type sourceType, VisitorState state) {
     TypeCompatibilityReport compatibilityReport =
-        TypeCompatibilityUtils.compatibilityOfTypes(targetType, sourceType, state);
+        typeCompatibilityUtils.compatibilityOfTypes(targetType, sourceType, state);
     if (compatibilityReport.isCompatible()) {
       return Stream.empty();
     }

core/src/test/java/com/google/errorprone/bugpatterns/EqualsIncompatibleTypeTest.java

@@ -151,4 +151,55 @@ public void unconstrainedWildcard_compatibleWithAnything() {
             "}")
         .doTest();
   }
+
+  @Test
+  public void enumsCanBeEqual() {
+    compilationHelper
+        .addSourceLines(
+            "Test.java",
+            "class Test {",
+            "  enum E {A, B}",
+            "  public void test() {",
+            "    E.A.equals(E.B);",
+            "  }",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  public void protoBuildersCannotBeEqual() {
+    compilationHelper
+        .addSourceLines(
+            "Test.java",
+            "import com.google.errorprone.bugpatterns.proto.ProtoTest.TestProtoMessage;",
+            "import com.google.errorprone.bugpatterns.proto.ProtoTest.TestOneOfMessage;",
+            "",
+            "public class Test {",
+            "  public void test() {",
+            "    // BUG: Diagnostic contains: implement equals",
+            "    TestProtoMessage.newBuilder().equals(TestProtoMessage.newBuilder());",
+            "    // BUG: Diagnostic contains:",
+            "    TestProtoMessage.newBuilder().equals(TestOneOfMessage.newBuilder());",
+            "    // BUG: Diagnostic contains:",
+            "    TestProtoMessage.newBuilder().equals(TestOneOfMessage.getDefaultInstance());",
+            "  }",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  public void flaggedOff_protoBuildersNotConsideredIncomparable() {
+    compilationHelper
+        .addSourceLines(
+            "Test.java",
+            "import com.google.errorprone.bugpatterns.proto.ProtoTest.TestProtoMessage;",
+            "",
+            "public class Test {",
+            "  public void test() {",
+            "    TestProtoMessage.newBuilder().equals(TestProtoMessage.newBuilder());",
+            "  }",
+            "}")
+        .setArgs("-XepOpt:TypeCompatibility:TreatBuildersAsIncomparable=false")
+        .doTest();
+  }
 }