From 8deaf20f4d6734dbf1672dbf659ddff99a963e40 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Mart=C3=ADn=20Sotuyo=20Dodero?= Date: Mon, 5 Mar 2018 02:13:14 -0300 Subject: [PATCH] findDescendantsOfType defaults to not crossing boundaries - A few places actually need to do so, and some other were simply wrong - We can now cross the boundary if searching dowm from it - Anonymous inner classes are still somewhat inconsistent --- .../pmd/lang/ast/AbstractNode.java | 24 +++++-------- .../java/ast/ASTClassOrInterfaceType.java | 7 ++-- .../impl/visitors/AtfdBaseVisitor.java | 2 +- .../bestpractices/PreserveStackTraceRule.java | 4 ++- .../bestpractices/UnusedPrivateFieldRule.java | 6 ++-- .../CommentDefaultAccessModifierRule.java | 11 ++---- .../java/rule/design/TooManyFieldsRule.java | 36 ++++--------------- .../documentation/AbstractCommentRule.java | 18 ++++++---- .../rule/errorprone/CloseResourceRule.java | 4 +-- .../java/ast/ASTVariableDeclaratorIdTest.java | 6 ++-- .../pmd/lang/java/ast/JDKVersionTest.java | 4 ++- .../lang/java/symboltable/AcceptanceTest.java | 4 ++- .../ScopeAndDeclarationFinderTest.java | 8 ++++- 13 files changed, 62 insertions(+), 72 deletions(-) diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/AbstractNode.java b/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/AbstractNode.java index 3c6b6d1c0c..8f9d12fa98 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/AbstractNode.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/AbstractNode.java @@ -237,15 +237,13 @@ public abstract class AbstractNode implements Node { return parents; } - @Override public List findDescendantsOfType(Class targetType) { List list = new ArrayList<>(); - findDescendantsOfType(this, targetType, list, true); + findDescendantsOfType(this, targetType, list, false); return list; } - @Override public void findDescendantsOfType(Class targetType, List results, boolean crossBoundaries) { findDescendantsOfType(this, targetType, results, crossBoundaries); @@ -254,17 +252,15 @@ public abstract class AbstractNode implements Node { private static void findDescendantsOfType(Node node, Class targetType, List results, boolean crossFindBoundaries) { - if (!crossFindBoundaries && node.isFindBoundary()) { - return; - } - for (int i = 0; i < node.jjtGetNumChildren(); i++) { Node child = node.jjtGetChild(i); if (child.getClass() == targetType) { results.add(targetType.cast(child)); } - findDescendantsOfType(child, targetType, results, crossFindBoundaries); + if (crossFindBoundaries || !child.isFindBoundary()) { + findDescendantsOfType(child, targetType, results, crossFindBoundaries); + } } } @@ -341,19 +337,17 @@ public abstract class AbstractNode implements Node { private static T getFirstDescendantOfType(final Class descendantType, final Node node) { - if (node.isFindBoundary()) { - return null; - } - final int n = node.jjtGetNumChildren(); for (int i = 0; i < n; i++) { Node n1 = node.jjtGetChild(i); if (descendantType.isAssignableFrom(n1.getClass())) { return descendantType.cast(n1); } - final T n2 = getFirstDescendantOfType(descendantType, n1); - if (n2 != null) { - return n2; + if (!n1.isFindBoundary()) { + final T n2 = getFirstDescendantOfType(descendantType, n1); + if (n2 != null) { + return n2; + } } } return null; diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTClassOrInterfaceType.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTClassOrInterfaceType.java index 7b86601c23..4a3bb535c4 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTClassOrInterfaceType.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTClassOrInterfaceType.java @@ -5,6 +5,7 @@ package net.sourceforge.pmd.lang.java.ast; +import java.util.ArrayList; import java.util.List; import net.sourceforge.pmd.lang.ast.Node; @@ -35,13 +36,15 @@ public class ASTClassOrInterfaceType extends AbstractJavaTypeNode { */ public boolean isReferenceToClassSameCompilationUnit() { ASTCompilationUnit root = getFirstParentOfType(ASTCompilationUnit.class); - List classes = root.findDescendantsOfType(ASTClassOrInterfaceDeclaration.class); + List classes = new ArrayList<>(); + root.findDescendantsOfType(ASTClassOrInterfaceDeclaration.class, classes, true); for (ASTClassOrInterfaceDeclaration c : classes) { if (c.hasImageEqualTo(getImage())) { return true; } } - List enums = root.findDescendantsOfType(ASTEnumDeclaration.class); + List enums = new ArrayList<>(); + root.findDescendantsOfType(ASTEnumDeclaration.class, enums, true); for (ASTEnumDeclaration e : enums) { if (e.hasImageEqualTo(getImage())) { return true; diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/impl/visitors/AtfdBaseVisitor.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/impl/visitors/AtfdBaseVisitor.java index 6c95fa3821..34873640c1 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/impl/visitors/AtfdBaseVisitor.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/impl/visitors/AtfdBaseVisitor.java @@ -103,7 +103,7 @@ public class AtfdBaseVisitor extends JavaParserVisitorAdapter { private boolean isAttributeAccess(ASTPrimaryExpression node) { - return node.findDescendantsOfType(ASTPrimarySuffix.class).isEmpty(); + return !node.hasDescendantOfType(ASTPrimarySuffix.class); } } diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/PreserveStackTraceRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/PreserveStackTraceRule.java index dbff360e20..9fb38e8fee 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/PreserveStackTraceRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/PreserveStackTraceRule.java @@ -4,6 +4,7 @@ package net.sourceforge.pmd.lang.java.rule.bestpractices; +import java.util.ArrayList; import java.util.List; import java.util.Map; import java.util.Objects; @@ -139,7 +140,8 @@ public class PreserveStackTraceRule extends AbstractJavaRule { private boolean checkForTargetUsage(String target, Node baseNode) { boolean match = false; if (target != null && baseNode != null) { - List nameNodes = baseNode.findDescendantsOfType(ASTName.class); + List nameNodes = new ArrayList<>(); + baseNode.findDescendantsOfType(ASTName.class, nameNodes, true); for (ASTName nameNode : nameNodes) { if (target.equals(nameNode.getImage())) { boolean isPartOfStringConcatenation = isStringConcat(nameNode, baseNode); diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/UnusedPrivateFieldRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/UnusedPrivateFieldRule.java index a1e4a5f0c9..5296aff40d 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/UnusedPrivateFieldRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/UnusedPrivateFieldRule.java @@ -100,14 +100,16 @@ public class UnusedPrivateFieldRule extends AbstractLombokAwareRule { nodes.addAll(enumConstants); for (JavaNode node : nodes) { - List primarySuffixes = node.findDescendantsOfType(ASTPrimarySuffix.class); + List primarySuffixes = new ArrayList<>(); + node.findDescendantsOfType(ASTPrimarySuffix.class, primarySuffixes, true); for (ASTPrimarySuffix primarySuffix : primarySuffixes) { if (decl.getImage().equals(primarySuffix.getImage())) { return true; // No violation } } - List primaryPrefixes = node.findDescendantsOfType(ASTPrimaryPrefix.class); + List primaryPrefixes = new ArrayList<>(); + node.findDescendantsOfType(ASTPrimaryPrefix.class, primaryPrefixes, true); for (ASTPrimaryPrefix primaryPrefix : primaryPrefixes) { ASTName name = primaryPrefix.getFirstDescendantOfType(ASTName.class); diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/CommentDefaultAccessModifierRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/CommentDefaultAccessModifierRule.java index faf19b4dbb..e91af11fd2 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/CommentDefaultAccessModifierRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/CommentDefaultAccessModifierRule.java @@ -117,14 +117,9 @@ public class CommentDefaultAccessModifierRule extends AbstractCommentRule { if (parent != null) { List annotations = parent.findChildrenOfType(ASTAnnotation.class); for (ASTAnnotation annotation : annotations) { - List names = annotation.findDescendantsOfType(ASTName.class); - for (ASTName name : names) { - if (name.hasImageEqualTo("VisibleForTesting")) { - result = false; - break; - } - } - if (result == false) { + final ASTName name = annotation.getFirstDescendantOfType(ASTName.class); + if (name.hasImageEqualTo("VisibleForTesting")) { + result = false; break; } } diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/TooManyFieldsRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/TooManyFieldsRule.java index febbffd080..7bf464f7c7 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/TooManyFieldsRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/TooManyFieldsRule.java @@ -4,50 +4,37 @@ package net.sourceforge.pmd.lang.java.rule.design; -import java.util.HashMap; import java.util.List; -import java.util.Map; -import net.sourceforge.pmd.lang.ast.Node; import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceDeclaration; -import net.sourceforge.pmd.lang.java.ast.ASTCompilationUnit; import net.sourceforge.pmd.lang.java.ast.ASTFieldDeclaration; import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule; import net.sourceforge.pmd.properties.IntegerProperty; -import net.sourceforge.pmd.util.NumericConstants; public class TooManyFieldsRule extends AbstractJavaRule { private static final int DEFAULT_MAXFIELDS = 15; - private Map stats; - private Map nodes; - private static final IntegerProperty MAX_FIELDS_DESCRIPTOR = new IntegerProperty("maxfields", "Max allowable fields", 1, 300, DEFAULT_MAXFIELDS, 1.0f); public TooManyFieldsRule() { definePropertyDescriptor(MAX_FIELDS_DESCRIPTOR); + addRuleChainVisit(ASTClassOrInterfaceDeclaration.class); } @Override - public Object visit(ASTCompilationUnit node, Object data) { + public Object visit(ASTClassOrInterfaceDeclaration node, Object data) { + final int maxFields = getProperty(MAX_FIELDS_DESCRIPTOR); + int counter = 0; - int maxFields = getProperty(MAX_FIELDS_DESCRIPTOR); - - stats = new HashMap<>(5); - nodes = new HashMap<>(5); - - List l = node.findDescendantsOfType(ASTFieldDeclaration.class); + final List l = node.findDescendantsOfType(ASTFieldDeclaration.class); for (ASTFieldDeclaration fd : l) { if (fd.isFinal() && fd.isStatic()) { continue; } - ASTClassOrInterfaceDeclaration clazz = fd.getFirstParentOfType(ASTClassOrInterfaceDeclaration.class); - if (clazz != null && !clazz.isInterface()) { - bumpCounterFor(clazz); - } + counter++; } for (Map.Entry entry : stats.entrySet()) { int val = entry.getValue(); @@ -56,16 +43,7 @@ public class TooManyFieldsRule extends AbstractJavaRule { addViolation(data, n); } } + return data; } - - private void bumpCounterFor(ASTClassOrInterfaceDeclaration clazz) { - String key = clazz.getImage(); - if (!stats.containsKey(key)) { - stats.put(key, NumericConstants.ZERO); - nodes.put(key, clazz); - } - Integer i = Integer.valueOf(stats.get(key) + 1); - stats.put(key, i); - } } diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/documentation/AbstractCommentRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/documentation/AbstractCommentRule.java index 5fd1da2575..9c7a33da50 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/documentation/AbstractCommentRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/documentation/AbstractCommentRule.java @@ -199,22 +199,26 @@ public abstract class AbstractCommentRule extends AbstractJavaRule { SortedMap itemsByLineNumber = new TreeMap<>(); - List packageDecl = cUnit - .findDescendantsOfType(ASTClassOrInterfaceDeclaration.class); - addDeclarations(itemsByLineNumber, packageDecl); + List classDecl = new ArrayList<>(); + cUnit.findDescendantsOfType(ASTClassOrInterfaceDeclaration.class, classDecl, true); + addDeclarations(itemsByLineNumber, classDecl); addDeclarations(itemsByLineNumber, cUnit.getComments()); - List fields = cUnit.findDescendantsOfType(ASTFieldDeclaration.class); + List fields = new ArrayList<>(); + cUnit.findDescendantsOfType(ASTFieldDeclaration.class, fields, true); addDeclarations(itemsByLineNumber, fields); - List methods = cUnit.findDescendantsOfType(ASTMethodDeclaration.class); + List methods = new ArrayList<>(); + cUnit.findDescendantsOfType(ASTMethodDeclaration.class, methods, true); addDeclarations(itemsByLineNumber, methods); - List constructors = cUnit.findDescendantsOfType(ASTConstructorDeclaration.class); + List constructors = new ArrayList<>(); + cUnit.findDescendantsOfType(ASTConstructorDeclaration.class, constructors, true); addDeclarations(itemsByLineNumber, constructors); - List enumDecl = cUnit.findDescendantsOfType(ASTEnumDeclaration.class); + List enumDecl = new ArrayList<>(); + cUnit.findDescendantsOfType(ASTEnumDeclaration.class, enumDecl, true); addDeclarations(itemsByLineNumber, enumDecl); return itemsByLineNumber; diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/CloseResourceRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/CloseResourceRule.java index 1002ac1fa3..487f48d705 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/CloseResourceRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/CloseResourceRule.java @@ -104,13 +104,13 @@ public class CloseResourceRule extends AbstractJavaRule { @Override public Object visit(ASTConstructorDeclaration node, Object data) { checkForResources(node, data); - return data; + return super.visit(node, data); } @Override public Object visit(ASTMethodDeclaration node, Object data) { checkForResources(node, data); - return data; + return super.visit(node, data); } private void checkForResources(Node node, Object data) { diff --git a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/ast/ASTVariableDeclaratorIdTest.java b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/ast/ASTVariableDeclaratorIdTest.java index 299c4348f7..c85b99069b 100644 --- a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/ast/ASTVariableDeclaratorIdTest.java +++ b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/ast/ASTVariableDeclaratorIdTest.java @@ -47,7 +47,8 @@ public class ASTVariableDeclaratorIdTest { @Test public void testLambdaWithType() throws Exception { ASTCompilationUnit acu = parseJava18(TEST_LAMBDA_WITH_TYPE); - ASTVariableDeclaratorId f = acu.findDescendantsOfType(ASTVariableDeclaratorId.class).get(1); + ASTLambdaExpression lambda = acu.getFirstDescendantOfType(ASTLambdaExpression.class); + ASTVariableDeclaratorId f = lambda.getFirstDescendantOfType(ASTVariableDeclaratorId.class); assertEquals("File", f.getTypeNode().getTypeImage()); assertEquals("File", f.getTypeNameNode().jjtGetChild(0).getImage()); } @@ -55,7 +56,8 @@ public class ASTVariableDeclaratorIdTest { @Test public void testLambdaWithoutType() throws Exception { ASTCompilationUnit acu = parseJava18(TEST_LAMBDA_WITHOUT_TYPE); - ASTVariableDeclaratorId f = acu.findDescendantsOfType(ASTVariableDeclaratorId.class).get(1); + ASTLambdaExpression lambda = acu.getFirstDescendantOfType(ASTLambdaExpression.class); + ASTVariableDeclaratorId f = lambda.getFirstDescendantOfType(ASTVariableDeclaratorId.class); assertNull(f.getTypeNode()); assertNull(f.getTypeNameNode()); } diff --git a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/ast/JDKVersionTest.java b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/ast/JDKVersionTest.java index 11771f620e..e50c0c651d 100644 --- a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/ast/JDKVersionTest.java +++ b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/ast/JDKVersionTest.java @@ -17,6 +17,7 @@ import static org.junit.Assert.fail; import java.io.IOException; import java.nio.charset.StandardCharsets; +import java.util.ArrayList; import java.util.List; import org.apache.commons.io.IOUtils; @@ -276,7 +277,8 @@ public class JDKVersionTest { @Test public final void jdk7PrivateMethodInnerClassInterface1() { ASTCompilationUnit acu = parseJava17(loadSource("private_method_in_inner_class_interface1.java")); - List methods = acu.findDescendantsOfType(ASTMethodDeclaration.class); + List methods = new ArrayList<>(); + acu.findDescendantsOfType(ASTMethodDeclaration.class, methods, true); assertEquals(3, methods.size()); for (ASTMethodDeclaration method : methods) { assertFalse(method.isInterfaceMember()); diff --git a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/symboltable/AcceptanceTest.java b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/symboltable/AcceptanceTest.java index 627e27ae7e..4ac457822b 100644 --- a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/symboltable/AcceptanceTest.java +++ b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/symboltable/AcceptanceTest.java @@ -18,6 +18,7 @@ import net.sourceforge.pmd.PMD; import net.sourceforge.pmd.lang.ast.Node; import net.sourceforge.pmd.lang.java.ast.ASTBlock; import net.sourceforge.pmd.lang.java.ast.ASTCatchStatement; +import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTEqualityExpression; import net.sourceforge.pmd.lang.java.ast.ASTInitializer; import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclaration; @@ -129,7 +130,8 @@ public class AcceptanceTest extends STBBaseTst { @Test public void testInnerOuterClass() { parseCode(TEST_INNER_CLASS); - ASTVariableDeclaratorId vdi = acu.findDescendantsOfType(ASTVariableDeclaratorId.class).get(0); + ASTVariableDeclaratorId vdi = acu.findDescendantsOfType(ASTClassOrInterfaceDeclaration.class).get(1) // get inner class + .findDescendantsOfType(ASTVariableDeclaratorId.class).get(0); // get first declaration List usages = vdi.getUsages(); assertEquals(2, usages.size()); assertEquals(5, usages.get(0).getLocation().getBeginLine()); diff --git a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/symboltable/ScopeAndDeclarationFinderTest.java b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/symboltable/ScopeAndDeclarationFinderTest.java index 09f57978fc..b541a1fd6e 100644 --- a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/symboltable/ScopeAndDeclarationFinderTest.java +++ b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/symboltable/ScopeAndDeclarationFinderTest.java @@ -4,6 +4,7 @@ package net.sourceforge.pmd.lang.java.symboltable; +import java.util.ArrayList; import java.util.List; import java.util.Set; @@ -13,6 +14,8 @@ import org.junit.Test; import net.sourceforge.pmd.PMD; import net.sourceforge.pmd.lang.LanguageRegistry; import net.sourceforge.pmd.lang.java.JavaLanguageModule; +import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceBody; +import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceBodyDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTLambdaExpression; import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclarator; @@ -65,7 +68,10 @@ public class ScopeAndDeclarationFinderTest extends STBBaseTst { ClassScope cs = (ClassScope) acu.getFirstDescendantOfType(ASTClassOrInterfaceDeclaration.class).getScope(); Assert.assertEquals(1, cs.getClassDeclarations().size()); // There should be 1 anonymous class - List methods = acu.findDescendantsOfType(ASTMethodDeclarator.class); + List methods = new ArrayList<>(); + acu.getFirstDescendantOfType(ASTClassOrInterfaceBody.class) // outer class + .getFirstDescendantOfType(ASTClassOrInterfaceBody.class) // inner class + .findDescendantsOfType(ASTMethodDeclarator.class, methods, true); // inner class methods Assert.assertEquals(2, methods.size()); ClassScope scope1 = methods.get(0).getScope().getEnclosingScope(ClassScope.class); ClassScope scope2 = methods.get(1).getScope().getEnclosingScope(ClassScope.class);