Checkin of the Type Resolution Facility

Re-implemented LooseCoupling as a new rule - new or changed type resolution rules should go in typeresolution to be separated.

I have not added this to 39.xml!

Sorry about the lack of comments, will go through and try and get important things commented.

There's a couple TODO's that if addressed should help performance.

Remember, to run compiled classes must be in the CLASSPATH!


git-svn-id: https://pmd.svn.sourceforge.net/svnroot/pmd/trunk@4744 51baf565-9d33-0410-a72c-fc3788e3496d
This commit is contained in:
Allan Caplan
2006-10-26 02:35:38 +00:00
parent 15abbd44ae
commit 6215d97b9e
26 changed files with 912 additions and 5 deletions

View File

@ -5,12 +5,13 @@
<classpathentry kind="src" path="src"/>
<classpathentry kind="var" path="JRE_LIB"/>
<classpathentry kind="lib" path="lib/jakarta-oro-2.0.8.jar"/>
<classpathentry kind="lib" path="lib/jaxen-1.1-beta-7.jar"/>
<classpathentry kind="lib" path="lib/xercesImpl-2.6.2.jar"/>
<classpathentry kind="lib" path="lib/xmlParserAPIs-2.6.2.jar"/>
<classpathentry kind="var" path="ANT_HOME/ant.jar"/>
<classpathentry kind="var" path="JUNIT_HOME/junit.jar"/>
<classpathentry kind="var" path="ECLIPSE_HOME/plugins/org.eclipse.jdt_3.1.0.jar"/>
<classpathentry kind="var" path="JDK1.5_HOME/lib/tools.jar"/>
<classpathentry kind="lib" path="lib/jaxen-1.1-beta-10.jar"/>
<classpathentry kind="lib" path="lib/asm-3.0_RC1.jar"/>
<classpathentry kind="output" path="build"/>
</classpath>

View File

@ -17,6 +17,7 @@
<include name="jakarta-oro-2.0.8.jar" />
<include name="xercesImpl-2.6.2.jar" />
<include name="xmlParserAPIs-2.6.2.jar" />
<include name="asm-3.0_RC1.jar" />
</fileset>
</path>

View File

@ -1,2 +1,2 @@
@echo off
java -cp %~dp0\..\lib\pmd-3.8.jar;%~dp0\..\lib\jaxen-1.1-beta-10.jar;%~dp0\..\lib\jakarta-oro-2.0.8.jar;%CLASSPATH% net.sourceforge.pmd.PMD %*
java -cp %~dp0\..\lib\pmd-3.8.jar;%~dp0\..\lib\jaxen-1.1-beta-10.jar;%~dp0\..\lib\jakarta-oro-2.0.8.jar;%~dp0\..\lib\asm-3.0_RC1.jar;%CLASSPATH% net.sourceforge.pmd.PMD %*

View File

@ -3,6 +3,7 @@ New rules:
Basic ruleset: BigIntegerInstantiation(1.4 and 1.5)
Codesize ruleset: NPathComplexity, NcssTypeCount, NcssMethodCount, NcssConstructorCount
Design ruleset: UseCollectionIsEmpty
Typeresolution ruleset: Loose Coupling - This is a re-implementation using the new Type Resolution facility
Fixed bug 1570915 - AvoidRethrowingException no longer reports a false positive for certain nested exceptions.
Fixed bug 1571324 - UselessStringValueOf no longer reports a false positive for additive expressions.
Fixed bug 1573795 - PreserveStackTrace doesn't throw CastClassException on exception with 0 args
@ -31,6 +32,7 @@ undo/redo added to text areas in Designer.
Better 'create rule XML' panel in Designer.
use of entrySet to iterate over Maps.
1.6 added as a valid option for targetjdk.
PMD now allows rules to use Type Resolution. This was referenced in patch 1257259.
Performance Refactoring, XPath rules re-written as Java:
AssignmentInOperand
AvoidDollarSigns

BIN
pmd/lib/asm-3.0_RC1.jar Normal file

Binary file not shown.

View File

