diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/OptionalOrElseMethodInvocation.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/OptionalOrElseMethodInvocation.java index 9f563a2d6..65bb3002a 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/OptionalOrElseMethodInvocation.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/OptionalOrElseMethodInvocation.java @@ -29,7 +29,6 @@ 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; @AutoService(BugChecker.class) @BugPattern( @@ -46,8 +45,13 @@ public final class OptionalOrElseMethodInvocation extends BugChecker implements .onExactClass("java.util.Optional") .named("orElse"); - private static final Matcher METHOD_INVOCATIONS = - Matchers.contains(ExpressionTree.class, MethodMatchers.anyMethod()); + private static final Matcher METHOD_OR_CONSTRUCTOR = Matchers.anyOf( + MethodMatchers.anyMethod(), + MethodMatchers.constructor()); + + private static final Matcher METHOD_INVOCATIONS = Matchers.anyOf( + METHOD_OR_CONSTRUCTOR, + Matchers.contains(ExpressionTree.class, METHOD_OR_CONSTRUCTOR)); @Override public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/CatchBlockLogExceptionTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/CatchBlockLogExceptionTest.java index eb8fae3ba..090afbb7b 100644 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/CatchBlockLogExceptionTest.java +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/CatchBlockLogExceptionTest.java @@ -34,10 +34,22 @@ public void before() { } @Test - public void testLogException() { + public void testLogIllegalArgumentException() { test(IllegalArgumentException.class, "log.info(\"hello\", e);", Optional.empty()); + } + + @Test + public void testLogRuntimeException() { test(RuntimeException.class, "log.info(\"hello\", e);", Optional.empty()); + } + + @Test + public void testLogException() { test(Exception.class, "log.info(\"hello\", e);", Optional.empty()); + } + + @Test + public void testLogThrowable() { test(Throwable.class, "log.info(\"hello\", e);", Optional.empty()); } diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/DangerousJsonTypeInfoUsageTests.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/DangerousJsonTypeInfoUsageTests.java index 81b7f643c..67fece80b 100644 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/DangerousJsonTypeInfoUsageTests.java +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/DangerousJsonTypeInfoUsageTests.java @@ -30,24 +30,56 @@ public void before() { } @Test - public void testMustNotUseClassVariants() throws Exception { + public void testClass() { positive("JsonTypeInfo.Id.CLASS"); - positive("JsonTypeInfo.Id.MINIMAL_CLASS"); + } + + @Test + public void testClass_fullyQualified() { positive("com.fasterxml.jackson.annotation.JsonTypeInfo.Id.CLASS"); + } + + @Test + public void testMinimalClass() { + positive("JsonTypeInfo.Id.MINIMAL_CLASS"); + } + + @Test + public void testMinimalClass_fullyQualified() { positive("com.fasterxml.jackson.annotation.JsonTypeInfo.Id.MINIMAL_CLASS"); } @Test - public void testMayUseNoneNameCustomVariants() throws Exception { + public void testNone() { negative("JsonTypeInfo.Id.NONE"); - negative("JsonTypeInfo.Id.NAME"); - negative("JsonTypeInfo.Id.CUSTOM"); + } + + @Test + public void testNone_fullyQualified() { negative("com.fasterxml.jackson.annotation.JsonTypeInfo.Id.NONE"); + } + + @Test + public void testName() { + negative("JsonTypeInfo.Id.NAME"); + } + + @Test + public void testName_fullyQualified() { negative("com.fasterxml.jackson.annotation.JsonTypeInfo.Id.NAME"); + } + + @Test + public void testCustom() { + negative("JsonTypeInfo.Id.CUSTOM"); + } + + @Test + public void testCustom_fullyQualified() { negative("com.fasterxml.jackson.annotation.JsonTypeInfo.Id.CUSTOM"); } - private void positive(String variant) throws Exception { + private void positive(String variant) { compilationHelper.addSourceLines( "Bean.java", "import com.fasterxml.jackson.annotation.JsonTypeInfo;", @@ -57,7 +89,7 @@ private void positive(String variant) throws Exception { ).doTest(); } - private void negative(String variant) throws Exception { + private void negative(String variant) { compilationHelper.addSourceLines( "Bean.java", "import com.fasterxml.jackson.annotation.JsonTypeInfo;", diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/PreconditionsConstantMessageTests.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/GuavaPreconditionsConstantMessageTests.java similarity index 58% rename from baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/PreconditionsConstantMessageTests.java rename to baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/GuavaPreconditionsConstantMessageTests.java index e29c4f08f..591942293 100644 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/PreconditionsConstantMessageTests.java +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/GuavaPreconditionsConstantMessageTests.java @@ -20,7 +20,9 @@ import org.junit.Before; import org.junit.Test; -public final class PreconditionsConstantMessageTests extends PreconditionsTests { +public final class GuavaPreconditionsConstantMessageTests extends PreconditionsTests { + + private static final String DIAGNOSTIC = "non-constant message"; private CompilationTestHelper compilationHelper; @@ -35,15 +37,34 @@ public CompilationTestHelper compilationHelper() { } @Test - public void positive() throws Exception { - String diagnostic = "non-constant message"; - failGuava(diagnostic, "Preconditions.checkArgument(param != \"string\", \"constant\" + param);"); - failGuava(diagnostic, "Preconditions.checkState(param != \"string\", \"constant\" + param);"); - failGuava(diagnostic, "Preconditions.checkNotNull(param, \"constant\" + param);"); + public void testCheckArgument_stringConcatenate() { + failGuava(DIAGNOSTIC, "Preconditions.checkArgument(param != \"string\", \"constant\" + param);"); + } - failGuava(diagnostic, + @Test + public void testCheckArgument_stringFormat() { + failGuava(DIAGNOSTIC, "Preconditions.checkArgument(param != \"string\", String.format(\"constant %s\", param));"); - failGuava(diagnostic, "Preconditions.checkState(param != \"string\", String.format(\"constant %s\", param));"); - failGuava(diagnostic, "Preconditions.checkNotNull(param, String.format(\"constant %s\", param));"); + } + + @Test + public void testCheckState_stringConcatenate() { + failGuava(DIAGNOSTIC, "Preconditions.checkState(param != \"string\", \"constant\" + param);"); + } + + @Test + public void testCheckState_stringFormat() { + failGuava(DIAGNOSTIC, + "Preconditions.checkState(param != \"string\", String.format(\"constant %s\", param));"); + } + + @Test + public void testCheckNotNull_stringConcatenate() { + failGuava(DIAGNOSTIC, "Preconditions.checkNotNull(param, \"constant\" + param);"); + } + + @Test + public void testCheckNotNull_stringFormat() { + failGuava(DIAGNOSTIC, "Preconditions.checkNotNull(param, String.format(\"constant %s\", param));"); } } diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/GuavaPreconditionsMessageFormatTests.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/GuavaPreconditionsMessageFormatTests.java index 39d372c0e..b4e4f2f53 100755 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/GuavaPreconditionsMessageFormatTests.java +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/GuavaPreconditionsMessageFormatTests.java @@ -21,6 +21,9 @@ import org.junit.Test; public final class GuavaPreconditionsMessageFormatTests extends PreconditionsTests { + + private static final String DIAGNOSTIC = "Use printf-style formatting"; + private CompilationTestHelper compilationHelper; @Before @@ -34,14 +37,32 @@ public CompilationTestHelper compilationHelper() { } @Test - public void positive() throws Exception { - String diagnostic = "Use printf-style formatting"; - failGuava(diagnostic, "Preconditions.checkArgument(param != \"string\", \"message {}\", 123L);"); - failGuava(diagnostic, "Preconditions.checkState(param != \"string\", \"message {}\", 123L);"); - failGuava(diagnostic, "Preconditions.checkNotNull(param, \"message {}\", 123L);"); - - failGuava(diagnostic, "Preconditions.checkArgument(param != \"string\", \"message {} {}\", 'a', 'b');"); - failGuava(diagnostic, "Preconditions.checkState(param != \"string\", \"message {} {}\", 'a', 'b');"); - failGuava(diagnostic, "Preconditions.checkNotNull(param, \"message {} {}\", 'a', 'b');"); + public void testCheckArgument() { + failGuava(DIAGNOSTIC, "Preconditions.checkArgument(param != \"string\", \"message {}\", 123L);"); + } + + @Test + public void testCheckArgument_multipleArgs() { + failGuava(DIAGNOSTIC, "Preconditions.checkArgument(param != \"string\", \"message {} {}\", 'a', 'b');"); + } + + @Test + public void testCheckState() { + failGuava(DIAGNOSTIC, "Preconditions.checkState(param != \"string\", \"message {}\", 123L);"); + } + + @Test + public void testCheckState_multipleArgs() { + failGuava(DIAGNOSTIC, "Preconditions.checkState(param != \"string\", \"message {} {}\", 'a', 'b');"); + } + + @Test + public void testCheckNotNull() { + failGuava(DIAGNOSTIC, "Preconditions.checkNotNull(param, \"message {}\", 123L);"); + } + + @Test + public void testCheckNotNull_multipleArgs() { + failGuava(DIAGNOSTIC, "Preconditions.checkNotNull(param, \"message {} {}\", 'a', 'b');"); } } diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/LogSafePreconditionsConstantMessageTests.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/LogSafePreconditionsConstantMessageTests.java index c181e0707..258cff293 100644 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/LogSafePreconditionsConstantMessageTests.java +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/LogSafePreconditionsConstantMessageTests.java @@ -22,6 +22,8 @@ public final class LogSafePreconditionsConstantMessageTests extends PreconditionsTests { + private static final String DIAGNOSTIC = "non-constant message"; + private CompilationTestHelper compilationHelper; @Before @@ -35,17 +37,34 @@ public CompilationTestHelper compilationHelper() { } @Test - public void positive() throws Exception { - String diagnostic = "non-constant message"; - - failLogSafe(diagnostic, "Preconditions.checkArgument(param != \"string\", \"constant\" + param);"); - failLogSafe(diagnostic, "Preconditions.checkState(param != \"string\", \"constant\" + param);"); - failLogSafe(diagnostic, "Preconditions.checkNotNull(param, \"constant\" + param);"); + public void testCheckArgument_stringConcatenate() { + failLogSafe(DIAGNOSTIC, "Preconditions.checkArgument(param != \"string\", \"constant\" + param);"); + } - failLogSafe(diagnostic, + @Test + public void testCheckArgument_stringFormat() { + failLogSafe(DIAGNOSTIC, "Preconditions.checkArgument(param != \"string\", String.format(\"constant %s\", param));"); - failLogSafe(diagnostic, + } + + @Test + public void testCheckState_stringConcatenate() { + failLogSafe(DIAGNOSTIC, "Preconditions.checkState(param != \"string\", \"constant\" + param);"); + } + + @Test + public void testCheckState_stringFormat() { + failLogSafe(DIAGNOSTIC, "Preconditions.checkState(param != \"string\", String.format(\"constant %s\", param));"); - failLogSafe(diagnostic, "Preconditions.checkNotNull(param, String.format(\"constant %s\", param));"); + } + + @Test + public void testCheckNotNull_stringConcatenate() { + failLogSafe(DIAGNOSTIC, "Preconditions.checkNotNull(param, \"constant\" + param);"); + } + + @Test + public void testCheckNotNull_stringFormat() { + failLogSafe(DIAGNOSTIC, "Preconditions.checkNotNull(param, String.format(\"constant %s\", param));"); } } diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/LogSafePreconditionsMessageFormatTests.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/LogSafePreconditionsMessageFormatTests.java index 8a617fb30..5b33d1fe2 100755 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/LogSafePreconditionsMessageFormatTests.java +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/LogSafePreconditionsMessageFormatTests.java @@ -21,6 +21,9 @@ import org.junit.Test; public final class LogSafePreconditionsMessageFormatTests extends PreconditionsTests { + + private static final String DIAGNOSTIC = "Do not use printf-style formatting"; + private CompilationTestHelper compilationHelper; @Before @@ -34,20 +37,38 @@ public CompilationTestHelper compilationHelper() { } @Test - public void positive() throws Exception { - String diagnostic = "Do not use printf-style formatting"; - failLogSafe(diagnostic, "Preconditions.checkArgument(param != \"string\", \"message %s\"," - + " UnsafeArg.of(\"long\", 123L));"); - failLogSafe(diagnostic, "Preconditions.checkState(param != \"string\", \"message %s\"," - + " UnsafeArg.of(\"long\", 123L));"); - failLogSafe(diagnostic, "Preconditions.checkNotNull(param, \"message %s\"," + public void testCheckArgument() { + failLogSafe(DIAGNOSTIC, "Preconditions.checkArgument(param != \"string\", \"message %s\"," + " UnsafeArg.of(\"long\", 123L));"); + } - failLogSafe(diagnostic, "Preconditions.checkArgument(param != \"string\", \"message %s %s\"," + @Test + public void testCheckArgument_multipleArgs() { + failLogSafe(DIAGNOSTIC, "Preconditions.checkArgument(param != \"string\", \"message %s %s\"," + " UnsafeArg.of(\"char1\", 'a'), UnsafeArg.of(\"char2\", 'b'));"); - failLogSafe(diagnostic, "Preconditions.checkState(param != \"string\", \"message %s %s\"," + } + + @Test + public void testCheckState() { + failLogSafe(DIAGNOSTIC, "Preconditions.checkState(param != \"string\", \"message %s\"," + + " UnsafeArg.of(\"long\", 123L));"); + } + + @Test + public void testCheckState_multipleArgs() { + failLogSafe(DIAGNOSTIC, "Preconditions.checkState(param != \"string\", \"message %s %s\"," + " UnsafeArg.of(\"char1\", 'a'), UnsafeArg.of(\"char2\", 'b'));"); - failLogSafe(diagnostic, "Preconditions.checkNotNull(param, \"message %s %s\"," + } + + @Test + public void testCheckNotNull() { + failLogSafe(DIAGNOSTIC, "Preconditions.checkNotNull(param, \"message %s\"," + + " UnsafeArg.of(\"long\", 123L));"); + } + + @Test + public void testCheckNotNull_multipleArgs() { + failLogSafe(DIAGNOSTIC, "Preconditions.checkNotNull(param, \"message %s %s\"," + " UnsafeArg.of(\"char1\", 'a'), UnsafeArg.of(\"char2\", 'b'));"); } } diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/OptionalOrElseMethodInvocationTests.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/OptionalOrElseMethodInvocationTests.java index e6a078edb..a9f82c7e4 100644 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/OptionalOrElseMethodInvocationTests.java +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/OptionalOrElseMethodInvocationTests.java @@ -33,66 +33,131 @@ public void before() { new OptionalOrElseMethodInvocation(), getClass()); } - private void test(String expr) { + @Test + public void testMethodInvocation() { compilationHelper .addSourceLines( "Test.java", "import java.util.Optional;", "class Test {", " String f() { return \"hello\"; }", - " String s = \"world\";", " // BUG: Diagnostic contains: invokes a method", - " private final String string = Optional.of(\"hello\").orElse(" + expr + ");", + " private final String string = Optional.of(\"hello\").orElse(f());", "}") .doTest(); } @Test - public void testNonCompileTimeConstantExpression() { - test("f()"); - test("s + s"); - test("\"world\" + s"); - test("\"world\".substring(1)"); + public void testContainsMethodInvocation() { + compilationHelper + .addSourceLines( + "Test.java", + "import java.util.Optional;", + "class Test {", + " String f() { return \"hello\"; }", + " // BUG: Diagnostic contains: invokes a method", + " private final String string = Optional.of(\"hello\").orElse(\"world\" + f());", + "}") + .doTest(); } @Test - public void testNonCompileTimeConstantExpression_replacement() { - refactoringTestHelper - .addInputLines( + public void testConstructor() { + compilationHelper + .addSourceLines( "Test.java", "import java.util.Optional;", "class Test {", - " String f() { return \"hello\"; }", - " private final String string = Optional.of(\"hello\").orElse(f());", + " // BUG: Diagnostic contains: invokes a method", + " private final String string = Optional.of(\"hello\").orElse(new String());", "}") - .addOutputLines( + .doTest(); + } + + @Test + public void testContainsConstructor() { + compilationHelper + .addSourceLines( "Test.java", "import java.util.Optional;", "class Test {", - " String f() { return \"hello\"; }", - " private final String string = Optional.of(\"hello\").orElseGet(() -> f());", + " // BUG: Diagnostic contains: invokes a method", + " private final String string = Optional.of(\"hello\").orElse(\"world\" + new String());", "}") - .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + .doTest(); + } + + @Test + public void testLiteral() { + compilationHelper + .addSourceLines( + "Test.java", + "import java.util.Optional;", + "class Test {", + " private final String string = Optional.of(\"hello\").orElse(\"constant\");", + "}") + .doTest(); } @Test - public void negative() { + public void testConstant() { + compilationHelper + .addSourceLines( + "Test.java", + "import java.util.Optional;", + "class Test {", + " private static final String constant = \"constant\";", + " private final String string = Optional.of(\"hello\").orElse(constant);", + "}") + .doTest(); + } + + @Test + public void testVariable() { compilationHelper .addSourceLines( "Test.java", "import java.util.Optional;", "class Test {", - " private static final String compileTimeConstant = \"constant\";", " String f() { return \"hello\"; }", " void test() {", - " Optional.of(\"hello\").orElse(\"constant\");", - " Optional.of(\"hello\").orElse(compileTimeConstant);", - " String string = f();", - " Optional.of(\"hello\").orElse(string);", - " Optional.of(\"hello\").orElseGet(() -> f());", + " String variable = f();", + " Optional.of(\"hello\").orElse(variable);", " }", "}") .doTest(); } + @Test + public void testOrElseGet() { + compilationHelper + .addSourceLines( + "Test.java", + "import java.util.Optional;", + "class Test {", + " String f() { return \"hello\"; }", + " private final String string = Optional.of(\"hello\").orElseGet(() -> f());", + "}") + .doTest(); + } + + @Test + public void testReplacement() { + refactoringTestHelper + .addInputLines( + "Test.java", + "import java.util.Optional;", + "class Test {", + " String f() { return \"hello\"; }", + " private final String string = Optional.of(\"hello\").orElse(f());", + "}") + .addOutputLines( + "Test.java", + "import java.util.Optional;", + "class Test {", + " String f() { return \"hello\"; }", + " private final String string = Optional.of(\"hello\").orElseGet(() -> f());", + "}") + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } } diff --git a/changelog/@unreleased/pr-702.v2.yml b/changelog/@unreleased/pr-702.v2.yml new file mode 100644 index 000000000..2d09c1d22 --- /dev/null +++ b/changelog/@unreleased/pr-702.v2.yml @@ -0,0 +1,5 @@ +type: feature +feature: + description: OptionalOrElseMethodInvocation now checks for constructor invocations. + links: + - https://github.com/palantir/gradle-baseline/pull/702