phoedos/pmd
1
1
Fork 1
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

[java] Make CloneMethodMustImplementCloneable over 500x faster

Browse Source 
- This was an excruciatingly slow rule, the slowest by far of all PMD
 - We speed it up by:
    * checking early if we are analyzing a `clone()` method to cut the AST tree
    * making better use of type information available
    * only performing additional checks when we know we can't rely on
       available type information
This commit is contained in:
Juan Martín Sotuyo Dodero
2016-11-28 03:19:40 -03:00
committed by Andreas Dangel
parent 795d437cd4
commit f4e727d274
1 changed files with 54 additions and 49 deletions
Download Patch File Download Diff File Expand all files Collapse all files

103
pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/rules/CloneMethodMustImplementCloneable.java
View File

@ -3,7 +3,6 @@
*/ */
package net.sourceforge.pmd.lang.java.typeresolution.rules; package net.sourceforge.pmd.lang.java.typeresolution.rules;
import java.util.Arrays;
import java.util.HashSet; import java.util.HashSet;
import java.util.List; import java.util.List;
import java.util.Set; import java.util.Set;
@ -33,66 +32,71 @@ import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule;
public class CloneMethodMustImplementCloneable extends AbstractJavaRule { public class CloneMethodMustImplementCloneable extends AbstractJavaRule {
@Override @Override
public Object visit(ASTClassOrInterfaceDeclaration node, Object data) { public Object visit(final ASTClassOrInterfaceDeclaration node, final Object data) {
if (extendsOrImplementsCloneable(node)) { if (extendsOrImplementsCloneable(node)) {
return data; return data;
} }
return super.visit(node, data); return super.visit(node, data);
} }
private boolean extendsOrImplementsCloneable(ASTClassOrInterfaceDeclaration node) { private boolean extendsOrImplementsCloneable(final ASTClassOrInterfaceDeclaration node) {
ASTImplementsList impl = node.getFirstChildOfType(ASTImplementsList.class); if (node.getType() != null) {
if (impl != null && impl.jjtGetParent().equals(node)) { return Cloneable.class.isAssignableFrom(node.getType());
}
// From this point on, this is a best effort, the auxclasspath is incomplete.
// TODO : Should we really care about this?
// Shouldn't the type resolver / symbol table report missing classes and the user
// know results are dependent on running under proper arguments?
final ASTImplementsList impl = node.getFirstChildOfType(ASTImplementsList.class);
if (impl != null) {
for (int ix = 0; ix < impl.jjtGetNumChildren(); ix++) { for (int ix = 0; ix < impl.jjtGetNumChildren(); ix++) {
Node child = impl.jjtGetChild(ix); final Node child = impl.jjtGetChild(ix);
if (child.getClass() != ASTClassOrInterfaceType.class) { if (child.getClass() != ASTClassOrInterfaceType.class) {
continue; continue;
} }
ASTClassOrInterfaceType type = (ASTClassOrInterfaceType) child; final ASTClassOrInterfaceType type = (ASTClassOrInterfaceType) child;
if (type.getType() == null) { if (type.getType() == null) {
if ("Cloneable".equals(type.getImage())) { if ("Cloneable".equals(type.getImage())) {
return true; return true;
} }
} else if (type.getType().equals(Cloneable.class)) {
return true;
} else if (Cloneable.class.isAssignableFrom(type.getType())) { } else if (Cloneable.class.isAssignableFrom(type.getType())) {
return true; return true;
} else {
List<Class<?>> implementors = Arrays.asList(type.getType().getInterfaces());
if (implementors.contains(Cloneable.class)) {
return true;
}
} }
} }
} }
if (node.jjtGetNumChildren() != 0 && node.jjtGetChild(0) instanceof ASTExtendsList) { if (node.jjtGetNumChildren() != 0 && node.jjtGetChild(0) instanceof ASTExtendsList) {
ASTClassOrInterfaceType type = (ASTClassOrInterfaceType) node.jjtGetChild(0).jjtGetChild(0); final ASTClassOrInterfaceType type = (ASTClassOrInterfaceType) node.jjtGetChild(0).jjtGetChild(0);
Class<?> clazz = type.getType(); final Class<?> clazz = type.getType();
if (clazz != null && clazz.equals(Cloneable.class)) { if (clazz != null) {
return true; return Cloneable.class.isAssignableFrom(clazz);
}
while (clazz != null && !Object.class.equals(clazz)) {
if (Arrays.asList(clazz.getInterfaces()).contains(Cloneable.class)) {
return true;
}
clazz = clazz.getSuperclass();
} }
} }
return false; return false;
} }
@Override @Override
public Object visit(ASTMethodDeclaration node, Object data) { public Object visit(final ASTMethodDeclaration node, final Object data) {
ASTClassOrInterfaceDeclaration classOrInterface = node.getFirstParentOfType(ASTClassOrInterfaceDeclaration.class); // Is this a clone method?
final ASTMethodDeclarator methodDeclarator = node.getFirstChildOfType(ASTMethodDeclarator.class);
if (!isCloneMethod(methodDeclarator)) {
return data;
}
// Is the clone method just throwing CloneNotSupportedException?
final ASTClassOrInterfaceDeclaration classOrInterface = node.getFirstParentOfType(ASTClassOrInterfaceDeclaration.class);
if (classOrInterface != null && //Don't analyze enums, which cannot subclass clone() if (classOrInterface != null && //Don't analyze enums, which cannot subclass clone()
(node.isFinal() || classOrInterface.isFinal())) { (node.isFinal() || classOrInterface.isFinal())) {
if (node.findDescendantsOfType(ASTBlock.class).size() == 1) { if (node.findDescendantsOfType(ASTBlock.class).size() == 1) {
List<ASTBlockStatement> blocks = node.findDescendantsOfType(ASTBlockStatement.class); final List<ASTBlockStatement> blocks = node.findDescendantsOfType(ASTBlockStatement.class);
if (blocks.size() == 1) { if (blocks.size() == 1) {
ASTBlockStatement block = blocks.get(0); final ASTBlockStatement block = blocks.get(0);
ASTClassOrInterfaceType type = block.getFirstDescendantOfType(ASTClassOrInterfaceType.class); final ASTClassOrInterfaceType type = block.getFirstDescendantOfType(ASTClassOrInterfaceType.class);
if (type != null && type.getType() != null && type.getNthParent(9).equals(node) if (type != null && type.getType() != null && type.getNthParent(9).equals(node)
&& type.getType().equals(CloneNotSupportedException.class)) { && type.getType().equals(CloneNotSupportedException.class)) {
return data; return data;
@ -104,30 +108,33 @@ public class CloneMethodMustImplementCloneable extends AbstractJavaRule {
} }
} }
// Now check other whether implemented or extended classes are defined inside the same file // TODO : Should we really care about this? It can only happen with an incomplete auxclasspath
if (classOrInterface != null) { if (classOrInterface != null && classOrInterface.getType() == null) {
Set<String> classesNames = determineTopLevelCloneableClasses(classOrInterface); // Now check other whether implemented or extended classes are defined inside the same file
final Set<String> classesNames = determineTopLevelCloneableClasses(classOrInterface);
ASTImplementsList implementsList = classOrInterface.getFirstChildOfType(ASTImplementsList.class); final ASTImplementsList implementsList = classOrInterface.getFirstChildOfType(ASTImplementsList.class);
if (implementsList != null) { if (implementsList != null) {
List<ASTClassOrInterfaceType> types = implementsList.findChildrenOfType(ASTClassOrInterfaceType.class); final List<ASTClassOrInterfaceType> types = implementsList.findChildrenOfType(ASTClassOrInterfaceType.class);
for (ASTClassOrInterfaceType t : types) { for (final ASTClassOrInterfaceType t : types) {
if (classesNames.contains(t.getImage())) { if (classesNames.contains(t.getImage())) {
return data; return data;
} }
} }
} }
ASTExtendsList extendsList = classOrInterface.getFirstChildOfType(ASTExtendsList.class); final ASTExtendsList extendsList = classOrInterface.getFirstChildOfType(ASTExtendsList.class);
if (extendsList != null) { if (extendsList != null) {
ASTClassOrInterfaceType type = extendsList.getFirstChildOfType(ASTClassOrInterfaceType.class); final ASTClassOrInterfaceType type = extendsList.getFirstChildOfType(ASTClassOrInterfaceType.class);
if (classesNames.contains(type.getImage())) { if (classesNames.contains(type.getImage())) {
return data; return data;
} }
} }
} }
return super.visit(node, data); // Nothing can save us now
addViolation(data, node);
return data;
} }
/** /**
@ -136,11 +143,11 @@ public class CloneMethodMustImplementCloneable extends AbstractJavaRule {
* @param currentClass the node of the class, that is currently analyzed (inside this compilation unit) * @param currentClass the node of the class, that is currently analyzed (inside this compilation unit)
* @return a Set of class/interface names * @return a Set of class/interface names
*/ */
private Set<String> determineTopLevelCloneableClasses(ASTClassOrInterfaceDeclaration currentClass) { private Set<String> determineTopLevelCloneableClasses(final ASTClassOrInterfaceDeclaration currentClass) {
List<ASTClassOrInterfaceDeclaration> classes = currentClass.getFirstParentOfType(ASTCompilationUnit.class) final List<ASTClassOrInterfaceDeclaration> classes = currentClass.getFirstParentOfType(ASTCompilationUnit.class)
.findDescendantsOfType(ASTClassOrInterfaceDeclaration.class); .findDescendantsOfType(ASTClassOrInterfaceDeclaration.class);
Set<String> classesNames = new HashSet<String>(); final Set<String> classesNames = new HashSet<String>();
for (ASTClassOrInterfaceDeclaration c : classes) { for (final ASTClassOrInterfaceDeclaration c : classes) {
if (c != currentClass && extendsOrImplementsCloneable(c)) { if (c != currentClass && extendsOrImplementsCloneable(c)) {
classesNames.add(c.getImage()); classesNames.add(c.getImage());
} }
@ -148,16 +155,14 @@ public class CloneMethodMustImplementCloneable extends AbstractJavaRule {
return classesNames; return classesNames;
} }
@Override public boolean isCloneMethod(final ASTMethodDeclarator node) {
public Object visit(ASTMethodDeclarator node, Object data) {
if (!"clone".equals(node.getImage())) { if (!"clone".equals(node.getImage())) {
return data; return false;
} }
int countParams = ((ASTFormalParameters) node.jjtGetChild(0)).jjtGetNumChildren(); final int countParams = ((ASTFormalParameters) node.jjtGetChild(0)).jjtGetNumChildren();
if (countParams != 0) { if (countParams != 0) {
return data; return false;
} }
addViolation(data, node); return true;
return data;
} }
} }