pmd: fix #1087 PreserveStackTrace (still) ignores initCause()

pmd/etc/changelog.txt

@@ -5,6 +5,7 @@ Fixed bug 1080: net.sourceforge.pmd.cpd.CPDTest test failing
 Fixed bug 1081: Regression: CPD skipping all files when using relative paths
 Fixed bug 1082: CPD performance issue on larger projects
 Fixed bug 1086: Unsupported Element and Attribute in Ant Task Example
+Fixed bug 1087: PreserveStackTrace (still) ignores initCause()
 
 
 April 5, 2013 - 5.0.3:

pmd/src/main/java/net/sourceforge/pmd/lang/java/rule/design/PreserveStackTraceRule.java

@@ -38,7 +38,6 @@ public class PreserveStackTraceRule extends AbstractJavaRule {
 	"./VariableInitializer/Expression/PrimaryExpression/PrimaryPrefix/AllocationExpression" +
 	"[ClassOrInterfaceType[contains(@Image,'Exception')] and Arguments[count(*)=0]]";
 
-    private static final String ILLEGAL_STATE_EXCEPTION = "IllegalStateException";
     private static final String FILL_IN_STACKTRACE = ".fillInStackTrace";
 
     @Override
@@ -56,43 +55,64 @@ public class PreserveStackTraceRule extends AbstractJavaRule {
                 }
                 continue;
             }
-            // If the thrown exception is IllegalStateException, no way to preserve the exception (the constructor has no args)
+            // Retrieve all argument for the throw exception (to see if the original exception is preserved)
-            if ( ! isThrownExceptionOfType(throwStatement,ILLEGAL_STATE_EXCEPTION) ) {
+            ASTArgumentList args = throwStatement.getFirstDescendantOfType(ASTArgumentList.class);
-	            // Retrieve all argument for the throw exception (to see if the original exception is preserved)
-	            ASTArgumentList args = throwStatement.getFirstDescendantOfType(ASTArgumentList.class);
 
-	            if (args != null) {
+            if (args != null) {
-	                ck(data, target, throwStatement, args);
+                ck(data, target, throwStatement, args);
-	            }
+            }
-	            else {
+            else {
-	        	Node child = throwStatement.jjtGetChild(0);
+                Node child = throwStatement.jjtGetChild(0);
-	                while (child != null && child.jjtGetNumChildren() > 0
+                while (child != null && child.jjtGetNumChildren() > 0
-	                        && !(child instanceof ASTName)) {
+                        && !(child instanceof ASTName)) {
-	                    child = child.jjtGetChild(0);
+                    child = child.jjtGetChild(0);
-	                }
+                }
-	                if (child != null){
+                if (child != null){
-	                    if ((child instanceof ASTName) && !target.equals(child.getImage()) && !child.hasImageEqualTo(target + FILL_IN_STACKTRACE)) {
+                    if ((child instanceof ASTName) && !target.equals(child.getImage()) && !child.hasImageEqualTo(target + FILL_IN_STACKTRACE)) {
-	                        Map<VariableNameDeclaration, List<NameOccurrence>> vars = ((ASTName) child).getScope().getVariableDeclarations();
+                        Map<VariableNameDeclaration, List<NameOccurrence>> vars = ((ASTName) child).getScope().getVariableDeclarations();
-		                    for (VariableNameDeclaration decl: vars.keySet()) {
+                        for (Map.Entry<VariableNameDeclaration, List<NameOccurrence>> entry : vars.entrySet()) {
-		                        if (decl.getImage().equals(child.getImage())) {
+                            VariableNameDeclaration decl = entry.getKey();
+                            List<NameOccurrence> occurrences = entry.getValue();
+	                        if (decl.getImage().equals(child.getImage())) {
+	                            if (!isInitCauseCalled(target, occurrences)) {
     		                        args = decl.getNode().jjtGetParent()
     		                                .getFirstDescendantOfType(ASTArgumentList.class);
     		                        if (args != null) {
     		                            ck(data, target, throwStatement, args);
     		                        }
-		                        }
+	                            }
-		                    }
+	                        }
-	                    } else if (child instanceof ASTClassOrInterfaceType){
-	                       addViolation(data, throwStatement);
 	                    }
-	                }
+                    } else if (child instanceof ASTClassOrInterfaceType){
-	            }
+                       addViolation(data, throwStatement);
+                    }
+                }
             }
-
         }
         return super.visit(catchStmt, data);
     }
 
