diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/TruthSelfEquals.java b/core/src/main/java/com/google/errorprone/bugpatterns/TruthSelfEquals.java index 533d933ae58..a398973df24 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/TruthSelfEquals.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/TruthSelfEquals.java @@ -25,6 +25,8 @@ import static com.google.errorprone.matchers.method.MethodMatchers.staticMethod; import static com.google.errorprone.util.ASTHelpers.getReceiver; import static com.google.errorprone.util.ASTHelpers.getSymbol; +import static com.google.errorprone.util.ASTHelpers.getType; +import static com.google.errorprone.util.ASTHelpers.isSameType; import static com.google.errorprone.util.ASTHelpers.sameVariable; import static java.lang.String.format; @@ -38,34 +40,41 @@ import com.sun.source.tree.MethodInvocationTree; import javax.inject.Inject; -/** - * Points out if an object is tested for equality/inequality to itself using Truth Libraries. - * - * @author bhagwani@google.com (Sumit Bhagwani) - */ -@BugPattern( - summary = - "isEqualTo should not be used to test an object for equality with itself; the" - + " assertion will never fail.", - severity = ERROR) +/** A {@link BugPattern}; see the summary. */ +// TODO(ghm): Rename to SelfAssertion or something. +@BugPattern(summary = "This assertion will always fail or succeed.", severity = ERROR) public final class TruthSelfEquals extends BugChecker implements MethodInvocationTreeMatcher { private final Matcher equalsMatcher = - allOf( - instanceMethod() - .anyClass() - .namedAnyOf( - "isEqualTo", - "isSameInstanceAs", - "containsExactlyElementsIn", - "containsAtLeastElementsIn", - "areEqualTo"), - this::receiverSameAsParentsArgument); + anyOf( + allOf( + instanceMethod() + .anyClass() + .namedAnyOf( + "isEqualTo", + "isSameInstanceAs", + "containsExactlyElementsIn", + "containsAtLeastElementsIn", + "areEqualTo"), + this::truthSameArguments), + allOf( + staticMethod() + .onClassAny( + "org.junit.Assert", "junit.framework.Assert", "junit.framework.TestCase") + .namedAnyOf("assertEquals", "assertArrayEquals"), + this::junitSameArguments)); private final Matcher notEqualsMatcher = - allOf( - instanceMethod().anyClass().namedAnyOf("isNotEqualTo", "isNotSameInstanceAs"), - this::receiverSameAsParentsArgument); + anyOf( + allOf( + instanceMethod().anyClass().namedAnyOf("isNotEqualTo", "isNotSameInstanceAs"), + this::truthSameArguments), + allOf( + staticMethod() + .onClassAny( + "org.junit.Assert", "junit.framework.Assert", "junit.framework.TestCase") + .namedAnyOf("assertNotEquals"), + this::junitSameArguments)); private static final Matcher ASSERT_THAT = anyOf( @@ -74,6 +83,7 @@ public final class TruthSelfEquals extends BugChecker implements MethodInvocatio instanceMethod() .onDescendantOf("com.google.common.truth.StandardSubjectBuilder") .named("that")); + private final ConstantExpressions constantExpressions; @Inject @@ -107,7 +117,26 @@ private static String generateSummary(String methodName, String constantOutput) methodName, constantOutput); } - private boolean receiverSameAsParentsArgument(MethodInvocationTree tree, VisitorState state) { + private boolean junitSameArguments(MethodInvocationTree tree, VisitorState state) { + var arguments = tree.getArguments(); + if (arguments.isEmpty()) { + return false; + } + var firstArgument = tree.getArguments().get(0); + ExpressionTree expected; + ExpressionTree actual; + if (tree.getArguments().size() > 2 + && isSameType(getType(firstArgument), state.getSymtab().stringType, state)) { + expected = arguments.get(1); + actual = arguments.get(2); + } else { + expected = arguments.get(0); + actual = arguments.get(1); + } + return sameExpression(state, expected, actual); + } + + private boolean truthSameArguments(MethodInvocationTree tree, VisitorState state) { ExpressionTree rec = getReceiver(tree); if (rec == null) { return false; @@ -122,6 +151,11 @@ private boolean receiverSameAsParentsArgument(MethodInvocationTree tree, Visitor } ExpressionTree receiverExpression = getOnlyElement(((MethodInvocationTree) rec).getArguments()); ExpressionTree invocationExpression = getOnlyElement(tree.getArguments()); + return sameExpression(state, receiverExpression, invocationExpression); + } + + private boolean sameExpression( + VisitorState state, ExpressionTree receiverExpression, ExpressionTree invocationExpression) { if (sameVariable(receiverExpression, invocationExpression)) { return true; } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/TruthSelfEqualsTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/TruthSelfEqualsTest.java index 03ec7439a89..bdb198cab99 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/TruthSelfEqualsTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/TruthSelfEqualsTest.java @@ -114,4 +114,38 @@ public void constantExpressions() { "}") .doTest(); } + + @Test + public void junitPositiveAssertion() { + compilationHelper + .addSourceLines( + "Test.java", + "import static org.junit.Assert.assertEquals;", + "abstract class Test {", + " void test(int x) {", + " // BUG: Diagnostic contains: pass", + " assertEquals(x, x);", + " // BUG: Diagnostic contains: pass", + " assertEquals(\"foo\", x, x);", + " }", + "}") + .doTest(); + } + + @Test + public void junitNegativeAssertion() { + compilationHelper + .addSourceLines( + "Test.java", + "import static org.junit.Assert.assertNotEquals;", + "abstract class Test {", + " void test(int x) {", + " // BUG: Diagnostic contains: fail", + " assertNotEquals(x, x);", + " // BUG: Diagnostic contains: fail", + " assertNotEquals(\"foo\", x, x);", + " }", + "}") + .doTest(); + } }