From 6ccd25207068397c8a7a01472a9641b8dcf79a09 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Mart=C3=ADn=20Sotuyo=20Dodero?= Date: Sun, 12 Mar 2017 23:51:38 -0300 Subject: [PATCH] [java] Revamp AccessorClassGeneration - Fixes #291 - Complete rewrite. Simpler and faster, using the RuleChain and SymbolTable. --- .../design/AccessorClassGenerationRule.java | 334 +++--------------- .../design/xml/AccessorClassGeneration.xml | 47 +-- 2 files changed, 80 insertions(+), 301 deletions(-) diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/AccessorClassGenerationRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/AccessorClassGenerationRule.java index e53754c53b..f0f8095fc1 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/AccessorClassGenerationRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/AccessorClassGenerationRule.java @@ -5,315 +5,91 @@ package net.sourceforge.pmd.lang.java.rule.design; import java.util.ArrayList; -import java.util.Iterator; +import java.util.HashMap; import java.util.List; -import java.util.ListIterator; +import java.util.Map; +import net.sourceforge.pmd.RuleContext; import net.sourceforge.pmd.lang.java.ast.ASTAllocationExpression; -import net.sourceforge.pmd.lang.java.ast.ASTAnnotationTypeDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTArguments; -import net.sourceforge.pmd.lang.java.ast.ASTArrayDimsAndInits; -import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceType; -import net.sourceforge.pmd.lang.java.ast.ASTCompilationUnit; import net.sourceforge.pmd.lang.java.ast.ASTConstructorDeclaration; -import net.sourceforge.pmd.lang.java.ast.ASTEnumDeclaration; -import net.sourceforge.pmd.lang.java.ast.AbstractJavaAccessTypeNode; -import net.sourceforge.pmd.lang.java.ast.JavaNode; import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule; -import net.sourceforge.pmd.lang.java.symboltable.SourceFileScope; +import net.sourceforge.pmd.lang.java.symboltable.ClassScope; /** * 1. Note all private constructors. 2. Note all instantiations from outside of * the class by way of the private constructor. 3. Flag instantiations. * - *

Parameter types can not be matched because they can come as exposed members + *

+ * Parameter types can not be matched because they can come as exposed members * of classes. In this case we have no way to know what the type is. We can make - * a best effort though which can filter some?

+ * a best effort though which can filter some? + *

