diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ThrowError.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ThrowError.java index f8d5ccc44..543026312 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ThrowError.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ThrowError.java @@ -20,11 +20,18 @@ import com.google.errorprone.BugPattern; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.fixes.SuggestedFixes; +import com.google.errorprone.matchers.CompileTimeConstantExpressionMatcher; import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matcher; import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.NewClassTree; import com.sun.source.tree.ThrowTree; +import com.sun.tools.javac.code.Type; +import java.util.List; +import java.util.Optional; @AutoService(BugChecker.class) @BugPattern( @@ -44,23 +51,60 @@ + "that throws AssertionError.") public final class ThrowError extends BugChecker implements BugChecker.ThrowTreeMatcher { + private static final Matcher compileTimeConstExpressionMatcher = + new CompileTimeConstantExpressionMatcher(); private static final String ERROR_NAME = Error.class.getName(); @Override public Description matchThrow(ThrowTree tree, VisitorState state) { ExpressionTree expression = tree.getExpression(); - if (expression instanceof NewClassTree) { - NewClassTree newClassTree = (NewClassTree) expression; - if (ASTHelpers.isCastable( - ASTHelpers.getType(newClassTree.getIdentifier()), - state.getTypeFromString(ERROR_NAME), - state) - // Don't discourage developers from testing edge cases involving Errors. - // It's also fine for tests throw AssertionError internally in test objects. - && !TestCheckUtils.isTestCode(state)) { - return describeMatch(tree); - } + if (!(expression instanceof NewClassTree)) { + return Description.NO_MATCH; } - return Description.NO_MATCH; + NewClassTree newClassTree = (NewClassTree) expression; + Type throwableType = ASTHelpers.getType(newClassTree.getIdentifier()); + if (!ASTHelpers.isCastable( + throwableType, + state.getTypeFromString(ERROR_NAME), + state) + // Don't discourage developers from testing edge cases involving Errors. + // It's also fine for tests throw AssertionError internally in test objects. + || TestCheckUtils.isTestCode(state)) { + return Description.NO_MATCH; + } + return buildDescription(tree) + .addFix(generateFix(newClassTree, state)) + .build(); + } + + private static Optional generateFix(NewClassTree newClassTree, VisitorState state) { + Type throwableType = ASTHelpers.getType(newClassTree.getIdentifier()); + // AssertionError is the most common failure case we've encountered, likely because it sounds + // similar to IllegalStateException. In this case we suggest replacing it with IllegalStateException. + if (!ASTHelpers.isSameType( + throwableType, + state.getTypeFromString(AssertionError.class.getName()), + state)) { + return Optional.empty(); + } + List arguments = newClassTree.getArguments(); + if (arguments.isEmpty()) { + SuggestedFix.Builder fix = SuggestedFix.builder(); + String qualifiedName = SuggestedFixes.qualifyType(state, fix, IllegalStateException.class.getName()); + return Optional.of(fix.replace(newClassTree.getIdentifier(), qualifiedName).build()); + } + ExpressionTree firstArgument = arguments.get(0); + if (ASTHelpers.isSameType( + ASTHelpers.getResultType(firstArgument), + state.getTypeFromString(String.class.getName()), + state)) { + SuggestedFix.Builder fix = SuggestedFix.builder(); + String qualifiedName = SuggestedFixes.qualifyType(state, fix, + compileTimeConstExpressionMatcher.matches(firstArgument, state) + ? "com.palantir.logsafe.exceptions.SafeIllegalStateException" + : IllegalStateException.class.getName()); + return Optional.of(fix.replace(newClassTree.getIdentifier(), qualifiedName).build()); + } + return Optional.empty(); } } diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ThrowErrorTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ThrowErrorTest.java index e08c300f9..a54fdcd13 100644 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ThrowErrorTest.java +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ThrowErrorTest.java @@ -16,6 +16,7 @@ package com.palantir.baseline.errorprone; +import com.google.errorprone.BugCheckerRefactoringTestHelper; import com.google.errorprone.CompilationTestHelper; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.parallel.Execution; @@ -78,7 +79,56 @@ void testRethrowIsAllowed() { ).doTest(); } + @Test + void testFix() { + fix() + .addInputLines( + "Test.java", + "class Test {", + " void f1() {", + " throw new AssertionError();", + " }", + " void f2(String nonConstant) {", + " throw new AssertionError(nonConstant);", + " }", + " void f3() {", + " throw new AssertionError(\"constant\");", + " }", + " void f4(String nonConstant, Throwable t) {", + " throw new AssertionError(nonConstant, t);", + " }", + " void f5(Throwable t) {", + " throw new AssertionError(\"constant\", t);", + " }", + "}") + .addOutputLines( + "Test.java", + "import com.palantir.logsafe.exceptions.SafeIllegalStateException;", + "class Test {", + " void f1() {", + " throw new IllegalStateException();", + " }", + " void f2(String nonConstant) {", + " throw new IllegalStateException(nonConstant);", + " }", + " void f3() {", + " throw new SafeIllegalStateException(\"constant\");", + " }", + " void f4(String nonConstant, Throwable t) {", + " throw new IllegalStateException(nonConstant, t);", + " }", + " void f5(Throwable t) {", + " throw new SafeIllegalStateException(\"constant\", t);", + " }", + "}") + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + private CompilationTestHelper helper() { return CompilationTestHelper.newInstance(ThrowError.class, getClass()); } + + private BugCheckerRefactoringTestHelper fix() { + return BugCheckerRefactoringTestHelper.newInstance(new ThrowError(), getClass()); + } } diff --git a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/extensions/BaselineErrorProneExtension.java b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/extensions/BaselineErrorProneExtension.java index 9fa021f08..dfbe56c16 100644 --- a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/extensions/BaselineErrorProneExtension.java +++ b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/extensions/BaselineErrorProneExtension.java @@ -34,6 +34,7 @@ public class BaselineErrorProneExtension { "PreferSafeLoggingPreconditions", "StrictUnusedVariable", "StringBuilderConstantParameters", + "ThrowError", // Built-in checks "ArrayEquals",