diff --git a/README.md b/README.md index 9cb9528d3..7d0886edd 100644 --- a/README.md +++ b/README.md @@ -170,6 +170,7 @@ Safe Logging can be found at [github.com/palantir/safe-logging](https://github.c - `StringBuilderConstantParameters`: StringBuilder with a constant number of parameters should be replaced by simple concatenation. - `JUnit5SuiteMisuse`: When migrating from JUnit4 -> JUnit5, classes annotated with `@RunWith(Suite.class)` are dangerous because if they reference any JUnit5 test classes, these tests will silently not run! - `PreferAssertj`: Prefer AssertJ fluent assertions. +- `ThrowError`: Prefer throwing a RuntimeException rather than Error. ### Programmatic Application 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 new file mode 100644 index 000000000..b876e5200 --- /dev/null +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ThrowError.java @@ -0,0 +1,110 @@ +/* + * (c) Copyright 2019 Palantir Technologies Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.palantir.baseline.errorprone; + +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.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( + name = "ThrowError", + link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks", + linkType = BugPattern.LinkType.CUSTOM, + severity = BugPattern.SeverityLevel.WARNING, + summary = "Prefer throwing a RuntimeException rather than Error. Errors are often handled poorly by libraries " + + "resulting in unexpected behavior and resource leaks. It's not obvious that " + + "'catch (Exception e)' does not catch Error.\n" + + "Errors are normally thrown by the JVM when the system, not just the application, " + + "is in a bad state. For example, LinkageError is thrown by the JVM when it encounters " + + "incompatible classes, and NoClassDefFoundError when a class cannot be found. These should be " + + "less common and handled differently from application failures.\n" + + "This check is intended to be advisory - it's fine to @SuppressWarnings(\"ThrowError\") " + + "in certain cases, but is usually not recommended unless you are writing a testing library " + + "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)) { + 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 new file mode 100644 index 000000000..a54fdcd13 --- /dev/null +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ThrowErrorTest.java @@ -0,0 +1,134 @@ +/* + * (c) Copyright 2019 Palantir Technologies Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +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; +import org.junit.jupiter.api.parallel.ExecutionMode; + +@Execution(ExecutionMode.CONCURRENT) +class ThrowErrorTest { + + @Test + void testAssertionError() { + helper().addSourceLines( + "Test.java", + "class Test {", + " void f() {", + " // BUG: Diagnostic contains: Prefer throwing a RuntimeException", + " throw new AssertionError();", + " }", + "}" + ).doTest(); + } + + @Test + void testError() { + helper().addSourceLines( + "Test.java", + "class Test {", + " void f() {", + " // BUG: Diagnostic contains: Prefer throwing a RuntimeException", + " throw new Error();", + " }", + "}" + ).doTest(); + } + + @Test + void testError_testCode() { + // It's common to avoid handling Error by catching and rethrowing, this should be allowed. This check + // is meant to dissuade developers from creating and throwing new Errors. + helper().addSourceLines( + "TestCase.java", + "import " + Test.class.getName() + ';', + "class TestCase {", + " @Test", + " void f() {", + " throw new Error();", + " }", + "}" + ).doTest(); + } + + @Test + void testRethrowIsAllowed() { + helper().addSourceLines( + "Test.java", + "class Test {", + " void f(Error e) {", + " throw e;", + " }", + "}" + ).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/changelog/@unreleased/pr-957.v2.yml b/changelog/@unreleased/pr-957.v2.yml new file mode 100644 index 000000000..c97386452 --- /dev/null +++ b/changelog/@unreleased/pr-957.v2.yml @@ -0,0 +1,13 @@ +type: improvement +improvement: + description: |- + Implement Error Prone `ThrowError` to discourage throwing Errors in production code + Errors are often handled poorly by libraries resulting in unexpected + behavior and resource leaks. It's not obvious that 'catch (Exception e)' + does not catch Error. + This check is intended to be advisory - it's fine to + `@SuppressWarnings("ThrowError")` in certain cases, but is usually not + recommended unless you are writing a testing library that throws + AssertionError. + links: + - https://github.com/palantir/gradle-baseline/pull/957 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",