Merge branch 'xpath-update-rules' of github.com:oowekyala/pmd into xpath-update-rules

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/AbstractClassWithoutAbstractMethodRule.java

@@ -0,0 +1,50 @@
+/*
+ * BSD-style license; for more info see http://pmd.sourceforge.net/license.html
+ */
+
+
+package net.sourceforge.pmd.lang.java.rule.bestpractices;
+
+import net.sourceforge.pmd.lang.java.ast.ASTAnyTypeBodyDeclaration;
+import net.sourceforge.pmd.lang.java.ast.ASTAnyTypeBodyDeclaration.DeclarationKind;
+import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceDeclaration;
+import net.sourceforge.pmd.lang.java.ast.ASTExtendsList;
+import net.sourceforge.pmd.lang.java.ast.ASTImplementsList;
+import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclaration;
+import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule;
+
+public class AbstractClassWithoutAbstractMethodRule extends AbstractJavaRule {
+
+    public AbstractClassWithoutAbstractMethodRule() {
+        addRuleChainVisit(ASTClassOrInterfaceDeclaration.class);
+    }
+
+    @Override
+    public Object visit(ASTClassOrInterfaceDeclaration node, Object data) {
+        if (!node.isAbstract() || doesExtend(node) || doesImplement(node)) {
+            return data;
+        }
+
+        int countOfAbstractMethods = 0;
+        for (ASTAnyTypeBodyDeclaration decl : node.getDeclarations()) {
+            if (decl.getKind() == DeclarationKind.METHOD) {
+                ASTMethodDeclaration methodDecl = (ASTMethodDeclaration) decl.getDeclarationNode();
+                if (methodDecl.isAbstract()) {
+                    countOfAbstractMethods++;
+                }
+            }
+        }
+        if (countOfAbstractMethods == 0) {
+            addViolation(data, node);
+        }
+        return data;
+    }
+
+    private boolean doesExtend(ASTClassOrInterfaceDeclaration node) {
+        return node.getFirstChildOfType(ASTExtendsList.class) != null;
+    }
+
+    private boolean doesImplement(ASTClassOrInterfaceDeclaration node) {
+        return node.getFirstChildOfType(ASTImplementsList.class) != null;
+    }
+}

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/AbstractPositionLiteralsFirstInComparisons.java

