Skip to content

Commit

Permalink
Make UndefinedEquals match Truth's is(Not)?EqualTo as well as JUnit a…
Browse files Browse the repository at this point in the history 
…ssertions with messages.

RELNOTES: N/A

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=207559362
  • Loading branch information
@graememorgan @cushon
graememorgan authored and cushon committed Aug 6, 2018
1 parent 52b9c65 commit 8c9ae01
Show file tree
Hide file tree
Showing 3 changed files with 91 additions and 34 deletions.
15 changes: 13 additions & 2 deletions check_api/src/main/java/com/google/errorprone/matchers/Matchers.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1542,7 +1542,18 @@ public static Matcher<ExpressionTree> 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<ExpressionTree> 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<MethodTree> equalsMethodDeclaration() {
return allOf(
methodIsNamed("equals"),
Expand All @@ -1551,7 +1562,7 @@ public static Matcher<MethodTree> equalsMethodDeclaration() {
anyOf(methodReturns(BOOLEAN_TYPE), methodReturns(JAVA_LANG_BOOLEAN_TYPE)));
}

/** Matches {@link toString} method declaration. */
/** Matches {@link Object#toString} method declaration. */
public static Matcher<MethodTree> toStringMethodDeclaration() {
return allOf(
methodIsNamed("toString"),
Expand Down
81 changes: 52 additions & 29 deletions core/src/main/java/com/google/errorprone/bugpatterns/UndefinedEquals.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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.
Expand All @@ -48,44 +58,57 @@ public final class UndefinedEquals extends BugChecker implements MethodInvocatio

private static final ImmutableList<String> 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<MethodInvocationTree> EQUALS_METHODS =
anyOf(staticEqualsInvocation(), instanceEqualsInvocation(), assertEqualsInvocation());
private static final Matcher<MethodInvocationTree> 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<? extends ExpressionTree> 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<String> 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);
}
}
29 changes: 26 additions & 3 deletions core/src/test/java/com/google/errorprone/bugpatterns/UndefinedEqualsTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);",
" }",
"}")
Expand All @@ -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();
Expand All @@ -93,13 +98,31 @@ public void positiveWithGenerics() {
"import java.util.Queue;",
"class Test {",
" <T> void f(Queue<String> a, Queue<T> 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 {",
" <T> void f(Queue<String> a, Queue<T> 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
Expand Down

0 comments on commit 8c9ae01

Please sign in to comment.