diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTCatchParameter.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTCatchParameter.java index f74529dc9e..875198d373 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTCatchParameter.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTCatchParameter.java @@ -4,9 +4,13 @@ package net.sourceforge.pmd.lang.java.ast; +import java.util.Collection; + import org.checkerframework.checker.nullness.qual.NonNull; import net.sourceforge.pmd.lang.ast.NodeStream; +import net.sourceforge.pmd.lang.java.types.JIntersectionType; +import net.sourceforge.pmd.lang.java.types.TypeSystem; /** @@ -68,6 +72,12 @@ public final class ASTCatchParameter extends AbstractJavaNode /** * Returns a stream of all declared exception types (expanding a union * type if present). + * + *

Note that this is the only reliable way to inspect multi-catch clauses, + * as the type mirror of a {@link ASTUnionType} is not itself a {@link JIntersectionType}, + * but the {@link TypeSystem#lub(Collection) LUB} of the components. + * Since exception types cannot be interfaces, the LUB always erases + * to a single class supertype (eg {@link RuntimeException}). */ public NodeStream getAllExceptionTypes() { ASTType typeNode = getTypeNode(); diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTUnionType.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTUnionType.java index 409e60f3bd..fe08133cce 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTUnionType.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTUnionType.java @@ -22,6 +22,8 @@ import net.sourceforge.pmd.lang.java.ast.InternalInterfaces.AtLeastOneChildOfTyp * UnionType ::= {@link ASTClassOrInterfaceType ClassType} ("|" {@link ASTClassOrInterfaceType ClassType})+ * * + * + * @see ASTCatchParameter#getAllExceptionTypes() */ public final class ASTUnionType extends AbstractJavaTypeNode implements ASTReferenceType, diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/internal/JavaAstUtils.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/internal/JavaAstUtils.java index 3d6d781a4f..5f6ab757d8 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/internal/JavaAstUtils.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/internal/JavaAstUtils.java @@ -32,9 +32,11 @@ import net.sourceforge.pmd.lang.java.ast.ASTArrayAllocation; import net.sourceforge.pmd.lang.java.ast.ASTAssignableExpr.ASTNamedReferenceExpr; import net.sourceforge.pmd.lang.java.ast.ASTAssignableExpr.AccessType; import net.sourceforge.pmd.lang.java.ast.ASTAssignmentExpression; +import net.sourceforge.pmd.lang.java.ast.ASTBlock; import net.sourceforge.pmd.lang.java.ast.ASTBooleanLiteral; import net.sourceforge.pmd.lang.java.ast.ASTBreakStatement; import net.sourceforge.pmd.lang.java.ast.ASTCastExpression; +import net.sourceforge.pmd.lang.java.ast.ASTCatchClause; import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceType; import net.sourceforge.pmd.lang.java.ast.ASTConstructorCall; import net.sourceforge.pmd.lang.java.ast.ASTExpression; @@ -58,6 +60,7 @@ import net.sourceforge.pmd.lang.java.ast.ASTSuperExpression; import net.sourceforge.pmd.lang.java.ast.ASTSwitchBranch; import net.sourceforge.pmd.lang.java.ast.ASTSwitchStatement; import net.sourceforge.pmd.lang.java.ast.ASTThisExpression; +import net.sourceforge.pmd.lang.java.ast.ASTThrowStatement; import net.sourceforge.pmd.lang.java.ast.ASTUnaryExpression; import net.sourceforge.pmd.lang.java.ast.ASTVariableAccess; import net.sourceforge.pmd.lang.java.ast.ASTVariableDeclaratorId; @@ -729,4 +732,19 @@ public final class JavaAstUtils { return false; } } + + /** + * Return true if the catch clause just rethrows the caught exception + * immediately. + */ + public static boolean isJustRethrowException(ASTCatchClause clause) { + ASTBlock body = clause.getBody(); + if (body.size() == 1) { + ASTStatement stmt = body.get(0); + return stmt instanceof ASTThrowStatement + && isReferenceToVar(((ASTThrowStatement) stmt).getExpr(), + clause.getParameter().getVarId().getSymbol()); + } + return false; + } } diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/ExceptionAsFlowControlRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/ExceptionAsFlowControlRule.java index 0820d8f475..17e26ac4ec 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/ExceptionAsFlowControlRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/ExceptionAsFlowControlRule.java @@ -4,11 +4,12 @@ package net.sourceforge.pmd.lang.java.rule.design; -import net.sourceforge.pmd.lang.ast.NodeStream; +import net.sourceforge.pmd.lang.java.ast.ASTBodyDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTCatchClause; import net.sourceforge.pmd.lang.java.ast.ASTThrowStatement; import net.sourceforge.pmd.lang.java.ast.ASTTryStatement; import net.sourceforge.pmd.lang.java.ast.JavaNode; +import net.sourceforge.pmd.lang.java.ast.internal.JavaAstUtils; import net.sourceforge.pmd.lang.java.rule.AbstractJavaRulechainRule; import net.sourceforge.pmd.lang.java.types.JTypeMirror; @@ -28,24 +29,31 @@ public class ExceptionAsFlowControlRule extends AbstractJavaRulechainRule { @Override public Object visit(ASTThrowStatement node, Object data) { - JavaNode firstTryOrCatch = node.ancestors().map(NodeStream.asInstanceOf(ASTTryStatement.class, ASTCatchClause.class)).first(); - NodeStream enclosingTries = node.ancestors(ASTTryStatement.class); - if (firstTryOrCatch instanceof ASTCatchClause) { - // if the exception is thrown in a catch block, then the - // first try we're looking for is the next one - enclosingTries = enclosingTries.drop(1); - } - if (enclosingTries.isEmpty()) { - return data; - } - JTypeMirror thrownType = node.getExpr().getTypeMirror(); - - enclosingTries.flatMap(ASTTryStatement::getCatchClauses) - .map(ASTCatchClause::getParameter) - .filter(exParam -> exParam.getAllExceptionTypes().any(type -> thrownType.isSubtypeOf(type.getTypeMirror()))) - .take(1) - .forEach(ex -> addViolation(data, ex)); - return data; + JavaNode parent = node.getParent(); + while (!(parent instanceof ASTBodyDeclaration)) { + if (parent instanceof ASTCatchClause) { + // if the exception is thrown in a catch block, then we + // have to ignore the try stmt (jump past it). + parent = parent.getParent().getParent(); + continue; + } + if (parent instanceof ASTTryStatement) { + // maybe the exception is being caught here. + for (ASTCatchClause catchClause : ((ASTTryStatement) parent).getCatchClauses()) { + if (catchClause.getParameter().getAllExceptionTypes().any(it -> thrownType.isSubtypeOf(it.getTypeMirror()))) { + if (!JavaAstUtils.isJustRethrowException(catchClause)) { + asCtx(data).addViolation(catchClause, node.getReportLocation().getStartLine()); + return null; + } else { + break; + } + } + } + } + parent = parent.getParent(); + } + return null; } + } diff --git a/pmd-java/src/main/resources/category/java/design.xml b/pmd-java/src/main/resources/category/java/design.xml index e69ded8f7e..20f8e3827d 100644 --- a/pmd-java/src/main/resources/category/java/design.xml +++ b/pmd-java/src/main/resources/category/java/design.xml @@ -604,12 +604,14 @@ public class Foo extends Error { } -Using Exceptions as form of flow control is not recommended as they obscure true exceptions when debugging. -Either add the necessary validation or use an alternate control structure. +This rule reports exceptions thrown and caught in an enclosing try statement. +This use of exceptions as a form of `goto` statement is discouraged, as that may +hide actual exceptions, and obscures control flow, especially when debugging. +To fix a violation, add the necessary validation or use an alternate control structure. 3 diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/design/xml/ExceptionAsFlowControl.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/design/xml/ExceptionAsFlowControl.xml index 5804c4b079..e07a11f1e6 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/design/xml/ExceptionAsFlowControl.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/design/xml/ExceptionAsFlowControl.xml @@ -75,6 +75,9 @@ public class Foo { Catch block for subtype 1 11 + + Exception thrown at line 10 is caught in this block. + + + #4434 simple propagation + 0 + getAll(Iterable keys) throws ExecutionException { + try { + Map result = cache.getAll(keys); + if (something()) { + throw new InvalidCacheLoadException("null key or value"); + } + return result; + } catch (NullPointerException | InvalidCacheLoadException e) { + throw e; // <-- Violation + } catch (CacheLoaderException e) { + throw new ExecutionException(e.getCause()); + } catch (Exception e) { + throw new UncheckedExecutionException(e); + } + } + } + + class ExecutionException extends Exception { + public ExecutionException(Throwable o) { super(o);} + } + class UncheckedExecutionException extends RuntimeException { + public UncheckedExecutionException(Throwable o) { super(o);} + } + class InvalidCacheLoadException extends RuntimeException { + public InvalidCacheLoadException(String msg) { super(msg);} + } + + ]]> + + + #4434 simple propagation, 2nd case + 0 + getAll(Iterable keys) { + try { + Map result = cache.getAll(keys); + if (something()) { + throw new InvalidCacheLoadException("null key or value"); + } + return result; + + } catch (NullPointerException | InvalidCacheLoadException e) { + // Note that technically this is still an code smell as + // the exception is rethrown directly, but this should be + // the purpose of another rule. + throw e; + } + } + } + + class InvalidCacheLoadException extends RuntimeException { + public InvalidCacheLoadException(String msg) { super(msg);} + } + + ]]> +