Prefer AssertJ's isNull() assertion over isEqualTo(null) (#133)

PicnicSupermarket · Jun 29, 2022 · c500516 · c500516
1 parent 85d68a4
commit c500516
Show file tree

Hide file tree

Showing 2 changed files with 122 additions and 0 deletions.
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
@@ -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.
+ *
+ * <p>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<MethodInvocationTree> 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/...rone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/AssertJIsNullCheckTest.java b/...rone-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);
+  }
+}