Merge branch 'pr-772'

docs/pages/release_notes.md

@@ -272,6 +272,10 @@ The rule reference documentation has been updated to reflect these changes.
 *   The Java rule `EmptyStaticInitializer` in ruleset `java-empty` is deprecated. Use the rule `EmptyInitializer`
     from the category `errorprone`, which covers both static and non-static empty initializers.`
 
+*   The Java rules `GuardDebugLogging` (ruleset `java-logging-jakarta-commons`) and `GuardLogStatementJavaUtil`
+    (ruleset `java-logging-java`) have been deprecated. Use the rule `GuardLogStatement` from the
+    category `bestpractices`, which covers all cases regardless of the logging framework.
+
 #### Removed Rules
 
 *   The deprecated Java rule `UseSingleton` has been removed from the ruleset `java-design`. The rule has been renamed
@@ -364,6 +368,7 @@ a warning will now be produced suggesting users to adopt it for better performan
     *   [#438](https://github.com/pmd/pmd/issues/438): \[java] Relax AbstractClassWithoutAnyMethod when class is annotated by @AutoValue
     *   [#590](https://github.com/pmd/pmd/issues/590): \[java] False positive on MissingStaticMethodInNonInstantiatableClass
 *    java-logging
+    *   [#457](https://github.com/pmd/pmd/issues/457): \[java] Merge all log guarding rules
     *   [#721](https://github.com/pmd/pmd/issues/721): \[java] NPE in PMD 5.8.1 InvalidSlf4jMessageFormat
 *   java-sunsecure
     *   [#468](https://github.com/pmd/pmd/issues/468): \[java] ArrayIsStoredDirectly false positive

pmd-core/src/main/java/net/sourceforge/pmd/RuleSetFactoryCompatibility.java

@@ -78,6 +78,10 @@ public class RuleSetFactoryCompatibility {
         addFilterRuleRenamed("java", "naming", "MisleadingVariableName", "MIsLeadingVariableName");
         addFilterRuleRenamed("java", "unnecessary", "UnnecessaryFinalModifier", "UnnecessaryModifier");
         addFilterRuleRenamed("java", "empty", "EmptyStaticInitializer", "EmptyInitializer");
+        // GuardLogStatementJavaUtil moved and renamed...
+        addFilterRuleMoved("java", "logging-java", "logging-jakarta-commons", "GuardLogStatementJavaUtil");
+        addFilterRuleRenamed("java", "logging-jakarta-commons", "GuardLogStatementJavaUtil", "GuardLogStatement");
+        addFilterRuleRenamed("java", "logging-jakarta-commons", "GuardDebugLogging", "GuardLogStatement");
     }
 
     void addFilterRuleRenamed(String language, String ruleset, String oldName, String newName) {

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

@@ -1,21 +0,0 @@
-/**
- * BSD-style license; for more info see http://pmd.sourceforge.net/license.html
- */
-
-package net.sourceforge.pmd.lang.java.rule.bestpractices;
-
-import java.util.HashMap;
-
-public class GuardDebugLoggingRule extends GuardLogStatementRule {
-
-    public GuardDebugLoggingRule() {
-        super.guardStmtByLogLevel = new HashMap<>(1);
-        super.guardStmtByLogLevel.put(".debug", "isDebugEnabled");
-    }
-
-    @Override
-    protected void extractProperties() {
-        // This rule is not configurable
-    }
-
-}

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

@@ -1,81 +0,0 @@
-/**
- * BSD-style license; for more info see http://pmd.sourceforge.net/license.html
- */
-
-package net.sourceforge.pmd.lang.java.rule.bestpractices;
-
-import java.util.List;
-import java.util.logging.Level;
-
-import net.sourceforge.pmd.lang.java.ast.ASTCompilationUnit;
-import net.sourceforge.pmd.lang.java.ast.ASTImportDeclaration;
-
-public class GuardLogStatementJavaUtilRule extends GuardLogStatementRule {
-
-    private static final String GUARD_METHOD_NAME = "isLoggable";
-
-    private static String extendedXPath = "//PrimaryPrefix[ends-with(Name/@Image, '.log')]\n"
-            + "[following-sibling::PrimarySuffix\n"
-            + "    [ends-with(.//PrimaryPrefix/Name/@Image, 'LOG_LEVEL_UPPERCASE')]\n"
-            + "    [count(../descendant::AdditiveExpression) > 0]\n" + "]\n"
-            + "[count(ancestor::IfStatement/Expression/descendant::PrimaryExpression\n"
-            + "    [ends-with(descendant::PrimaryPrefix[1]/Name/@Image,'GUARD')]) = 0\n" + "or\n"
-            + "count(ancestor::IfStatement/Expression/descendant::PrimaryExpression\n"
-            + "    [ends-with(descendant::PrimaryPrefix[2]/Name/@Image,'LOG_LEVEL_UPPERCASE')]) = 0]";
-
-    @Override
-    public Object visit(ASTCompilationUnit unit, Object data) {
-        if (isSlf4jOrLog4jImported(unit)) {
-            return data;
-        }
-
-        String[] logLevels = getProperty(LOG_LEVELS).toArray(new String[0]); // TODO:cf convert to list
-        String[] guardMethods = getProperty(GUARD_METHODS).toArray(new String[0]);
-
-        if (super.guardStmtByLogLevel.isEmpty() && logLevels.length > 0 && guardMethods.length > 0) {
-            configureGuards(logLevels, guardMethods);
-        } else if (super.guardStmtByLogLevel.isEmpty()) {
-            configureDefaultGuards();
-        }
-
-        findViolationForEachLogStatement(unit, data, extendedXPath);
-        return super.visit(unit, data);
-    }
-
-    private boolean isSlf4jOrLog4jImported(ASTCompilationUnit unit) {
-        List<ASTImportDeclaration> imports = unit.findChildrenOfType(ASTImportDeclaration.class);
-        for (ASTImportDeclaration i : imports) {
-            if (i.getImportedName().startsWith("org.slf4j") || i.getImportedName().startsWith("org.apache.log4j")) {
-                return true;
-            }
-        }
-        return false;
-    }
-
-    private void configureGuards(String[] logLevels, String[] guardMethods) {
-        String[] methods = guardMethods;
-        if (methods.length != logLevels.length) {
-            String firstMethodName = guardMethods[0];
-            methods = new String[logLevels.length];
-            for (int i = 0; i < logLevels.length; i++) {
-                methods[i] = firstMethodName;
-            }
-        }
-        for (int i = 0; i < logLevels.length; i++) {
-            super.guardStmtByLogLevel.put("." + logLevels[i], methods[i]);
-        }
-    }
-
-    private void configureDefaultGuards() {
-        super.guardStmtByLogLevel.put(formatLogLevelString(Level.FINEST), GUARD_METHOD_NAME);
-        super.guardStmtByLogLevel.put(formatLogLevelString(Level.FINER), GUARD_METHOD_NAME);
-        super.guardStmtByLogLevel.put(formatLogLevelString(Level.FINE), GUARD_METHOD_NAME);
-        super.guardStmtByLogLevel.put(formatLogLevelString(Level.INFO), GUARD_METHOD_NAME);
-        super.guardStmtByLogLevel.put(formatLogLevelString(Level.WARNING), GUARD_METHOD_NAME);
-        super.guardStmtByLogLevel.put(formatLogLevelString(Level.SEVERE), GUARD_METHOD_NAME);
-    }
-
-    private String formatLogLevelString(Level logLevel) {
-        return "." + logLevel.toString().toLowerCase();
-    }
-}

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

@@ -30,19 +30,52 @@ import net.sourceforge.pmd.properties.StringMultiProperty;
  * 
  */
 public class GuardLogStatementRule extends AbstractJavaRule implements Rule {
+    /*
+     * guard methods and log levels:
+     * 
+     * log4j + apache commons logging (jakarta):
+     * trace -> isTraceEnabled
+     * debug -> isDebugEnabled
+     * info  -> isInfoEnabled
+     * warn  -> isWarnEnabled
+     * error -> isErrorEnabled
+     * 
+     * 
+     * java util:
+     * log(Level.FINE) ->  isLoggable
+     * finest ->  isLoggable
+     * finer  ->  isLoggable
+     * fine   ->  isLoggable
+     * info   ->  isLoggable
+     * warning -> isLoggable
+     * severe  -> isLoggable
+     */
+    private static final StringMultiProperty LOG_LEVELS = new StringMultiProperty("logLevels", "LogLevels to guard",
+            new String[] {"trace", "debug", "info", "warn", "error",
+                "log", "finest", "finer", "fine", "info", "warning", "severe", }, 1.0f, ',');
 
-    public static final StringMultiProperty LOG_LEVELS = new StringMultiProperty("logLevels", "LogLevels to guard",
+    private static final StringMultiProperty GUARD_METHODS = new StringMultiProperty("guardsMethods",
-            new String[] {}, 1.0f, ',');
+            "method use to guard the log statement",
+            new String[] {"isTraceEnabled", "isDebugEnabled", "isInfoEnabled", "isWarnEnabled", "isErrorEnabled",
+                "isLoggable", }, 2.0f, ',');
 
-    public static final StringMultiProperty GUARD_METHODS = new StringMultiProperty("guardsMethods",
+    private Map<String, String> guardStmtByLogLevel = new HashMap<>(12);
-            "method use to guard the log statement", new String[] {}, 2.0f, ',');
 
-    protected Map<String, String> guardStmtByLogLevel = new HashMap<>(5);
+    private static final String XPATH_EXPRESSION =
-
+            // first part deals with log4j / apache commons logging
-    private static final String XPATH_EXPRESSION = "//PrimaryPrefix[ends-with(Name/@Image, 'LOG_LEVEL')]"
+            "//StatementExpression/PrimaryExpression/PrimaryPrefix[ends-with(Name/@Image, 'LOG_LEVEL')]\n"
-            + "[count(../descendant::AdditiveExpression) > 0]"
+            + "[..//AdditiveExpression]\n"
-            + "[count(ancestor::IfStatement/Expression/descendant::PrimaryExpression["
+            + "[not(ancestor::IfStatement) or\n"
-            + "ends-with(descendant::PrimaryPrefix/Name/@Image,'GUARD')]) = 0]";
+            + " not(ancestor::IfStatement/Expression/PrimaryExpression/PrimaryPrefix/Name[contains('GUARD', substring-after(@Image, '.'))])]\n"
+            + "|\n"
+            // this part deals with java util
+            + "//StatementExpression/PrimaryExpression/PrimaryPrefix[ends-with(Name/@Image, 'LOG_LEVEL_UPPERCASE')]\n"
+            + "[../../..//AdditiveExpression]\n"
+            + "[not(ancestor::IfStatement) or\n"
+            + " not(ancestor::IfStatement/Expression/PrimaryExpression\n"
+            + "    [contains('GUARD', substring-after(PrimaryPrefix/Name/@Image, '.'))]\n"
+            + "    [ends-with(PrimarySuffix//Name/@Image, 'LOG_LEVEL_UPPERCASE')])\n"
+            + "]";
 
     public GuardLogStatementRule() {
         definePropertyDescriptor(LOG_LEVELS);
@@ -65,9 +98,9 @@ public class GuardLogStatementRule extends AbstractJavaRule implements Rule {
     }
 
 
-    protected void findViolationForEachLogStatement(ASTCompilationUnit unit, Object data, String xpathExpression) {
+    private void findViolationForEachLogStatement(ASTCompilationUnit unit, Object data, String xpathExpression) {
         for (Entry<String, String> entry : guardStmtByLogLevel.entrySet()) {
-            List<? extends Node> nodes = findViolations(unit, entry.getKey(), entry.getValue(), xpathExpression);
+            List<Node> nodes = findViolations(unit, entry.getKey(), entry.getValue(), xpathExpression);
             for (Node node : nodes) {
                 super.addViolation(data, node);
             }
@@ -75,64 +108,52 @@ public class GuardLogStatementRule extends AbstractJavaRule implements Rule {
     }
 
     @SuppressWarnings("unchecked")
-    private List<? extends Node> findViolations(ASTCompilationUnit unit, String logLevel, String guard,
+    private List<Node> findViolations(ASTCompilationUnit unit, String logLevel, String guard,
             String xpathExpression) {
         try {
-            return unit
+            String xpath = xpathExpression.replaceAll("LOG_LEVEL_UPPERCASE", logLevel.toUpperCase())
-                    .findChildNodesWithXPath(xpathExpression.replaceAll("LOG_LEVEL_UPPERCASE", logLevel.toUpperCase())
+                    .replaceAll("LOG_LEVEL", logLevel).replaceAll("GUARD", guard);
-                            .replaceAll("LOG_LEVEL", logLevel).replaceAll("GUARD", guard));
+            return unit.findChildNodesWithXPath(xpath);
         } catch (JaxenException e) {
             e.printStackTrace();
         }
         return Collections.EMPTY_LIST;
     }
 
-    private void setPropertiesDefaultValues(List<String> logLevels, List<String> guardMethods) {
+    private void extractProperties() {
-        logLevels.add("trace");
-        logLevels.add("debug");
-        logLevels.add("info");
-        logLevels.add("warn");
-        logLevels.add("error");
-
-        guardMethods.clear();
-        guardMethods.add("isTraceEnabled");
-        guardMethods.add("isDebugEnabled");
-        guardMethods.add("isInfoEnabled");
-        guardMethods.add("isWarnEnabled");
-        guardMethods.add("isErrorEnabled");
-    }
-
-    protected void extractProperties() {
         if (guardStmtByLogLevel.isEmpty()) {
 
             List<String> logLevels = new ArrayList<>(super.getProperty(LOG_LEVELS));
             List<String> guardMethods = new ArrayList<>(super.getProperty(GUARD_METHODS));
 
             if (guardMethods.isEmpty() && !logLevels.isEmpty()) {
-                throw new IllegalArgumentException("Can't specify guardMethods without specifiying logLevels.");
+                throw new IllegalArgumentException("Can't specify logLevels without specifying guardMethods.");
             }
-
+            if (logLevels.size() > guardMethods.size()) {
-            if (logLevels.isEmpty()) {
+                // reuse the last guardMethod for the remaining log levels
-                setPropertiesDefaultValues(logLevels, guardMethods);
+                int needed = logLevels.size() - guardMethods.size();
+                String lastGuard = guardMethods.get(guardMethods.size() - 1);
+                for (int i = 0; i < needed; i++) {
+                    guardMethods.add(lastGuard);
+                }
+            }
+            if (logLevels.size() != guardMethods.size()) {
+                throw new IllegalArgumentException("For each logLevel a guardMethod must be specified.");
             }
 
             buildGuardStatementMap(logLevels, guardMethods);
         }
     }
 
-    protected void buildGuardStatementMap(List<String> logLevels, List<String> guardMethods) {
+    private void buildGuardStatementMap(List<String> logLevels, List<String> guardMethods) {
-        for (String logLevel : logLevels) {
+        for (int i = 0; i < logLevels.size(); i++) {
-            boolean found = false;
+            String logLevel = "." + logLevels.get(i);
-            for (String guardMethod : guardMethods) {
+            if (guardStmtByLogLevel.containsKey(logLevel)) {
-                if (!found && guardMethod.toLowerCase().contains(logLevel.toLowerCase())) {
+                String combinedGuard = guardStmtByLogLevel.get(logLevel);
-                    found = true;
+                combinedGuard += "|" + guardMethods.get(i);
-                    guardStmtByLogLevel.put("." + logLevel, guardMethod);
+                guardStmtByLogLevel.put(logLevel, combinedGuard);
-                }
+            } else {
-            }
+                guardStmtByLogLevel.put(logLevel, guardMethods.get(i));
-
-            if (!found) {
-                throw new IllegalArgumentException("No guard method associated to the logLevel:" + logLevel
-                        + ". Should be something like 'is" + logLevel + "Enabled'.");
             }
         }
     }

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

@@ -383,44 +383,6 @@ public class MyClass {
         </example>
     </rule>
 
-    <rule name="GuardDebugLogging"
-          language="java"
-          since="4.3"
-          message="debug logging that involves string concatenation should be guarded with isDebugEnabled() checks"
-          class="net.sourceforge.pmd.lang.java.rule.bestpractices.GuardDebugLoggingRule"
-          externalInfoUrl="${pmd.website.baseurl}/pmd_rules_java_bestpractices.html#guarddebuglogging">
-        <description>
-When log messages are composed by concatenating strings, the whole section should be guarded
-by a isDebugEnabled() check to avoid performance and memory issues.
-        </description>
-        <priority>3</priority>
-        <example>
-<![CDATA[
-public class Test {
-    private static final Log __log = LogFactory.getLog(Test.class);
-    public void test() {
-        // okay:
-        __log.debug("log something");
-
-        // okay:
-        __log.debug("log something with exception", e);
-
-        // bad:
-        __log.debug("log something" + " and " + "concat strings");
-
-        // bad:
-        __log.debug("log something" + " and " + "concat strings", e);
-
-        // good:
-        if (__log.isDebugEnabled()) {
-        __log.debug("bla" + "",e );
-        }
-    }
-}
-]]>
-        </example>
-    </rule>
-
     <rule name="GuardLogStatement"
           language="java"
           since="5.1.0"
@@ -441,28 +403,6 @@ otherwise skip the associate String creation and manipulation.
         </example>
     </rule>
 
-    <rule name="GuardLogStatementJavaUtil"
-          language="java"
-          since="5.1.0"
-          message="There is log block not surrounded by if"
-          class="net.sourceforge.pmd.lang.java.rule.bestpractices.GuardLogStatementJavaUtilRule"
-          externalInfoUrl="${pmd.website.baseurl}/pmd_rules_java_bestpractices.html#guardlogstatementjavautil">
-        <description>
-Whenever using a log level, one should check if the loglevel is actually enabled, or
-otherwise skip the associate String creation and manipulation.
-        </description>
-        <priority>2</priority>
-        <example>
- <![CDATA[
-//...
-// Add this for performance
-if (log.isLoggable(Level.FINE)) {
-    log.fine("log something" + " and " + "concat strings");
-}
-]]>
-        </example>
-    </rule>
-
     <rule name="JUnit4SuitesShouldUseSuiteAnnotation"
           language="java"
           since="4.0"

pmd-java/src/main/resources/rulesets/java/logging-jakarta-commons.xml

@@ -12,7 +12,8 @@ The Jakarta Commons Logging ruleset contains a collection of rules that find que
     <rule ref="category/java/errorprone.xml/ProperLogger" deprecated="true" />
     <rule ref="category/java/errorprone.xml/UseCorrectExceptionLogging" deprecated="true" />
 
-    <rule ref="category/java/bestpractices.xml/GuardDebugLogging" deprecated="true" />
+    <!-- GuardDebugLogging has been merged into GuardLogStatement -->
+    <rule ref="category/java/bestpractices.xml/GuardLogStatement" name="GuardDebugLogging" deprecated="true" />
     <rule ref="category/java/bestpractices.xml/GuardLogStatement" deprecated="true" />
 
 </ruleset>

pmd-java/src/main/resources/rulesets/java/logging-java.xml

@@ -15,7 +15,8 @@ The Java Logging ruleset contains a collection of rules that find questionable u
     <rule ref="category/java/errorprone.xml/MoreThanOneLogger" deprecated="true" />
 
     <rule ref="category/java/bestpractices.xml/AvoidPrintStackTrace" deprecated="true" />
-    <rule ref="category/java/bestpractices.xml/GuardLogStatementJavaUtil" deprecated="true" />
+    <!-- GuardLogStatementJavaUtil has been merged into GuardLogStatement -->
+    <rule ref="category/java/bestpractices.xml/GuardLogStatement" name="GuardLogStatementJavaUtil" deprecated="true" />
     <rule ref="category/java/bestpractices.xml/SystemPrintln" deprecated="true" />
 
 </ruleset>

pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/bestpractices/BestPracticesRulesTest.java

@@ -28,9 +28,7 @@ public class BestPracticesRulesTest extends SimpleAggregatorTst {
         addRule(RULESET, "ConstantsInInterface");
         addRule(RULESET, "DefaultLabelNotLastInSwitchStmt");
         addRule(RULESET, "ForLoopCanBeForeach");
-        addRule(RULESET, "GuardDebugLogging");
         addRule(RULESET, "GuardLogStatement");
-        addRule(RULESET, "GuardLogStatementJavaUtil");
         addRule(RULESET, "JUnit4SuitesShouldUseSuiteAnnotation");
         addRule(RULESET, "JUnit4TestShouldUseAfterAnnotation");
         addRule(RULESET, "JUnit4TestShouldUseBeforeAnnotation");

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

@@ -1,140 +0,0 @@
-<?xml version="1.0" encoding="UTF-8"?>
-<test-data
-    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, no error expected
-     ]]></description>
-        <expected-problems>0</expected-problems>
-        <code><![CDATA[
-public class Test {
-    private static final Log __log = LogFactory.getLog(Test.class);
-    public void test() {
-        // good:
-        if (__log.isDebugEnabled()) {
-        __log.debug("bla" + "",e );
-        }
-    }
-}
-     ]]></code>
-    </test-code>
-    <test-code>
-        <description><![CDATA[
-Complex logging without guard
-     ]]></description>
-        <expected-problems>2</expected-problems>
-        <code><![CDATA[
-public class Test {
-    private static final Log __log = LogFactory.getLog(Test.class);
-    public void test() {
-
-        // bad:
-        __log.debug("log something" + " and " + "concat strings");
-
-        // bad:
-        __log.debug("log something" + " and " + "concat strings", e);
-
-        // good:
-        if (__log.isDebugEnabled()) {
-        __log.debug("bla" + "",e );
-        }
-    }
-}
-     ]]></code>
-    </test-code>
-    <test-code>
-        <description><![CDATA[
-Complex logging wit misplaced guard
-     ]]></description>
-        <expected-problems>1</expected-problems>
-        <code><![CDATA[
-public class Test {
-    private static final Log __log = LogFactory.getLog(Test.class);
-    public void test() {
-        if (true) {
-            // bad:
-            __log.debug("log something" + " and " + "concat strings");
-            // this is useless:
-            __log.isDebugEnabled();
-        }
-    }
-}
-     ]]></code>
-    </test-code>
-    <test-code>
-        <description>ok #1189 GuardLogStatementRule and GuardDebugLoggingRule broken for log4j2</description>
-        <expected-problems>0</expected-problems>
-        <code><![CDATA[
-public class Test {
-    public void test() {
-        final Marker m = MarkerManager.getMarker(mymarker);
-        if (logger.isDebugEnabled(m)) {
-          logger.debug(m, message + "");
-        }
-    }
-}
-        ]]></code>
-    </test-code>
-    <test-code>
-        <description>violation - wrong guard #1189 GuardLogStatementRule and GuardDebugLoggingRule broken for log4j2</description>
-        <expected-problems>1</expected-problems>
-        <code><![CDATA[
-public class Test {
-    public void test() {
-        final Marker m = MarkerManager.getMarker(mymarker);
-        if (logger.isTraceEnabled(m)) {
-          logger.debug(m, message + "");
-        }
-    }
-}
-        ]]></code>
-    </test-code>
-    <test-code>
-        <description>violation - no if #1189 GuardLogStatementRule and GuardDebugLoggingRule broken for log4j2</description>
-        <expected-problems>1</expected-problems>
-        <code><![CDATA[
-public class Test {
-    public void test() {
-        final Marker m = MarkerManager.getMarker(mymarker);
-        logger.isDebugEnabled(m); // must be within an if
-        logger.debug(m, message + "");
-    }
-}
-        ]]></code>
-    </test-code>
-    <test-code>
-        <description>#1224 GuardDebugLogging broken in 5.1.1 - missing additive statement check in log statement</description>
-        <expected-problems>0</expected-problems>
-        <code><![CDATA[
-public class Test {
-    private static final Log __log = LogFactory.getLog(Test.class);
-    public void test() {
-        // okay:
-        __log.debug("log something");
-
-        // okay:
-        __log.debug("log something with exception", e);
-
-        // good:
-        if (__log.isDebugEnabled()) {
-        __log.debug("bla" + "",e );
-        }
-    }
-}
-        ]]></code>
-    </test-code>
-    <test-code>
-        <description>#1341 pmd:GuardDebugLogging violates LOGGER.debug with format "{}"</description>
-        <expected-problems>0</expected-problems>
-        <code><![CDATA[
-public class GuardDebugFalsePositive {
-    public void test() {
-        String tempSelector = "a";
-        LOGGER.debug("MessageSelector={}" , tempSelector);
-    }
-}
-        ]]></code>
-    </test-code>
-</test-data>

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

@@ -4,9 +4,7 @@
     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[
+        <description>OK, guard is here - log4j</description>
-OK, guard is here
-     ]]></description>
         <expected-problems>0</expected-problems>
         <code><![CDATA[
 public class Foo {
@@ -20,9 +18,36 @@ public class Foo {
      ]]></code>
     </test-code>
     <test-code>
-        <description><![CDATA[
+        <description>ok, no error expected - apache commons logging</description>
-KO, missing guard 1
+        <expected-problems>0</expected-problems>
-     ]]></description>
+        <code><![CDATA[
+public class Test {
+    private static final Log __log = LogFactory.getLog(Test.class);
+    public void test() {
+        // good:
+        if (__log.isDebugEnabled()) {
+        __log.debug("bla" + "",e );
+        }
+    }
+}
+     ]]></code>
+    </test-code>
+    <test-code>
+        <description>Guarded call - OK - java util</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+public class Foo {
+
+ private void foo(Logger logger) { 
+    if ( logger.isLoggable(Level.FINE) ) {
+        logger.fine("debug message" + ""); 
+    } 
+ }  
+}
+     ]]></code>
+    </test-code>
+    <test-code>
+        <description>KO, missing guard 1 - log4j</description>
         <expected-problems>1</expected-problems>
         <code><![CDATA[
 public class Foo {
@@ -35,9 +60,7 @@ public class Foo {
      ]]></code>
     </test-code>
     <test-code>
-        <description><![CDATA[
+        <description>KO, missing guard 2 - log4j</description>
-KO, missing guard 2
-        ]]></description>
         <rule-property name="logLevels">debug,trace</rule-property>
         <rule-property name="guardsMethods">isDebugEnabled,isTraceEnabled</rule-property>
         <expected-problems>1</expected-problems>
@@ -56,6 +79,57 @@ public class Foo {
 }
        ]]></code>
     </test-code>
+    <test-code>
+        <description>Complex logging without guard - apache commons logging</description>
+        <expected-problems>2</expected-problems>
+        <code><![CDATA[
+public class Test {
+    private static final Log __log = LogFactory.getLog(Test.class);
+    public void test() {
+
+        // bad:
+        __log.debug("log something" + " and " + "concat strings");
+
+        // bad:
+        __log.debug("log something" + " and " + "concat strings", e);
+
+        // good:
+        if (__log.isDebugEnabled()) {
+        __log.debug("bla" + "",e );
+        }
+    }
+}
+     ]]></code>
+    </test-code>
+    <test-code>
+        <description>Complex logging with misplaced guard - apache commons logging</description>
+        <expected-problems>1</expected-problems>
+        <code><![CDATA[
+public class Test {
+    private static final Log __log = LogFactory.getLog(Test.class);
+    public void test() {
+        if (true) {
+            // bad:
+            __log.debug("log something" + " and " + "concat strings");
+            // this is useless:
+            __log.isDebugEnabled();
+        }
+    }
+}
+     ]]></code>
+    </test-code>
+    <test-code>
+        <description>Unguarded call - KO - java util</description>
+        <expected-problems>1</expected-problems>
+        <code><![CDATA[
+public class Foo {
+
+ private void foo(Logger logger) { 
+    logger.fine("debug message" + ""); 
+ }  
+}
+     ]]></code>
+    </test-code>
     <test-code>
         <description>ok #1189 GuardLogStatementRule and GuardDebugLoggingRule broken for log4j2</description>
         <expected-problems>0</expected-problems>
@@ -90,4 +164,120 @@ public class Test {
 }
         ]]></code>
     </test-code>
+    <test-code>
+        <description>violation - no if #1189 GuardLogStatementRule and GuardDebugLoggingRule broken for log4j2</description>
+        <expected-problems>1</expected-problems>
+        <code><![CDATA[
+public class Test {
+    public void test() {
+        final Marker m = MarkerManager.getMarker(mymarker);
+        logger.isDebugEnabled(m); // must be within an if
+        logger.debug(m, message + "");
+    }
+}
+        ]]></code>
+    </test-code>
+    <test-code>
+        <description>#1224 GuardDebugLogging broken in 5.1.1 - missing additive statement check in log statement - apache commons logging</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+public class Test {
+    private static final Log __log = LogFactory.getLog(Test.class);
+    public void test() {
+        // okay:
+        __log.debug("log something");
+
+        // okay:
+        __log.debug("log something with exception", e);
+
+        // good:
+        if (__log.isDebugEnabled()) {
+        __log.debug("bla" + "",e );
+        }
+    }
+}
+        ]]></code>
+    </test-code>
+    <test-code>
+        <description>#1341 pmd:GuardDebugLogging violates LOGGER.debug with format "{}" - slf4j</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+public class GuardDebugFalsePositive {
+    public void test() {
+        String tempSelector = "a";
+        LOGGER.debug("MessageSelector={}" , tempSelector);
+    }
+}
+        ]]></code>
+    </test-code>
+    <test-code>
+        <description>#1203 GuardLogStatementJavaUtil issues warning for severe level not being specified as property</description>
+        <rule-property name="logLevels">finest,finer,fine,info</rule-property>
+        <rule-property name="guardsMethods">isLoggable</rule-property>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+import java.util.logging.Logger;
+import java.util.logging.Level;
+
+public class Foo {
+    Logger LOGGER;
+
+    public void foo() {
+        LOGGER.severe("This is a severe message" + " and concat");
+    }
+}
+        ]]></code>
+    </test-code>
+    <test-code>
+        <description>#1227 GuardLogStatementJavaUtil doesn't catch log(Level.FINE, "msg" + " msg") calls</description>
+        <expected-problems>1</expected-problems>
+        <expected-linenumbers>8</expected-linenumbers>
+        <code><![CDATA[
+import java.util.logging.Logger;
+import java.util.logging.Level;
+
+public class Foo {
+    Logger LOGGER;
+
+    public void foo() {
+        LOGGER.log(Level.FINE, "This is a severe message" + " and concat"); // violation
+        if (LOGGER.isLoggable(Level.FINE)) {
+            LOGGER.log(Level.FINE, "This is a severe message" + " and concat"); // no violation
+        }
+    }
+}
+        ]]></code>
+    </test-code>
+    <test-code>
+        <description>#1347 False positive for GuardLogStatementJavaUtil with slf4j</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+import org.slf4j.Logger;
+
+public class GuardLogTest {
+    Logger LOG;
+    public void foo() {
+        if (LOG.isInfoEnabled()) {
+            LOG.info("update: After spool map size: " + map.size());
+        }
+    }
+}
+        ]]></code>
+    </test-code>
+    <test-code>
+        <description>#1398 False positive for GuardLogStatementJavaUtil with Log4j</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+import org.apache.log4j.Logger;
+
+public class GuardLogTest {
+    Logger LOG;
+    public void foo() {
+        if (LOG.isInfoEnabled()) {
+            LOG.info("update: After spool map size: " + map.size());
+        }
+    }
+}
+]]></code>
+    </test-code>
 </test-data>

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

@@ -1,122 +0,0 @@
-<?xml version="1.0" encoding="UTF-8"?>
-<test-data
-    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[
-Guarded call - OK
-     ]]></description>
-        <expected-problems>0</expected-problems>
-        <code><![CDATA[
-public class Foo {
-
- private void foo(Logger logger) { 
- 	if ( logger.isLoggable(Level.FINE) ) {
- 		logger.fine("debug message" + ""); 
- 	} 
- }  
-}
-     ]]></code>
-    </test-code>
-        <test-code>
-        <description><![CDATA[
-Unguarded call - KO
-     ]]></description>
-        <expected-problems>1</expected-problems>
-        <code><![CDATA[
-public class Foo {
-
- private void foo(Logger logger) { 
- 	logger.fine("debug message" + ""); 
- }  
-}
-     ]]></code>
-    </test-code>
-    <test-code>
-        <description>#1203 GuardLogStatementJavaUtil issues warning for severe level not being specified as property</description>
-        <rule-property name="logLevels">finest,finer,fine,info</rule-property>
-        <rule-property name="guardsMethods">isLoggable</rule-property>
-        <expected-problems>0</expected-problems>
-        <code><![CDATA[
-import java.util.logging.Logger;
-import java.util.logging.Level;
-
-public class Foo {
-    Logger LOGGER;
-
-    public void foo() {
-        LOGGER.severe("This is a severe message" + " and concat");
-    }
-}
-        ]]></code>
-    </test-code>
-    <test-code>
-        <description>#1227 GuardLogStatementJavaUtil doesn't catch log(Level.FINE, "msg" + " msg") calls</description>
-        <expected-problems>1</expected-problems>
-        <expected-linenumbers>8</expected-linenumbers>
-        <code><![CDATA[
-import java.util.logging.Logger;
-import java.util.logging.Level;
-
-public class Foo {
-    Logger LOGGER;
-
-    public void foo() {
-        LOGGER.log(Level.FINE, "This is a severe message" + " and concat"); // violation
-        if (LOGGER.isLoggable(Level.FINE)) {
-            LOGGER.log(Level.FINE, "This is a severe message" + " and concat"); // no violation
-        }
-    }
-}
-        ]]></code>
-    </test-code>
-    <test-code>
-        <description>#1335 GuardLogStatementJavaUtil should not apply to SLF4J Logger</description>
-        <expected-problems>0</expected-problems>
-        <code><![CDATA[
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
-
-public class GuardLogTest {
-    Logger LOGGER = LoggerFactory.getLogger(getClass());
-
-    public void foo() {
-        LOGGER.info("foo" + "bar");
-    }
-}
-        ]]></code>
-    </test-code>
-    <test-code>
-        <description>#1347 False positive for GuardLogStatementJavaUtil</description>
-        <expected-problems>0</expected-problems>
-        <code><![CDATA[
-import org.slf4j.Logger;
-
-public class GuardLogTest {
-    Logger LOG;
-    public void foo() {
-        if (LOG.isInfoEnabled()) {
-            LOG.info("update: After spool map size: " + map.size());
-        }
-    }
-}
-        ]]></code>
-    </test-code>
-    <test-code>
-        <description>#1398 False positive for GuardLogStatementJavaUtil with Log4j</description>
-        <expected-problems>0</expected-problems>
-        <code><![CDATA[
-import org.apache.log4j.Logger;
-
-public class GuardLogTest {
-    Logger LOG;
-    public void foo() {
-        if (LOG.isInfoEnabled()) {
-            LOG.info("update: After spool map size: " + map.size());
-        }
-    }
-}
-]]></code>
-    </test-code>
-</test-data>