+    private boolean isInitCauseCalled(String target, List<NameOccurrence> occurrences) {
+        boolean initCauseCalled = false;
+        for (NameOccurrence occurrence : occurrences) {
+            String image = null;
+            if (occurrence.getLocation() != null) {
+                image = occurrence.getLocation().getImage();
+            }
+            if (image != null && image.endsWith("initCause")) {
+                ASTPrimaryExpression primaryExpression = occurrence.getLocation().getFirstParentOfType(ASTPrimaryExpression.class);
+                if (primaryExpression != null) {
+                    ASTArgumentList args2 = primaryExpression.getFirstDescendantOfType(ASTArgumentList.class);
+                    if (checkArgumentList(target, args2)) {
+                        initCauseCalled = true;
+                        break;
+                    }
+                }
+            }
+        }
+        return initCauseCalled;
+    }
+
     @Override
     public Object visit(ASTVariableDeclarator node, Object data) {
 	// Search Catch stmt nodes for variable used to store improperly created throwable or exception
@@ -131,21 +151,29 @@ public class PreserveStackTraceRule extends AbstractJavaRule {
 		return false;
 	}
 
-    private boolean isThrownExceptionOfType(ASTThrowStatement throwStatement,String type) {
+    /**
-        return throwStatement.hasDescendantMatchingXPath("Expression/PrimaryExpression/PrimaryPrefix/AllocationExpression/ClassOrInterfaceType[@Image = '" + type + "']");
+     * Checks whether the given target is in the argument list.
-    }
+     * If this is the case, then the target (root exception) is used as the cause.
+     * @param target
+     * @param args
+     */
+	private boolean checkArgumentList(String target, ASTArgumentList args) {
+	    boolean match = false;
+	    if (target != null && args != null) {
+            List<ASTName> nameNodes = args.findDescendantsOfType(ASTName.class);
+            for (ASTName nameNode : nameNodes) {
+                if (target.equals(nameNode.getImage())) {
+                    match = true;
+                    break;
+                }
+            }
+	    }
+        return match;
+	}
 
 	private void ck(Object data, String target, ASTThrowStatement throwStatement,
                     ASTArgumentList args) {
-        boolean match = false;
+        if (!checkArgumentList(target, args)) {
-        List<ASTName> nameNodes = args.findDescendantsOfType(ASTName.class);
-        for (ASTName nameNode : nameNodes) {
-            if (target.equals(nameNode.getImage())) {
-                match = true;
-                break;
-            }
-        }
-        if ( ! match) {
             RuleContext ctx = (RuleContext) data;
             addViolation(ctx, throwStatement);
         }

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

@@ -246,9 +246,9 @@ public class B {
 
 	<test-code>
         <description><![CDATA[
-15, IllegalStateException does not have any constructor which takes original exception.
+15, IllegalStateException can take a cause exception in the constructor, too.
      ]]></description>
-        <expected-problems>0</expected-problems>
+        <expected-problems>1</expected-problems>
         <code><![CDATA[
 public class Foo {
     public void bar() {
@@ -444,6 +444,44 @@ public class Stuff {
     private void doMoreStuff() {
         // Stuff happens
     }
+}
+        ]]></code>
+    </test-code>
+
+    <test-code>
+        <description>#1087 PreserveStackTrace (still) ignores initCause()</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+public class Test {
+    public void foo() {
+        try {
+            // do something
+        } catch (ArrayIndexOutOfBoundsException arrayIndexOutOfBoundsException) {
+            NoSuchElementException noSuchElementException = new NoSuchElementException(
+                    "Cannot return next element, because there is none!");
+            noSuchElementException.initCause(arrayIndexOutOfBoundsException);
+            throw noSuchElementException;
+        }
+    }
+}
+        ]]></code>
+    </test-code>
+
+    <test-code>
+        <description>#1087 PreserveStackTrace (still) ignores initCause() - negative test case</description>
+        <expected-problems>1</expected-problems>
+        <code><![CDATA[
+public class Test {
+    public void foo() {
+        try {
+            // do something
+        } catch (ArrayIndexOutOfBoundsException arrayIndexOutOfBoundsException) {
+            NoSuchElementException noSuchElementException = new NoSuchElementException(
+                    "Cannot return next element, because there is none!");
+            noSuchElementException.initCause(new RuntimeException("some other unrelated exception"));
+            throw noSuchElementException;
+        }
+    }
 }
         ]]></code>
     </test-code>