Fixing bug : False -: DoubleCheckedLocking with reversed null check - ID: 2835074

git-svn-id: https://pmd.svn.sourceforge.net/svnroot/pmd/branches/pmd/4.2.x@6976 51baf565-9d33-0410-a72c-fc3788e3496d

pmd/etc/changelog.txt

@@ -5,6 +5,7 @@ Fixed bug 2317099 - False + in SimplifyCondition
 Fixed bug 2606609 - False "UnusedImports" positive in package-info.java
 Fixed bug 2645268 - ClassCastException in UselessOperationOnImmutable.getDeclaration
 Fixed bug 2724653 - AvoidThreadGroup reports false positives
+Fixed bug 2835074 - False -: DoubleCheckedLocking with reversed null check  	 
 Correct -benchmark reporting of Rule visits via the RuleChain
 Fix issue with Type Resolution incorrectly handling of Classes with same name as a java.lang Class.
 

pmd/regress/test/net/sourceforge/pmd/rules/basic/xml/DoubleCheckedLocking.xml

@@ -55,4 +55,48 @@ public class Foo {
      ]]></code>
         <source-type>java 1.5</source-type>
     </test-code>
+    <test-code>
+        <description><![CDATA[
+inversed null check -see bug 2835074  	 False -: DoubleCheckedLocking with reversed null check
+2835074  	 False -: DoubleCheckedLocking with reversed null check
+     ]]></description>
+        <expected-problems>1</expected-problems>
+        <code><![CDATA[
+public class Foo {
+      Object baz;
+      Object bar() {
+        if(null == baz) { //baz may be non-null yet not fully created
+          synchronized(this){
+            if(baz == null){
+              baz = new Object();
+            }
+          }
+        }
+        return baz;
+      }
+}
+     ]]></code>
+    </test-code>
+    <test-code>
+        <description><![CDATA[
+inversed null check -see bug 2835074  	 False -: DoubleCheckedLocking with reversed null check
+2835074  	 False -: DoubleCheckedLocking with reversed null check
+     ]]></description>
+        <expected-problems>1</expected-problems>
+        <code><![CDATA[
+public class Foo {
+      Object baz;
+      Object bar() {
+        if(baz == null) { //baz may be non-null yet not fully created
+          synchronized(this){
+            if(null == baz){
+              baz = new Object();
+            }
+          }
+        }
+        return baz;
+      }
+}
+     ]]></code>
+    </test-code>    
 </test-data>

pmd/src/net/sourceforge/pmd/rules/DoubleCheckedLocking.java

@@ -3,6 +3,7 @@
  */
 package net.sourceforge.pmd.rules;
 
+import net.sourceforge.pmd.AbstractJavaRule;
 import net.sourceforge.pmd.ast.ASTAssignmentOperator;
 import net.sourceforge.pmd.ast.ASTClassOrInterfaceDeclaration;
 import net.sourceforge.pmd.ast.ASTIfStatement;
@@ -41,7 +42,7 @@ import java.util.List;
  *
  * @author CL Gilbert (dnoyeb@users.sourceforge.net)
  */
-public class DoubleCheckedLocking extends net.sourceforge.pmd.AbstractRule {
+public class DoubleCheckedLocking extends AbstractJavaRule {
 
     public Object visit(ASTClassOrInterfaceDeclaration node, Object data) {
         if (node.isInterface()) {
@@ -119,12 +120,11 @@ public class DoubleCheckedLocking extends net.sourceforge.pmd.AbstractRule {
     private boolean ifVerify(ASTIfStatement is, String varname) {
         List<ASTPrimaryExpression> finder = new ArrayList<ASTPrimaryExpression>();
         is.findChildrenOfType(ASTPrimaryExpression.class, finder, true);
-        if (finder.size() > 1) {
+        if (finder.size() > 1) { 
-            ASTPrimaryExpression apeLeft = finder.get(0);
+            ASTPrimaryExpression nullStmt = findNonVariableStmt(varname,finder.get(0),finder.get(1));
-            if (matchName(apeLeft, varname)) {
+            if ( nullStmt != null ) {
-                ASTPrimaryExpression apeRight = finder.get(1);
+                if ((nullStmt.jjtGetNumChildren() == 1) && (nullStmt.jjtGetChild(0) instanceof ASTPrimaryPrefix)) {
-                if ((apeRight.jjtGetNumChildren() == 1) && (apeRight.jjtGetChild(0) instanceof ASTPrimaryPrefix)) {
+                    ASTPrimaryPrefix pp2 = (ASTPrimaryPrefix) nullStmt.jjtGetChild(0);
-                    ASTPrimaryPrefix pp2 = (ASTPrimaryPrefix) apeRight.jjtGetChild(0);
                     if ((pp2.jjtGetNumChildren() == 1) && (pp2.jjtGetChild(0) instanceof ASTLiteral)) {
                         ASTLiteral lit = (ASTLiteral) pp2.jjtGetChild(0);
                         if ((lit.jjtGetNumChildren() == 1) && (lit.jjtGetChild(0) instanceof ASTNullLiteral)) {
@@ -137,7 +137,26 @@ public class DoubleCheckedLocking extends net.sourceforge.pmd.AbstractRule {
         return false;
     }
 
-    private boolean matchName(ASTPrimaryExpression ape, String name) {
+    /** 
+     * <p>Sort out if apeLeft or apeRight are variable with the provided 'variableName'.</p>
+     * 
+     * @param variableName
+     * @param apeLeft
+     * @param apeRight
+     * @return reference from either apeLeft or apeRight, if one of them match, or 'null', if none match.
+     */
+	private ASTPrimaryExpression findNonVariableStmt(String variableName,
+			ASTPrimaryExpression apeLeft, ASTPrimaryExpression apeRight) {
+    	if (matchName(apeLeft, variableName) ) {
+    		return apeRight;
+    	}
+    	else if (matchName(apeRight, variableName) ) {
+    		return apeLeft;
+    	}
+		return null;
+	}
+
+	private boolean matchName(ASTPrimaryExpression ape, String name) {
         if ((ape.jjtGetNumChildren() == 1) && (ape.jjtGetChild(0) instanceof ASTPrimaryPrefix)) {
             ASTPrimaryPrefix pp = (ASTPrimaryPrefix) ape.jjtGetChild(0);
             String name2 = getNameFromPrimaryPrefix(pp);