@@ -0,0 +1,119 @@
+/*
+ * BSD-style license; for more info see http://pmd.sourceforge.net/license.html
+ */
+
+package net.sourceforge.pmd.lang.java.rule.bestpractices;
+
+import net.sourceforge.pmd.lang.ast.Node;
+import net.sourceforge.pmd.lang.java.ast.ASTArgumentList;
+import net.sourceforge.pmd.lang.java.ast.ASTArguments;
+import net.sourceforge.pmd.lang.java.ast.ASTConditionalAndExpression;
+import net.sourceforge.pmd.lang.java.ast.ASTConditionalOrExpression;
+import net.sourceforge.pmd.lang.java.ast.ASTEqualityExpression;
+import net.sourceforge.pmd.lang.java.ast.ASTExpression;
+import net.sourceforge.pmd.lang.java.ast.ASTLiteral;
+import net.sourceforge.pmd.lang.java.ast.ASTName;
+import net.sourceforge.pmd.lang.java.ast.ASTNullLiteral;
+import net.sourceforge.pmd.lang.java.ast.ASTPrimaryExpression;
+import net.sourceforge.pmd.lang.java.ast.ASTPrimaryPrefix;
+import net.sourceforge.pmd.lang.java.ast.ASTPrimarySuffix;
+import net.sourceforge.pmd.lang.java.ast.JavaNode;
+import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule;
+
+class AbstractPositionLiteralsFirstInComparisons extends AbstractJavaRule {
+
+    private final String equalsImage;
+
+    AbstractPositionLiteralsFirstInComparisons(String equalsImage) {
+        addRuleChainVisit(ASTPrimaryExpression.class);
+        this.equalsImage = equalsImage;
+    }
+
+    @Override
+    public Object visit(ASTPrimaryExpression node, Object data) {
+        ASTPrimaryPrefix primaryPrefix = node.getFirstChildOfType(ASTPrimaryPrefix.class);
+        ASTPrimarySuffix primarySuffix = node.getFirstChildOfType(ASTPrimarySuffix.class);
+        if (primaryPrefix != null && primarySuffix != null) {
+            ASTName name = primaryPrefix.getFirstChildOfType(ASTName.class);
+            if (name == null || !name.getImage().endsWith(equalsImage)) {
+                return data;
+            }
+            if (!isSingleStringLiteralArgument(primarySuffix)) {
+                return data;
+            }
+            if (isWithinNullComparison(node)) {
+                return data;
+            }
+            addViolation(data, node);
+        }
+        return node;
+    }
+
+    private boolean isWithinNullComparison(ASTPrimaryExpression node) {
+        for (ASTExpression parentExpr : node.getParentsOfType(ASTExpression.class)) {
+            if (isComparisonWithNull(parentExpr, "==", ASTConditionalOrExpression.class)
+                    || isComparisonWithNull(parentExpr, "!=", ASTConditionalAndExpression.class)) {
+                return true;
+            }
+        }
+        return false;
+    }
+
+    /*
+     * Expression/ConditionalAndExpression//EqualityExpression(@Image='!=']//NullLiteral
+     * Expression/ConditionalOrExpression//EqualityExpression(@Image='==']//NullLiteral
+     */
+    private boolean isComparisonWithNull(ASTExpression parentExpr, String equalOperator, Class<? extends JavaNode> condition) {
+        Node condExpr = null;
+        ASTEqualityExpression eqExpr = null;
+        if (parentExpr != null) {
+            condExpr = parentExpr.getFirstChildOfType(condition);
+        }
+        if (condExpr != null) {
+            eqExpr = condExpr.getFirstDescendantOfType(ASTEqualityExpression.class);
+        }
+        if (eqExpr != null) {
+            return eqExpr.hasImageEqualTo(equalOperator) && eqExpr.hasDescendantOfType(ASTNullLiteral.class);
+        }
+        return false;
+    }
+
+    /*
+     * This corresponds to the following XPath expression:
+     * (../PrimarySuffix/Arguments/ArgumentList/Expression/PrimaryExpression/PrimaryPrefix/Literal[@StringLiteral= true()])
+     *       and
+     * ( count(../PrimarySuffix/Arguments/ArgumentList/Expression) = 1 )
+     */
+    private boolean isSingleStringLiteralArgument(ASTPrimarySuffix primarySuffix) {
+        if (!primarySuffix.isArguments() || primarySuffix.getArgumentCount() != 1) {
+            return false;
+        }
+        Node node = primarySuffix;
+        node = node.getFirstChildOfType(ASTArguments.class);
+        if (node != null) {
+            node = node.getFirstChildOfType(ASTArgumentList.class);
+            if (node.getNumChildren() != 1) {
+                return false;
+            }
+        }
+        if (node != null) {
+            node = node.getFirstChildOfType(ASTExpression.class);
+        }
+        if (node != null) {
+            node = node.getFirstChildOfType(ASTPrimaryExpression.class);
+        }
+        if (node != null) {
+            node = node.getFirstChildOfType(ASTPrimaryPrefix.class);
+        }
+        if (node != null) {
+            node = node.getFirstChildOfType(ASTLiteral.class);
+        }
+        if (node != null) {
+            ASTLiteral literal = (ASTLiteral) node;
+            if (literal.isStringLiteral()) {
+                return true;
+            }
+        }
+        return false;
+    }
+}

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/PositionLiteralsFirstInCaseInsensitiveComparisonsRule.java

