Fix #4434

Rewrite ExceptionAsFlowControl to a more efficient form.
Change message to be more informative.
Rewrite description of the rule.

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).
+     *
+     * <p>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<ASTClassOrInterfaceType> getAllExceptionTypes() {
         ASTType typeNode = getTypeNode();

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})+
  *
  * </pre>
+ *
+ * @see ASTCatchParameter#getAllExceptionTypes()
  */
 public final class ASTUnionType extends AbstractJavaTypeNode
     implements ASTReferenceType,

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;
+    }
 }

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().<JavaNode>map(NodeStream.asInstanceOf(ASTTryStatement.class, ASTCatchClause.class)).first();
-        NodeStream<ASTTryStatement> 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;
     }
+
 }

pmd-java/src/main/resources/category/java/design.xml

@@ -604,12 +604,14 @@ public class Foo extends Error { }
     <rule name="ExceptionAsFlowControl"
           language="java"
           since="1.8"
-          message="Avoid using exceptions as flow control."
+          message="Exception thrown at line {0} is caught in this block."
           class="net.sourceforge.pmd.lang.java.rule.design.ExceptionAsFlowControlRule"
           externalInfoUrl="${pmd.website.baseurl}/pmd_rules_java_design.html#exceptionasflowcontrol">
         <description>
-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.
         </description>
         <priority>3</priority>
         <example>

pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/design/xml/ExceptionAsFlowControl.xml

@@ -75,6 +75,9 @@ public class Foo {
         <description>Catch block for subtype</description>
         <expected-problems>1</expected-problems>
         <expected-linenumbers>11</expected-linenumbers>
+        <expected-messages>
+            <message>Exception thrown at line 10 is caught in this block.</message>
+        </expected-messages>
         <code><![CDATA[
 public class Foo {{
 
@@ -95,4 +98,70 @@ class SubE extends TopE { }
 
         ]]></code>
     </test-code>
+    <test-code>
+        <description>#4434 simple propagation</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+            import java.util.*;
+            public class Foo {
+
+                public Map<K, V> getAll(Iterable<? extends K> keys) throws ExecutionException {
+                    try {
+                        Map<K, V> 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);}
+            }
+
+            ]]></code>
+    </test-code>
+    <test-code>
+        <description>#4434 simple propagation, 2nd case</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+            import java.util.*;
+            public class Foo {
+
+                public Map<K, V> getAll(Iterable<? extends K> keys) {
+                    try {
+                        Map<K, V> 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);}
+            }
+
+            ]]></code>
+    </test-code>
 </test-data>