Optimizations and false positive fixes in PreserveStackTrace

git-svn-id: https://pmd.svn.sourceforge.net/svnroot/pmd/branches/pmd/4.2.x@6232 51baf565-9d33-0410-a72c-fc3788e3496d

pmd/etc/changelog.txt

@@ -6,6 +6,7 @@ Upgrading UselessOperationOnImmutable to detect more use cases, especially on St
 Fixed bug 1988829 - Violation reported without source file name (actually a fix to ConsecutiveLiteralAppends)
 Fixed bug 1989814 - false +: ConsecutiveLiteralAppends
 Fixed bug 1977230 - false positive: UselessOverridingMethod
+Optimizations and false positive fixes in PreserveStackTrace
 
 New rule:
 	Basic ruleset:	EmptyInitializer

pmd/regress/test/net/sourceforge/pmd/rules/design/xml/PreserveStackTrace.xml

@@ -302,4 +302,87 @@ public class Foo {
      ]]></code>
     </test-code>
 
+    <test-code>
+        <description><![CDATA[
+18, side effects on rules
+     ]]></description>
+        <expected-problems>1</expected-problems>
+        <code><![CDATA[
+public class Foo {
+    public void bar1() {
+        try {
+			;
+        } catch (Exception notUsed) {
+			RuntimeException ex = new RuntimeException();
+			throw ex;
+		}
+    }
+
+    public void bar2() {
+        try {
+			;
+        } catch (Exception e) {
+			IllegalStateException ex = new IllegalStateException();
+			ex.initCause(e);
+			throw ex;
+		}
+    }
+}
+     ]]></code>
+    </test-code>
+
+    <test-code>
+        <description><![CDATA[
+19, False positive
+     ]]></description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+public class Foo {
+
+    private static boolean isInstanceOf(final MBeanServer mbs,
+                                        final ObjectName name,
+                                        final String className) {
+        PrivilegedExceptionAction<Boolean> act =
+            new PrivilegedExceptionAction<Boolean>() {
+                public Boolean run() throws InstanceNotFoundException {
+                    return mbs.isInstanceOf(name, className);
+                }
+            };
+        try {
+            return AccessController.doPrivileged(act);
+        } catch (Exception e) {
+            logger.fine("isInstanceOf", "failed: " + e);
+            logger.debug("isInstanceOf", e);
+            return false;
+        }
+    }
+}
+     ]]></code>
+    </test-code>
+
+    <test-code>
+        <description><![CDATA[
+20, False positive
+     ]]></description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+public class Foo {
+
+ private CodeException[] getCodeExceptions() {
+    int             size  = exception_vec.size();
+    CodeException[] c_exc = new CodeException[size];
+
+    try {
+      for(int i=0; i < size; i++) {
+        CodeExceptionGen c = (CodeExceptionGen)exception_vec.get(i);
+        c_exc[i] = c.getCodeException(cp);
+      }
+    } catch(ArrayIndexOutOfBoundsException e) {}
+
+    return c_exc;
+  }
+}
+     ]]></code>
+    </test-code>
+
 </test-data>

pmd/rulesets/design.xml

