From 28a24dec2a06b0e3472b6091202cc9c61a80dac2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Mart=C3=ADn=20Sotuyo=20Dodero?= Date: Mon, 28 Nov 2016 14:13:01 -0300 Subject: [PATCH 1/2] [java] Speedup PreserveStackTrace by over 7X - By avoiding to look at all variable declarators we can greatly improve analysis speed without loosing accurancy --- .../rule/design/PreserveStackTraceRule.java | 73 +++++-------------- 1 file changed, 17 insertions(+), 56 deletions(-) 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. From 48e803392a59802f8df5f511e4ef46aba4d092b9 Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Sat, 10 Dec 2016 10:13:11 +0100 Subject: [PATCH 2/2] Update changelog --- src/site/markdown/overview/changelog.md | 1 + 1 file changed, 1 insertion(+) diff --git a/src/site/markdown/overview/changelog.md b/src/site/markdown/overview/changelog.md index 9e74a187ac..a10ad664ea 100644 --- a/src/site/markdown/overview/changelog.md +++ b/src/site/markdown/overview/changelog.md @@ -20,6 +20,7 @@ * [#130](https://github.com/pmd/pmd/pull/130); \[core] Reduce thread contention * [#133](https://github.com/pmd/pmd/pull/133): \[java] UnnecessaryFullyQualifiedName can detect conflicts * [#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 * [#152](https://github.com/pmd/pmd/pull/152): \[java] fixes #1552 continue does not require break * [#154](https://github.com/pmd/pmd/pull/154): \[java] Fix #1547: UnusedImports: Adjust regex to support underscores