From 6380cc27fbc25c7de55be3e5a891e5025ff8cca8 Mon Sep 17 00:00:00 2001 From: cpovirk Date: Thu, 17 Oct 2024 07:06:11 -0700 Subject: [PATCH] Warn about `throwIfUnchecked(unchecked)`, which could be just `throw unchecked`. This was prompted by https://github.com/google/guava/pull/7434, in which the correct fix was `throwIfUnchecked(unchecked.getCause())`. (Compare [one other instance of that](https://github.com/googleapis/google-auth-library-java/blob/f154edb3d8503d29f0020b6904dfa40e034ded93/oauth2_http/java/com/google/auth/oauth2/ServiceAccountJwtAccessCredentials.java#L373).) But most hits in practice appear to be `catch (RuntimeException e) { throwIfUnchecked(e); }`, which isn't a bug, just unnecessary code (which can typically be further simplified sometimes as far as to remove the `try`-`catch` block entirely, as suggested by the Google-internal `RethrowException` check). This check could be written with Refaster (unknown commit), _but_: - That would prevent it from running at compile time. - That would in practice prevent us from open-sourcing it. - We'd need to use placeholders if we want to continue to remove the code after `throwIfUnchecked`, and placeholders can have slow runtime and won't remove code that has comments. (This check doesn't remove comments, either, but at least it will remove the code around them.) - Refaster can't handle `RuntimeException | Error e`, which comes up a couple times in our depot. - To cover all the cases we can correctly, the Refaster templates need to be written in just the right order (expression templates before block templates), and they end up about as complicated as this plain old Error Prone check. PiperOrigin-RevId: 686901145 --- .../ThrowIfUncheckedKnownUnchecked.java | 105 ++++++++++++ .../scanner/BuiltInCheckerSuppliers.java | 2 + .../ThrowIfUncheckedKnownUncheckedTest.java | 157 ++++++++++++++++++ 3 files changed, 264 insertions(+) create mode 100644 core/src/main/java/com/google/errorprone/bugpatterns/ThrowIfUncheckedKnownUnchecked.java create mode 100644 core/src/test/java/com/google/errorprone/bugpatterns/ThrowIfUncheckedKnownUncheckedTest.java diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/ThrowIfUncheckedKnownUnchecked.java b/core/src/main/java/com/google/errorprone/bugpatterns/ThrowIfUncheckedKnownUnchecked.java new file mode 100644 index 00000000000..3e10fa95395 --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/ThrowIfUncheckedKnownUnchecked.java @@ -0,0 +1,105 @@ +/* + * Copyright 2016 The Error Prone Authors. + * + * 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.google.errorprone.bugpatterns; + +import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; +import static com.google.errorprone.matchers.Description.NO_MATCH; +import static com.google.errorprone.matchers.Matchers.allOf; +import static com.google.errorprone.matchers.Matchers.anyOf; +import static com.google.errorprone.matchers.Matchers.argument; +import static com.google.errorprone.matchers.Matchers.argumentCount; +import static com.google.errorprone.matchers.method.MethodMatchers.staticMethod; +import static com.sun.source.tree.Tree.Kind.BLOCK; +import static com.sun.source.tree.Tree.Kind.EXPRESSION_STATEMENT; + +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; +import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.BlockTree; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.tools.javac.code.Symtab; +import com.sun.tools.javac.code.Type; +import com.sun.tools.javac.code.Types; +import javax.lang.model.type.UnionType; + +/** A BugPattern; see the summary. */ +@BugPattern( + summary = "`throwIfUnchecked(knownUnchecked)` is equivalent to `throw knownUnchecked`.", + severity = WARNING) +public class ThrowIfUncheckedKnownUnchecked extends BugChecker + implements MethodInvocationTreeMatcher { + + private static final Matcher IS_THROW_IF_UNCHECKED = + allOf( + anyOf( + staticMethod().onClass("com.google.common.base.Throwables").named("throwIfUnchecked"), + staticMethod() + .onClass("com.google.common.base.Throwables") + .named("propagateIfPossible")), + argumentCount(1)); + + private static final Matcher IS_KNOWN_UNCHECKED = + new Matcher() { + @Override + public boolean matches(ExpressionTree tree, VisitorState state) { + Type type = ASTHelpers.getType(tree); + if (type.isUnion()) { + return ((UnionType) type) + .getAlternatives().stream().allMatch(t -> isKnownUnchecked(state, (Type) t)); + } else { + return isKnownUnchecked(state, type); + } + } + + boolean isKnownUnchecked(VisitorState state, Type type) { + Types types = state.getTypes(); + Symtab symtab = state.getSymtab(); + // Check erasure for generics. + // TODO(cpovirk): Is that necessary here or in ThrowIfUncheckedKnownChecked? + type = types.erasure(type); + return types.isSubtype(type, symtab.errorType) + || types.isSubtype(type, symtab.runtimeExceptionType); + } + }; + + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + if (IS_THROW_IF_UNCHECKED.matches(tree, state) + && argument(0, IS_KNOWN_UNCHECKED).matches(tree, state)) { + var fix = SuggestedFix.builder(); + fix.replace(tree, "throw " + state.getSourceForNode(tree.getArguments().get(0))); + /* + * Changing to `throw ...` make the compiler recognize everything afterward in the block as + * unreachable. To avoid build errors from that, we remove everything afterward. + * + * We might still produce build errors if code *after* the block becomes unreachable (because + * it's now possible to fall out of this block). That seems tolerable. + */ + var parent = state.getPath().getParentPath().getLeaf(); + var grandparent = state.getPath().getParentPath().getParentPath().getLeaf(); + if (parent.getKind() == EXPRESSION_STATEMENT && grandparent.getKind() == BLOCK) { + ((BlockTree) grandparent) + .getStatements().stream().dropWhile(t -> t != parent).skip(1).forEach(fix::delete); + } + return describeMatch(tree, fix.build()); + } + return NO_MATCH; + } +} diff --git a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java index 6cb6218f418..9806e788dd4 100644 --- a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java +++ b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java @@ -380,6 +380,7 @@ import com.google.errorprone.bugpatterns.ThreadLocalUsage; import com.google.errorprone.bugpatterns.ThreeLetterTimeZoneID; import com.google.errorprone.bugpatterns.ThrowIfUncheckedKnownChecked; +import com.google.errorprone.bugpatterns.ThrowIfUncheckedKnownUnchecked; import com.google.errorprone.bugpatterns.ThrowNull; import com.google.errorprone.bugpatterns.ThrowSpecificExceptions; import com.google.errorprone.bugpatterns.ThrowsUncheckedException; @@ -1086,6 +1087,7 @@ public static ScannerSupplier warningChecks() { ThreadLocalUsage.class, ThreadPriorityCheck.class, ThreeLetterTimeZoneID.class, + ThrowIfUncheckedKnownUnchecked.class, TimeUnitConversionChecker.class, ToStringReturnsNull.class, TraditionalSwitchExpression.class, diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/ThrowIfUncheckedKnownUncheckedTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/ThrowIfUncheckedKnownUncheckedTest.java new file mode 100644 index 00000000000..477f5db5334 --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/ThrowIfUncheckedKnownUncheckedTest.java @@ -0,0 +1,157 @@ +/* + * Copyright 2024 The Error Prone Authors. + * + * 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.google.errorprone.bugpatterns; + +import com.google.errorprone.BugCheckerRefactoringTestHelper; +import com.google.errorprone.CompilationTestHelper; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public class ThrowIfUncheckedKnownUncheckedTest { + private final CompilationTestHelper compilationHelper = + CompilationTestHelper.newInstance(ThrowIfUncheckedKnownUnchecked.class, getClass()); + private final BugCheckerRefactoringTestHelper refactoringHelper = + BugCheckerRefactoringTestHelper.newInstance(ThrowIfUncheckedKnownUnchecked.class, getClass()); + + @Test + public void knownRuntimeException() { + compilationHelper + .addSourceLines( + "Foo.java", + """ + import static com.google.common.base.Throwables.throwIfUnchecked; + + class Foo { + void x(IllegalArgumentException e) { + // BUG: Diagnostic contains: + throwIfUnchecked(e); + } + } + """) + .doTest(); + } + + @Test + public void knownError() { + compilationHelper + .addSourceLines( + "Foo.java", + """ + import static com.google.common.base.Throwables.throwIfUnchecked; + + class Foo { + void x(LinkageError e) { + // BUG: Diagnostic contains: + throwIfUnchecked(e); + } + } + """) + .doTest(); + } + + @Test + public void knownUncheckedMulticatch() { + compilationHelper + .addSourceLines( + "Foo.java", + """ + import static com.google.common.base.Throwables.throwIfUnchecked; + + class Foo { + void x() { + try { + } catch (RuntimeException | Error e) { + // BUG: Diagnostic contains: + throwIfUnchecked(e); + } + } + } + """) + .doTest(); + } + + @Test + public void knownCheckedException() { + compilationHelper + .addSourceLines( + "Foo.java", + """ + import static com.google.common.base.Throwables.throwIfUnchecked; + + import java.io.IOException; + + class Foo { + void x(IOException e) { + throwIfUnchecked(e); + } + } + """) + .doTest(); + } + + @Test + public void unknownType() { + compilationHelper + .addSourceLines( + "Foo.java", + """ + import static com.google.common.base.Throwables.throwIfUnchecked; + + class Foo { + void x(Exception e) { + throwIfUnchecked(e); + } + } + """) + .doTest(); + } + + @Test + public void refactoring() { + refactoringHelper + .addInputLines( + "in/Foo.java", + """ + import static com.google.common.base.Throwables.throwIfUnchecked; + + class Foo { + void x(IllegalArgumentException e) { + log(e); + throwIfUnchecked(e); + throw new RuntimeException(e); + } + + void log(Throwable t) {} + } + """) + .addOutputLines( + "out/Foo.java", + """ + import static com.google.common.base.Throwables.throwIfUnchecked; + + class Foo { + void x(IllegalArgumentException e) { + log(e); + throw e; + } + + void log(Throwable t) {} + } + """) + .doTest(); + } +}