@@ -1329,12 +1329,12 @@ public class Foo {
 
   <rule name="PreserveStackTrace"
       since="3.7"
-      message="Caught exception is rethrown, original stack trace may be lost"
+      message="New exception is thrown in catch block, original stack trace may be lost"
       class="net.sourceforge.pmd.rules.design.PreserveStackTrace"
           externalInfoUrl="http://pmd.sourceforge.net/rules/design.html#PreserveStackTrace">
       <description>
 Throwing a new exception from a catch block without passing the original exception into the
-new Exception will cause the true stack trace to be lost, and can make it difficult to
+new exception will cause the true stack trace to be lost, and can make it difficult to
 debug effectively.
       </description>
       <priority>3</priority>

pmd/src/net/sourceforge/pmd/rules/design/PreserveStackTrace.java

@@ -17,6 +17,7 @@ import net.sourceforge.pmd.ast.ASTName;
 import net.sourceforge.pmd.ast.ASTPrimaryExpression;
 import net.sourceforge.pmd.ast.ASTPrimaryPrefix;
 import net.sourceforge.pmd.ast.ASTThrowStatement;
+import net.sourceforge.pmd.ast.ASTVariableDeclarator;
 import net.sourceforge.pmd.ast.Node;
 import net.sourceforge.pmd.ast.SimpleNode;
 import net.sourceforge.pmd.symboltable.NameOccurrence;
@@ -34,23 +35,18 @@ public class PreserveStackTrace extends AbstractJavaRule {
 
     private List<ASTName> nameNodes = new ArrayList<ASTName>();
 
-    // FUTURE: This is dectection is name based, it should probably used Type Resolution, to become type "based"
+    // FUTURE: This detection is name based, it should probably use Type Resolution, to become type "based"
-    private static final String FIND_THROWABLE_INSTANCE = "//VariableDeclaratorId[" +
+    // it assumes the exception class contains 'Exception' in its name
-						    "(../descendant::VariableInitializer/Expression/PrimaryExpression/PrimaryPrefix/AllocationExpression/ClassOrInterfaceType" +
+    private static final String FIND_THROWABLE_INSTANCE =
-						    "[" +
+	"./VariableInitializer/Expression/PrimaryExpression/PrimaryPrefix/AllocationExpression" +
-						    			"contains(@Image,'Exception')" + // Assuming the Exception class does contains 'Exception' in its name
+	"[ClassOrInterfaceType[contains(@Image,'Exception')] and Arguments[count(*)=0]]";
-						    			"and" +
-						    			"(not (../Arguments/ArgumentList))" +
-						    "]" +
-						    ")]";
 
     private static final String ILLEGAL_STATE_EXCEPTION = "IllegalStateException";
     private static final String FILL_IN_STACKTRACE = ".fillInStackTrace";
 
+    @Override
     public Object visit(ASTCatchStatement catchStmt, Object data) {
         String target = ((SimpleNode)catchStmt.jjtGetChild(0).jjtGetChild(1)).getImage();
-        // Gather every variable used to store exception instance created without any argument, inside the catch
-        gatherVariableWithExceptionRef(catchStmt,data);
         // Inspect all the throw stmt inside the catch stmt
         List<ASTThrowStatement> lstThrowStatements = catchStmt.findChildrenOfType(ASTThrowStatement.class);
         for (ASTThrowStatement throwStatement : lstThrowStatements) {
@@ -72,13 +68,13 @@ public class PreserveStackTrace extends AbstractJavaRule {
 	                ck(data, target, throwStatement, args);
 	            }
 	            else {
-	        	Node child = throwStatement.jjtGetChild(0);
+	        	SimpleNode child = (SimpleNode)throwStatement.jjtGetChild(0);
 	                while (child != null && child.jjtGetNumChildren() > 0
 	                        && !child.getClass().equals(ASTName.class)) {
-	                    child = child.jjtGetChild(0);
+	                    child = (SimpleNode)child.jjtGetChild(0);
 	                }
 	                if (child != null){
-	                    if( child.getClass().equals(ASTName.class) && (!target.equals(((SimpleNode)child).getImage()) && !((SimpleNode)child).hasImageEqualTo(target + FILL_IN_STACKTRACE))) {
+	                    if( child.getClass().equals(ASTName.class) && !target.equals(child.getImage()) && !child.hasImageEqualTo(target + FILL_IN_STACKTRACE)) {
 	                        Map<VariableNameDeclaration, List<NameOccurrence>> vars = ((ASTName) child).getScope().getVariableDeclarations();
 		                    for (VariableNameDeclaration decl: vars.keySet()) {
 		                        args = ((SimpleNode)decl.getNode().jjtGetParent())
@@ -98,33 +94,41 @@ public class PreserveStackTrace extends AbstractJavaRule {
         return super.visit(catchStmt, data);
     }
 
-    /*
+    @Override
-     * Search Catch stmt nodes for variable used to store unproperly created throwable or exception
+    public Object visit(ASTVariableDeclarator node, Object data) {
-     */
+	// Search Catch stmt nodes for variable used to store improperly created throwable or exception
-    private void gatherVariableWithExceptionRef(ASTCatchStatement catchStmt, Object data) {
 	try {
-			List<Node> nodes = catchStmt.findChildNodesWithXPath(FIND_THROWABLE_INSTANCE);
+	    List<Node> nodes = node.findChildNodesWithXPath(FIND_THROWABLE_INSTANCE);
-			for ( Node node : nodes ) {
+	    if (nodes.size() > 0) {
-				List <Node> violations = catchStmt.findChildNodesWithXPath("//Expression/PrimaryExpression/PrimaryPrefix/Name[@Image = '" + ((SimpleNode)node).getImage() + "']");
+		String variableName = ((SimpleNode)node.jjtGetChild(0)).getImage(); // VariableDeclatorId
-				if ( violations != null && violations.size() > 0 ) {
+		ASTCatchStatement catchStmt = node.getFirstParentOfType(ASTCatchStatement.class);
+
+		while (catchStmt != null) {
+		    List<SimpleNode> violations = catchStmt.findChildNodesWithXPath("//Expression/PrimaryExpression/PrimaryPrefix/Name[@Image = '" + variableName + "']");
+		    if (violations != null && violations.size() > 0) {
 			// If, after this allocation, the 'initCause' method is called, and the ex passed
 			// this is not a violation
-					if ( ! useInitCause((Node)violations.get(0),catchStmt) ) //FIXME: iterate, better than get(0) ?
+			if (!useInitCause(violations.get(0), catchStmt)) {
-						super.addViolation(data,(SimpleNode) node);
+			    addViolation(data, node);
 			}
 		    }
+
+		    // check ASTCatchStatement higher up
+		    catchStmt = catchStmt.getFirstParentOfType(ASTCatchStatement.class);
+		}
+	    }
+	    return super.visit(node, data);
 	} catch (JaxenException e) {
 	    // XPath is valid, this should never happens...
-			e.printStackTrace();
+	    throw new IllegalStateException(e);
+	}
     }
 
-	}
+	private boolean useInitCause(SimpleNode node, ASTCatchStatement catchStmt) throws JaxenException {
-
-	private boolean useInitCause(Node node, ASTCatchStatement catchStmt) throws JaxenException {
 		// In case of NPE...
-		if ( node != null && ((SimpleNode)node).getImage() != null )
+		if ( node != null && node.getImage() != null )
 		{
-			List <Node> nodes = catchStmt.findChildNodesWithXPath("descendant::StatementExpression/PrimaryExpression/PrimaryPrefix/Name[@Image = '" + ((SimpleNode)node).getImage() + ".initCause']");
+			List <Node> nodes = catchStmt.findChildNodesWithXPath("descendant::StatementExpression/PrimaryExpression/PrimaryPrefix/Name[@Image = '" + node.getImage() + ".initCause']");
 			if ( nodes != null && nodes.size() > 0 )
 			{
 				return true;