-
Notifications
You must be signed in to change notification settings - Fork 134
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
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.
- Loading branch information
1 parent
cad7a38
commit a3494b5
Showing
2 changed files
with
150 additions
and
0 deletions.
There are no files selected for viewing
66 changes: 66 additions & 0 deletions
66
baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ThrowError.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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; | ||
} | ||
} |
84 changes: 84 additions & 0 deletions
84
baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ThrowErrorTest.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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()); | ||
} | ||
} |