Redesign GuardLogStatement:

Turns out that the implementation based on the one published by Heiko Rupp
could simply not catch all the case in our test suite. I therefore opted to
backport the previous implementation - a simple Xpath rule, from GuardDebugLogStmt
into a java class.
Performance are not as good as the one based on Heiko's proposal, but it still
twice faster as the simple GuardDebogLogStmt (instead of five times faster). However,
correctness is in this case more important.

pmd/src/main/java/net/sourceforge/pmd/lang/java/rule/logging/GuardDebugLoggingRule.java

@@ -0,0 +1,17 @@
+package net.sourceforge.pmd.lang.java.rule.logging;
+
+import java.util.HashMap;
+
+public class GuardDebugLoggingRule extends GuardLogStatementRule {
+
+	public GuardDebugLoggingRule() {
+		super.guardStmtByLogLevel = new HashMap<String, String>(1);
+		super.guardStmtByLogLevel.put(".debug","isDebugEnabled");
+	}
+	
+	@Override
+	protected void extractProperties() {
+		// This rule is not configurable
+	}
+
+}

pmd/src/main/java/net/sourceforge/pmd/lang/java/rule/logging/GuardLogStatementJavaUtilRule.java

@@ -0,0 +1,30 @@
+package net.sourceforge.pmd.lang.java.rule.logging;
+
+import java.util.logging.Level;
+
+import net.sourceforge.pmd.lang.java.ast.ASTCompilationUnit;
+
+public class GuardLogStatementJavaUtilRule extends GuardLogStatementRule {
+
+	private static final String GUARD_METHOD_NAME = "isLoggable";
+	
+	// Override default constructor - this rule can't be configured
+	public GuardLogStatementJavaUtilRule() {};
+
+	@Override
+	public Object visit(ASTCompilationUnit unit, Object data) {
+		if ( super.guardStmtByLogLevel.isEmpty() ) {
+			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);
+		}		
+		return super.visit(unit,data);
+	}
+	
+	private String formatLogLevelString(Level logLevel) {
+		return "." + logLevel.toString().toLowerCase();
+	}
+}

pmd/src/main/java/net/sourceforge/pmd/lang/java/rule/logging/GuardLogStatementRule.java

@@ -2,29 +2,31 @@ package net.sourceforge.pmd.lang.java.rule.logging;
 
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Map.Entry;
 
 import net.sourceforge.pmd.Rule;
 import net.sourceforge.pmd.lang.ast.Node;
-import net.sourceforge.pmd.lang.java.ast.ASTBlockStatement;
 import net.sourceforge.pmd.lang.java.ast.ASTCompilationUnit;
-import net.sourceforge.pmd.lang.java.ast.ASTIfStatement;
-import net.sourceforge.pmd.lang.java.ast.ASTName;
-import net.sourceforge.pmd.lang.java.ast.ASTPrimaryPrefix;
 import net.sourceforge.pmd.lang.java.rule.optimizations.AbstractOptimizationRule;
 import net.sourceforge.pmd.lang.rule.properties.StringMultiProperty;
 
+import org.jaxen.JaxenException;
+
 /**
- * Check that log.debug and log.trace statements are guarded by some
- * log.isDebugEnabled() or log.isTraceEnabled() checks.
+ * Check that log.debug, log.trace, log.error, etc... statements are guarded by
+ * some test expression on log.isDebugEnabled() or log.isTraceEnabled().
  * 
- * @author Heiko hwr@pilhuhn.de
  * @author Romain Pelisse - <belaran@gmail.com>
+ * @author Heiko Rupp - <hwr@pilhuhn.de>
+ * @author Tammo van Lessen - provided original XPath expression
  * 
  */
