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 0395bba3fc..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 @@ -1,318 +1,95 @@ -/** - * BSD-style license; for more info see http://pmd.sourceforge.net/license.html - */ -package net.sourceforge.pmd.lang.java.rule.design; - -import java.util.ArrayList; -import java.util.Iterator; -import java.util.List; -import java.util.ListIterator; - -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; - -/** - * 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 - * 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? - * - * @author CL Gilbert (dnoyeb@users.sourceforge.net) - * @author David Konecny (david.konecny@) - * @author Romain PELISSE, belaran@gmail.com, patch bug#1807370 - */ -public class AccessorClassGenerationRule extends AbstractJavaRule { - - private List classDataList = new ArrayList<>(); - private int classID = -1; - private String packageName; - - 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 - */ - private List classQualifyingNames; - - public 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; - } - } - - private static class AllocData { - private String name; - private int argumentCount; - private ASTAllocationExpression allocationExpression; - private boolean isArray; - - public 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()); - - // 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()); - 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()); - } - } - } - } - } - } - - 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; - } - -} +/** + * BSD-style license; for more info see http://pmd.sourceforge.net/license.html + */ + +package net.sourceforge.pmd.lang.java.rule.design; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +import net.sourceforge.pmd.RuleContext; +import net.sourceforge.pmd.lang.java.ast.ASTAllocationExpression; +import net.sourceforge.pmd.lang.java.ast.ASTArguments; +import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceType; +import net.sourceforge.pmd.lang.java.ast.ASTConstructorDeclaration; +import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule; +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 + * 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? + *

+ * + * @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 Map> privateConstructors = new HashMap<>(); + + public AccessorClassGenerationRule() { + super(); + /* + * Order is important. Visit constructors first to find the private + * ones, then visit allocations to find violations + */ + addRuleChainVisit(ASTConstructorDeclaration.class); + addRuleChainVisit(ASTAllocationExpression.class); + } + + @Override + public void end(final RuleContext ctx) { + super.end(ctx); + // Clean up all references to the AST + privateConstructors.clear(); + } + + @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; + } + + @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; + } + } + } + } + + return data; + } +} 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 + + diff --git a/src/site/markdown/overview/changelog.md b/src/site/markdown/overview/changelog.md index 78711a6d20..067ab6a92f 100644 --- a/src/site/markdown/overview/changelog.md +++ b/src/site/markdown/overview/changelog.md @@ -19,6 +19,7 @@ The PMD team is pleased to announce PMD 5.5.5. * java-design * [#274](https://github.com/pmd/pmd/issues/274): \[java] AccessorMethodGeneration: Method inside static inner class incorrectly reported * [#275](https://github.com/pmd/pmd/issues/275): \[java] FinalFieldCouldBeStatic: Constant in @interface incorrectly reported as "could be made static" + * [#291](https://github.com/pmd/pmd/issues/291): \[java] Improve quality of AccessorClassGeneration ### API Changes