[java] Update rule ProperLogger

2021-11-11 17:23:49 +01:00
parent 149d8ebde4
commit 3983e1d839
4 changed files with 73 additions and 26 deletions
--- a/.ci/files/all-java.xml
+++ b/.ci/files/all-java.xml
@@ -243,7 +243,7 @@
    <!-- <rule ref="category/java/errorprone.xml/NullAssignment"/> -->
    <rule ref="category/java/errorprone.xml/OverrideBothEqualsAndHashcode"/>
    <rule ref="category/java/errorprone.xml/ProperCloneImplementation"/>
-    <!-- <rule ref="category/java/errorprone.xml/ProperLogger"/> -->
+    <rule ref="category/java/errorprone.xml/ProperLogger"/>
    <rule ref="category/java/errorprone.xml/ReturnEmptyCollectionRatherThanNull"/>
    <rule ref="category/java/errorprone.xml/ReturnFromFinallyBlock"/>
    <!-- <rule ref="category/java/errorprone.xml/SimpleDateFormatNeedsLocale"/> -->
--- a/pmd-java/src/main/resources/category/java/errorprone.xml
+++ b/pmd-java/src/main/resources/category/java/errorprone.xml
@@ -2580,36 +2580,32 @@ with the restriction that the logger needs to be passed into the constructor.
                <value>
 <![CDATA[
 //FieldDeclaration
-[Type/ReferenceType/ClassOrInterfaceType[pmd-java:typeIs($loggerClass)]]
-[
-    (: check modifiers :)
-    (@Private = false() or @Final = false())
-    (: check logger name :)
-    or (@Static and VariableDeclarator/VariableDeclaratorId[@Name != $staticLoggerName])
-    or (@Static = false() and VariableDeclarator/VariableDeclaratorId[@Name != $loggerName])
+    [ClassOrInterfaceType[pmd-java:typeIs($loggerClass)]]
+    [
+        (: check modifiers :)
+        (not(pmd-java:modifiers() = 'private') or not(pmd-java:modifiers() = 'final'))
+        (: check logger name :)
+        or (pmd-java:modifiers() = 'static' and VariableDeclarator/VariableDeclaratorId/@Name != $staticLoggerName)
+        or (not(pmd-java:modifiers() = 'static') and VariableDeclarator/VariableDeclaratorId/@Name != $loggerName)
+        (: check logger argument type matches class or enum name :)
+        or .//ArgumentList/ClassLiteral/ClassOrInterfaceType/@SimpleName != ancestor::ClassOrInterfaceDeclaration/@SimpleName
+        or .//ArgumentList/ClassLiteral/ClassOrInterfaceType/@SimpleName != ancestor::EnumDeclaration/@SimpleName

-    (: check logger argument type matches class or enum name :)
-    or .//ArgumentList//ClassOrInterfaceType[@Image != ancestor::ClassOrInterfaceDeclaration/@SimpleName]
-    or .//ArgumentList//ClassOrInterfaceType[@Image != ancestor::EnumDeclaration/@SimpleName]
-]
-[not(
-     (: special case - final logger initialized inside constructor :)
-     not(VariableDeclarator/VariableInitializer)
-     and @Static = false()
-     and
-     ancestor::ClassOrInterfaceBody//ConstructorDeclaration//StatementExpression
-        [PrimaryExpression[PrimaryPrefix[@ThisModifier]]/PrimarySuffix[@Image=$loggerName]]
-        [AssignmentOperator[@Image = '=']]
-        [Expression/PrimaryExpression/PrimaryPrefix/Name[@Image = ancestor::ConstructorDeclaration//FormalParameter/VariableDeclaratorId/@Name]]
-        [not(.//AllocationExpression)]
-  )
-]
+        (: special case - final logger initialized inside constructor :)
+        or (VariableDeclarator/@Initializer = false()
+            and not(pmd-java:modifiers() = 'static')
+            and not(ancestor::ClassOrInterfaceBody/ConstructorDeclaration
+                //AssignmentExpression[@Operator = '=']
+                    [FieldAccess[1]/@Name = $loggerName or VariableAccess[1]/@Name = $loggerName]
+                    [*[2][@Name = ancestor::ConstructorDeclaration//FormalParameter/VariableDeclaratorId/@Name]])
+        )
+    ]
 ]]>
                </value>
            </property>
            <property name="staticLoggerName" type="String" description="Name of the static Logger variable" value="LOG"/>
            <property name="loggerName" type="String" description="Name of the Logger instance variable" value="log"/>
-            <property name="loggerClass" type="String" description="Class name of the logger" value="Log"/>
+            <property name="loggerClass" type="String" description="Class name of the logger" value="org.apache.commons.logging.Log"/>
        </properties>
        <example>
 <![CDATA[
--- a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/errorprone/ProperLoggerTest.java
+++ b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/errorprone/ProperLoggerTest.java
@@ -6,7 +6,6 @@ package net.sourceforge.pmd.lang.java.rule.errorprone;

 import net.sourceforge.pmd.testframework.PmdRuleTst;

-@org.junit.Ignore("Rule has not been updated yet")
 public class ProperLoggerTest extends PmdRuleTst {
    // no additional unit tests
 }
--- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/ProperLogger.xml
+++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/ProperLogger.xml
@@ -8,6 +8,8 @@
        <description>Ok</description>
        <expected-problems>0</expected-problems>
        <code><![CDATA[
+import org.apache.commons.logging.LogFactory;
+import org.apache.commons.logging.Log;
 public class Foo {
    private static final Log LOG = LogFactory.getLog(Foo.class);
 }
@@ -17,10 +19,14 @@ public class Foo {
    <test-code>
        <description>Wrong class name</description>
        <expected-problems>1</expected-problems>
+        <expected-linenumbers>4</expected-linenumbers>
        <code><![CDATA[
+import org.apache.commons.logging.LogFactory;
+import org.apache.commons.logging.Log;
 public class Foo {
    private static final Log LOG = LogFactory.getLog(Bar.class);
 }
+class Bar {}
        ]]></code>
    </test-code>

@@ -28,6 +34,7 @@ public class Foo {
        <description>Ok, special case</description>
        <expected-problems>0</expected-problems>
        <code><![CDATA[
+import org.apache.commons.logging.Log;
 public class Foo {
    private final Log log;
    Foo(Log log) {
@@ -40,11 +47,15 @@ public class Foo {
    <test-code>
        <description>Enum wrong class name</description>
        <expected-problems>1</expected-problems>
+        <expected-linenumbers>5</expected-linenumbers>
        <code><![CDATA[
+import org.apache.commons.logging.LogFactory;
+import org.apache.commons.logging.Log;
 public enum Foo {
    TEST;
    private static final Log LOG = LogFactory.getLog(Bar.class);
 }
+class Bar {}
        ]]></code>
    </test-code>

@@ -52,6 +63,7 @@ public enum Foo {
        <description>bug 1626232, a separate variable initialization should not confuse the rule</description>
        <expected-problems>0</expected-problems>
        <code><![CDATA[
+import org.apache.commons.logging.Log;
 public class Foo {
    private String bar = "";
    private final Log log;
@@ -66,6 +78,8 @@ public class Foo {
        <description>Enums shoud be accepted for declaration</description>
        <expected-problems>0</expected-problems>
        <code><![CDATA[
+import org.apache.commons.logging.LogFactory;
+import org.apache.commons.logging.Log;
 public enum Foo {
    TEST;
    private static final Log LOG = LogFactory.getLog(Foo.class);
@@ -76,7 +90,10 @@ public enum Foo {
    <test-code>
        <description>bug 1626232, extra loggers with different names are not allowed by default (use NOPMD if you want them)</description>
        <expected-problems>1</expected-problems>
+        <expected-linenumbers>5</expected-linenumbers>
        <code><![CDATA[
+import org.apache.commons.logging.LogFactory;
+import org.apache.commons.logging.Log;
 public class Foo {
    private static final Log LOG = LogFactory.getLog(Foo.class);
    private static final Log LOG2 = LogFactory.getLog(Foo.class + ".foo");
@@ -88,17 +105,25 @@ public class Foo {
        <description>Custom logger-class with invalid logger variable name</description>
        <rule-property name="loggerClass">MyLog</rule-property>
        <expected-problems>1</expected-problems>
+        <expected-linenumbers>4</expected-linenumbers>
        <code><![CDATA[
+import org.apache.commons.logging.LogFactory;
+import org.apache.commons.logging.Log;
 public class Foo {
    private static final MyLog INVALID_LOG_NAME = LogFactory.getLog(Foo.class);
 }
+
+abstract class MyLog implements Log {}
        ]]></code>
    </test-code>

    <test-code>
        <description>Public logger</description>
        <expected-problems>1</expected-problems>
+        <expected-linenumbers>4</expected-linenumbers>
        <code><![CDATA[
+import org.apache.commons.logging.LogFactory;
+import org.apache.commons.logging.Log;
 public class Foo {
    public static final Log LOG = LogFactory.getLog(Foo.class);
 }
@@ -108,7 +133,10 @@ public class Foo {
    <test-code>
        <description>package-private logger</description>
        <expected-problems>1</expected-problems>
+        <expected-linenumbers>4</expected-linenumbers>
        <code><![CDATA[
+import org.apache.commons.logging.LogFactory;
+import org.apache.commons.logging.Log;
 public class Foo {
    static final Log LOG = LogFactory.getLog(Foo.class);
 }
@@ -118,7 +146,10 @@ public class Foo {
    <test-code>
        <description>package-private logger non static</description>
        <expected-problems>1</expected-problems>
+        <expected-linenumbers>4</expected-linenumbers>
        <code><![CDATA[
+import org.apache.commons.logging.LogFactory;
+import org.apache.commons.logging.Log;
 public class Foo {
    final Log LOG = LogFactory.getLog(Foo.class);
 }
@@ -128,7 +159,10 @@ public class Foo {
    <test-code>
        <description>package-private logger non static and non final</description>
        <expected-problems>1</expected-problems>
+        <expected-linenumbers>4</expected-linenumbers>
        <code><![CDATA[
+import org.apache.commons.logging.LogFactory;
+import org.apache.commons.logging.Log;
 public class Foo {
    Log LOG = LogFactory.getLog(Foo.class);
 }
@@ -138,7 +172,10 @@ public class Foo {
    <test-code>
        <description>Public logger when static final required</description>
        <expected-problems>1</expected-problems>
+        <expected-linenumbers>4</expected-linenumbers>
        <code><![CDATA[
+import org.apache.commons.logging.LogFactory;
+import org.apache.commons.logging.Log;
 public class Foo {
    public static final Log LOG = LogFactory.getLog(Foo.class);
 }
@@ -161,6 +198,7 @@ public class Foo {
        <description>non-private Logger initialized as String constant from class name</description>
        <rule-property name="loggerClass">java.util.logging.Logger</rule-property>
        <expected-problems>1</expected-problems>
+        <expected-linenumbers>4</expected-linenumbers>
        <code><![CDATA[
 import java.util.logging.Logger;

@@ -173,7 +211,10 @@ public class Foo {
    <test-code>
        <description>Package-protected logger when static final is not required</description>
        <expected-problems>1</expected-problems>
+        <expected-linenumbers>4</expected-linenumbers>
        <code><![CDATA[
+import org.apache.commons.logging.LogFactory;
+import org.apache.commons.logging.Log;
 public class Foo {
    Log LOG = LogFactory.getLog(Foo.class);
 }
@@ -183,7 +224,10 @@ public class Foo {
    <test-code>
        <description>Package-protected logger when static final is required</description>
        <expected-problems>1</expected-problems>
+        <expected-linenumbers>4</expected-linenumbers>
        <code><![CDATA[
+import org.apache.commons.logging.LogFactory;
+import org.apache.commons.logging.Log;
 public class Foo {
    Log LOG = LogFactory.getLog(Foo.class);
 }
@@ -208,6 +252,7 @@ public class Foo {
        <description>Check type resolution of logger with invalid logger specified</description>
        <rule-property name="loggerClass">org.slf4j.Logger</rule-property>
        <expected-problems>1</expected-problems>
+        <expected-linenumbers>5</expected-linenumbers>
        <code><![CDATA[
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -215,13 +260,17 @@ import org.slf4j.LoggerFactory;
 public class Foo {
    private static final Logger LOG = LoggerFactory.getLogger(Bar.class);
 }
+class Bar {}
        ]]></code>
    </test-code>

    <test-code>
        <description>similar to special case but with creating a new logger instead of passing in</description>
        <expected-problems>1</expected-problems>
+        <expected-linenumbers>4</expected-linenumbers>
        <code><![CDATA[
+import org.apache.commons.logging.LogFactory;
+import org.apache.commons.logging.Log;
 public class Foo {
    private final Log log;
    public Foo() {
@@ -234,7 +283,9 @@ public class Foo {
    <test-code>
        <description>similar to special case but with creating a new logger instead of passing in</description>
        <expected-problems>1</expected-problems>
+        <expected-linenumbers>3</expected-linenumbers>
        <code><![CDATA[
+import org.apache.commons.logging.Log;
 public class Foo {
    private final Log log;
    public Foo() {
@@ -247,6 +298,7 @@ public class Foo {
    <test-code>
        <description>false negative with apache commons logging</description>
        <expected-problems>1</expected-problems>
+        <expected-linenumbers>5</expected-linenumbers>
        <code><![CDATA[
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;