* * @author CL Gilbert (dnoyeb@users.sourceforge.net) * @author David Konecny (david.konecny@) * @author Romain PELISSE, belaran@gmail.com, patch bug#1807370 + * @author Juan Martin Sotuyo Dodero (juansotuyo@gmail.com), complete rewrite */ public class AccessorClassGenerationRule extends AbstractJavaRule { - private List classDataList = new ArrayList<>(); - private int classID = -1; - private String packageName; + private Map> privateConstructors = new HashMap<>(); - public Object visit(ASTEnumDeclaration node, Object data) { - return data; // just skip Enums - } - - public Object visit(ASTCompilationUnit node, Object data) { - classDataList.clear(); - packageName = node.getScope().getEnclosingScope(SourceFileScope.class).getPackageName(); - return super.visit(node, data); - } - - private static class ClassData { - private String className; - private List privateConstructors; - private List instantiations; - /** - * List of outer class names that exist above this class + public AccessorClassGenerationRule() { + super(); + /* + * Order is important. Visit constructors first to find the private + * ones, then visit allocations to find violations */ - private List classQualifyingNames; - - ClassData(String className) { - this.className = className; - this.privateConstructors = new ArrayList<>(); - this.instantiations = new ArrayList<>(); - this.classQualifyingNames = new ArrayList<>(); - } - - public void addInstantiation(AllocData ad) { - instantiations.add(ad); - } - - public Iterator getInstantiationIterator() { - return instantiations.iterator(); - } - - public void addConstructor(ASTConstructorDeclaration cd) { - privateConstructors.add(cd); - } - - public Iterator getPrivateConstructorIterator() { - return privateConstructors.iterator(); - } - - public String getClassName() { - return className; - } - - public void addClassQualifyingName(String name) { - classQualifyingNames.add(name); - } - - public List getClassQualifyingNamesList() { - return classQualifyingNames; - } + addRuleChainVisit(ASTConstructorDeclaration.class); + addRuleChainVisit(ASTAllocationExpression.class); } - private static class AllocData { - private String name; - private int argumentCount; - private ASTAllocationExpression allocationExpression; - private boolean isArray; + @Override + public void end(final RuleContext ctx) { + super.end(ctx); + // Clean up all references to the AST + privateConstructors.clear(); + } - AllocData(ASTAllocationExpression node, String aPackageName, List classQualifyingNames) { - if (node.jjtGetChild(1) instanceof ASTArguments) { - ASTArguments aa = (ASTArguments) node.jjtGetChild(1); - argumentCount = aa.getArgumentCount(); - // Get name and strip off all superfluous data - // strip off package name if it is current package - if (!(node.jjtGetChild(0) instanceof ASTClassOrInterfaceType)) { - throw new RuntimeException( - "BUG: Expected a ASTClassOrInterfaceType, got a " + node.jjtGetChild(0).getClass()); - } - ASTClassOrInterfaceType an = (ASTClassOrInterfaceType) node.jjtGetChild(0); - name = stripString(aPackageName + '.', an.getImage()); + @Override + public Object visit(final ASTConstructorDeclaration node, final Object data) { + if (node.isPrivate()) { + final String className = node.jjtGetParent().jjtGetParent().jjtGetParent().getImage(); + if (!privateConstructors.containsKey(className)) { + privateConstructors.put(className, new ArrayList()); + } + privateConstructors.get(className).add(node); + } + return data; + } - // strip off outer class names - // try OuterClass, then try OuterClass.InnerClass, then try - // OuterClass.InnerClass.InnerClass2, etc... - String findName = ""; - for (ListIterator li = classQualifyingNames.listIterator(classQualifyingNames.size()); li - .hasPrevious();) { - String aName = li.previous(); - findName = aName + '.' + findName; - if (name.startsWith(findName)) { - // strip off name and exit - name = name.substring(findName.length()); + @Override + public Object visit(final ASTAllocationExpression node, final Object data) { + if (node.jjtGetChild(0) instanceof ASTClassOrInterfaceType) { // Ignore primitives + final ASTClassOrInterfaceType type = (ASTClassOrInterfaceType) node.jjtGetChild(0); + final List constructors = privateConstructors.get(type.getImage()); + + if (constructors != null) { + final ASTArguments callArguments = (ASTArguments) node.jjtGetChild(1); + final ClassScope enclosingScope = node.getScope().getEnclosingScope(ClassScope.class); + + for (final ASTConstructorDeclaration cd : constructors) { + // Are we within the same class scope? + if (cd.getScope().getEnclosingScope(ClassScope.class) == enclosingScope) { + break; + } + + if (cd.getParameterCount() == callArguments.getArgumentCount()) { + // TODO : Check types + addViolation(data, node); break; } } - } else if (node.jjtGetChild(1) instanceof ASTArrayDimsAndInits) { - // this is incomplete because I dont need it. - // child 0 could be primitive or object (ASTName or - // ASTPrimitiveType) - isArray = true; - } - allocationExpression = node; - } - - public String getName() { - return name; - } - - public int getArgumentCount() { - return argumentCount; - } - - public ASTAllocationExpression getASTAllocationExpression() { - return allocationExpression; - } - - public boolean isArray() { - return isArray; - } - } - - private boolean isToplevelType(JavaNode node) { - return node.jjtGetParent().jjtGetParent() instanceof ASTCompilationUnit; - } - - /** - * Outer interface visitation - */ - @Override - public Object visit(ASTClassOrInterfaceDeclaration node, Object data) { - if (node.isInterface()) { - return visitInterface(node, data); - } - - if (!isToplevelType(node)) { - return handleInnerType(node, data); - } - return handleToplevelType(node, data); - } - - private Object visitInterface(ASTClassOrInterfaceDeclaration node, Object data) { - if (!isToplevelType(node)) { - return handleInnerType(node, data); - } - return handleToplevelType(node, data); - } - - @Override - public Object visit(ASTAnnotationTypeDeclaration node, Object data) { - if (!isToplevelType(node)) { - return handleInnerType(node, data); - } - return handleToplevelType(node, data); - } - - private Object handleToplevelType(AbstractJavaAccessTypeNode node, Object data) { - if (!node.isStatic()) { // See bug# 1807370 - String typeName = node.getImage(); - classDataList.clear(); - setClassID(0); // first class - classDataList.add(getClassID(), new ClassData(typeName)); - } - Object o = super.visit(node, data); - if (o != null && !node.isStatic()) { // See bug# 1807370 - processRule(o); - } else { - processRule(data); - } - setClassID(-1); - return o; - } - - private Object handleInnerType(AbstractJavaAccessTypeNode node, Object data) { - String typeName = node.getImage(); - int formerID = getClassID(); - setClassID(classDataList.size()); - ClassData newClassData = new ClassData(typeName); - // store the names of any outer classes of this class in the - // classQualifyingName List - ClassData formerClassData = classDataList.get(formerID); - newClassData.addClassQualifyingName(formerClassData.getClassName()); - classDataList.add(getClassID(), newClassData); - Object o = super.visit(node, data); - setClassID(formerID); - return o; - } - - /** - * Store all target constructors - */ - public Object visit(ASTConstructorDeclaration node, Object data) { - if (node.isPrivate()) { - getCurrentClassData().addConstructor(node); - } - return super.visit(node, data); - } - - public Object visit(ASTAllocationExpression node, Object data) { - // TODO - // this is a hack to bail out here - // but I'm not sure why this is happening - // TODO - if (classID == -1 || getCurrentClassData() == null) { - return data; - } - AllocData ad = new AllocData(node, packageName, getCurrentClassData().getClassQualifyingNamesList()); - if (!ad.isArray()) { - getCurrentClassData().addInstantiation(ad); - } - return super.visit(node, data); - } - - private void processRule(Object ctx) { - // check constructors of outerIterator against allocations of - // innerIterator - for (ClassData outerDataSet : classDataList) { - for (Iterator constructors = outerDataSet - .getPrivateConstructorIterator(); constructors.hasNext();) { - ASTConstructorDeclaration cd = constructors.next(); - - for (ClassData innerDataSet : classDataList) { - if (outerDataSet.equals(innerDataSet)) { - continue; - } - for (Iterator allocations = innerDataSet.getInstantiationIterator(); allocations - .hasNext();) { - AllocData ad = allocations.next(); - // if the constructor matches the instantiation - // flag the instantiation as a generator of an extra - // class - if (outerDataSet.getClassName().equals(ad.getName()) - && cd.getParameterCount() == ad.getArgumentCount()) { - addViolation(ctx, ad.getASTAllocationExpression()); - } - } - } } } + + return data; } - - private ClassData getCurrentClassData() { - // TODO - // this is a hack to bail out here - // but I'm not sure why this is happening - // TODO - if (classID >= classDataList.size()) { - return null; - } - return classDataList.get(classID); - } - - private void setClassID(int id) { - classID = id; - } - - private int getClassID() { - return classID; - } - - // remove = Fire. - // value = someFire.Fighter - // 0123456789012345 - // index = 4 - // remove.size() = 5 - // value.substring(0,4) = some - // value.substring(4 + remove.size()) = Fighter - // return "someFighter" - - // TODO move this into StringUtil - private static String stripString(String remove, String value) { - String returnValue; - int index = value.indexOf(remove); - if (index != -1) { - // if the package name can start anywhere but 0 - // please inform the author because this will break - returnValue = value.substring(0, index) + value.substring(index + remove.length()); - } else { - returnValue = value; - } - return returnValue; - } - } diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/design/xml/AccessorClassGeneration.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/design/xml/AccessorClassGeneration.xml index 38c02df0d5..cbe0fe1446 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/design/xml/AccessorClassGeneration.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/design/xml/AccessorClassGeneration.xml @@ -99,28 +99,7 @@ public class Foo1 { } } ]]> - - + #1452 ArrayIndexOutOfBoundsException with Annotations for AccessorClassGenerationRule 0 @@ -128,4 +107,28 @@ public class Foo1 { public @interface Something { public interface SomthingElse{}; } ]]> + + + 1 + +