From a3494b574c5cd9a39e6f690634426d5c1f2561ee Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Fri, 11 Oct 2019 14:21:31 -0400 Subject: [PATCH 1/5] Implement Error Prone `ThrowError` to discourage throwing Errors 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. --- .../baseline/errorprone/ThrowError.java | 66 +++++++++++++++ .../baseline/errorprone/ThrowErrorTest.java | 84 +++++++++++++++++++ 2 files changed, 150 insertions(+) create mode 100644 baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ThrowError.java create mode 100644 baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ThrowErrorTest.java 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..f8d5ccc44 --- /dev/null +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ThrowError.java @@ -0,0 +1,66 @@ +/* + * (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.matchers.Description; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.NewClassTree; +import com.sun.source.tree.ThrowTree; + +@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 OutOfMemoryError when allocations fail. 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 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); + } + } + return Description.NO_MATCH; + } +} 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..e08c300f9 --- /dev/null +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ThrowErrorTest.java @@ -0,0 +1,84 @@ +/* + * (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.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(); + } + + private CompilationTestHelper helper() { + return CompilationTestHelper.newInstance(ThrowError.class, getClass()); + } +} From 0dfcca6c371ea19d43fcbc6d4ff3418b78e6090e Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Fri, 11 Oct 2019 15:31:07 -0400 Subject: [PATCH 2/5] Implement a suggested fix --- .../baseline/errorprone/ThrowError.java | 68 +++++++++++++++---- .../baseline/errorprone/ThrowErrorTest.java | 50 ++++++++++++++ .../BaselineErrorProneExtension.java | 1 + 3 files changed, 107 insertions(+), 12 deletions(-) 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", From df5a245ce0b495fe031795cfdf80ad2c2aef2c6a Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Fri, 11 Oct 2019 19:31:07 +0000 Subject: [PATCH 3/5] Add generated changelog entries --- changelog/@unreleased/pr-957.v2.yml | 13 +++++++++++++ 1 file changed, 13 insertions(+) create mode 100644 changelog/@unreleased/pr-957.v2.yml 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 From 6386b56ad22226715e343d5651373f2cb8702f7e Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Sun, 13 Oct 2019 14:20:14 -0400 Subject: [PATCH 4/5] update readme --- README.md | 1 + 1 file changed, 1 insertion(+) 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 From 027e34059dddaf5917d2b0fe125da9ea66cf769b Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Mon, 14 Oct 2019 09:27:40 -0400 Subject: [PATCH 5/5] Update example from OOM to CNF --- .../main/java/com/palantir/baseline/errorprone/ThrowError.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 543026312..b876e5200 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 @@ -44,7 +44,7 @@ + "'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 OutOfMemoryError when allocations fail. These should be " + + "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 "