diff --git a/README.md b/README.md index cbf7aa4aa..6d1267dea 100644 --- a/README.md +++ b/README.md @@ -177,6 +177,7 @@ Safe Logging can be found at [github.com/palantir/safe-logging](https://github.c - `FinalClass`: A class should be declared final if all of its constructors are private. - `RedundantModifier`: Avoid using redundant modifiers. - `StrictCollectionIncompatibleType`: Likely programming error due to using the wrong type in a method that accepts Object. +- `InvocationHandlerDelegation`: InvocationHandlers which delegate to another object must catch and unwrap InvocationTargetException. ### Programmatic Application diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/InvocationHandlerDelegation.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/InvocationHandlerDelegation.java new file mode 100644 index 000000000..e186623c4 --- /dev/null +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/InvocationHandlerDelegation.java @@ -0,0 +1,142 @@ +/* + * (c) Copyright 2018 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.common.reflect.AbstractInvocationHandler; +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.matchers.ChildMultiMatcher; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.matchers.Matchers; +import com.google.errorprone.matchers.method.MethodMatchers; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.ConditionalExpressionTree; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.IfTree; +import com.sun.source.tree.InstanceOfTree; +import com.sun.source.tree.LambdaExpressionTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.source.tree.MethodTree; +import com.sun.source.tree.NewClassTree; +import com.sun.source.tree.Tree; +import com.sun.source.tree.TryTree; +import java.lang.reflect.InvocationHandler; +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Method; + +@AutoService(BugChecker.class) +@BugPattern( + name = "InvocationHandlerDelegation", + link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks", + linkType = BugPattern.LinkType.CUSTOM, + severity = BugPattern.SeverityLevel.WARNING, + summary = "InvocationHandlers which delegate to another object must catch and unwrap " + + "InvocationTargetException, otherwise an UndeclaredThrowableException will be thrown " + + "each time the delegate throws an exception.\n" + + "This check is intended to be advisory. It's fine to " + + "@SuppressWarnings(\"InvocationHandlerDelegation\") in certain cases, " + + "but is usually not recommended.") +public final class InvocationHandlerDelegation extends BugChecker implements BugChecker.MethodInvocationTreeMatcher { + + private static final Matcher INVOCATION_HANDLER = Matchers.anyOf( + Matchers.allOf( + Matchers.not(Matchers.isStatic()), + MoreMatchers.hasSignature("invoke(java.lang.Object,java.lang.reflect.Method,java.lang.Object[])"), + Matchers.enclosingClass(Matchers.isSubtypeOf(InvocationHandler.class.getName()))), + Matchers.allOf( + Matchers.not(Matchers.isStatic()), + MoreMatchers.hasSignature( + "handleInvocation(java.lang.Object,java.lang.reflect.Method,java.lang.Object[])"), + Matchers.enclosingClass(Matchers.isSubtypeOf(AbstractInvocationHandler.class.getName())))); + + private static final Matcher METHOD_INVOKE = MethodMatchers.instanceMethod() + .onExactClass(Method.class.getName()) + .withSignature("invoke(java.lang.Object,java.lang.Object...)"); + + private static final Matcher METHOD_INVOKE_ENCLOSED_BY_INVOCATION_HANDLER = Matchers.allOf( + METHOD_INVOKE, + Matchers.enclosingMethod(INVOCATION_HANDLER)); + + private static final Matcher CONTAINS_METHOD_INVOKE = Matchers.contains( + ExpressionTree.class, METHOD_INVOKE); + + private static final Matcher UNWRAP_THROWABLE = MethodMatchers.instanceMethod() + .onDescendantOf(Throwable.class.getName()) + .named("getCause") + .withParameters(); + + private static final Matcher CONTAINS_UNWRAP_THROWABLE = + Matchers.contains(ExpressionTree.class, UNWRAP_THROWABLE); + + private static final Matcher UNWRAP_ITE = MethodMatchers.instanceMethod() + .onDescendantOf(InvocationTargetException.class.getName()) + // getTargetException is deprecated, but does work correctly. + .namedAnyOf("getCause", "getTargetException") + .withParameters(); + + private static final Matcher PASS_ITE = Matchers.methodInvocation( + Matchers.anyMethod(), + ChildMultiMatcher.MatchType.AT_LEAST_ONE, + Matchers.isSubtypeOf(InvocationTargetException.class)); + + private static final Matcher CONTAINS_INSTANCEOF_ITE = Matchers.contains( + InstanceOfTree.class, + (instanceOfTree, state) -> ASTHelpers.isSameType( + ASTHelpers.getType(instanceOfTree.getType()), + state.getTypeFromString(InvocationTargetException.class.getName()), + state)); + + private static final Matcher CONTAINS_UNWRAP_ITE = Matchers.anyOf( + Matchers.contains(ExpressionTree.class, UNWRAP_ITE), + Matchers.contains(ExpressionTree.class, PASS_ITE), + Matchers.contains( + IfTree.class, + (Matcher) (ifExpression, state) -> + CONTAINS_INSTANCEOF_ITE.matches(ifExpression.getCondition(), state) + && CONTAINS_UNWRAP_THROWABLE.matches(ifExpression.getThenStatement(), state)), + Matchers.contains( + ConditionalExpressionTree.class, + (Matcher) (ifExpression, state) -> + CONTAINS_INSTANCEOF_ITE.matches(ifExpression.getCondition(), state) + && CONTAINS_UNWRAP_THROWABLE.matches(ifExpression.getTrueExpression(), state))); + + private static final Matcher HANDLES_ITE = + Matchers.anyOf( + Matchers.contains(TryTree.class, (Matcher) (tree, state) -> + CONTAINS_METHOD_INVOKE.matches(tree.getBlock(), state) + && tree.getCatches().stream() + .anyMatch(catchTree -> CONTAINS_UNWRAP_ITE.matches(catchTree.getBlock(), state))), + // If Method.invoke occurs in a lambda or anonymous class, we don't have enough + // conviction that it's a bug. + Matchers.contains(LambdaExpressionTree.class, CONTAINS_METHOD_INVOKE::matches), + Matchers.contains(NewClassTree.class, CONTAINS_METHOD_INVOKE::matches)); + + private static final Matcher MATCHER = Matchers.allOf( + METHOD_INVOKE_ENCLOSED_BY_INVOCATION_HANDLER, + Matchers.not(Matchers.enclosingMethod(HANDLES_ITE))); + + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + if (MATCHER.matches(tree, state)) { + return describeMatch(tree); + } + return Description.NO_MATCH; + } +} diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/MoreMatchers.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/MoreMatchers.java index 77bb03000..dde473792 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/MoreMatchers.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/MoreMatchers.java @@ -19,12 +19,14 @@ import com.google.errorprone.VisitorState; import com.google.errorprone.matchers.Matcher; import com.google.errorprone.matchers.Matchers; +import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.ClassTree; import com.sun.source.tree.MethodTree; import com.sun.source.tree.ModifiersTree; import com.sun.source.tree.Tree; import com.sun.source.tree.VariableTree; import com.sun.source.util.TreePath; +import com.sun.tools.javac.code.Symbol; import java.util.Locale; import javax.lang.model.element.Modifier; @@ -109,5 +111,16 @@ private static boolean containsModifier(ModifiersTree tree, VisitorState state, return source.contains(modifier.name().toLowerCase(Locale.ENGLISH)); } + /** Matches a {@link MethodTree} by method signature. */ + static Matcher hasSignature(String signature) { + return (methodTree, state) -> { + Symbol.MethodSymbol symbol = ASTHelpers.getSymbol(methodTree); + if (symbol == null) { + return false; + } + return signature.equals(symbol.toString()); + }; + } + private MoreMatchers() {} } diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/InvocationHandlerDelegationTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/InvocationHandlerDelegationTest.java new file mode 100644 index 000000000..be9df755a --- /dev/null +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/InvocationHandlerDelegationTest.java @@ -0,0 +1,252 @@ +/* + * (c) Copyright 2018 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 InvocationHandlerDelegationTest { + + @Test + void testSimpleInvocationHandler() { + helper().addSourceLines( + "Test.java", + "import java.lang.reflect.InvocationHandler;", + "import java.lang.reflect.Method;", + "final class Test implements InvocationHandler {", + " @Override", + " public Object invoke(Object proxy, Method method, Object[] args) throws Throwable {", + " // BUG: Diagnostic contains: unwrap InvocationTargetException", + " return method.invoke(this, args);", + " }", + "}") + .doTest(); + } + + @Test + void testSimpleAbstractInvocationHandler() { + helper().addSourceLines( + "Test.java", + "import com.google.common.reflect.AbstractInvocationHandler;", + "import java.lang.reflect.Method;", + "final class Test extends AbstractInvocationHandler {", + " @Override", + " public Object handleInvocation(Object proxy, Method method, Object[] args) throws Throwable {", + " // BUG: Diagnostic contains: unwrap InvocationTargetException", + " return method.invoke(this, args);", + " }", + "}") + .doTest(); + } + + @Test + void testAllowedWithoutDynamicInvocation() { + helper().addSourceLines( + "Test.java", + "import com.google.common.reflect.AbstractInvocationHandler;", + "import java.lang.reflect.Method;", + "final class Test extends AbstractInvocationHandler {", + " @Override", + " public Object handleInvocation(Object proxy, Method method, Object[] args) throws Throwable {", + " return this.toString();", + " }", + "}") + .doTest(); + } + + @Test + void testCorrectAbstractInvocationHandler_getCause() { + helper().addSourceLines( + "Test.java", + "import com.google.common.reflect.AbstractInvocationHandler;", + "import java.lang.reflect.Method;", + "import java.lang.reflect.InvocationTargetException;", + "final class Test extends AbstractInvocationHandler {", + " @Override", + " public Object handleInvocation(Object proxy, Method method, Object[] args) throws Throwable {", + " try {", + " return method.invoke(this, args);", + " } catch (InvocationTargetException e) {", + " throw e.getCause();", + " }", + " }", + "}") + .doTest(); + } + + @Test + void testCorrectInvocationHandler_getTargetException() { + helper().addSourceLines( + "Test.java", + "import java.lang.reflect.InvocationHandler;", + "import java.lang.reflect.Method;", + "import java.lang.reflect.InvocationTargetException;", + "final class Test implements InvocationHandler {", + " @Override", + " public Object invoke(Object proxy, Method method, Object[] args) throws Throwable {", + " try {", + " return method.invoke(this, args);", + " } catch (InvocationTargetException e) {", + " throw e.getTargetException();", + " }", + " }", + "}") + .doTest(); + } + + @Test + void testCorrectInvocationHandler_doesntRethrow() { + helper().addSourceLines( + "Test.java", + "import java.lang.reflect.InvocationHandler;", + "import java.lang.reflect.Method;", + "import java.lang.reflect.InvocationTargetException;", + "final class Test implements InvocationHandler {", + " @Override", + " public Object invoke(Object proxy, Method method, Object[] args) throws Throwable {", + " try {", + " return method.invoke(this, args);", + " } catch (InvocationTargetException e) {", + // Some proxies may check and rewrap the cause, the + // important part is that it's handled in some way. + " return e.getCause();", + " }", + " }", + "}") + .doTest(); + } + + @Test + void testCorrectInvocationHandler_lambda() { + helper().addSourceLines( + "Test.java", + "import java.lang.reflect.*;", + "import com.google.common.util.concurrent.*;", + "abstract class Test implements InvocationHandler {", + " @Override", + " public Object invoke(Object proxy, Method method, Object[] args) throws Throwable {", + // Avoid flagging invocations nested in anonymous classes or lambdas + " return FluentFuture.from(executor().submit(() -> method.invoke(this, args)))", + " .catching(InvocationTargetException.class, ignored -> null, MoreExecutors.directExecutor())", + " .get();", + " }", + " abstract ListeningExecutorService executor();", + "}") + .doTest(); + } + + @Test + void testCorrectInvocationHandler_delegatesException() { + helper().addSourceLines( + "Test.java", + "import java.lang.reflect.InvocationHandler;", + "import java.lang.reflect.Method;", + "import java.lang.reflect.InvocationTargetException;", + "final class Test implements InvocationHandler {", + " @Override", + " public Object invoke(Object proxy, Method method, Object[] args) throws Throwable {", + " try {", + " return method.invoke(this, args);", + " } catch (InvocationTargetException e) {", + " throw handleInvocationTargetException(e);", + " }", + " }", + " private Throwable handleInvocationTargetException(InvocationTargetException e) throws Throwable {", + " throw e.getCause();", + " }", + "}") + .doTest(); + } + + @Test + void testInvocationHandler_instanceOf_if() { + helper().addSourceLines( + "Test.java", + "import java.lang.reflect.InvocationHandler;", + "import java.lang.reflect.Method;", + "import java.lang.reflect.InvocationTargetException;", + "final class Test implements InvocationHandler {", + " @Override", + " public Object invoke(Object proxy, Method method, Object[] args) throws Throwable {", + " try {", + " return method.invoke(this, args);", + " } catch (Throwable t) {", + " if (t instanceof InvocationTargetException) {", + " throw t.getCause();", + " }", + " throw t;", + " }", + " }", + "}") + .doTest(); + } + + @Test + void testInvocationHandler_instanceOf_ternary() { + helper().addSourceLines( + "Test.java", + "import java.lang.reflect.InvocationHandler;", + "import java.lang.reflect.Method;", + "import java.lang.reflect.InvocationTargetException;", + "final class Test implements InvocationHandler {", + " @Override", + " public Object invoke(Object proxy, Method method, Object[] args) throws Throwable {", + " try {", + " return method.invoke(this, args);", + " } catch (Throwable t) {", + " throw (t instanceof InvocationTargetException) ? t.getCause() : t;", + " }", + " }", + "}") + .doTest(); + } + + @Test + void testInvocationHandler_delegatedThrowable() { + // This implementation is functionally correct, but risks breaking when code is refactored. + helper().addSourceLines( + "Test.java", + "import java.lang.reflect.InvocationHandler;", + "import java.lang.reflect.Method;", + "import java.lang.reflect.InvocationTargetException;", + "final class Test implements InvocationHandler {", + " @Override", + " public Object invoke(Object proxy, Method method, Object[] args) throws Throwable {", + " try {", + " // BUG: Diagnostic contains: unwrap InvocationTargetException", + " return method.invoke(this, args);", + " } catch (Throwable t) {", + " return handle(t);", + " }", + " }", + " private static Object handle(Throwable t) throws Throwable {", + " if (t instanceof InvocationTargetException) {", + " throw t.getCause();", + " }", + " throw t;", + " }", + "}") + .doTest(); + } + + private CompilationTestHelper helper() { + return CompilationTestHelper.newInstance(InvocationHandlerDelegation.class, getClass()); + } +} diff --git a/changelog/@unreleased/pr-1032.v2.yml b/changelog/@unreleased/pr-1032.v2.yml new file mode 100644 index 000000000..5d8f93b02 --- /dev/null +++ b/changelog/@unreleased/pr-1032.v2.yml @@ -0,0 +1,8 @@ +type: improvement +improvement: + description: |- + InvocationHandlers which delegate to another object must catch and unwrap + `InvocationTargetException`, otherwise an `UndeclaredThrowableException` will be thrown + each time the delegate throws an exception. + links: + - https://github.com/palantir/gradle-baseline/pull/1032