From d595a584295aa88b5cf123cfd93c59ea702d20b0 Mon Sep 17 00:00:00 2001 From: Gijs de Jong Date: Tue, 14 Jun 2022 14:59:22 +0200 Subject: [PATCH 01/10] Add BugPattern to rewrite `assertThat(...).isEqualTo(null)` to `assertThat(...).isNull()` Run google-java-format Add some XXXs Suggest fix in AsserThatIsNullCheck Add javadoc Make test class final Add typecheck for literal tree fmt Improve method invocation matching --- .../bugpatterns/AssertThatIsNullCheck.java | 45 ++++++++++++ .../AssertThatIsNullCheckTest.java | 70 +++++++++++++++++++ 2 files changed, 115 insertions(+) create mode 100644 error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssertThatIsNullCheck.java create mode 100644 error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/AssertThatIsNullCheckTest.java diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssertThatIsNullCheck.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssertThatIsNullCheck.java new file mode 100644 index 0000000000..eae432e43f --- /dev/null +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssertThatIsNullCheck.java @@ -0,0 +1,45 @@ +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 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.SuggestedFixes; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.source.tree.Tree; +import com.sun.tools.javac.code.Symbol.MethodSymbol; +import org.assertj.core.api.AbstractAssert; + +/** + * A {@link BugChecker} which flags {@code asserThat(someValue).isEqualTo(null)} and suggests {@link + * AbstractAssert#isNull()}. + */ +@AutoService(BugChecker.class) +@BugPattern( + name = "AssertThatIsNull", + summary = "asserThat(...).isEqualTo(null) should be assertThat(...).isNull()", + linkType = NONE, + severity = SUGGESTION, + tags = SIMPLIFICATION) +public final class AssertThatIsNullCheck extends BugChecker implements MethodInvocationTreeMatcher { + private static final long serialVersionUID = 1L; + + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + MethodSymbol symbol = ASTHelpers.getSymbol(tree); + if (symbol == null + || tree.getArguments().size() != 1 + || tree.getArguments().get(0).getKind() != Tree.Kind.NULL_LITERAL) { + return Description.NO_MATCH; + } + + return describeMatch(tree, SuggestedFixes.renameMethodInvocation(tree, "isNull()", state)); + } +} diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/AssertThatIsNullCheckTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/AssertThatIsNullCheckTest.java new file mode 100644 index 0000000000..b391fd23a3 --- /dev/null +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/AssertThatIsNullCheckTest.java @@ -0,0 +1,70 @@ +package tech.picnic.errorprone.bugpatterns; + +import com.google.errorprone.BugCheckerRefactoringTestHelper; +import com.google.errorprone.CompilationTestHelper; +import org.junit.jupiter.api.Test; + +public final class AssertThatIsNullCheckTest { + private final CompilationTestHelper compilationTestHelper = + CompilationTestHelper.newInstance(AssertThatIsNullCheck.class, getClass()); + + private final BugCheckerRefactoringTestHelper refactoringTestHelper = + BugCheckerRefactoringTestHelper.newInstance(AssertThatIsNullCheck.class, getClass()); + + // XXX: Methods always start with lowercase. I thought we had a Checkstyle thing for this. + @Test + void identification() { + compilationTestHelper + .addSourceLines( + "A.java", + "import static org.assertj.core.api.Assertions.assertThat;", + "import com.google.common.collect.ImmutableSortedSet;", + "import org.junit.jupiter.api.Test;", + "public final class A {", + " @Test", + " public void testAssertThat() {", + " String nullValue = null;", + " assertThat(12).isEqualTo(12);", + " // BUG: Diagnostic contains: assertThat(...).isNull()", + " assertThat(\"value\").isEqualTo(null);", + " // BUG: Diagnostic contains: assertThat(...).isNull()", + " assertThat(nullValue).isEqualTo(null);", + " }", + "} ") + .doTest(); + } + + @Test + void replacement() { + refactoringTestHelper + .addInputLines( + "A.java", + "import static org.assertj.core.api.Assertions.assertThat;", + "import com.google.common.collect.ImmutableSortedSet;", + "import org.junit.jupiter.api.Test;", + "public final class A {", + " @Test", + " public void testAssertThat() {", + " String nullValue = null;", + " assertThat(12).isEqualTo(12);", + " assertThat(\"value\").isEqualTo(null);", + " assertThat(nullValue).isEqualTo(null);", + " }", + "} ") + .addOutputLines( + "A.java", + "import static org.assertj.core.api.Assertions.assertThat;", + "import com.google.common.collect.ImmutableSortedSet;", + "import org.junit.jupiter.api.Test;", + "public final class A {", + " @Test", + " public void testAssertThat() {", + " String nullValue = null;", + " assertThat(12).isEqualTo(12);", + " assertThat(\"value\").isNull();", + " assertThat(nullValue).isNull();", + " }", + "} ") + .doTest(); + } +} From de8480e450b640687f750241d5316717bc62188f Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Thu, 16 Jun 2022 13:07:37 +0200 Subject: [PATCH 02/10] Remove public modifier --- .../errorprone/bugpatterns/AssertThatIsNullCheckTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/AssertThatIsNullCheckTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/AssertThatIsNullCheckTest.java index b391fd23a3..2cea06f8df 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/AssertThatIsNullCheckTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/AssertThatIsNullCheckTest.java @@ -4,7 +4,7 @@ import com.google.errorprone.CompilationTestHelper; import org.junit.jupiter.api.Test; -public final class AssertThatIsNullCheckTest { +final class AssertThatIsNullCheckTest { private final CompilationTestHelper compilationTestHelper = CompilationTestHelper.newInstance(AssertThatIsNullCheck.class, getClass()); From 766954d448188da3f2e2c372de75720ee3db245f Mon Sep 17 00:00:00 2001 From: Gijs de Jong Date: Thu, 16 Jun 2022 14:06:22 +0200 Subject: [PATCH 03/10] Requested changes --- ...k.java => AssertThatIsNullUsageCheck.java} | 29 +++++++++++-------- ...va => AssertThatIsNullUsageCheckTest.java} | 25 +++++++++++++--- 2 files changed, 38 insertions(+), 16 deletions(-) rename error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/{AssertThatIsNullCheck.java => AssertThatIsNullUsageCheck.java} (60%) rename error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/{AssertThatIsNullCheckTest.java => AssertThatIsNullUsageCheckTest.java} (75%) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssertThatIsNullCheck.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssertThatIsNullUsageCheck.java similarity index 60% rename from error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssertThatIsNullCheck.java rename to error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssertThatIsNullUsageCheck.java index eae432e43f..f35f2e79f0 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssertThatIsNullCheck.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssertThatIsNullUsageCheck.java @@ -3,6 +3,7 @@ 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.sun.source.tree.Tree.Kind.NULL_LITERAL; import com.google.auto.service.AutoService; import com.google.errorprone.BugPattern; @@ -11,32 +12,36 @@ import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; import com.google.errorprone.fixes.SuggestedFixes; import com.google.errorprone.matchers.Description; -import com.google.errorprone.util.ASTHelpers; +import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.matchers.method.MethodMatchers; +import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.MethodInvocationTree; -import com.sun.source.tree.Tree; -import com.sun.tools.javac.code.Symbol.MethodSymbol; -import org.assertj.core.api.AbstractAssert; /** - * A {@link BugChecker} which flags {@code asserThat(someValue).isEqualTo(null)} and suggests {@link - * AbstractAssert#isNull()}. + * A {@link BugChecker} which flags {@code asserThat(someValue).isEqualTo(null)} for further + * simplification */ @AutoService(BugChecker.class) @BugPattern( - name = "AssertThatIsNull", - summary = "asserThat(...).isEqualTo(null) should be assertThat(...).isNull()", + name = "AssertThatIsNullUsage", + summary = "`asserThat(...).isEqualTo(null)` should be `assertThat(...).isNull()`", linkType = NONE, severity = SUGGESTION, tags = SIMPLIFICATION) -public final class AssertThatIsNullCheck extends BugChecker implements MethodInvocationTreeMatcher { +public final class AssertThatIsNullUsageCheck extends BugChecker + implements MethodInvocationTreeMatcher { private static final long serialVersionUID = 1L; + private final Matcher ASSERT_IS_EQUAL = + MethodMatchers.instanceMethod() + .onDescendantOf("org.assertj.core.api.Assert") + .named("isEqualTo"); + @Override public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { - MethodSymbol symbol = ASTHelpers.getSymbol(tree); - if (symbol == null + if (!ASSERT_IS_EQUAL.matches(tree, state) || tree.getArguments().size() != 1 - || tree.getArguments().get(0).getKind() != Tree.Kind.NULL_LITERAL) { + || tree.getArguments().get(0).getKind() != NULL_LITERAL) { return Description.NO_MATCH; } diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/AssertThatIsNullCheckTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/AssertThatIsNullUsageCheckTest.java similarity index 75% rename from error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/AssertThatIsNullCheckTest.java rename to error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/AssertThatIsNullUsageCheckTest.java index 2cea06f8df..f0e8028f5f 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/AssertThatIsNullCheckTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/AssertThatIsNullUsageCheckTest.java @@ -4,14 +4,13 @@ import com.google.errorprone.CompilationTestHelper; import org.junit.jupiter.api.Test; -final class AssertThatIsNullCheckTest { +final class AssertThatIsNullUsageCheckTest { private final CompilationTestHelper compilationTestHelper = - CompilationTestHelper.newInstance(AssertThatIsNullCheck.class, getClass()); + CompilationTestHelper.newInstance(AssertThatIsNullUsageCheck.class, getClass()); private final BugCheckerRefactoringTestHelper refactoringTestHelper = - BugCheckerRefactoringTestHelper.newInstance(AssertThatIsNullCheck.class, getClass()); + BugCheckerRefactoringTestHelper.newInstance(AssertThatIsNullUsageCheck.class, getClass()); - // XXX: Methods always start with lowercase. I thought we had a Checkstyle thing for this. @Test void identification() { compilationTestHelper @@ -29,7 +28,13 @@ void identification() { " assertThat(\"value\").isEqualTo(null);", " // BUG: Diagnostic contains: assertThat(...).isNull()", " assertThat(nullValue).isEqualTo(null);", + " isEqualTo(\"foo\");", + " isEqualTo(null);", " }", + "", + " private boolean isEqualTo (Object value) {", + " return value.equals(\"bar\");", + " }", "} ") .doTest(); } @@ -49,7 +54,13 @@ void replacement() { " assertThat(12).isEqualTo(12);", " assertThat(\"value\").isEqualTo(null);", " assertThat(nullValue).isEqualTo(null);", + " isEqualTo(\"foo\");", + " isEqualTo(null);", " }", + "", + " private boolean isEqualTo (Object value) {", + " return value.equals(\"bar\");", + " }", "} ") .addOutputLines( "A.java", @@ -63,7 +74,13 @@ void replacement() { " assertThat(12).isEqualTo(12);", " assertThat(\"value\").isNull();", " assertThat(nullValue).isNull();", + " isEqualTo(\"foo\");", + " isEqualTo(null);", " }", + "", + " private boolean isEqualTo (Object value) {", + " return value.equals(\"bar\");", + " }", "} ") .doTest(); } From 6e49242b00b72cfd8fc736f8ffbaaddd4af7f7ea Mon Sep 17 00:00:00 2001 From: Gijs de Jong Date: Thu, 16 Jun 2022 15:46:24 +0200 Subject: [PATCH 04/10] Simplify matcher --- .../bugpatterns/AssertThatIsNullUsageCheck.java | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssertThatIsNullUsageCheck.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssertThatIsNullUsageCheck.java index f35f2e79f0..fb095b838d 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssertThatIsNullUsageCheck.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssertThatIsNullUsageCheck.java @@ -3,6 +3,7 @@ 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.instanceMethod; import static com.sun.source.tree.Tree.Kind.NULL_LITERAL; import com.google.auto.service.AutoService; @@ -13,6 +14,7 @@ import com.google.errorprone.fixes.SuggestedFixes; import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.matchers.Matchers; import com.google.errorprone.matchers.method.MethodMatchers; import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.MethodInvocationTree; @@ -32,16 +34,15 @@ public final class AssertThatIsNullUsageCheck extends BugChecker implements MethodInvocationTreeMatcher { private static final long serialVersionUID = 1L; - private final Matcher ASSERT_IS_EQUAL = - MethodMatchers.instanceMethod() - .onDescendantOf("org.assertj.core.api.Assert") - .named("isEqualTo"); + private static final Matcher ASSERT_IS_EQUAL = + Matchers.allOf( + instanceMethod().onDescendantOf("org.assertj.core.api.Assert").named("isEqualTo"), + Matchers.argumentCount(1), + Matchers.argument(0, Matchers.nullLiteral())); @Override public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { - if (!ASSERT_IS_EQUAL.matches(tree, state) - || tree.getArguments().size() != 1 - || tree.getArguments().get(0).getKind() != NULL_LITERAL) { + if (!ASSERT_IS_EQUAL.matches(tree, state)) { return Description.NO_MATCH; } From 11ab9b9af8a8aef18d4730eb8cd4bf95c99a7d01 Mon Sep 17 00:00:00 2001 From: Gijs de Jong Date: Thu, 16 Jun 2022 15:51:27 +0200 Subject: [PATCH 05/10] Remove unused imports --- .../errorprone/bugpatterns/AssertThatIsNullUsageCheck.java | 3 --- 1 file changed, 3 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssertThatIsNullUsageCheck.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssertThatIsNullUsageCheck.java index fb095b838d..07ca28fe35 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssertThatIsNullUsageCheck.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssertThatIsNullUsageCheck.java @@ -4,7 +4,6 @@ import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION; import static com.google.errorprone.BugPattern.StandardTags.SIMPLIFICATION; import static com.google.errorprone.matchers.Matchers.instanceMethod; -import static com.sun.source.tree.Tree.Kind.NULL_LITERAL; import com.google.auto.service.AutoService; import com.google.errorprone.BugPattern; @@ -15,8 +14,6 @@ import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Matcher; import com.google.errorprone.matchers.Matchers; -import com.google.errorprone.matchers.method.MethodMatchers; -import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.MethodInvocationTree; /** From b869e909a82493047c7df3788303046e0e5ef788 Mon Sep 17 00:00:00 2001 From: Gijs de Jong Date: Mon, 20 Jun 2022 10:41:55 +0200 Subject: [PATCH 06/10] Stylistic changes --- .../bugpatterns/AssertThatIsNullUsageCheck.java | 5 +---- .../bugpatterns/AssertThatIsNullUsageCheckTest.java | 13 ++++++------- 2 files changed, 7 insertions(+), 11 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssertThatIsNullUsageCheck.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssertThatIsNullUsageCheck.java index 07ca28fe35..ba7202b982 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssertThatIsNullUsageCheck.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssertThatIsNullUsageCheck.java @@ -16,10 +16,7 @@ import com.google.errorprone.matchers.Matchers; import com.sun.source.tree.MethodInvocationTree; -/** - * A {@link BugChecker} which flags {@code asserThat(someValue).isEqualTo(null)} for further - * simplification - */ +/** A {@link BugChecker} which flags {@code Assert.isEqualTo(null)} for further simplification */ @AutoService(BugChecker.class) @BugPattern( name = "AssertThatIsNullUsage", diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/AssertThatIsNullUsageCheckTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/AssertThatIsNullUsageCheckTest.java index f0e8028f5f..f9d970e294 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/AssertThatIsNullUsageCheckTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/AssertThatIsNullUsageCheckTest.java @@ -7,7 +7,6 @@ final class AssertThatIsNullUsageCheckTest { private final CompilationTestHelper compilationTestHelper = CompilationTestHelper.newInstance(AssertThatIsNullUsageCheck.class, getClass()); - private final BugCheckerRefactoringTestHelper refactoringTestHelper = BugCheckerRefactoringTestHelper.newInstance(AssertThatIsNullUsageCheck.class, getClass()); @@ -25,10 +24,10 @@ void identification() { " String nullValue = null;", " assertThat(12).isEqualTo(12);", " // BUG: Diagnostic contains: assertThat(...).isNull()", - " assertThat(\"value\").isEqualTo(null);", + " assertThat(\"foo\").isEqualTo(null);", " // BUG: Diagnostic contains: assertThat(...).isNull()", " assertThat(nullValue).isEqualTo(null);", - " isEqualTo(\"foo\");", + " isEqualTo(\"bar\");", " isEqualTo(null);", " }", "", @@ -52,9 +51,9 @@ void replacement() { " public void testAssertThat() {", " String nullValue = null;", " assertThat(12).isEqualTo(12);", - " assertThat(\"value\").isEqualTo(null);", + " assertThat(\"foo\").isEqualTo(null);", " assertThat(nullValue).isEqualTo(null);", - " isEqualTo(\"foo\");", + " isEqualTo(\"bar\");", " isEqualTo(null);", " }", "", @@ -72,9 +71,9 @@ void replacement() { " public void testAssertThat() {", " String nullValue = null;", " assertThat(12).isEqualTo(12);", - " assertThat(\"value\").isNull();", + " assertThat(\"foo\").isNull();", " assertThat(nullValue).isNull();", - " isEqualTo(\"foo\");", + " isEqualTo(\"bar\");", " isEqualTo(null);", " }", "", From 3bc7405e3b6800f69452e5cd5a1073635937db8d Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sat, 25 Jun 2022 20:06:25 +0200 Subject: [PATCH 07/10] Suggestions --- ...sageCheck.java => AssertJIsNullCheck.java} | 32 ++++--- .../bugpatterns/AssertJIsNullCheckTest.java | 64 ++++++++++++++ .../AssertThatIsNullUsageCheckTest.java | 86 ------------------- 3 files changed, 83 insertions(+), 99 deletions(-) rename error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/{AssertThatIsNullUsageCheck.java => AssertJIsNullCheck.java} (55%) create mode 100644 error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/AssertJIsNullCheckTest.java delete mode 100644 error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/AssertThatIsNullUsageCheckTest.java diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssertThatIsNullUsageCheck.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssertJIsNullCheck.java similarity index 55% rename from error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssertThatIsNullUsageCheck.java rename to error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssertJIsNullCheck.java index ba7202b982..4240a8721e 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssertThatIsNullUsageCheck.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssertJIsNullCheck.java @@ -3,43 +3,49 @@ 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.google.errorprone.matchers.Matchers; import com.sun.source.tree.MethodInvocationTree; -/** A {@link BugChecker} which flags {@code Assert.isEqualTo(null)} for further simplification */ +/** A {@link BugChecker} which flags AssertJ {@code isEqualTo(null)} checks for simplification. */ @AutoService(BugChecker.class) @BugPattern( - name = "AssertThatIsNullUsage", - summary = "`asserThat(...).isEqualTo(null)` should be `assertThat(...).isNull()`", + name = "AssertJIsNull", + summary = "Prefer `.isNull()` over `.isEqualTo(null)`", linkType = NONE, severity = SUGGESTION, tags = SIMPLIFICATION) -public final class AssertThatIsNullUsageCheck extends BugChecker - implements MethodInvocationTreeMatcher { +public final class AssertJIsNullCheck extends BugChecker implements MethodInvocationTreeMatcher { private static final long serialVersionUID = 1L; - - private static final Matcher ASSERT_IS_EQUAL = - Matchers.allOf( + private static final Matcher ASSERT_IS_EQUAL_TO_NULL = + allOf( instanceMethod().onDescendantOf("org.assertj.core.api.Assert").named("isEqualTo"), - Matchers.argumentCount(1), - Matchers.argument(0, Matchers.nullLiteral())); + argumentCount(1), + argument(0, nullLiteral())); @Override public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { - if (!ASSERT_IS_EQUAL.matches(tree, state)) { + if (!ASSERT_IS_EQUAL_TO_NULL.matches(tree, state)) { return Description.NO_MATCH; } - return describeMatch(tree, SuggestedFixes.renameMethodInvocation(tree, "isNull()", state)); + 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); + } +} diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/AssertThatIsNullUsageCheckTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/AssertThatIsNullUsageCheckTest.java deleted file mode 100644 index f9d970e294..0000000000 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/AssertThatIsNullUsageCheckTest.java +++ /dev/null @@ -1,86 +0,0 @@ -package tech.picnic.errorprone.bugpatterns; - -import com.google.errorprone.BugCheckerRefactoringTestHelper; -import com.google.errorprone.CompilationTestHelper; -import org.junit.jupiter.api.Test; - -final class AssertThatIsNullUsageCheckTest { - private final CompilationTestHelper compilationTestHelper = - CompilationTestHelper.newInstance(AssertThatIsNullUsageCheck.class, getClass()); - private final BugCheckerRefactoringTestHelper refactoringTestHelper = - BugCheckerRefactoringTestHelper.newInstance(AssertThatIsNullUsageCheck.class, getClass()); - - @Test - void identification() { - compilationTestHelper - .addSourceLines( - "A.java", - "import static org.assertj.core.api.Assertions.assertThat;", - "import com.google.common.collect.ImmutableSortedSet;", - "import org.junit.jupiter.api.Test;", - "public final class A {", - " @Test", - " public void testAssertThat() {", - " String nullValue = null;", - " assertThat(12).isEqualTo(12);", - " // BUG: Diagnostic contains: assertThat(...).isNull()", - " assertThat(\"foo\").isEqualTo(null);", - " // BUG: Diagnostic contains: assertThat(...).isNull()", - " assertThat(nullValue).isEqualTo(null);", - " isEqualTo(\"bar\");", - " 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;", - "import com.google.common.collect.ImmutableSortedSet;", - "import org.junit.jupiter.api.Test;", - "public final class A {", - " @Test", - " public void testAssertThat() {", - " String nullValue = null;", - " assertThat(12).isEqualTo(12);", - " assertThat(\"foo\").isEqualTo(null);", - " assertThat(nullValue).isEqualTo(null);", - " isEqualTo(\"bar\");", - " isEqualTo(null);", - " }", - "", - " private boolean isEqualTo (Object value) {", - " return value.equals(\"bar\");", - " }", - "} ") - .addOutputLines( - "A.java", - "import static org.assertj.core.api.Assertions.assertThat;", - "import com.google.common.collect.ImmutableSortedSet;", - "import org.junit.jupiter.api.Test;", - "public final class A {", - " @Test", - " public void testAssertThat() {", - " String nullValue = null;", - " assertThat(12).isEqualTo(12);", - " assertThat(\"foo\").isNull();", - " assertThat(nullValue).isNull();", - " isEqualTo(\"bar\");", - " isEqualTo(null);", - " }", - "", - " private boolean isEqualTo (Object value) {", - " return value.equals(\"bar\");", - " }", - "} ") - .doTest(); - } -} From fa7a8c888fc7570d678dcfa7a528d298ace96fb0 Mon Sep 17 00:00:00 2001 From: Gijs de Jong Date: Mon, 27 Jun 2022 11:13:20 +0200 Subject: [PATCH 08/10] Add reasoning for using a BugPattern instead of Refaster template --- .../picnic/errorprone/bugpatterns/AssertJIsNullCheck.java | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) 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 index 4240a8721e..6220a1b595 100644 --- 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 @@ -20,7 +20,13 @@ import com.google.errorprone.matchers.Matcher; import com.sun.source.tree.MethodInvocationTree; -/** A {@link BugChecker} which flags AssertJ {@code isEqualTo(null)} checks for simplification. */ +/** + * A {@link BugChecker} which flags AssertJ {@code isEqualTo(null)} checks for simplification.
+ * This cannot be done using a refaster template, as refaster is unable to match the abstraction + * layers in AssertJ + * + *

Example: assertThat("foo").isEqualTo(null) will not be matched by refaster. + */ @AutoService(BugChecker.class) @BugPattern( name = "AssertJIsNull", From 6263b17828a2ee0e9bb1ab46ca875feac80a9abb Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Tue, 28 Jun 2022 15:26:11 +0200 Subject: [PATCH 09/10] Javadoc suggestions --- .../picnic/errorprone/bugpatterns/AssertJIsNullCheck.java | 7 +++---- 1 file changed, 3 insertions(+), 4 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 index 6220a1b595..b3eadfed89 100644 --- 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 @@ -21,11 +21,10 @@ import com.sun.source.tree.MethodInvocationTree; /** - * A {@link BugChecker} which flags AssertJ {@code isEqualTo(null)} checks for simplification.
- * This cannot be done using a refaster template, as refaster is unable to match the abstraction - * layers in AssertJ + * A {@link BugChecker} which flags AssertJ {@code isEqualTo(null)} checks for simplification. * - *

Example: assertThat("foo").isEqualTo(null) will not be matched by refaster. + *

This cannot be done by using only Refaster templates, as Refaster is not able to match *all* + * {@code AbstractAssert} subtypes correctly. */ @AutoService(BugChecker.class) @BugPattern( From dabe4371d5408e29642dcf9edc355cb2c93181b7 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Wed, 29 Jun 2022 09:56:25 +0200 Subject: [PATCH 10/10] Suggestion --- .../picnic/errorprone/bugpatterns/AssertJIsNullCheck.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 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 index b3eadfed89..a20551e4c1 100644 --- 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 @@ -23,8 +23,10 @@ /** * A {@link BugChecker} which flags AssertJ {@code isEqualTo(null)} checks for simplification. * - *

This cannot be done by using only Refaster templates, as Refaster is not able to match *all* - * {@code AbstractAssert} subtypes correctly. + *

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(