@ -0,0 +1,115 @@
/**
* BSD-style license; for more info see http://pmd.sourceforge.net/license.html
*/
package test.net.sourceforge.pmd.rules.typeresolution.rules;
import net.sourceforge.pmd.PMD;
import net.sourceforge.pmd.Rule;
import test.net.sourceforge.pmd.testframework.SimpleAggregatorTst;
import test.net.sourceforge.pmd.testframework.TestDescriptor;
public class LooseCouplingTest extends SimpleAggregatorTst {
private Rule rule;
public void setUp() {
rule = findRule("typeresolution", "LooseCoupling");
}
public void testAll() {
runTests(new TestDescriptor[]{
new TestDescriptor(TEST1, "returning a HashSet, bad", 1, rule),
new TestDescriptor(TEST2, "returning a Map, OK", 0, rule),
new TestDescriptor(TEST3, "no problemo", 0, rule),
new TestDescriptor(TEST4, "returning a set", 0, rule),
new TestDescriptor(TEST5, "field declared of type HashSet", 1, rule),
new TestDescriptor(TEST6, "field, return type both HashSet", 2, rule),
new TestDescriptor(TEST7, "two fields", 2, rule),
new TestDescriptor(TEST8, "method param is HashMap", 1, rule),
new TestDescriptor(TEST9, "Vector could be List", 1, rule),
new TestDescriptor(TEST10, "ArrayList could be List", 1, rule),
new TestDescriptor(TEST11, "java.util.HashMap should be Map", 1, rule),
});
}
private static final String TEST1 =
"import java.util.HashSet;" + PMD.EOL +
"public class Foo {" + PMD.EOL +
" HashSet foo() {" + PMD.EOL +
" return new HashSet();" + PMD.EOL +
" }" + PMD.EOL +
"}";
private static final String TEST2 =
"import java.util.Map;" + PMD.EOL +
"import java.util.HashMap;" + PMD.EOL +
"public class Foo {" + PMD.EOL +
" Map getFoo() {" + PMD.EOL +
" return new HashMap();" + PMD.EOL +
" }" + PMD.EOL +
"}";
private static final String TEST3 =
"public class Foo {" + PMD.EOL +
" void foo() {}" + PMD.EOL +
"}";
private static final String TEST4 =
"import java.util.*;" + PMD.EOL +
"public class Foo {" + PMD.EOL +
" Set fooSet = new HashSet(); // OK" + PMD.EOL +
" Set foo() {" + PMD.EOL +
" return fooSet;" + PMD.EOL +
" }" + PMD.EOL +
"}";
private static final String TEST5 =
"import java.util.HashSet;" + PMD.EOL +
"public class Foo {" + PMD.EOL +
" HashSet fooSet = new HashSet(); // NOT OK" + PMD.EOL +
"}";
private static final String TEST6 =
"import java.util.HashSet;" + PMD.EOL +
"public class Foo {" + PMD.EOL +
" HashSet fooSet = new HashSet(); // NOT OK" + PMD.EOL +
" HashSet foo() { // NOT OK" + PMD.EOL +
" return fooSet;" + PMD.EOL +
" }" + PMD.EOL +
"}";
private static final String TEST7 =
"import java.util.HashSet;" + PMD.EOL +
"import java.util.HashMap;" + PMD.EOL +
"public class Foo {" + PMD.EOL +
" HashSet fooSet = new HashSet();" + PMD.EOL +
" HashMap fooMap = new HashMap();" + PMD.EOL +
"}";
private static final String TEST8 =
"import java.util.HashMap;" + PMD.EOL +
"public class Foo {" + PMD.EOL +
" void foo(HashMap bar) {}" + PMD.EOL +
"}";
private static final String TEST9 =
"import java.util.Vector;" + PMD.EOL +
"import java.util.*;" + PMD.EOL +
"public class Foo {" + PMD.EOL +
" public void foo(Vector bar) {}" + PMD.EOL +
"}";
private static final String TEST10 =
"import java.util.ArrayList;" + PMD.EOL +
"import java.util.*;" + PMD.EOL +
"public class Foo {" + PMD.EOL +
" public void foo(ArrayList bar) {}" + PMD.EOL +
"}";
private static final String TEST11 =
"public class Foo {" + PMD.EOL +
" java.util.HashSet fooSet = new java.util.HashSet(); // NOT OK" + PMD.EOL +
"}";
}

View File

