diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/PreserveStackTraceRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/PreserveStackTraceRule.java index 92f0dc033b..d58a00ea91 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/PreserveStackTraceRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/PreserveStackTraceRule.java @@ -17,14 +17,12 @@ import net.sourceforge.pmd.lang.java.ast.ASTName; import net.sourceforge.pmd.lang.java.ast.ASTPrimaryExpression; import net.sourceforge.pmd.lang.java.ast.ASTPrimaryPrefix; import net.sourceforge.pmd.lang.java.ast.ASTThrowStatement; -import net.sourceforge.pmd.lang.java.ast.ASTVariableDeclarator; import net.sourceforge.pmd.lang.java.ast.ASTVariableDeclaratorId; +import net.sourceforge.pmd.lang.java.ast.ASTVariableInitializer; import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule; import net.sourceforge.pmd.lang.java.symboltable.VariableNameDeclaration; import net.sourceforge.pmd.lang.symboltable.NameOccurrence; -import org.jaxen.JaxenException; - /** * * @author Unknown, @@ -33,12 +31,6 @@ import org.jaxen.JaxenException; */ public class PreserveStackTraceRule extends AbstractJavaRule { - // FUTURE: This detection is name based, it should probably use Type - // Resolution, to become type "based" - // it assumes the exception class contains 'Exception' in its name - private static final String FIND_THROWABLE_INSTANCE = "./VariableInitializer/Expression/PrimaryExpression/PrimaryPrefix/AllocationExpression" - + "[ClassOrInterfaceType[contains(@Image,'Exception')] and Arguments[count(*)=0]]"; - private static final String FILL_IN_STACKTRACE = ".fillInStackTrace"; @Override @@ -82,10 +74,17 @@ public class PreserveStackTraceRule extends AbstractJavaRule { List 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); + // Check how the variable is initialized + ASTVariableInitializer initializer = decl.getNode().jjtGetParent() + .getFirstDescendantOfType(ASTVariableInitializer.class); + if (initializer != null) { + args = initializer.getFirstDescendantOfType(ASTArgumentList.class); + if (args != null) { + // constructor with args? + ck(data, target, throwStatement, args); + } else if (!isFillInStackTraceCalled(target, initializer)) { + addViolation(data, throwStatement); + } } } } @@ -99,6 +98,11 @@ public class PreserveStackTraceRule extends AbstractJavaRule { return super.visit(catchStmt, data); } + private boolean isFillInStackTraceCalled(final String target, final ASTVariableInitializer initializer) { + final ASTName astName = initializer.getFirstDescendantOfType(ASTName.class); + return astName != null && astName.hasImageEqualTo(target + FILL_IN_STACKTRACE); + } + private boolean isInitCauseCalled(String target, List occurrences) { boolean initCauseCalled = false; for (NameOccurrence occurrence : occurrences) { @@ -121,49 +125,6 @@ public class PreserveStackTraceRule extends AbstractJavaRule { return initCauseCalled; } - @Override - public Object visit(ASTVariableDeclarator node, Object data) { - // Search Catch stmt nodes for variable used to store improperly created - // throwable or exception - try { - if (node.hasDescendantMatchingXPath(FIND_THROWABLE_INSTANCE)) { - String variableName = node.jjtGetChild(0).getImage(); // VariableDeclatorId - ASTCatchStatement catchStmt = node.getFirstParentOfType(ASTCatchStatement.class); - - while (catchStmt != null) { - List violations = catchStmt - .findChildNodesWithXPath("//Expression/PrimaryExpression/PrimaryPrefix/Name[@Image = '" - + variableName + "']"); - if (!violations.isEmpty()) { - // If, after this allocation, the 'initCause' method is - // called, and the ex passed - // this is not a violation - if (!useInitCause(violations.get(0), catchStmt)) { - 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... - throw new IllegalStateException(e); - } - } - - private boolean useInitCause(Node node, ASTCatchStatement catchStmt) { - // In case of NPE... - if (node != null && node.getImage() != null) { - return catchStmt - .hasDescendantMatchingXPath("./Block/BlockStatement/Statement/StatementExpression/PrimaryExpression/PrimaryPrefix/Name[@Image = '" - + node.getImage() + ".initCause']"); - } - return false; - } - /** * 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. diff --git a/src/site/markdown/overview/changelog.md b/src/site/markdown/overview/changelog.md index bd00c408e5..42d60b1b95 100644 --- a/src/site/markdown/overview/changelog.md +++ b/src/site/markdown/overview/changelog.md @@ -38,6 +38,7 @@ * [#137](https://github.com/pmd/pmd/pull/137): \[apex] Adjusted remediation points * [#138](https://github.com/pmd/pmd/pull/138): \[java] Make ClasspathClassLoader parallel capable * [#140](https://github.com/pmd/pmd/pull/140): \[java] Make CloneMethodMustImplementCloneable over 500x faster +* [#141](https://github.com/pmd/pmd/pull/141): \[java] Speedup PreserveStackTraceRule by over 7X * [#146](https://github.com/pmd/pmd/pull/146): \[apex] Detection of missing Apex CRUD checks for SOQL/DML operations * [#147](https://github.com/pmd/pmd/pull/147): \[apex] Adding XSS detection to return statements * [#148](https://github.com/pmd/pmd/pull/148): \[apex] Improving detection of SOQL injection