diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/EqualsIncompatibleType.java b/core/src/main/java/com/google/errorprone/bugpatterns/EqualsIncompatibleType.java index f3c24d36f9d..c23c62467dd 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/EqualsIncompatibleType.java +++ b/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(); } diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/TypeCompatibilityUtils.java b/core/src/main/java/com/google/errorprone/bugpatterns/TypeCompatibilityUtils.java index ab867690238..744058767e6 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/TypeCompatibilityUtils.java +++ b/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 @@ *

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 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 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); } } diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/collectionincompatibletype/CollectionIncompatibleType.java b/core/src/main/java/com/google/errorprone/bugpatterns/collectionincompatibletype/CollectionIncompatibleType.java index 5ca88bd2841..03f96ae8789 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/collectionincompatibletype/CollectionIncompatibleType.java +++ b/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)) { diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/collectionincompatibletype/IncompatibleArgumentType.java b/core/src/main/java/com/google/errorprone/bugpatterns/collectionincompatibletype/IncompatibleArgumentType.java index 188ab7e2de1..82d33000d52 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/collectionincompatibletype/IncompatibleArgumentType.java +++ b/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)); diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/collectionincompatibletype/TruthIncompatibleType.java b/core/src/main/java/com/google/errorprone/bugpatterns/collectionincompatibletype/TruthIncompatibleType.java index ea632a950d2..64d65d33284 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/collectionincompatibletype/TruthIncompatibleType.java +++ b/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 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 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(); } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/EqualsIncompatibleTypeTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/EqualsIncompatibleTypeTest.java index 5559593f4c8..e6b1e3b9249 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/EqualsIncompatibleTypeTest.java +++ b/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(); + } }