@@ -0,0 +1,13 @@
+/*
+ * BSD-style license; for more info see http://pmd.sourceforge.net/license.html
+ */
+
+package net.sourceforge.pmd.lang.java.rule.bestpractices;
+
+public class PositionLiteralsFirstInCaseInsensitiveComparisonsRule extends AbstractPositionLiteralsFirstInComparisons {
+
+    public PositionLiteralsFirstInCaseInsensitiveComparisonsRule() {
+        super(".equalsIgnoreCase");
+    }
+
+}

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/PositionLiteralsFirstInComparisonsRule.java

@@ -0,0 +1,13 @@
+/*
+ * BSD-style license; for more info see http://pmd.sourceforge.net/license.html
+ */
+
+package net.sourceforge.pmd.lang.java.rule.bestpractices;
+
+public class PositionLiteralsFirstInComparisonsRule extends AbstractPositionLiteralsFirstInComparisons {
+
+    public PositionLiteralsFirstInComparisonsRule() {
+        super(".equals");
+    }
+
+}

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/JUnitSpellingRule.java

@@ -0,0 +1,28 @@
+/*
+ * BSD-style license; for more info see http://pmd.sourceforge.net/license.html
+ */
+
+
+package net.sourceforge.pmd.lang.java.rule.errorprone;
+
+import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclaration;
+import net.sourceforge.pmd.lang.java.rule.AbstractJUnitRule;
+
+public class JUnitSpellingRule extends AbstractJUnitRule {
+
+    @Override
+    public Object visit(ASTMethodDeclaration node, Object data) {
+        if (node.getArity() != 0) {
+            return super.visit(node, data);
+        }
+
+        String name = node.getName();
+        if (!"setUp".equals(name) && "setup".equalsIgnoreCase(name)) {
+            addViolation(data, node);
+        }
+        if (!"tearDown".equals(name) && "teardown".equalsIgnoreCase(name)) {
+            addViolation(data, node);
+        }
+        return super.visit(node, data);
+    }
+}

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/JUnitStaticSuiteRule.java

@@ -0,0 +1,24 @@
+/*
+ * BSD-style license; for more info see http://pmd.sourceforge.net/license.html
+ */
+
+
+package net.sourceforge.pmd.lang.java.rule.errorprone;
+
+import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclaration;
+import net.sourceforge.pmd.lang.java.rule.AbstractJUnitRule;
+
+public class JUnitStaticSuiteRule extends AbstractJUnitRule {
+
+    @Override
+    public Object visit(ASTMethodDeclaration node, Object data) {
+        if (node.getArity() != 0) {
+            return super.visit(node, data);
+        }
+        String name = node.getName();
+        if ("suite".equals(name) && (!node.isStatic() || !node.isPublic())) {
+            addViolation(data, node);
+        }
+        return super.visit(node, data);
+    }
+}

pmd-java/src/main/resources/category/java/bestpractices.xml

@@ -13,7 +13,7 @@ Rules which enforce generally accepted best practices.
           language="java"
           since="3.0"
           message="This abstract class does not have any abstract methods"
-          class="net.sourceforge.pmd.lang.rule.XPathRule"
+          class="net.sourceforge.pmd.lang.java.rule.bestpractices.AbstractClassWithoutAbstractMethodRule"
           externalInfoUrl="${pmd.website.baseurl}/pmd_rules_java_bestpractices.html#abstractclasswithoutabstractmethod">
         <description>
 The abstract class does not contain any abstract methods. An abstract class suggests
@@ -22,20 +22,6 @@ abstract methods. If the class is intended to be used as a base class only (not
 directly) a protected constructor can be provided prevent direct instantiation.
         </description>
         <priority>3</priority>
