diff --git a/pmd/etc/changelog.txt b/pmd/etc/changelog.txt index a27d4ca047..aac18748dd 100644 --- a/pmd/etc/changelog.txt +++ b/pmd/etc/changelog.txt @@ -47,6 +47,7 @@ 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. Renderers use less memory when generating reports. +New DynamicXPathRule class to speed up XPath based rules by providing a base type for the XPath expression. Performance Refactoring, XPath rules re-written as Java: AssignmentInOperand AvoidDollarSigns diff --git a/pmd/rulesets/basic.xml b/pmd/rulesets/basic.xml index 18c9e155de..77e621cb3e 100644 --- a/pmd/rulesets/basic.xml +++ b/pmd/rulesets/basic.xml @@ -12,7 +12,8 @@ The Basic Ruleset contains a collection of good practices which everyone should Empty Catch Block finds instances where an exception is caught, @@ -23,7 +24,7 @@ which should either be acted on or reported. Empty If Statement finds instances where a condition is checked but nothing is done about it. @@ -61,7 +63,7 @@ Empty If Statement finds instances where a condition is checked but nothing is d @@ -83,7 +85,8 @@ public class Foo { Empty While Statement finds all instances where a while statement @@ -94,7 +97,7 @@ it's a while loop that does a lot in the exit expression, rewrite it to make it @@ -117,7 +120,8 @@ public class Foo { Avoid empty try blocks - what's the point? @@ -126,7 +130,7 @@ Avoid empty try blocks - what's the point? @@ -149,7 +153,8 @@ public class Foo { Avoid empty finally blocks - these can be deleted. @@ -158,7 +163,7 @@ Avoid empty finally blocks - these can be deleted. @@ -183,7 +188,8 @@ public class Foo { Avoid empty switch statements. @@ -192,7 +198,7 @@ Avoid empty switch statements. @@ -216,7 +222,8 @@ public class Foo { Avoid jumbled loop incrementers - it's usually a mistake, and it's confusing even if it's what's intended. @@ -225,7 +232,7 @@ Avoid jumbled loop incrementers - it's usually a mistake, and it's confusing eve Some for loops can be simplified to while loops - this makes them more concise. @@ -262,7 +270,7 @@ Some for loops can be simplified to while loops - this makes them more concise. 1] [not(ForInit)] [not(ForUpdate)] @@ -374,7 +382,8 @@ public class Foo { Avoid returning from a finally block - this can discard exceptions. @@ -383,7 +392,7 @@ Avoid returning from a finally block - this can discard exceptions. @@ -409,7 +418,8 @@ public class Bar { Avoid empty synchronized blocks - they're useless. @@ -418,7 +428,7 @@ public class Bar { @@ -462,7 +472,8 @@ public class Foo { An empty static initializer was found. @@ -471,7 +482,7 @@ An empty static initializer was found. @@ -490,7 +501,8 @@ public class Foo { Do not use "if" statements that are always true or always false. @@ -499,7 +511,7 @@ Do not use "if" statements that are always true or always false. @@ -522,7 +534,8 @@ public class Foo { An empty statement (aka a semicolon by itself) that is not used @@ -534,7 +547,7 @@ removed. When a class has the final modifier, all the methods are automatically final. @@ -592,7 +606,7 @@ When a class has the final modifier, all the methods are automatically final. @@ -615,7 +629,8 @@ public final class Foo { Sometimes two 'if' statements can be consolidated by separating their conditions with a boolean short-circuit operator. @@ -624,10 +639,10 @@ Sometimes two 'if' statements can be consolidated by separating their conditions @@ -671,7 +686,8 @@ public String foo() { if you need to get an array of a class from your Collection, @@ -683,7 +699,7 @@ ClassCastException. One might assume that "new BigDecimal(.1)" is exactly equal @@ -742,7 +759,7 @@ if executed) The null check here is misplaced. if the variable is null you'll get a NullPointerException. @@ -812,7 +830,7 @@ class Test { After checking an object reference for null, you should invoke equals() on that object rather than passing it to another object's equals() method. @@ -849,7 +868,7 @@ public class Foo { The null check is broken since it will throw a Nullpointer itself. @@ -954,7 +974,7 @@ It is likely that you used || instead of && or vice versa. 1]//PrimaryPrefix/Name/@Image, diff --git a/pmd/rulesets/braces.xml b/pmd/rulesets/braces.xml index 113593941c..5d90aebc96 100644 --- a/pmd/rulesets/braces.xml +++ b/pmd/rulesets/braces.xml @@ -11,7 +11,8 @@ The Braces Ruleset contains a collection of braces rules. Avoid using if statements without using curly braces. @@ -20,7 +21,7 @@ Avoid using if statements without using curly braces. @@ -41,7 +42,8 @@ Avoid using if statements without using curly braces. Avoid using 'while' statements without using curly braces. @@ -50,7 +52,7 @@ Avoid using 'while' statements without using curly braces. @@ -69,7 +71,8 @@ public void doSomething() { Avoid using if..else statements without using curly braces. @@ -78,7 +81,7 @@ Avoid using if..else statements without using curly braces. Avoid using 'for' statements without using curly braces. @@ -114,7 +118,7 @@ Avoid using 'for' statements without using curly braces. diff --git a/pmd/rulesets/clone.xml b/pmd/rulesets/clone.xml index 26c348727d..79c2d7f41d 100644 --- a/pmd/rulesets/clone.xml +++ b/pmd/rulesets/clone.xml @@ -11,7 +11,8 @@ The Clone Implementation ruleset contains a collection of rules that find questi Object clone() should be implemented with super.clone(). @@ -20,7 +21,7 @@ Object clone() should be implemented with super.clone(). The method clone() should throw a CloneNotSupportedException. @@ -57,7 +59,7 @@ The method clone() should throw a CloneNotSupportedException. The method clone() should only be implemented if the class implements the Cloneable interface with the exception of a final method that only throws CloneNotSupportedException. @@ -95,7 +98,7 @@ The method clone() should only be implemented if the class implements the Clonea This rule detects when a constructor is not necessary; i.e., when there's only one constructor, @@ -25,8 +26,7 @@ it's public, has an empty body, and takes no arguments. Each class should declare at least one constructor. @@ -163,7 +164,7 @@ Each class should declare at least one constructor. @@ -227,7 +228,8 @@ public class Foo { It is a good practice to call super() in a constructor. If super() is not called but @@ -237,7 +239,7 @@ It is a good practice to call super() in a constructor. If super() is not called 0 ] +.[ count (ExtendsList/*) > 0 ] /ClassOrInterfaceBody /ClassOrInterfaceBodyDeclaration /ConstructorDeclaration[ count (.//ExplicitConstructorInvocation)=0 ] @@ -265,7 +267,8 @@ public class Foo extends Bar{ Sometimes expressions are wrapped in unnecessary parentheses, @@ -275,7 +278,7 @@ making them look like a function call. Use explicit scoping instead of the default package private level. @@ -327,7 +331,7 @@ Use explicit scoping instead of the default package private level. Use bitwise inversion to invert boolean values - it's the fastest way to do this. @@ -350,7 +355,7 @@ See http://www.javaspecialists.co.za/archive/newsletter.do?issue=042&locale= diff --git a/pmd/rulesets/design.xml b/pmd/rulesets/design.xml index 0f728e44bf..c65c3cc18f 100644 --- a/pmd/rulesets/design.xml +++ b/pmd/rulesets/design.xml @@ -61,7 +61,8 @@ public class Foo { Avoid unnecessary comparisons in boolean expressions - this complicates simple code. @@ -70,7 +71,7 @@ Avoid unnecessary comparisons in boolean expressions - this complicates simple c @@ -92,7 +93,8 @@ public class Bar { Switch statements should have a default label. @@ -101,7 +103,7 @@ Switch statements should have a default label. @@ -268,7 +270,8 @@ public class Outer { If a final field is assigned to a compile-time constant, it could be @@ -278,7 +281,7 @@ made static, thus saving overhead in each object at runtime. A nonstatic initializer block will be called any time a constructor @@ -341,7 +345,7 @@ is a valid language construct, it is rarely used and is confusing. @@ -361,7 +365,8 @@ public class MyClass { By convention, the default label should be the last label in a switch statement. @@ -370,7 +375,7 @@ By convention, the default label should be the last label in a switch statement. @@ -397,7 +402,8 @@ public class Foo { A non-case label (e.g. a named break/continue label) was present in a switch statement. @@ -407,7 +413,7 @@ This legal, but confusing. It is easy to mix up the case labels and the non-case @@ -434,7 +440,8 @@ public class Foo { A call to Collection.toArray can use the Collection's size vs an empty Array of the desired type. @@ -443,7 +450,7 @@ A call to Collection.toArray can use the Collection's size vs an empty Array of Avoid equality comparisons with Double.NaN - these are likely to be logic errors. @@ -483,7 +491,7 @@ Avoid equality comparisons with Double.NaN - these are likely to be logic errors @@ -502,7 +510,8 @@ public class Bar { Inexperienced programmers sometimes confuse comparison concepts @@ -512,7 +521,7 @@ and use equals() to compare to null. Avoid instantiating an object just to call getClass() on it; use the .class public member instead. @@ -589,7 +599,7 @@ public class Foo { Be sure to specify a Locale when creating a new instance of SimpleDateFormat. @@ -634,7 +645,7 @@ Be sure to specify a Locale when creating a new instance of SimpleDateFormat. @@ -679,7 +690,8 @@ public class Foo { When doing a String.toLowerCase()/toUpperCase() call, use a Locale. This avoids @@ -707,7 +719,7 @@ class Foo { Do not use protected fields in final classes since they cannot be subclassed. @@ -731,7 +744,7 @@ Clarify your intent by using private or package access modifiers instead. @@ -772,7 +785,8 @@ public class StaticField { A class that has private constructors and does not have any static methods or fields cannot be used. @@ -781,7 +795,7 @@ A class that has private constructors and does not have any static methods or fi 0 and count(./ClassOrInterfaceBody/ClassOrInterfaceBodyDeclaration/ConstructorDeclaration) = count(./ClassOrInterfaceBody/ClassOrInterfaceBodyDeclaration/ConstructorDeclaration[@Private='true']) ) and @@ -812,7 +826,8 @@ public class Foo { Method level synchronization can backfire when new code is added to the method. Block-level @@ -822,7 +837,7 @@ public class Foo { @@ -846,7 +861,8 @@ public class Foo { A switch statement without an enclosed break statement may be a bug. @@ -855,7 +871,7 @@ A switch statement without an enclosed break statement may be a bug. 0] [count(BlockStatement/Statement/ReturnStatement) @@ -888,7 +904,8 @@ public class Foo { Thread.notify() awakens a thread monitoring the object. If more than one thread is monitoring, then only @@ -898,7 +915,7 @@ one is chosen. The thread chosen is arbitrary; thus it's usually safer to call Each caught exception type should be handled in its own catch clause. @@ -937,7 +955,7 @@ Each caught exception type should be handled in its own catch clause. The abstract class does not contain any abstract methods. An abstract class suggests @@ -979,7 +998,7 @@ direcly) a protected constructor can be provided prevent direct instantiation. No need to check for null before an instanceof; the instanceof keyword returns false when given a null argument. @@ -1011,7 +1031,7 @@ No need to check for null before an instanceof; the instanceof keyword returns f Position literals first in String comparisons - that way if the String is null you won't get a NullPointerException, it'll just return false. @@ -1076,7 +1097,7 @@ class Foo { @@ -1154,7 +1175,8 @@ public static Foo getFoo() { Uncommented Empty Method finds instances where a method does not contain @@ -1166,7 +1188,7 @@ empty methods. @@ -1182,7 +1204,8 @@ public void doSomething() { Uncommented Empty Constructor finds instances where a constructor does not @@ -1194,7 +1217,7 @@ and unintentional empty constructors. @@ -1214,7 +1237,8 @@ public Foo() { + class="net.sourceforge.pmd.rules.DynamicXPathRule" + type="ClassOrInterfaceDeclaration"> An interface should be used only to model a behaviour of a class: using an interface as a container of constants is a poor usage pattern. @@ -1223,7 +1247,7 @@ public Foo() { If the finalize() method is empty, then it does not need to exist. @@ -21,7 +22,7 @@ If the finalize() method is empty, then it does not need to exist. @@ -39,7 +40,8 @@ public class Foo { If the finalize() is implemented, it should do something besides just calling @@ -49,7 +51,7 @@ super.finalize(). Methods named finalize() should not have parameters. It is @@ -84,7 +87,7 @@ not be called by the VM. 0]] ]]> @@ -104,7 +107,8 @@ public class Foo { If the finalize() is implemented, its last action should be to call super.finalize. @@ -115,7 +119,7 @@ If the finalize() is implemented, its last action should be to call super.finali If you override finalize(), make it protected. If you make @@ -151,7 +156,7 @@ If you override finalize(), make it protected. If you make diff --git a/pmd/rulesets/j2ee.xml b/pmd/rulesets/j2ee.xml index a5c6d1f954..b6c4148095 100644 --- a/pmd/rulesets/j2ee.xml +++ b/pmd/rulesets/j2ee.xml @@ -8,7 +8,8 @@ + class="net.sourceforge.pmd.rules.DynamicXPathRule" + type="PrimarySuffix"> In J2EE getClassLoader() might not work as expected. Use Thread.currentThread().getContextClassLoader() instead. @@ -16,7 +17,7 @@ @@ -31,4 +32,4 @@ public class Foo { - \ No newline at end of file + diff --git a/pmd/rulesets/javabeans.xml b/pmd/rulesets/javabeans.xml index 3b6de11e7a..1890100ff3 100644 --- a/pmd/rulesets/javabeans.xml +++ b/pmd/rulesets/javabeans.xml @@ -49,7 +49,8 @@ provide getFoo and setFoo methods. Classes that are serializable should provide a serialVersionUID field. @@ -58,7 +59,7 @@ Classes that are serializable should provide a serialVersionUID field. - \ No newline at end of file + diff --git a/pmd/rulesets/junit.xml b/pmd/rulesets/junit.xml index b184182331..736e5577e5 100644 --- a/pmd/rulesets/junit.xml +++ b/pmd/rulesets/junit.xml @@ -11,7 +11,8 @@ These rules deal with different problems that can occur with JUnit tests. The suite() method in a JUnit test needs to be both public and static. @@ -20,7 +21,7 @@ The suite() method in a JUnit test needs to be both public and static. @@ -42,7 +43,8 @@ public class Foo extends TestCase { Some JUnit framework methods are easy to misspell. @@ -51,7 +53,7 @@ Some JUnit framework methods are easy to misspell. A JUnit test assertion with a boolean literal is unnecessary since it always will eval to the same thing. @@ -155,7 +158,7 @@ statements like assertTrue(true) and assertFalse(false). If you just want a tes This rule detects JUnit assertions in object equality. These assertions @@ -200,7 +204,7 @@ should be made by more specific methods, like assertEquals. This rule detects JUnit assertions in object references equality. These assertions @@ -236,7 +241,7 @@ should be made by more specific methods, like assertSame, assertNotSame. This rule detects JUnit assertions in object references equality. These assertions @@ -273,7 +279,7 @@ public class FooTest extends TestCase { Avoid negation in an assertTrue or assertFalse test. @@ -315,7 +322,7 @@ assertFalse(expr); To make sure the full stacktrace is printed out, use the logging statement with 2 arguments: a String and a Throwable. @@ -19,7 +20,7 @@ To make sure the full stacktrace is printed out, use the logging statement with Logger should normally be defined private static final and have the correct class. @@ -57,7 +59,7 @@ public class Main { In most cases, the Logger can be declared static and final. @@ -41,7 +42,7 @@ In most cases, the Logger can be declared static and final. System.(out|err).print is used, consider using a logger. @@ -110,7 +112,7 @@ System.(out|err).print is used, consider using a logger. Avoid printStackTrace(); use a logger call instead. @@ -145,7 +148,7 @@ Avoid printStackTrace(); use a logger call instead. diff --git a/pmd/rulesets/migrating.xml b/pmd/rulesets/migrating.xml index 1b1efdb8f3..11acccf2a9 100644 --- a/pmd/rulesets/migrating.xml +++ b/pmd/rulesets/migrating.xml @@ -13,7 +13,8 @@ Contains rules about migrating from one JDK version to another. Don't use these Consider replacing Vector usages with the newer java.util.ArrayList if expensive threadsafe operation is not required. @@ -22,7 +23,7 @@ Contains rules about migrating from one JDK version to another. Don't use these @@ -41,7 +42,8 @@ public class Foo { Consider replacing this Hashtable with the newer java.util.Map @@ -50,7 +52,7 @@ public class Foo { @@ -69,7 +71,8 @@ public class Foo { Consider replacing this Enumeration with the newer java.util.Iterator @@ -78,7 +81,7 @@ public class Foo { @@ -101,14 +104,15 @@ public class Foo implements Enumeration { Finds all places 'enum' is used as an identifier is used. @@ -127,14 +131,15 @@ public class Foo implements Enumeration { Finds all places 'assert' is used as an identifier is used. @@ -153,13 +158,14 @@ public class Foo implements Enumeration { + class="net.sourceforge.pmd.rules.DynamicXPathRule" + type="PrimaryPrefix"> In JDK 1.5, calling new Integer() causes memory allocation. Integer.valueOf() is more memory friendly. Detects when a field, local, or parameter has a very short name. @@ -21,7 +22,7 @@ Detects when a field, local, or parameter has a very short name. @@ -46,7 +47,8 @@ public class Something { Detects when a field, formal or local variable is declared with a long name. @@ -57,7 +59,7 @@ Detects when a field, formal or local variable is declared with a long name. $minimum] +.[string-length(@Image) > $minimum] ]]> @@ -79,7 +81,8 @@ public class Something { Detects when very short method names are used. @@ -89,7 +92,7 @@ Detects when very short method names are used. @@ -167,7 +170,8 @@ public class Foo {} Abstract classes should be named 'AbstractXXX'. @@ -177,7 +181,7 @@ Abstract classes should be named 'AbstractXXX'. @@ -250,7 +254,8 @@ public class Foo { A field name is all in uppercase characters, which in Sun's Java naming @@ -260,7 +265,7 @@ conventions indicate a constant. However, the field is not final. The method name and parameter number are suspiciously close to @@ -295,7 +301,7 @@ method. Detects when a class or interface does not have a package definition. @@ -400,7 +407,7 @@ Detects when a class or interface does not have a package definition. @@ -417,7 +424,8 @@ public class ClassInDefaultPackage { Detects when a package definition contains upper case characters. @@ -426,7 +434,7 @@ public class ClassInDefaultPackage { @@ -443,7 +451,8 @@ public class SomeClass { Detects when a non-field has a name starting with 'm_'. This usually @@ -453,7 +462,7 @@ indicates a field and thus is confusing. diff --git a/pmd/rulesets/optimizations.xml b/pmd/rulesets/optimizations.xml index c1ffa9225b..6a01cb345c 100644 --- a/pmd/rulesets/optimizations.xml +++ b/pmd/rulesets/optimizations.xml @@ -70,7 +70,8 @@ public class Something { ArrayList is a much better Collection implementation than Vector. @@ -79,7 +80,7 @@ ArrayList is a much better Collection implementation than Vector. @@ -101,7 +102,8 @@ public class SimpleTest extends TestCase { Since it passes in a literal of length 1, this call to String.startsWith can be rewritten using String.charAt(0) to save some time. @@ -110,7 +112,7 @@ Since it passes in a literal of length 1, this call to String.startsWith can be The class java.util.Arrays has a "asList" method that @@ -176,7 +179,7 @@ public class Foo { Instead of copying data between two arrays, use @@ -238,7 +242,7 @@ public class Foo { Integers literals should not start with zero. @@ -24,7 +25,7 @@ interprated as an octal. Code should never throw NPE under normal circumstances. A catch block may hide the original error, causing other more subtle errors in its wake. @@ -100,7 +101,7 @@ public class Foo { @@ -110,7 +111,8 @@ public class Foo { Avoid throwing certain exception types. Rather than throw a raw RuntimeException, Throwable, @@ -130,7 +132,7 @@ public void bar() throws Exception { Avoid throwing a NullPointerException - it's confusing because most people will assume that the @@ -165,7 +168,7 @@ public class Foo { @@ -174,7 +177,8 @@ public class Foo { + class="net.sourceforge.pmd.rules.DynamicXPathRule" + type="CatchStatement"> Catch blocks that merely rethrow a caught exception only add to code size and runtime complexity. @@ -195,7 +199,7 @@ public class Foo { StringBuffer sb = new StringBuffer('c'); The @@ -302,7 +303,7 @@ StringBuffer size. ", "()V", null, null); + mv.visitCode(); + mv.visitVarInsn(ALOAD, 0); + mv.visitMethodInsn(INVOKESPECIAL, "net/sourceforge/pmd/rules/DynamicXPathRule", "", "()V"); + mv.visitInsn(RETURN); + mv.visitMaxs(1, 1); + mv.visitEnd(); + + mv = cw.visitMethod(ACC_PUBLIC, "visit", methodSig, null, null); + mv.visitCode(); + mv.visitVarInsn(ALOAD, 0); + mv.visitVarInsn(ALOAD, 1); + mv.visitVarInsn(ALOAD, 2); + mv.visitMethodInsn(INVOKEVIRTUAL, className, "evaluate", "(Lnet/sourceforge/pmd/ast/Node;Ljava/lang/Object;)V"); + mv.visitVarInsn(ALOAD, 0); + mv.visitVarInsn(ALOAD, 1); + mv.visitVarInsn(ALOAD, 2); + mv.visitMethodInsn(INVOKESPECIAL, "net/sourceforge/pmd/rules/DynamicXPathRule", "visit", methodSig); + mv.visitInsn(ARETURN); + mv.visitMaxs(3, 3); + mv.visitEnd(); + + cw.visitEnd(); + + return cw.toByteArray(); + } + + + private XPath xpath; + + private boolean regexpFunctionRegistered; + + /** + * Evaluate the AST with compilationUnit as root-node, against + * the XPath expression found as property with name "xpath". + * All matches are reported as violations. + * + * @param compilationUnit the Node that is the root of the AST to be checked + * @param data + */ + public void evaluate(Node compilationUnit, Object data) { + try { + initializeXPathExpression(); + List results = xpath.selectNodes(compilationUnit); + for (Iterator i = results.iterator(); i.hasNext();) { + SimpleNode n = (SimpleNode) i.next(); + if (n instanceof ASTVariableDeclaratorId && getBooleanProperty("pluginname")) { + addViolation(data, n, n.getImage()); + } else { + addViolation(data, (SimpleNode) n, getMessage()); + } + } + } catch (JaxenException ex) { + throw new RuntimeException(ex); + } + } + + private void initializeXPathExpression() throws JaxenException { + if (xpath != null) { + return; + } + + if (!regexpFunctionRegistered) { + MatchesFunction.registerSelfInSimpleContext(); + regexpFunctionRegistered = true; + } + + xpath = new BaseXPath(getStringProperty("xpath"), new DocumentNavigator()); + if (properties.size() > 1) { + SimpleVariableContext vc = new SimpleVariableContext(); + for (Iterator i = properties.entrySet().iterator(); i.hasNext();) { + Entry e = (Entry) i.next(); + if (!"xpath".equals(e.getKey())) { + vc.setVariableValue((String) e.getKey(), e.getValue()); + } + } + xpath.setVariableContext(vc); + } + } + +} +