From ef0c41ca8721c69242b6421a84071e866c335ff6 Mon Sep 17 00:00:00 2001 From: Robert Kruszewski Date: Tue, 2 Jun 2020 21:18:58 +0100 Subject: [PATCH 1/6] Improvement: Add error-prone check that forbids extending java.lang.Error --- README.md | 1 + .../baseline/errorprone/ExtendsError.java | 64 +++++++++++++++++++ .../baseline/errorprone/ExtendsErrorTest.java | 55 ++++++++++++++++ 3 files changed, 120 insertions(+) create mode 100644 baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ExtendsError.java create mode 100644 baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ExtendsErrorTest.java diff --git a/README.md b/README.md index 3b2523249..5bf659566 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. +- `ExtendsError`: Avoid extending java.lang.Error. ### Programmatic Application diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ExtendsError.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ExtendsError.java new file mode 100644 index 000000000..d6b70b513 --- /dev/null +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ExtendsError.java @@ -0,0 +1,64 @@ +/* + * (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; + +@AutoService(BugChecker.class) +@BugPattern( + name = "ExtendsError", + 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 java.lang.Error. While allowed by java it can lead to surprising behaviour" + + " if users end up catching Error.") +public final class ExtendsError extends BugChecker implements BugChecker.ClassTreeMatcher { + private static final Matcher ERROR_MATCHER = Matchers.isSubtypeOf(Error.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; + } + + if (!ERROR_MATCHER.matches(tree, state)) { + return Description.NO_MATCH; + } + + return buildDescription(tree).addFix(buildFix(tree, state)).build(); + } + + private static SuggestedFix buildFix(ClassTree tree, VisitorState state) { + Type exceptionType = Suppliers.typeFromClass(Exception.class).get(state); + String prettyExceptionType = SuggestedFixes.prettyType(exceptionType, state); + return SuggestedFix.replace(tree.getExtendsClause(), prettyExceptionType); + } +} diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ExtendsErrorTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ExtendsErrorTest.java new file mode 100644 index 000000000..43590fa10 --- /dev/null +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ExtendsErrorTest.java @@ -0,0 +1,55 @@ +/* + * (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.CompilationTestHelper; +import org.junit.jupiter.api.Test; + +public final class ExtendsErrorTest { + @Test + void testSimple() { + helper().addSourceLines( + "Test.java", + "// BUG: Diagnostic contains: Class should not extend java.lang.Error", + "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 testFix() { + fix().addInputLines("Test.java", "public class Test extends Error {", " public Test() {}", "}") + .addOutputLines("Test.java", "public class Test extends Exception {", " public Test() {}", "}") + .doTest(); + } + + private CompilationTestHelper helper() { + return CompilationTestHelper.newInstance(ExtendsError.class, getClass()); + } + + private RefactoringValidator fix() { + return RefactoringValidator.of(new ExtendsError(), getClass()); + } +} From b25d914cadf3e485e4db96a140a23102538edd8d Mon Sep 17 00:00:00 2001 From: Robert Kruszewski Date: Tue, 2 Jun 2020 21:28:28 +0100 Subject: [PATCH 2/6] Fix fix --- .../java/com/palantir/baseline/errorprone/ExtendsError.java | 2 +- .../java/com/palantir/baseline/errorprone/ExtendsErrorTest.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ExtendsError.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ExtendsError.java index d6b70b513..9283fdd76 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ExtendsError.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ExtendsError.java @@ -57,7 +57,7 @@ public Description matchClass(ClassTree tree, VisitorState state) { } private static SuggestedFix buildFix(ClassTree tree, VisitorState state) { - Type exceptionType = Suppliers.typeFromClass(Exception.class).get(state); + Type exceptionType = Suppliers.typeFromClass(RuntimeException.class).get(state); String prettyExceptionType = SuggestedFixes.prettyType(exceptionType, state); return SuggestedFix.replace(tree.getExtendsClause(), prettyExceptionType); } diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ExtendsErrorTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ExtendsErrorTest.java index 43590fa10..ddd5c0c0e 100644 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ExtendsErrorTest.java +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ExtendsErrorTest.java @@ -41,7 +41,7 @@ void testNonJavaLangError() { @Test void testFix() { fix().addInputLines("Test.java", "public class Test extends Error {", " public Test() {}", "}") - .addOutputLines("Test.java", "public class Test extends Exception {", " public Test() {}", "}") + .addOutputLines("Test.java", "public class Test extends RuntimeException {", " public Test() {}", "}") .doTest(); } From 9c0b21629fba43364dfc3496d9b6ec49e159c757 Mon Sep 17 00:00:00 2001 From: Robert Kruszewski Date: Tue, 2 Jun 2020 20:28:28 +0000 Subject: [PATCH 3/6] Add generated changelog entries --- changelog/@unreleased/pr-1379.v2.yml | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 changelog/@unreleased/pr-1379.v2.yml 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 From 91566efafa683ae10a252b683ea9746dbdbb4b9a Mon Sep 17 00:00:00 2001 From: Robert Kruszewski Date: Tue, 2 Jun 2020 23:29:30 +0100 Subject: [PATCH 4/6] rename --- README.md | 2 +- ...rror.java => ExtendsErrorOrThrowable.java} | 29 ++++-- .../ExtendsErrorOrThrowableTest.java | 99 +++++++++++++++++++ .../baseline/errorprone/ExtendsErrorTest.java | 55 ----------- 4 files changed, 119 insertions(+), 66 deletions(-) rename baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/{ExtendsError.java => ExtendsErrorOrThrowable.java} (57%) create mode 100644 baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ExtendsErrorOrThrowableTest.java delete mode 100644 baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ExtendsErrorTest.java diff --git a/README.md b/README.md index 5bf659566..1a9c7c29d 100644 --- a/README.md +++ b/README.md @@ -185,7 +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. -- `ExtendsError`: Avoid extending java.lang.Error. +- `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/ExtendsError.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ExtendsErrorOrThrowable.java similarity index 57% rename from baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ExtendsError.java rename to baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ExtendsErrorOrThrowable.java index 9283fdd76..414d55728 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ExtendsError.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ExtendsErrorOrThrowable.java @@ -29,18 +29,22 @@ 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 = "ExtendsError", + 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 java.lang.Error. While allowed by java it can lead to surprising behaviour" - + " if users end up catching Error.") -public final class ExtendsError extends BugChecker implements BugChecker.ClassTreeMatcher { - private static final Matcher ERROR_MATCHER = Matchers.isSubtypeOf(Error.class); + 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) { @@ -49,16 +53,21 @@ public Description matchClass(ClassTree tree, VisitorState state) { return Description.NO_MATCH; } - if (!ERROR_MATCHER.matches(tree, state)) { + // 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 SuggestedFix buildFix(ClassTree tree, VisitorState state) { - Type exceptionType = Suppliers.typeFromClass(RuntimeException.class).get(state); - String prettyExceptionType = SuggestedFixes.prettyType(exceptionType, state); - return SuggestedFix.replace(tree.getExtendsClause(), prettyExceptionType); + 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..fcffea51d --- /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 java.lang.Error", + "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 java.lang.Error", + "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 java.lang.Error", + "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/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ExtendsErrorTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ExtendsErrorTest.java deleted file mode 100644 index ddd5c0c0e..000000000 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ExtendsErrorTest.java +++ /dev/null @@ -1,55 +0,0 @@ -/* - * (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.CompilationTestHelper; -import org.junit.jupiter.api.Test; - -public final class ExtendsErrorTest { - @Test - void testSimple() { - helper().addSourceLines( - "Test.java", - "// BUG: Diagnostic contains: Class should not extend java.lang.Error", - "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 testFix() { - fix().addInputLines("Test.java", "public class Test extends Error {", " public Test() {}", "}") - .addOutputLines("Test.java", "public class Test extends RuntimeException {", " public Test() {}", "}") - .doTest(); - } - - private CompilationTestHelper helper() { - return CompilationTestHelper.newInstance(ExtendsError.class, getClass()); - } - - private RefactoringValidator fix() { - return RefactoringValidator.of(new ExtendsError(), getClass()); - } -} From 2c130fd7b80e517481808b8d4d530cc8a0bac4c6 Mon Sep 17 00:00:00 2001 From: Robert Kruszewski Date: Tue, 2 Jun 2020 23:47:30 +0100 Subject: [PATCH 5/6] fix test --- .../baseline/errorprone/ExtendsErrorOrThrowableTest.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) 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 index fcffea51d..e9e9e4146 100644 --- 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 @@ -25,7 +25,7 @@ public final class ExtendsErrorOrThrowableTest { void testSimple() { helper().addSourceLines( "Test.java", - "// BUG: Diagnostic contains: Class should not extend java.lang.Error", + "// BUG: Diagnostic contains: Class should not extend Error or Throwable directly", "public class Test extends Error {", " public Test() {}", "}") @@ -50,7 +50,7 @@ void testSimpleException() { void testSpecificJavaError() { helper().addSourceLines( "Test.java", - "// BUG: Diagnostic contains: Class should not extend java.lang.Error", + "// BUG: Diagnostic contains: Class should not extend Error or Throwable directly", "public class Test extends OutOfMemoryError {", " public Test() {}", "}") @@ -75,7 +75,7 @@ void testFixError() { void testThrowableDiagnostic() { helper().addSourceLines( "Test.java", - "// BUG: Diagnostic contains: Class should not extend java.lang.Error", + "// BUG: Diagnostic contains: Class should not extend Error or Throwable directly", "public class Test extends Throwable {", " public Test() {}", "}") From fa4c7fde45d24b27e2babd312f8a0ac5e9f3f363 Mon Sep 17 00:00:00 2001 From: Robert Kruszewski Date: Wed, 3 Jun 2020 19:09:15 +0100 Subject: [PATCH 6/6] add to list --- .../baseline/extensions/BaselineErrorProneExtension.java | 1 + 1 file changed, 1 insertion(+) 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",