-        <properties>
-            <property name="version" value="2.0"/>
-            <property name="xpath">
-                <value>
-<![CDATA[
-//ClassOrInterfaceDeclaration
-  [@Abstract= true()]
-  [not(ImplementsList)]
-  [not(ExtendsList)]
-  [not(ClassOrInterfaceBody/ClassOrInterfaceBodyDeclaration/MethodDeclaration[@Abstract= true()])]
-]]>
-                </value>
-            </property>
-        </properties>
         <example>
 <![CDATA[
 public abstract class Foo {
@@ -1029,36 +1015,13 @@ String name,
           language="java"
           since="5.1"
           message="Position literals first in String comparisons for EqualsIgnoreCase"
-          class="net.sourceforge.pmd.lang.rule.XPathRule"
+          class="net.sourceforge.pmd.lang.java.rule.bestpractices.PositionLiteralsFirstInCaseInsensitiveComparisonsRule"
           externalInfoUrl="${pmd.website.baseurl}/pmd_rules_java_bestpractices.html#positionliteralsfirstincaseinsensitivecomparisons">
         <description>
 Position literals first in comparisons, if the second argument is null then NullPointerExceptions
 can be avoided, they will just return false.
         </description>
         <priority>3</priority>
-        <properties>
-            <property name="version" value="2.0"/>
-            <property name="xpath">
-                <value>
-<![CDATA[
-//PrimaryExpression[
-        PrimaryPrefix[Name
-                [
-    (ends-with(@Image, '.equalsIgnoreCase'))
-                ]
-        ]
-        [
-                   (../PrimarySuffix/Arguments/ArgumentList/Expression/PrimaryExpression/PrimaryPrefix/Literal)
-    and
-    ( count(../PrimarySuffix/Arguments/ArgumentList/Expression) = 1 )
-        ]
-]
-[not(ancestor::Expression/ConditionalAndExpression//EqualityExpression[@Image='!=']//NullLiteral)]
-[not(ancestor::Expression/ConditionalOrExpression//EqualityExpression[@Image='==']//NullLiteral)]
-]]>
-                </value>
-            </property>
-        </properties>
         <example>
 <![CDATA[
 class Foo {
@@ -1074,32 +1037,13 @@ class Foo {
           language="java"
           since="3.3"
           message="Position literals first in String comparisons"
-          class="net.sourceforge.pmd.lang.rule.XPathRule"
+          class="net.sourceforge.pmd.lang.java.rule.bestpractices.PositionLiteralsFirstInComparisonsRule"
           externalInfoUrl="${pmd.website.baseurl}/pmd_rules_java_bestpractices.html#positionliteralsfirstincomparisons">
         <description>
 Position literals first in comparisons, if the second argument is null then NullPointerExceptions
 can be avoided, they will just return false.
         </description>
         <priority>3</priority>
-        <properties>
-            <property name="version" value="2.0"/>
-            <property name="xpath">
-                <value>
-<![CDATA[
-//PrimaryExpression[
-    PrimaryPrefix[Name[(ends-with(@Image, '.equals'))]]
-        [
-            (../PrimarySuffix/Arguments/ArgumentList/Expression/PrimaryExpression/PrimaryPrefix/Literal[@StringLiteral= true()])
-            and
-            ( count(../PrimarySuffix/Arguments/ArgumentList/Expression) = 1 )
-        ]
-]
-[not(ancestor::Expression/ConditionalAndExpression//EqualityExpression[@Image='!=']//NullLiteral)]
-[not(ancestor::Expression/ConditionalOrExpression//EqualityExpression[@Image='==']//NullLiteral)]
-]]>
-                </value>
-            </property>
-        </properties>
         <example>
 <![CDATA[
 class Foo {

pmd-java/src/main/resources/category/java/errorprone.xml

@@ -2172,37 +2172,12 @@ public class JumbledIncrementerRule1 {
           language="java"
           since="1.0"
           message="You may have misspelled a JUnit framework method (setUp or tearDown)"
-          class="net.sourceforge.pmd.lang.rule.XPathRule"
+          class="net.sourceforge.pmd.lang.java.rule.errorprone.JUnitSpellingRule"
           externalInfoUrl="${pmd.website.baseurl}/pmd_rules_java_errorprone.html#junitspelling">
         <description>
 Some JUnit framework methods are easy to misspell.
         </description>
         <priority>3</priority>
-        <properties>
-            <property name="version" value="2.0"/>
-            <property name="xpath">
-                <value>
-<![CDATA[
-//ClassOrInterfaceDeclaration[
-    pmd-java:typeIs('junit.framework.TestCase')
-    or .//MarkerAnnotation/Name[
-        pmd-java:typeIs('org.junit.Test')
-        or pmd-java:typeIs('org.junit.jupiter.api.Test')
-        or pmd-java:typeIs('org.junit.jupiter.api.RepeatedTest')
-        or pmd-java:typeIs('org.junit.jupiter.api.TestFactory')
-        or pmd-java:typeIs('org.junit.jupiter.api.TestTemplate')
-        or pmd-java:typeIs('org.junit.jupiter.params.ParameterizedTest')
-        ]
-    ]
-//MethodDeclaration[(not(@Name = 'setUp')
- and translate(@Name, 'SETuP', 'setUp') = 'setUp')
- or (not(@Name = 'tearDown')
- and translate(@Name, 'TEARdOWN', 'tearDown') = 'tearDown')]
- [@Arity = 0]
-]]>
-                </value>
-            </property>
-        </properties>
         <example>
 <![CDATA[
 import junit.framework.*;
@@ -2219,36 +2194,13 @@ public class Foo extends TestCase {
           language="java"
           since="1.0"
           message="You have a suite() method that is not both public and static, so JUnit won't call it to get your TestSuite.  Is that what you wanted to do?"
-          class="net.sourceforge.pmd.lang.rule.XPathRule"
+          class="net.sourceforge.pmd.lang.java.rule.errorprone.JUnitStaticSuiteRule"
           typeResolution="true"
           externalInfoUrl="${pmd.website.baseurl}/pmd_rules_java_errorprone.html#junitstaticsuite">
         <description>
 The suite() method in a JUnit test needs to be both public and static.
         </description>
         <priority>3</priority>
-        <properties>
-            <property name="version" value="2.0"/>
-            <property name="xpath">
-                <value>
-<![CDATA[
-//ClassOrInterfaceDeclaration[
-    pmd-java:typeIs('junit.framework.TestCase')
-    or .//Name[parent::MarkerAnnotation][
-           pmd-java:typeIs('org.junit.Test')
-        or pmd-java:typeIs('org.junit.jupiter.api.Test')
-        or pmd-java:typeIs('org.junit.jupiter.api.RepeatedTest')
-        or pmd-java:typeIs('org.junit.jupiter.api.TestFactory')
-        or pmd-java:typeIs('org.junit.jupiter.api.TestTemplate')
-        or pmd-java:typeIs('org.junit.jupiter.params.ParameterizedTest')
-        ]
-    ]
-/ClassOrInterfaceBody/ClassOrInterfaceBodyDeclaration/MethodDeclaration[not(@Static= true()) or not(@Public= true())]
-[@Name='suite']
-[@Arity = 0]
-]]>
-                </value>
-            </property>
-        </properties>
         <example>
 <![CDATA[
 import junit.framework.*;
@@ -3405,12 +3357,16 @@ To make sure the full stacktrace is printed out, use the logging statement with
                 <value>
 <![CDATA[
 //CatchStatement/Block/BlockStatement/Statement/StatementExpression
-/PrimaryExpression[PrimaryPrefix/Name[starts-with(@Image,
-concat(ancestor::ClassOrInterfaceDeclaration/ClassOrInterfaceBody/ClassOrInterfaceBodyDeclaration/FieldDeclaration
-[Type//ClassOrInterfaceType[@Image='Log']]
-/VariableDeclarator/VariableDeclaratorId/@Image, '.'))]]
-[PrimarySuffix/Arguments[@Size= 1]]
-[PrimarySuffix/Arguments//Name/@Image = ancestor::CatchStatement/FormalParameter/VariableDeclaratorId/@Image]
+    /PrimaryExpression
+        [PrimaryPrefix/Name
+            [starts-with(@Image,
+             concat((ancestor::ClassOrInterfaceDeclaration/ClassOrInterfaceBody/ClassOrInterfaceBodyDeclaration/FieldDeclaration
+                [Type//ClassOrInterfaceType[@Image='Log']]
+                /VariableDeclarator/VariableDeclaratorId/@Image)[1], '.'))
+            ]
+        ]
+        [PrimarySuffix/Arguments[@Size= 1]]
+        [PrimarySuffix/Arguments//Name/@Image = ancestor::CatchStatement/FormalParameter/VariableDeclaratorId/@Image]
 ]]>
                 </value>
             </property>

pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/PositionLiteralsFirstInComparisons.xml

@@ -40,6 +40,15 @@ public class Foo {
   if((str == null) || (str.equals(""))) {
    str = "snafu";
   }
+  if(str == null || str.equals("")) {
+   str = "snafu";
+  }
+  if((str != null) && (str.equals(""))) {
+   str = "snafu";
+  }
+  if(str != null && str.equals("")) {
+   str = "snafu";
+  }
  }
 }
      ]]></code>

pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/UseCorrectExceptionLogging.xml

@@ -3,10 +3,9 @@
     xmlns="http://pmd.sourceforge.net/rule-tests"
     xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
     xsi:schemaLocation="http://pmd.sourceforge.net/rule-tests http://pmd.sourceforge.net/rule-tests_1_0_0.xsd">
+
     <test-code>
-        <description><![CDATA[
-ok
-     ]]></description>
+        <description>ok</description>
         <expected-problems>0</expected-problems>
         <code><![CDATA[
 public class Foo {
@@ -19,10 +18,9 @@ public class Foo {
 }
      ]]></code>
     </test-code>
+
     <test-code>
-        <description><![CDATA[
-failure case - two calls
-     ]]></description>
+        <description>failure case - two calls</description>
         <expected-problems>2</expected-problems>
         <code><![CDATA[
 public class Foo {
@@ -36,10 +34,9 @@ public class Foo {
 }
      ]]></code>
     </test-code>
+
     <test-code>
-        <description><![CDATA[
-must be in a catch block
-     ]]></description>
+        <description>must be in a catch block</description>
         <expected-problems>0</expected-problems>
         <code><![CDATA[
 public class Foo {
@@ -50,10 +47,9 @@ public class Foo {
 }
      ]]></code>
     </test-code>
+
     <test-code>
-        <description><![CDATA[
-bug 1626232, the rule should not be confused by inner classes
-     ]]></description>
+        <description>bug 1626232, the rule should not be confused by inner classes</description>
         <expected-problems>0</expected-problems>
         <code><![CDATA[
 public class Foo {
@@ -71,10 +67,9 @@ public class Foo {
 }
      ]]></code>
     </test-code>
+
     <test-code>
-        <description><![CDATA[
-bug 1626232, should work with a static block
-     ]]></description>
+        <description>bug 1626232, should work with a static block</description>
         <expected-problems>1</expected-problems>
         <code><![CDATA[
 public class Foo {
@@ -90,4 +85,23 @@ public class Foo {
 }
      ]]></code>
     </test-code>
+
+    <test-code>
+        <description>XPath problem: A sequence of more than one item is not allowed as the first argument of concat()</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+public class UseCorrectExceptionLoggingCase {
+    protected static final Log logger1 = LogFactory.getLog(DISCONNECTED_CLIENT_LOG_CATEGORY);
+    protected final Log logger2 = LogFactory.getLog(getClass());
+
+    public void run() {
+        try {
+            //...
+        } catch (Exception e) {
+            logger2.debug("Error", e);
+        }
+    }
+}
+        ]]></code>
+    </test-code>
 </test-data>