diff --git a/pmd/etc/changelog.txt b/pmd/etc/changelog.txt index 6c3d38aaa7..51ede7396c 100644 --- a/pmd/etc/changelog.txt +++ b/pmd/etc/changelog.txt @@ -3,6 +3,8 @@ '41' and '42' shortcuts for rulesets added Fixed bug 1928009 - Error using migration ruleset in PMD 4.2 Fixed bug 1932242 - EmptyMethodInAbstractClassShouldBeAbstract false + +Fixed bug 1808110 - PreserveStackTrace + AvoidDuplicateLiteralRule now has 'skipAnnotations' boolean property ruleset.dtd and ruleset_xml_schema.xsd added to jar file in rulesets directory Update RuleSetWriter to handle non-Apache TRAX implementations, add an option to not use XML Namespaces diff --git a/pmd/regress/test/net/sourceforge/pmd/rules/design/xml/PreserveStackTrace.xml b/pmd/regress/test/net/sourceforge/pmd/rules/design/xml/PreserveStackTrace.xml index cb5d7cd255..d36b79bb6f 100644 --- a/pmd/regress/test/net/sourceforge/pmd/rules/design/xml/PreserveStackTrace.xml +++ b/pmd/regress/test/net/sourceforge/pmd/rules/design/xml/PreserveStackTrace.xml @@ -162,8 +162,8 @@ public class Foo { public void foo(String a) { try { int i = Integer.parseInt(a); - } catch(Exception e){ - Exception e1 = new Exception(e); + } catch(Exception e10){ + Exception e1 = new Exception(e10); throw e1; } } @@ -243,4 +243,63 @@ public class B { } ]]> + + + + 0 + + + + + + 1 + + + + + + 0 + + + \ No newline at end of file diff --git a/pmd/src/net/sourceforge/pmd/rules/design/PreserveStackTrace.java b/pmd/src/net/sourceforge/pmd/rules/design/PreserveStackTrace.java index 372139b88b..e9c94f6312 100644 --- a/pmd/src/net/sourceforge/pmd/rules/design/PreserveStackTrace.java +++ b/pmd/src/net/sourceforge/pmd/rules/design/PreserveStackTrace.java @@ -1,6 +1,13 @@ +/** + * BSD-style license; for more info see http://pmd.sourceforge.net/license.html + */ package net.sourceforge.pmd.rules.design; -import net.sourceforge.pmd.AbstractRule; +import java.util.ArrayList; +import java.util.List; +import java.util.Map; + +import net.sourceforge.pmd.AbstractJavaRule; import net.sourceforge.pmd.RuleContext; import net.sourceforge.pmd.ast.ASTArgumentList; import net.sourceforge.pmd.ast.ASTCastExpression; @@ -10,61 +17,138 @@ 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.Node; import net.sourceforge.pmd.ast.SimpleNode; import net.sourceforge.pmd.symboltable.NameOccurrence; import net.sourceforge.pmd.symboltable.VariableNameDeclaration; -import java.util.ArrayList; -import java.util.List; -import java.util.Map; +import org.jaxen.JaxenException; -public class PreserveStackTrace extends AbstractRule { +/** + * + * @author Unknown, + * @author Romain PELISSE, belaran@gmail.com, fix for bug 1808110 + * + */ +public class PreserveStackTrace extends AbstractJavaRule { private List nameNodes = new ArrayList(); - public Object visit(ASTCatchStatement node, Object data) { - String target = (((SimpleNode) node.jjtGetChild(0).jjtGetChild(1)).getImage()); - List lstThrowStatements = node.findChildrenOfType(ASTThrowStatement.class); + // FUTURE: This is dectection is name based, it should probably used Type Resolution, to become type "based" + private static final String FIND_THROWABLE_INSTANCE = "//VariableDeclaratorId[" + + "(../descendant::VariableInitializer/Expression/PrimaryExpression/PrimaryPrefix/AllocationExpression/ClassOrInterfaceType" + + "[" + + "contains(@Image,'Exception')" + // Assuming the Exception class does contains 'Exception' in its name + "and" + + "(not (../Arguments/ArgumentList))" + + "]" + + ")]"; + + private static final String ILLEGAL_STATE_EXCEPTION = "IllegalStateException"; + private static final String FILL_IN_STACKTRACE = ".fillInStackTrace"; + + 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 lstThrowStatements = catchStmt.findChildrenOfType(ASTThrowStatement.class); for (ASTThrowStatement throwStatement : lstThrowStatements) { - SimpleNode sn = (SimpleNode) throwStatement.jjtGetChild(0).jjtGetChild(0); - if (sn.getClass().equals(ASTCastExpression.class)) { - ASTPrimaryExpression expr = (ASTPrimaryExpression) sn.jjtGetChild(1); + Node n = throwStatement.jjtGetChild(0).jjtGetChild(0); + if (n.getClass().equals(ASTCastExpression.class)) { + ASTPrimaryExpression expr = (ASTPrimaryExpression) n.jjtGetChild(1); if (expr.jjtGetNumChildren() > 1 && expr.jjtGetChild(1).getClass().equals(ASTPrimaryPrefix.class)) { RuleContext ctx = (RuleContext) data; addViolation(ctx, throwStatement); } continue; } - ASTArgumentList args = throwStatement.getFirstChildOfType(ASTArgumentList.class); + // If the thrown exception is IllegalStateException, no way to preserve the exception (the constructor has no args) + if ( ! isThrownExceptionOfType(throwStatement,ILLEGAL_STATE_EXCEPTION) ) { + // Retrieve all argument for the throw exception (to see if the original exception is preserved) + ASTArgumentList args = throwStatement.getFirstChildOfType(ASTArgumentList.class); - if (args != null) { - ck(data, target, throwStatement, args); - } else { - SimpleNode child = (SimpleNode) throwStatement.jjtGetChild(0); - while (child != null && child.jjtGetNumChildren() > 0 - && !child.getClass().equals(ASTName.class)) { - child = (SimpleNode) child.jjtGetChild(0); - } - if (child != null){ - if( child.getClass().equals(ASTName.class) && (!target.equals(child.getImage()) && !child.hasImageEqualTo(target + ".fillInStackTrace"))) { - Map> vars = ((ASTName) child).getScope().getVariableDeclarations(); - for (VariableNameDeclaration decl: vars.keySet()) { - args = ((SimpleNode) decl.getNode().jjtGetParent()) - .getFirstChildOfType(ASTArgumentList.class); - if (args != null) { - ck(data, target, throwStatement, args); - } + if (args != null) { + ck(data, target, throwStatement, args); + } + else { + Node child = throwStatement.jjtGetChild(0); + while (child != null && child.jjtGetNumChildren() > 0 + && !child.getClass().equals(ASTName.class)) { + child = child.jjtGetChild(0); + } + if (child != null){ + if( child.getClass().equals(ASTName.class) && (!target.equals(((SimpleNode)child).getImage()) && !((SimpleNode)child).hasImageEqualTo(target + FILL_IN_STACKTRACE))) { + Map> vars = ((ASTName) child).getScope().getVariableDeclarations(); + for (VariableNameDeclaration decl: vars.keySet()) { + args = ((SimpleNode)decl.getNode().jjtGetParent()) + .getFirstChildOfType(ASTArgumentList.class); + if (args != null) { + ck(data, target, throwStatement, args); + } + } + } else if(child.getClass().equals(ASTClassOrInterfaceType.class)){ + addViolation(data, throwStatement); } - } else if(child.getClass().equals(ASTClassOrInterfaceType.class)){ - addViolation(data, throwStatement); - } - } + } + } } + } - return super.visit(node, data); + return super.visit(catchStmt, data); } - private void ck(Object data, String target, ASTThrowStatement throwStatement, + /* + * Search Catch stmt nodes for variable used to store unproperly created throwable or exception + */ + private void gatherVariableWithExceptionRef(ASTCatchStatement catchStmt, Object data) { + try { + List nodes = catchStmt.findChildNodesWithXPath(FIND_THROWABLE_INSTANCE); + for ( Node node : nodes ) { + List violations = catchStmt.findChildNodesWithXPath("//Expression/PrimaryExpression/PrimaryPrefix/Name[@Image = '" + ((SimpleNode)node).getImage() + "']"); + 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) ? + super.addViolation(data,(SimpleNode) node); + } + } + } catch (JaxenException e) { + // XPath is valid, this should never happens... + e.printStackTrace(); + } + + } + + private boolean useInitCause(Node node, ASTCatchStatement catchStmt) throws JaxenException { + // In case of NPE... + if ( node != null && ((SimpleNode)node).getImage() != null ) + { + List nodes = catchStmt.findChildNodesWithXPath("descendant::StatementExpression/PrimaryExpression/PrimaryPrefix/Name[@Image = '" + ((SimpleNode)node).getImage() + ".initCause']"); + if ( nodes != null && nodes.size() > 0 ) + { + return true; + } + } + return false; + } + + private boolean isThrownExceptionOfType(ASTThrowStatement throwStatement,String type) { + boolean status = false; + try { + List results = throwStatement.findChildNodesWithXPath("Expression/PrimaryExpression/PrimaryPrefix/AllocationExpression/ClassOrInterfaceType[@Image = '" + type + "']"); + // If we have a match, return true + if ( results != null && results.size() == 1 ) { + status = true; + } + } catch (JaxenException e) { + // XPath is valid, this should never happens ! + e.printStackTrace(); + } + return status; + } + + private void ck(Object data, String target, ASTThrowStatement throwStatement, ASTArgumentList args) { boolean match = false; nameNodes.clear(); @@ -75,7 +159,7 @@ public class PreserveStackTrace extends AbstractRule { break; } } - if (!match) { + if ( ! match) { RuleContext ctx = (RuleContext) data; addViolation(ctx, throwStatement); }