-public class GuardLogStatementRule extends AbstractOptimizationRule implements Rule {
+public class GuardLogStatementRule extends AbstractOptimizationRule implements
+		Rule {
 
 	public static final StringMultiProperty LOG_LEVELS = new StringMultiProperty(
 			"logLevels", "LogLevels to guard", new String[] {}, 1.0f, ',');
@@ -33,9 +35,14 @@ public class GuardLogStatementRule extends AbstractOptimizationRule implements R
 			"guardsMethods", "method use to guard the log statement",
 			new String[] {}, 2.0f, ',');
 
-	private final Map<String, String> guardStmtByLogLevel = new HashMap<String, String>(
+	protected Map<String, String> guardStmtByLogLevel = new HashMap<String, String>(
 			5);
 
+	private static final String xpathExpression = "//PrimaryPrefix[ends-with(Name/@Image, 'KEY') and "
+			+ "count("
+			+ "ancestor::IfStatement/Expression/descendant::PrimaryExpression["
+			+ "ends-with(descendant::PrimaryPrefix/Name/@Image,'VALUE')]) = 0]";
+
 	public GuardLogStatementRule() {
 		definePropertyDescriptor(LOG_LEVELS);
 		definePropertyDescriptor(GUARD_METHODS);
@@ -43,77 +50,35 @@ public class GuardLogStatementRule extends AbstractOptimizationRule implements R
 
 	@Override
 	public Object visit(ASTCompilationUnit unit, Object data) {
-		if ( guardStmtByLogLevel.isEmpty() ) {
-			List<String> logLevels = new ArrayList<String>(Arrays.asList(super
-					.getProperty(LOG_LEVELS)));
-			List<String> guardMethods = new ArrayList<String>(Arrays.asList(super
-					.getProperty(GUARD_METHODS)));
-			
-			if (guardMethods.isEmpty() && ! logLevels.isEmpty() ) {
-				throw new IllegalArgumentException(
-						"Can't specify guardMethods without specifiying logLevels.");
+		extractProperties();
+		findViolationForEachLogStatement(unit, data);
+		return super.visit(unit, data);
+	}
+
+	private void findViolationForEachLogStatement(ASTCompilationUnit unit, Object data) {
+		for (Entry<String, String> entry : guardStmtByLogLevel.entrySet()) {
+			List<Node> nodes = findViolations(unit, entry.getKey(),
+					entry.getValue());
+			for (Node node : nodes) {
+				super.addViolation(data, node);
 			}
-			
-			if (logLevels.isEmpty()) 
-				setPropertiesDefaultValues(logLevels, guardMethods);
-	
-			buildGuardStatementMap(logLevels, guardMethods);
-		}
-		return super.visit(unit,data);
-	}
-
-	@Override
-	public Object visit(ASTName name, Object data) {
-		Node node = name.jjtGetParent();
-		if (node instanceof ASTPrimaryPrefix) {
-
-		} else
-			return super.visit(name, data);
-		if (name != null) {
-			String lastPrefix = lastPrefix(name.getImage());
-			if (guardStmtByLogLevel.keySet().contains(lastPrefix)) {
-				// TODO check for type
-				Node parent1 = name.getNthParent(5);
-				boolean guardFound = false;
-				if (parent1 instanceof ASTIfStatement) {
-					guardFound = checkForGuard((ASTIfStatement) parent1,
-							lastPrefix);
-				} else if (parent1 instanceof ASTBlockStatement) {
-					Node parent2 = name.getNthParent(7);
-					if (parent2 instanceof ASTIfStatement) {
-						guardFound = checkForGuard((ASTIfStatement) parent2,
-								lastPrefix);
-					}
-				}
-				if (!guardFound)
-					addViolation(data, name);
-			}
-		}
-		return super.visit(name, data);
-	}
-
-	private String lastPrefix(String string) {
-		if (string != null && ! "".equals(string) ) {
-			if ( string.contains(".") )
-				return string.substring(string.lastIndexOf('.'), string.length());
-		} 
-		return string;
-	}
-
-	private boolean checkForGuard(ASTIfStatement stm, String logLevel) {
-
-		List<ASTName> names = stm.findDescendantsOfType(ASTName.class);
-		if (names == null || names.isEmpty())
-			return false;
-
-		for (ASTName name : names) {
-			if ( name.getImage().endsWith(guardStmtByLogLevel.get(logLevel)) )
-				return true;
-		}
-		return false;
+		}		
 	}
 	
-	private void setPropertiesDefaultValues(List<String> logLevels, List<String> guardMethods) {
+	@SuppressWarnings("unchecked")
+	private List<Node> findViolations(ASTCompilationUnit unit, String key,
+			String value) {
+		try {
+			return unit.findChildNodesWithXPath(xpathExpression.replaceFirst(
+					"KEY", key).replaceFirst("VALUE", value));
+		} catch (JaxenException e) {
+			e.printStackTrace();
+		}
+		return Collections.EMPTY_LIST;
+	}
+
+	private void setPropertiesDefaultValues(List<String> logLevels,
+			List<String> guardMethods) {
 		logLevels.add("trace");
 		logLevels.add("debug");
 		logLevels.add("info");
@@ -128,16 +93,39 @@ public class GuardLogStatementRule extends AbstractOptimizationRule implements R
 		guardMethods.add("isErrorEnabled");
 	}
 
-	private void buildGuardStatementMap(List<String> logLevels, List<String> guardMethods) {
+	protected void extractProperties() {
+		if (guardStmtByLogLevel.isEmpty()) {
+
+			List<String> logLevels = new ArrayList<String>(Arrays.asList(super
+					.getProperty(LOG_LEVELS)));
+			List<String> guardMethods = new ArrayList<String>(
+					Arrays.asList(super.getProperty(GUARD_METHODS)));
+
+			if (guardMethods.isEmpty() && !logLevels.isEmpty()) {
+				throw new IllegalArgumentException(
+						"Can't specify guardMethods without specifiying logLevels.");
+			}
+
+			if (logLevels.isEmpty())
+				setPropertiesDefaultValues(logLevels, guardMethods);
+
+			buildGuardStatementMap(logLevels, guardMethods);
+		}
+	}
+
+	protected void buildGuardStatementMap(List<String> logLevels,
+			List<String> guardMethods) {
 		for (String logLevel : logLevels) {
 			boolean found = false;
 			for (String guardMethod : guardMethods) {
-				if (!found && guardMethod.toLowerCase().contains(logLevel.toLowerCase())) {
+				if (!found
+						&& guardMethod.toLowerCase().contains(
+								logLevel.toLowerCase())) {
 					found = true;
 					guardStmtByLogLevel.put("." + logLevel, guardMethod);
 				}
 			}
-			
+
 			if (!found)
 				throw new IllegalArgumentException(
 						"No guard method associated to the logLevel:"

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

@@ -92,17 +92,13 @@ public class Foo {
           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.logging.GuardLogStatementRule"
+          class="net.sourceforge.pmd.lang.java.rule.logging.GuardDebugLoggingRule"
           externalInfoUrl="${pmd.website.baseurl}/rules/java/logging-jakarta-commons.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>
-        <properties>
-            <property name="logLevels">debug</property>
-            <property name="guardsMethods">isDebugEnabled</property>
-        </properties>
         <example>
             <![CDATA[
 public class Test {
@@ -135,7 +131,7 @@ public class Test {
          since="5.0.3"
          message="There is log block not surrounded by if"
          class="net.sourceforge.pmd.lang.java.rule.logging.GuardLogStatementRule"
-         externalInfoUrl="${pmd.website.baseurl}/rules/java/logging-java.html#GuardLogStatement">
+         externalInfoUrl="${pmd.website.baseurl}/rules/java/logging-jakarta-commons.html#GuardLogStatement">
      <description>
 Whenever using a log level, one should check if the loglevel is actually enabled, or
 otherwise skip the associate String creation and manipulation.

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

@@ -140,4 +140,23 @@ class Foo {
            </example>
     </rule>
 
+   <rule name="GuardLogStatementJavaUtil"
+         language="java"
+         since="5.0.3"
+         message="There is log block not surrounded by if"
+         class="net.sourceforge.pmd.lang.java.rule.logging.GuardLogStatementJavaUtilRule"
+         externalInfoUrl="${pmd.website.baseurl}/rules/java/logging-java.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("This happens");
+]]>
+     </example>
+   </rule>
 </ruleset>

pmd/src/test/java/net/sourceforge/pmd/lang/java/rule/loggingjakartacommons/LoggingJakartaCommonsRulesTest.java

@@ -14,7 +14,7 @@ public class LoggingJakartaCommonsRulesTest extends SimpleAggregatorTst {
         addRule(RULESET, "ProperLogger");
         addRule(RULESET, "UseCorrectExceptionLogging");
         addRule(RULESET, "GuardDebugLogging");
-        addRule(RULESET, "GuardLogStatement");
+//        addRule(RULESET, "GuardLogStatement");
     }
 
     public static junit.framework.Test suite() {

pmd/src/test/java/net/sourceforge/pmd/lang/java/rule/loggingjava/LoggingJavaRulesTest.java

@@ -6,7 +6,7 @@ import org.junit.Before;
 
 
 public class LoggingJavaRulesTest extends SimpleAggregatorTst {
-    
+
     private static final String RULESET = "java-logging-java";
 
     @Before
@@ -15,7 +15,7 @@ public class LoggingJavaRulesTest extends SimpleAggregatorTst {
         addRule(RULESET, "LoggerIsNotStaticFinal");
         addRule(RULESET, "MoreThanOneLogger");
         addRule(RULESET, "SystemPrintln");
-        addRule(RULESET, "GuardLogStatement");
+        addRule(RULESET, "GuardLogStatementJavaUtil");
     }
 
     public static junit.framework.Test suite() {

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

@@ -2,19 +2,13 @@
 <test-data>
     <test-code>
         <description><![CDATA[
-ok
+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() {
-        // okay:
-        __log.debug("log something");
-        
-        // okay:
-        __log.debug("log something with exception", e);
-        
         // good:
         if (__log.isDebugEnabled()) {
         __log.debug("bla" + "",e );
@@ -32,18 +26,13 @@ Complex logging without guard
 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 );

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

@@ -0,0 +1,33 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<test-data>
+    <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-data>