diff --git a/README.md b/README.md index 3b2523249..1a9c7c29d 100644 --- a/README.md +++ b/README.md @@ -185,6 +185,7 @@ Safe Logging can be found at [github.com/palantir/safe-logging](https://github.c - `RawTypes`: Avoid raw types; add appropriate type parameters if possible. - `VisibleForTestingPackagePrivate`: `@VisibleForTesting` members should be package-private. - `OptionalFlatMapOfNullable`: Optional.map functions may return null to safely produce an empty result. +- `ExtendsErrorOrThrowable`: Avoid extending Error (or subclasses of it) or Throwable directly. ### Programmatic Application diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ExtendsErrorOrThrowable.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ExtendsErrorOrThrowable.java new file mode 100644 index 000000000..414d55728 --- /dev/null +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ExtendsErrorOrThrowable.java @@ -0,0 +1,73 @@ +/* + * (c) Copyright 2020 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.Description; +import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.matchers.Matchers; +import com.google.errorprone.suppliers.Suppliers; +import com.sun.source.tree.ClassTree; +import com.sun.source.tree.Tree; +import com.sun.tools.javac.code.Type; +import java.util.Optional; + +@AutoService(BugChecker.class) +@BugPattern( + name = "ExtendsErrorOrThrowable", + link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks", + linkType = BugPattern.LinkType.CUSTOM, + providesFix = BugPattern.ProvidesFix.REQUIRES_HUMAN_ATTENTION, + severity = BugPattern.SeverityLevel.ERROR, + summary = "Class should not extend Error or Throwable directly. While allowed by java " + + "it can lead to surprising behaviour if users end up catching Error or Throwable.") +public final class ExtendsErrorOrThrowable extends BugChecker implements BugChecker.ClassTreeMatcher { + private static final Matcher IS_ERROR_SUBCLASS = Matchers.isSubtypeOf(Error.class); + private static final Matcher IS_THROWABLE = Matchers.isSameType(Throwable.class); + private static final Matcher IS_ERROR_OR_THROWABLE = + Matchers.anyOf(Matchers.isSameType(Error.class), Matchers.isSameType(Throwable.class)); + + @Override + public Description matchClass(ClassTree tree, VisitorState state) { + if (tree.getKind() != Tree.Kind.CLASS) { + // Don't apply to out interfaces and enums + return Description.NO_MATCH; + } + + // Match only cases where we extend from any Error type or directly from Throwable + if (!(IS_ERROR_SUBCLASS.matches(tree, state) || IS_THROWABLE.matches(tree.getExtendsClause(), state))) { + return Description.NO_MATCH; + } + + return buildDescription(tree).addFix(buildFix(tree, state)).build(); + } + + private static Optional buildFix(ClassTree tree, VisitorState state) { + if (IS_ERROR_OR_THROWABLE.matches(tree.getExtendsClause(), state)) { + Type exceptionType = Suppliers.typeFromClass(RuntimeException.class).get(state); + String prettyExceptionType = SuggestedFixes.prettyType(exceptionType, state); + return Optional.of(SuggestedFix.replace(tree.getExtendsClause(), prettyExceptionType)); + } else { + return Optional.empty(); + } + } +} diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ExtendsErrorOrThrowableTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ExtendsErrorOrThrowableTest.java new file mode 100644 index 000000000..e9e9e4146 --- /dev/null +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ExtendsErrorOrThrowableTest.java @@ -0,0 +1,99 @@ +/* + * (c) Copyright 2020 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; + +public final class ExtendsErrorOrThrowableTest { + @Test + void testSimple() { + helper().addSourceLines( + "Test.java", + "// BUG: Diagnostic contains: Class should not extend Error or Throwable directly", + "public class Test extends Error {", + " public Test() {}", + "}") + .doTest(); + } + + @Test + void testNonJavaLangError() { + helper().addSourceLines("Error.java", "public class Error {", " public Error() {}", "}") + .addSourceLines("Test.java", "public class Test extends Error {", " public Test() {}", "}") + .doTest(); + } + + @Test + void testSimpleException() { + helper().addSourceLines("Test.java", "public class Test extends RuntimeException {", " public Test() {}", "}") + .expectNoDiagnostics() + .doTest(); + } + + @Test + void testSpecificJavaError() { + helper().addSourceLines( + "Test.java", + "// BUG: Diagnostic contains: Class should not extend Error or Throwable directly", + "public class Test extends OutOfMemoryError {", + " public Test() {}", + "}") + .doTest(); + } + + @Test + void testSpecificJavaErrorNoFix() { + fix().addInputLines("Test.java", "public class Test extends OutOfMemoryError {", " public Test() {}", "}") + .expectUnchanged() + .doTestExpectingFailure(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + + @Test + void testFixError() { + fix().addInputLines("Test.java", "public class Test extends Error {", " public Test() {}", "}") + .addOutputLines("Test.java", "public class Test extends RuntimeException {", " public Test() {}", "}") + .doTest(); + } + + @Test + void testThrowableDiagnostic() { + helper().addSourceLines( + "Test.java", + "// BUG: Diagnostic contains: Class should not extend Error or Throwable directly", + "public class Test extends Throwable {", + " public Test() {}", + "}") + .doTest(); + } + + @Test + void testFixThrowable() { + fix().addInputLines("Test.java", "public class Test extends Throwable {", " public Test() {}", "}") + .addOutputLines("Test.java", "public class Test extends RuntimeException {", " public Test() {}", "}") + .doTest(); + } + + private CompilationTestHelper helper() { + return CompilationTestHelper.newInstance(ExtendsErrorOrThrowable.class, getClass()); + } + + private RefactoringValidator fix() { + return RefactoringValidator.of(new ExtendsErrorOrThrowable(), getClass()); + } +} diff --git a/changelog/@unreleased/pr-1379.v2.yml b/changelog/@unreleased/pr-1379.v2.yml new file mode 100644 index 000000000..3bf683851 --- /dev/null +++ b/changelog/@unreleased/pr-1379.v2.yml @@ -0,0 +1,7 @@ +type: improvement +improvement: + description: Add error-prone check that forbids extending java.lang.Error (reports + ERROR). Provides a fix to replace `extends Error` with `extends RuntimeException` + for clearer contract to the consumers. + links: + - https://github.com/palantir/gradle-baseline/pull/1379 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 6e0a61ffc..750c1ada0 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 @@ -28,6 +28,7 @@ public class BaselineErrorProneExtension { "CollectionStreamForEach", // TODO(ckozak): re-enable pending scala check // "CatchSpecificity", + "ExtendsErrorOrThrowable", "ExecutorSubmitRunnableFutureIgnored", "FinalClass", "LambdaMethodReference",