From 038503677bc5b6c7c25100278f741f6c58d0a23c Mon Sep 17 00:00:00 2001 From: oowekyala Date: Fri, 11 Aug 2017 17:49:00 +0200 Subject: [PATCH] Added more tests --- .../design/ForLoopShouldBeForeachRule.java | 114 ++++++++++-------- .../design/xml/ForLoopShouldBeForeach.xml | 95 ++++++++++++++- 2 files changed, 155 insertions(+), 54 deletions(-) diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/ForLoopShouldBeForeachRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/ForLoopShouldBeForeachRule.java index 02d7598a24..30b4d31cb7 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/ForLoopShouldBeForeachRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/ForLoopShouldBeForeachRule.java @@ -18,7 +18,6 @@ import net.sourceforge.pmd.lang.java.ast.ASTName; import net.sourceforge.pmd.lang.java.ast.ASTPrimaryPrefix; import net.sourceforge.pmd.lang.java.ast.ASTPrimarySuffix; import net.sourceforge.pmd.lang.java.ast.ASTRelationalExpression; -import net.sourceforge.pmd.lang.java.ast.ASTVariableDeclaratorId; import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule; import net.sourceforge.pmd.lang.java.symboltable.VariableNameDeclaration; import net.sourceforge.pmd.lang.symboltable.NameOccurrence; @@ -29,69 +28,63 @@ import net.sourceforge.pmd.lang.symboltable.Scope; */ public class ForLoopShouldBeForeachRule extends AbstractJavaRule { - @Override public Object visit(ASTForStatement node, Object data) { - ASTForInit init = node.getFirstChildOfType(ASTForInit.class); + final ASTForInit init = node.getFirstChildOfType(ASTForInit.class); + final ASTForUpdate update = node.getFirstChildOfType(ASTForUpdate.class); + final ASTExpression guardCondition = node.getFirstChildOfType(ASTExpression.class); - if (init == null) { + if (init == null && update == null || guardCondition == null) { // The loop may be replaced with a while return super.visit(node, data); } - String itName = init.getFirstDescendantOfType(ASTVariableDeclaratorId.class).getImage(); - - - ASTExpression guardCondition = node.getFirstChildOfType(ASTExpression.class); - - if (guardCondition == null || !isForUpdateSimpleEnough(node.getFirstChildOfType(ASTForUpdate.class), itName)) { + VariableNameDeclaration index = getIndexVarDeclaration(init, update); + if (index == null || !"int".equals(index.getTypeImage())) { return super.visit(node, data); } - String iterableName = getIterableNameAndCheckGuardExpression(guardCondition, itName); + String itName = index.getName(); + String iterableName = getIterableNameOrNullToAbort(guardCondition, itName); - if (iterableName == null) { + if (!isForUpdateSimpleEnough(update, itName) || iterableName == null) { return super.visit(node, data); } - List occurrences = getIteratorOccurencesAndCheckInit(init); + + List occurrences = guardCondition.getScope().getDeclarations(VariableNameDeclaration.class).get(index); if (occurrences == null) { return super.visit(node, data); } - VariableNameDeclaration iterableDeclaration = findDeclaration(iterableName, node.getScope()); if (iterableDeclaration == null) { return super.visit(node, data); } - if (iterableDeclaration.isArray()) { - loopOverArrayCanBeReplaced(node); - } else if ("List".equals(iterableDeclaration.getTypeImage())) { - if (loopOverListCanBeReplaced(node, occurrences, iterableDeclaration)) { - addViolation(data, node); - } + if (iterableDeclaration.isArray() && loopOverArrayCanBeReplaced(node)) { + addViolation(data, node); + } else if ("List".equals(iterableDeclaration.getTypeImage()) + && loopOverListCanBeReplaced(node, occurrences, iterableDeclaration)) { + addViolation(data, node); } return super.visit(node, data); } - /** - * Gets the name occurences of the iterator in the code block, and checks that the iterator is of type int - * - * @param init For init - * - * @return name occurences or null (then abort) - */ - private List getIteratorOccurencesAndCheckInit(ASTForInit init) { + /* Finds the declaration of the index variable to find its name occurrences */ + private VariableNameDeclaration getIndexVarDeclaration(ASTForInit init, ASTForUpdate update) { + if (init == null) { + return guessIndexVarFromUpdate(update); + } + Map> decls = init.getScope().getDeclarations(VariableNameDeclaration.class); VariableNameDeclaration theIterator = null; - for (VariableNameDeclaration decl : decls.keySet()) { ASTForInit declInit = decl.getNode().getFirstParentOfType(ASTForInit.class); if (declInit == init) { @@ -100,11 +93,26 @@ public class ForLoopShouldBeForeachRule extends AbstractJavaRule { } } - if (theIterator == null || !"int".equals(theIterator.getTypeImage())) { + return theIterator; + + } + + + /** Does a best guess to find the index variable, gives up if the update has several statements */ + private VariableNameDeclaration guessIndexVarFromUpdate(ASTForUpdate update) { + + Node name; + try { + name = update.findChildNodesWithXPath(getSimpleForStatementXpath(null)).get(0); + } catch (JaxenException je) { + throw new RuntimeException(je); + } + + if (name == null || name.getImage() == null) { return null; } - return decls.get(theIterator); + return findDeclaration(name.getImage(), update.getScope().getParent()); } @@ -112,24 +120,29 @@ public class ForLoopShouldBeForeachRule extends AbstractJavaRule { * @return true if there's only one update statement of the form i++ or ++i. */ private boolean isForUpdateSimpleEnough(ASTForUpdate update, String itName) { - return update.hasDescendantMatchingXPath("//StatementExpressionList[count(*)=1]" - + "/StatementExpression" - + "/*[self::PostfixExpression and @Image='++' or self::PreIncrementExpression]" - + "/PrimaryExpression" - + "/PrimaryPrefix" - + "/Name[@Image='" + itName + "']"); + return update.hasDescendantMatchingXPath(getSimpleForStatementXpath(itName)); + } + + + private String getSimpleForStatementXpath(String itName) { + return "//StatementExpressionList[count(*)=1]" + + "/StatementExpression" + + "/*[self::PostfixExpression and @Image='++' or self::PreIncrementExpression]" + + "/PrimaryExpression" + + "/PrimaryPrefix" + + "/Name" + + (itName == null ? "" : ("[@Image='" + itName + "']")); } /** * Gets the name of the iterable array or list. * - * @param guardCondition The guard condition - * @param itName The name of the iterator variable + * @param itName The name of the iterator variable * * @return The name, or null if it couldn't be found or the guard condition is not safe to refactor (then abort) */ - private String getIterableNameAndCheckGuardExpression(ASTExpression guardCondition, String itName) { + private String getIterableNameOrNullToAbort(ASTExpression guardCondition, String itName) { if (guardCondition.jjtGetNumChildren() > 0 @@ -137,13 +150,11 @@ public class ForLoopShouldBeForeachRule extends AbstractJavaRule { ASTRelationalExpression relationalExpression = (ASTRelationalExpression) guardCondition.jjtGetChild(0); - if (relationalExpression.hasImageEqualTo("<")) { + if (relationalExpression.hasImageEqualTo("<") || relationalExpression.hasImageEqualTo("<=")) { try { - List left - = guardCondition.findChildNodesWithXPath( - "//RelationalExpression[@Image='<']/PrimaryExpression/PrimaryPrefix/Name[@Image='" + itName - + "']"); + List left = guardCondition.findChildNodesWithXPath( + "//RelationalExpression/PrimaryExpression/PrimaryPrefix/Name[@Image='" + itName + "']"); List right = guardCondition.findChildNodesWithXPath( "//RelationalExpression[@Image='<']/PrimaryExpression/PrimaryPrefix" @@ -161,9 +172,8 @@ public class ForLoopShouldBeForeachRule extends AbstractJavaRule { return null; } - } catch (JaxenException e) { - e.printStackTrace(); - return null; + } catch (JaxenException je) { + throw new RuntimeException(je); } } } @@ -177,7 +187,7 @@ public class ForLoopShouldBeForeachRule extends AbstractJavaRule { } - private boolean loopOverListCanBeReplaced(ASTForStatement node, List occurrences, + private boolean loopOverListCanBeReplaced(ASTForStatement stmt, List occurrences, VariableNameDeclaration listDeclaration) { String listName = listDeclaration.getName(); @@ -185,9 +195,9 @@ public class ForLoopShouldBeForeachRule extends AbstractJavaRule { for (NameOccurrence occ : occurrences) { - if (occ.getLocation().getFirstParentOfType(ASTForUpdate.class) == null - && occ.getLocation().getFirstParentOfType(ASTExpression.class) != node.getFirstChildOfType(ASTExpression.class) + && occ.getLocation().getFirstParentOfType(ASTExpression.class) + != stmt.getFirstChildOfType(ASTExpression.class) && !occurenceIsListGet(occ, listName)) { return false; } @@ -236,6 +246,4 @@ public class ForLoopShouldBeForeachRule extends AbstractJavaRule { return null; } - - } diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/design/xml/ForLoopShouldBeForeach.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/design/xml/ForLoopShouldBeForeach.xml index c5ef120a67..d35d3b17a2 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/design/xml/ForLoopShouldBeForeach.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/design/xml/ForLoopShouldBeForeach.xml @@ -1,7 +1,7 @@ - Simple failure case + Positive with list 1 + + + Positive with lower or equal + 1 + lo) { + for (int i = 0; i <= lo.size() - 1; i++) { + System.out.println(lo.get(i)); + } + } + } + ]]> + +- + + Usage of index var outside get + 0 + l) { + for (int i = 0; i < l.size(); i++) { + System.out.println(i + ": " + l.get(i)); + + } + } + } + ]]> + + + + Subclass of List + + 0 + l) { + for (int i = 0; i < l.size(); i++) { + System.out.println(l.get(i)); + + } + } + } + ]]> + + + + Get called on another list + 0 + l) { + List l2 = new ArrayList<>(l); + for (int i = 0; i < l.size(); i++) { + System.out.println(l2.get(i)); + } + } + } + ]]> + + + + Backwards iteration + 0 + l) { + for (int i = l.size() - 1; i > 0; i--) { + System.out.println(i + ": " + l.get(i)); + } + } + } + ]]> + + + + Index var initialized outside for init + + 0 + l) { + int i = 0; + for (; i < l.size(); i++) { + System.out.println(l.get(i)); + } + } + } + ]]> + + + +