diff --git a/check_api/src/main/java/com/google/errorprone/matchers/Matchers.java b/check_api/src/main/java/com/google/errorprone/matchers/Matchers.java index 63189e15f3e..d191acab1ec 100644 --- a/check_api/src/main/java/com/google/errorprone/matchers/Matchers.java +++ b/check_api/src/main/java/com/google/errorprone/matchers/Matchers.java @@ -1542,7 +1542,18 @@ public static Matcher assertEqualsInvocation() { staticMethod().onClass("junit.framework.TestCase").named("assertEquals")); } - /** Matches {@link equals} method declaration. */ + /** + * Matches calls to the method {link org.junit.Assert#assertNotEquals} and corresponding methods + * in JUnit 3.x. + */ + public static Matcher assertNotEqualsInvocation() { + return anyOf( + staticMethod().onClass("org.junit.Assert").named("assertNotEquals"), + staticMethod().onClass("junit.framework.Assert").named("assertNotEquals"), + staticMethod().onClass("junit.framework.TestCase").named("assertNotEquals")); + } + + /** Matches {@link Object#equals} method declaration. */ public static Matcher equalsMethodDeclaration() { return allOf( methodIsNamed("equals"), @@ -1551,7 +1562,7 @@ public static Matcher equalsMethodDeclaration() { anyOf(methodReturns(BOOLEAN_TYPE), methodReturns(JAVA_LANG_BOOLEAN_TYPE))); } - /** Matches {@link toString} method declaration. */ + /** Matches {@link Object#toString} method declaration. */ public static Matcher toStringMethodDeclaration() { return allOf( methodIsNamed("toString"), diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/UndefinedEquals.java b/core/src/main/java/com/google/errorprone/bugpatterns/UndefinedEquals.java index a16ea1f9f2e..0ed4078e952 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/UndefinedEquals.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/UndefinedEquals.java @@ -16,11 +16,19 @@ package com.google.errorprone.bugpatterns; +import static com.google.common.collect.Iterables.getLast; +import static com.google.common.collect.Iterables.getOnlyElement; import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; +import static com.google.errorprone.matchers.Matchers.allOf; import static com.google.errorprone.matchers.Matchers.anyOf; import static com.google.errorprone.matchers.Matchers.assertEqualsInvocation; +import static com.google.errorprone.matchers.Matchers.assertNotEqualsInvocation; import static com.google.errorprone.matchers.Matchers.instanceEqualsInvocation; +import static com.google.errorprone.matchers.Matchers.receiverOfInvocation; import static com.google.errorprone.matchers.Matchers.staticEqualsInvocation; +import static com.google.errorprone.matchers.Matchers.staticMethod; +import static com.google.errorprone.matchers.method.MethodMatchers.instanceMethod; +import static com.google.errorprone.util.ASTHelpers.getReceiver; import static com.google.errorprone.util.ASTHelpers.getReceiverType; import static com.google.errorprone.util.ASTHelpers.getType; import static com.google.errorprone.util.ASTHelpers.isSameType; @@ -31,9 +39,11 @@ import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Matcher; +import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.MethodInvocationTree; import com.sun.tools.javac.code.Type; -import java.util.Optional; +import java.util.List; +import java.util.regex.Pattern; /** * Collection, Iterable, Multimap, and Queue do not have well-defined equals behavior. @@ -48,44 +58,57 @@ public final class UndefinedEquals extends BugChecker implements MethodInvocatio private static final ImmutableList BAD_CLASS_LIST = ImmutableList.of( - "java.util.Queue", - "java.util.Collection", "com.google.common.collect.Multimap", - "java.lang.Iterable"); + "java.lang.Iterable", + "java.util.Collection", + "java.util.Queue"); - private static final Matcher EQUALS_METHODS = - anyOf(staticEqualsInvocation(), instanceEqualsInvocation(), assertEqualsInvocation()); + private static final Matcher ASSERT_THAT_EQUALS = + allOf( + instanceMethod() + .onDescendantOf("com.google.common.truth.Subject") + .withNameMatching(Pattern.compile("is(Not)?EqualTo")), + receiverOfInvocation( + anyOf( + staticMethod().onClass("com.google.common.truth.Truth").named("assertThat"), + instanceMethod() + .onDescendantOf("com.google.common.truth.StandardSubjectBuilder") + .named("that")))); @Override public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { - if (!EQUALS_METHODS.matches(tree, state)) { - return Description.NO_MATCH; - } - Type receiverType; Type argumentType; + List arguments = tree.getArguments(); - if (staticEqualsInvocation().matches(tree, state)) { - receiverType = getType(tree.getArguments().get(0)); - argumentType = getType(tree.getArguments().get(1)); - } else { + if (staticEqualsInvocation().matches(tree, state) + || assertEqualsInvocation().matches(tree, state) + || assertNotEqualsInvocation().matches(tree, state)) { + receiverType = getType(arguments.get(arguments.size() - 2)); + argumentType = getType(getLast(arguments)); + } else if (instanceEqualsInvocation().matches(tree, state)) { receiverType = getReceiverType(tree); - argumentType = getType(tree.getArguments().get(0)); + argumentType = getType(arguments.get(0)); + } else if (ASSERT_THAT_EQUALS.matches(tree, state)) { + receiverType = getType(getOnlyElement(arguments)); + argumentType = + getType(getOnlyElement(((MethodInvocationTree) getReceiver(tree)).getArguments())); + } else { + return Description.NO_MATCH; } - Optional badType = - BAD_CLASS_LIST - .stream() - .filter( - t -> - isSameType(receiverType, state.getTypeFromString(t), state) - || isSameType(argumentType, state.getTypeFromString(t), state)) - .findFirst(); - - return badType.isPresent() - ? buildDescription(tree) - .setMessage(badType.get() + " does not have well-defined equals behavior") - .build() - : Description.NO_MATCH; + return BAD_CLASS_LIST + .stream() + .filter( + t -> + isSameType(receiverType, state.getTypeFromString(t), state) + || isSameType(argumentType, state.getTypeFromString(t), state)) + .findFirst() + .map( + b -> + buildDescription(tree) + .setMessage(b + " does not have well-defined equals behavior") + .build()) + .orElse(Description.NO_MATCH); } } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/UndefinedEqualsTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/UndefinedEqualsTest.java index 35e33a57375..7440c6104b0 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/UndefinedEqualsTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/UndefinedEqualsTest.java @@ -61,7 +61,7 @@ public void positiveStaticEquals() { "import java.util.Objects;", "class Test {", " void f(Collection a, Collection b) {", - " // BUG: Diagnostic contains: java.util.Collection does not have", + " // BUG: Diagnostic contains: java.util.Collection does not have", " Objects.equals(a,b);", " }", "}") @@ -76,10 +76,15 @@ public void positiveAssertEquals() { "import java.util.List;", "import com.google.common.collect.Iterables;", "import static org.junit.Assert.assertEquals;", + "import static org.junit.Assert.assertNotEquals;", "class Test {", " void test(List myList, List otherList) {", - " // BUG: Diagnostic contains: java.lang.Iterable does not have", + " // BUG: Diagnostic contains: java.lang.Iterable does not have", " assertEquals(Iterables.skip(myList, 1), Iterables.skip(myList, 2));", + " // BUG: Diagnostic contains: java.lang.Iterable does not have", + " assertNotEquals(Iterables.skip(myList, 1), Iterables.skip(myList, 2));", + " // BUG: Diagnostic contains: java.lang.Iterable does not have", + " assertEquals(\"foo\", Iterables.skip(myList, 1), Iterables.skip(myList, 2));", " }", "}") .doTest(); @@ -93,13 +98,31 @@ public void positiveWithGenerics() { "import java.util.Queue;", "class Test {", " void f(Queue a, Queue b) {", - " // BUG: Diagnostic contains: java.util.Queue does not have", + " // BUG: Diagnostic contains: java.util.Queue does not have", " a.equals(b);", " }", "}") .doTest(); } + @Test + public void positiveTruth() { + compilationHelper + .addSourceLines( + "Test.java", + "import static com.google.common.truth.Truth.assertThat;", + "import java.util.Queue;", + "class Test {", + " void f(Queue a, Queue b) {", + " // BUG: Diagnostic contains: java.util.Queue does not have", + " assertThat(a).isEqualTo(b);", + " // BUG: Diagnostic contains: java.util.Queue does not have", + " assertThat(a).isNotEqualTo(b);", + " }", + "}") + .doTest(); + } + @Test public void negative() { compilationHelper