From c500516bb42a0eed0dbd29f21b247f47aa2b6417 Mon Sep 17 00:00:00 2001 From: Gijs de Jong <14833076+oxkitsune@users.noreply.github.com> Date: Wed, 29 Jun 2022 18:55:22 +0200 Subject: [PATCH] Prefer AssertJ's `isNull()` assertion over `isEqualTo(null)` (#133) --- .../bugpatterns/AssertJIsNullCheck.java | 58 +++++++++++++++++ .../bugpatterns/AssertJIsNullCheckTest.java | 64 +++++++++++++++++++ 2 files changed, 122 insertions(+) create mode 100644 error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssertJIsNullCheck.java create mode 100644 error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/AssertJIsNullCheckTest.java diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssertJIsNullCheck.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssertJIsNullCheck.java new file mode 100644 index 0000000000..a20551e4c1 --- /dev/null +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssertJIsNullCheck.java @@ -0,0 +1,58 @@ +package tech.picnic.errorprone.bugpatterns; + +import static com.google.errorprone.BugPattern.LinkType.NONE; +import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION; +import static com.google.errorprone.BugPattern.StandardTags.SIMPLIFICATION; +import static com.google.errorprone.matchers.Matchers.allOf; +import static com.google.errorprone.matchers.Matchers.argument; +import static com.google.errorprone.matchers.Matchers.argumentCount; +import static com.google.errorprone.matchers.Matchers.instanceMethod; +import static com.google.errorprone.matchers.Matchers.nullLiteral; + +import com.google.auto.service.AutoService; +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; +import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.fixes.SuggestedFixes; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matcher; +import com.sun.source.tree.MethodInvocationTree; + +/** + * A {@link BugChecker} which flags AssertJ {@code isEqualTo(null)} checks for simplification. + * + *

This bug checker cannot be replaced with a simple Refaster template, as the Refaster approach + * would require that all overloads of {@link org.assertj.core.api.Assert#isEqualTo(Object)} (such + * as {@link org.assertj.core.api.AbstractStringAssert#isEqualTo(String)}) are explicitly + * enumerated. This bug checker generically matches all such current and future overloads. + */ +@AutoService(BugChecker.class) +@BugPattern( + name = "AssertJIsNull", + summary = "Prefer `.isNull()` over `.isEqualTo(null)`", + linkType = NONE, + severity = SUGGESTION, + tags = SIMPLIFICATION) +public final class AssertJIsNullCheck extends BugChecker implements MethodInvocationTreeMatcher { + private static final long serialVersionUID = 1L; + private static final Matcher ASSERT_IS_EQUAL_TO_NULL = + allOf( + instanceMethod().onDescendantOf("org.assertj.core.api.Assert").named("isEqualTo"), + argumentCount(1), + argument(0, nullLiteral())); + + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + if (!ASSERT_IS_EQUAL_TO_NULL.matches(tree, state)) { + return Description.NO_MATCH; + } + + SuggestedFix.Builder fix = + SuggestedFix.builder().merge(SuggestedFixes.renameMethodInvocation(tree, "isNull", state)); + tree.getArguments().forEach(arg -> fix.merge(SuggestedFix.delete(arg))); + + return describeMatch(tree, fix.build()); + } +} diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/AssertJIsNullCheckTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/AssertJIsNullCheckTest.java new file mode 100644 index 0000000000..e2c99babb4 --- /dev/null +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/AssertJIsNullCheckTest.java @@ -0,0 +1,64 @@ +package tech.picnic.errorprone.bugpatterns; + +import static com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH; + +import com.google.errorprone.BugCheckerRefactoringTestHelper; +import com.google.errorprone.CompilationTestHelper; +import org.junit.jupiter.api.Test; + +final class AssertJIsNullCheckTest { + private final CompilationTestHelper compilationTestHelper = + CompilationTestHelper.newInstance(AssertJIsNullCheck.class, getClass()); + private final BugCheckerRefactoringTestHelper refactoringTestHelper = + BugCheckerRefactoringTestHelper.newInstance(AssertJIsNullCheck.class, getClass()); + + @Test + void identification() { + compilationTestHelper + .addSourceLines( + "A.java", + "import static org.assertj.core.api.Assertions.assertThat;", + "", + "class A {", + " void m() {", + " assertThat(1).isEqualTo(1);", + " // BUG: Diagnostic contains:", + " assertThat(1).isEqualTo(null);", + " // BUG: Diagnostic contains:", + " assertThat(\"foo\").isEqualTo(null);", + " isEqualTo(null);", + " }", + "", + " private boolean isEqualTo(Object value) {", + " return value.equals(\"bar\");", + " }", + "}") + .doTest(); + } + + @Test + void replacement() { + refactoringTestHelper + .addInputLines( + "A.java", + "import static org.assertj.core.api.Assertions.assertThat;", + "", + "class A {", + " void m() {", + " assertThat(1).isEqualTo(null);", + " assertThat(\"foo\").isEqualTo(null);", + " }", + "}") + .addOutputLines( + "A.java", + "import static org.assertj.core.api.Assertions.assertThat;", + "", + "class A {", + " void m() {", + " assertThat(1).isNull();", + " assertThat(\"foo\").isNull();", + " }", + "}") + .doTest(TEXT_MATCH); + } +}