@ -0,0 +1,40 @@
<?xml version="1.0"?>
<ruleset name="Type Resolution Rules"
xmlns="http://pmd.sf.net/ruleset/1.0.0"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://pmd.sf.net/ruleset/1.0.0 http://pmd.sf.net/ruleset_xml_schema.xsd"
xsi:noNamespaceSchemaLocation="http://pmd.sf.net/ruleset_xml_schema.xsd">
<description>
These are rules which resolve java Class files for comparisson, as opposed to a String
</description>
<rule name="LooseCoupling"
message="Avoid using implementation types like ''{0}''; use the interface instead"
class="net.sourceforge.pmd.typeresolution.rules.LooseCoupling"
typeResolution="true"
externalInfoUrl="http://pmd.sourceforge.net/rules/typeresolution.html#LooseCoupling">
<description>
Avoid using implementation types (i.e., HashSet); use the interface (i.e, Set) instead
</description>
<priority>3</priority>
<example>
<![CDATA[
import java.util.ArrayList;
import java.util.HashSet;
public class Bar {
// Use List instead
private ArrayList list = new ArrayList();
// Use Set instead
public HashSet getFoo() {
return new HashSet();
}
}
]]>
</example>
</rule>
</ruleset>

View File

@ -28,6 +28,7 @@ public abstract class AbstractRule extends JavaParserVisitorAdapter implements R
protected String ruleSetName;
protected boolean include;
protected boolean usesDFA;
protected boolean usesTypeResolution;
protected int priority = LOWEST_PRIORITY;
protected String externalInfoUrl;
@ -352,6 +353,14 @@ public abstract class AbstractRule extends JavaParserVisitorAdapter implements R
return this.usesDFA;
}
public void setUsesTypeResolution() {
this.usesTypeResolution = true;
}
public boolean usesTypeResolution() {
return this.usesTypeResolution;
}
protected void visitAll(List acus, RuleContext ctx) {
for (Iterator i = acus.iterator(); i.hasNext();) {
ASTCompilationUnit node = (ASTCompilationUnit) i.next();

View File

@ -22,6 +22,7 @@ public abstract class CommonAbstractRule implements Rule {
protected String ruleSetName;
protected boolean include;
protected boolean usesDFA;
protected boolean usesTypeResolution;
protected int priority = LOWEST_PRIORITY;
protected String externalInfoUrl;
@ -171,6 +172,14 @@ public abstract class CommonAbstractRule implements Rule {
return this.usesDFA;
}
public void setUsesTypeResolution() {
this.usesTypeResolution= true;
}
public boolean usesTypeResolution() {
return this.usesTypeResolution;
}
/**
* Adds a violation to the report.

View File

@ -82,6 +82,10 @@ public class PMD {
sourceTypeHandler.getDataFlowFacade().start(rootNode);
}
if (ruleSets.usesTypeResolution(language)) {
sourceTypeHandler.getTypeResolutionFacade().start(rootNode);
}
List acus = new ArrayList();
acus.add(rootNode);

View File

@ -68,4 +68,8 @@ public interface Rule {
boolean usesDFA();
PropertyDescriptor propertyDescriptorFor(String name);
void setUsesTypeResolution();
boolean usesTypeResolution();
}

View File

@ -145,4 +145,14 @@ public class RuleSet {
this.description = description;
}
public boolean usesTypeResolution() {
for (Iterator i = rules.iterator(); i.hasNext();) {
Rule r = (Rule) i.next();
if (r.usesTypeResolution()) {
return true;
}
}
return false;
}
}

View File

@ -292,6 +292,11 @@ public class RuleSetFactory {
rule.setUsesDFA();
}
if (ruleElement.hasAttribute("typeResolution")
&& ruleElement.getAttribute("typeResolution").equals("true")) {
rule.setUsesTypeResolution();
}
for (int i = 0; i < ruleElement.getChildNodes().getLength(); i++) {
Node node = ruleElement.getChildNodes().item(i);
if (node.getNodeType() == Node.ELEMENT_NODE) {

View File

@ -136,4 +136,14 @@ public class RuleSets {
}
return rule;
}
public boolean usesTypeResolution(Language language) {
for (Iterator i = ruleSets.iterator(); i.hasNext();) {
RuleSet ruleSet = (RuleSet) i.next();
if (applies(language, ruleSet.getLanguage()) && ruleSet.usesTypeResolution()) {
return true;
}
}
return false;
}
}

View File

@ -65,6 +65,7 @@ public class SimpleRuleSetNameMapper {
nameMap.put("strictexception", "rulesets/strictexception.xml");
nameMap.put("strings", "rulesets/strings.xml");
nameMap.put("sunsecure", "rulesets/sunsecure.xml");
nameMap.put("typeresolution", "rulesets/typeresolution.xml");
nameMap.put("unusedcode", "rulesets/unusedcode.xml");
nameMap.put("33", "rulesets/releases/33.xml");
nameMap.put("34", "rulesets/releases/34.xml");

View File

@ -18,5 +18,14 @@ public class ASTClassOrInterfaceType extends SimpleJavaNode {
public Object jjtAccept(JavaParserVisitor visitor, Object data) {
return visitor.visit(this, data);
}
private Class type;
public void setType(Class type){
this.type = type;
}
public Class getType(){
return type;
}
}

View File

@ -26,6 +26,7 @@ public abstract class AbstractJspRule extends JspParserVisitorAdapter implements
protected String ruleSetName;
protected boolean include;
protected boolean usesDFA;
protected boolean usesTypeResolution;
protected int priority = LOWEST_PRIORITY;
protected String externalInfoUrl;
@ -179,6 +180,13 @@ public abstract class AbstractJspRule extends JspParserVisitorAdapter implements
return this.usesDFA;
}
public void setUsesTypeResolution() {
}
public boolean usesTypeResolution() {
return false;
}
protected void visitAll(List acus, RuleContext ctx) {
for (Iterator i = acus.iterator(); i.hasNext();) {
SimpleNode node = (SimpleNode) i.next();

View File

@ -3,6 +3,7 @@ package net.sourceforge.pmd.sourcetypehandlers;
import net.sourceforge.pmd.ast.ASTCompilationUnit;
import net.sourceforge.pmd.dfa.DataFlowFacade;
import net.sourceforge.pmd.symboltable.SymbolFacade;
import net.sourceforge.pmd.typeresolution.TypeResolutionFacade;
/**
* Implementation of VisitorsFactory for the Java AST. It uses anonymous classes
@ -13,7 +14,7 @@ import net.sourceforge.pmd.symboltable.SymbolFacade;
public abstract class JavaTypeHandler implements SourceTypeHandler {
private DataFlowFacade dataFlowFacade = new DataFlowFacade();
private SymbolFacade stb = new SymbolFacade();
private TypeResolutionFacade tr = new TypeResolutionFacade();
public VisitorStarter getDataFlowFacade() {
return new VisitorStarter() {
@ -30,4 +31,14 @@ public abstract class JavaTypeHandler implements SourceTypeHandler {
}
};
}
public VisitorStarter getTypeResolutionFacade() {
return new VisitorStarter() {
public void start(Object rootNode) {
tr.initializeWith((ASTCompilationUnit) rootNode);
}
};
}
}

View File

@ -37,4 +37,8 @@ public class JspTypeHandler implements SourceTypeHandler {
return new JspSymbolFacade();
}
public VisitorStarter getTypeResolutionFacade() {
return VisitorStarter.dummy;
}
}

View File

@ -30,4 +30,13 @@ public interface SourceTypeHandler {
* @return VisitorStarter
*/
VisitorStarter getSymbolFacade();
/**
* Get the getTypeResolutionFacade.
*
* @return VisitorStarter
*/
VisitorStarter getTypeResolutionFacade();
}

View File

@ -0,0 +1,119 @@
/**
* BSD-style license; for more info see http://pmd.sourceforge.net/license.html
*/
package net.sourceforge.pmd.typeresolution;
import net.sourceforge.pmd.ast.ASTClassOrInterfaceDeclaration;
import net.sourceforge.pmd.ast.ASTClassOrInterfaceType;
import net.sourceforge.pmd.ast.ASTCompilationUnit;
import net.sourceforge.pmd.ast.ASTImportDeclaration;
import net.sourceforge.pmd.ast.ASTName;
import net.sourceforge.pmd.ast.ASTPackageDeclaration;
import net.sourceforge.pmd.ast.JavaParserVisitorAdapter;
import java.util.Collections;
import java.util.HashMap;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
public class ClassTypeResolver extends JavaParserVisitorAdapter {
private static Map myPrimitiveTypes;
private static PMDASMClassLoader pmdClassLoader = new PMDASMClassLoader();
static {
Map thePrimitiveTypes = new HashMap();
thePrimitiveTypes.put("short", Short.TYPE);
thePrimitiveTypes.put("byte", Byte.TYPE);
thePrimitiveTypes.put("char", Character.TYPE);
thePrimitiveTypes.put("int", Integer.TYPE);
thePrimitiveTypes.put("long", Long.TYPE);
thePrimitiveTypes.put("float", Float.TYPE);
thePrimitiveTypes.put("double", Double.TYPE);
thePrimitiveTypes.put("boolean", Boolean.TYPE);
thePrimitiveTypes.put("void", Void.TYPE);
myPrimitiveTypes = Collections.unmodifiableMap(thePrimitiveTypes);
}
private Map importedClasses;
private String className;
public Object visit(ASTCompilationUnit node, Object data) {
try {
populateClassName(node);
} catch (ClassNotFoundException e) {
populateImports(node);
}
return super.visit(node, data);
}
/**
* If the outer class wasn't found then we'll get in here
*
* @param node
*/
private void populateImports(ASTCompilationUnit node) {
List theImportDeclarations = node.findChildrenOfType(ASTImportDeclaration.class);
importedClasses = new HashMap();
// go through the imports
for (Iterator anIterator = theImportDeclarations.iterator(); anIterator.hasNext();) {
ASTImportDeclaration anImportDeclaration = (ASTImportDeclaration) anIterator.next();
if (!anImportDeclaration.isImportOnDemand()) {
String strPackage = anImportDeclaration.getPackageName();
String strName = anImportDeclaration.getImportedName();
importedClasses.put(strName, strName);
importedClasses.put(strName.substring(strPackage.length() + 1), strName);
}
}
importedClasses.put("String", "java.lang.String");
importedClasses.put("Object", "java.lang.Object");
}
private void populateClassName(ASTCompilationUnit node) throws ClassNotFoundException {
ASTClassOrInterfaceDeclaration decl = (ASTClassOrInterfaceDeclaration) node
.getFirstChildOfType(ASTClassOrInterfaceDeclaration.class);
if (decl != null) {
ASTPackageDeclaration pkgDecl = (ASTPackageDeclaration) node
.getFirstChildOfType(ASTPackageDeclaration.class);
className = pkgDecl == null ? decl.getImage() : ((ASTName) pkgDecl.jjtGetChild(0)).getImage() + "."
+ decl.getImage();
pmdClassLoader.loadClass(className);
importedClasses = pmdClassLoader.getImportedClasses(className);
}
}
public Object visit(ASTClassOrInterfaceType node, Object data) {
String className = node.getImage();
String qualifiedName = className;
Class myType = (Class) myPrimitiveTypes.get(className);
if (myType == null && importedClasses != null) {
if (importedClasses.containsKey(className)) {
qualifiedName = (String) importedClasses.get(className);
} else if (importedClasses.containsValue(className)) {
qualifiedName = className;
}
if (qualifiedName != null) {
try {
/*
* TODO - the map right now contains just class names. if we use a map of
* classname/class then we don't have to hit the class loader for every type -
* much faster
*/
myType = pmdClassLoader.loadClass(qualifiedName);
} catch (ClassNotFoundException e) {
//@TODO What should we do if it's not found?
}
}
}
if (myType != null) {
node.setType(myType);
}
return data;
}
}

View File

@ -0,0 +1,76 @@
/**
* BSD-style license; for more info see http://pmd.sourceforge.net/license.html
*/
package net.sourceforge.pmd.typeresolution;
import net.sourceforge.pmd.typeresolution.visitors.PMDASMVisitor;
import org.objectweb.asm.ClassReader;
import org.objectweb.asm.ClassWriter;
import java.io.IOException;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
public class PMDASMClassLoader extends ClassLoader {
public PMDASMClassLoader() {
}
public synchronized Class loadClass(String name) throws ClassNotFoundException {
return defineClass(name);
}
private Map importedClasses = new HashMap();
private Set dontBother = new HashSet();
public Map getImportedClasses(String className) {
Map ret = (Map) importedClasses.get(className);
return ret == null ? new HashMap() : ret;
}
private Class defineClass(String name) throws ClassNotFoundException {
if (dontBother.contains(name)) {
throw new ClassNotFoundException(name);
}
try {
if (name.startsWith("java.")) {
return Class.forName(name);
}
if (importedClasses.containsKey(name)) {
if (super.findLoadedClass(name) != null) {
return super.findLoadedClass(name);
}
}
ClassReader reader = new ClassReader(getResourceAsStream(name.replace('.', '/') + ".class"));
PMDASMVisitor asmVisitor = new PMDASMVisitor();
reader.accept(asmVisitor, 0);
List inner = asmVisitor.getInnerClasses();
if (inner != null && !inner.isEmpty()) {
for (int ix = 0; ix < inner.size(); ix++) {
String str = (String) inner.get(ix);
ClassReader innerReader = new ClassReader(getResourceAsStream(str.replace('.', '/') + ".class"));
innerReader.accept(asmVisitor, 0);
}
}
importedClasses.put(name, asmVisitor.getPackages());
ClassWriter writer = new ClassWriter(ClassWriter.COMPUTE_MAXS);
reader.accept(writer, 0);
byte[] byteCode = writer.toByteArray();
return defineClass(name, byteCode, 0, byteCode.length);
} catch (ClassNotFoundException e) {
dontBother.add(name);
throw e;
} catch (IOException e) {
dontBother.add(name);
throw new ClassNotFoundException(name, e);
}
}
}

View File

@ -0,0 +1,19 @@
/**
* BSD-style license; for more info see http://pmd.sourceforge.net/license.html
*/
package net.sourceforge.pmd.typeresolution;
import net.sourceforge.pmd.ast.ASTCompilationUnit;
import net.sourceforge.pmd.ast.JavaParserVisitorAdapter;
/**
* @author Allan Caplan
*/
public class TypeResolutionFacade extends JavaParserVisitorAdapter {
public void initializeWith(ASTCompilationUnit node) {
ClassTypeResolver ctr = new ClassTypeResolver();
node.jjtAccept(ctr, null);
}
}

View File

@ -0,0 +1,33 @@
/**
* BSD-style license; for more info see http://pmd.sourceforge.net/license.html
*/
package net.sourceforge.pmd.typeresolution.rules;
import net.sourceforge.pmd.AbstractRule;
import net.sourceforge.pmd.ast.ASTClassOrInterfaceType;
import net.sourceforge.pmd.ast.ASTFieldDeclaration;
import net.sourceforge.pmd.ast.ASTFormalParameter;
import net.sourceforge.pmd.ast.ASTResultType;
import net.sourceforge.pmd.ast.Node;
import net.sourceforge.pmd.util.CollectionUtil;
/**
* This is a separate rule, uses the type resolution facade
*/
public class LooseCoupling extends AbstractRule {
public LooseCoupling() {
super();
}
public Object visit(ASTClassOrInterfaceType node, Object data) {
Node parent = node.jjtGetParent().jjtGetParent().jjtGetParent();
Class clazzType = node.getType();
boolean isType = CollectionUtil.isCollectionType(clazzType, false);
if (isType
&& (parent instanceof ASTFieldDeclaration || parent instanceof ASTFormalParameter || parent instanceof ASTResultType)) {
addViolation(data, node, node.getImage());
}
return data;
}
}

File diff suppressed because it is too large Load Diff

View File

@ -64,7 +64,23 @@ public class CollectionUtil {
return includeInterfaces && collectionInterfacesByNames.contains(typeName);
}
/**
* Return whether we can identify the typeName as a java.util collection class
* or interface as specified.
*
* @param clazzType Class
* @param includeInterfaces boolean
* @return boolean
*/
public static boolean isCollectionType(Class clazzType, boolean includeInterfaces) {
if (collectionClassesByNames.contains(clazzType)) {
return true;
}
return includeInterfaces && collectionInterfacesByNames.contains(clazzType);
}
/**
* Returns the items as a populated set.
*
@ -145,7 +161,7 @@ public class CollectionUtil {
}
return true;
}
/**
* A comprehensive isEqual method that handles nulls and arrays safely.
*