Fixing bug 2826119 - False +: DoubleCheckedLocking warning with volatile field

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

pmd/etc/changelog.txt

@@ -6,6 +6,8 @@ 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  	 
+Fixed bug 2826119 - False +: DoubleCheckedLocking warning with volatile field
+
 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

@@ -57,8 +57,8 @@ public class Foo {
     </test-code>
     <test-code>
         <description><![CDATA[
-inversed null check -see bug 2835074  	 False -: DoubleCheckedLocking with reversed null check
+inversed null check
-2835074  	 False -: DoubleCheckedLocking with reversed null check
+see bug 2835074 False -: DoubleCheckedLocking with reversed null check (1)
      ]]></description>
         <expected-problems>1</expected-problems>
         <code><![CDATA[
@@ -79,8 +79,8 @@ public class Foo {
     </test-code>
     <test-code>
         <description><![CDATA[
-inversed null check -see bug 2835074  	 False -: DoubleCheckedLocking with reversed null check
+inversed null check
-2835074  	 False -: DoubleCheckedLocking with reversed null check
+see bug 2835074 False -: DoubleCheckedLocking with reversed null check (2)
      ]]></description>
         <expected-problems>1</expected-problems>
         <code><![CDATA[
@@ -99,4 +99,26 @@ public class Foo {
 }
      ]]></code>
     </test-code>    
+    <test-code>
+        <description><![CDATA[
+Use of volatile keyword
+see bug 2826119 - False +: DoubleCheckedLocking warning with volatile field
+     ]]></description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+public class Foo {
+      volatile 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,9 +3,14 @@
  */
 package net.sourceforge.pmd.rules;
 
+import java.util.ArrayList;
+import java.util.List;
+
 import net.sourceforge.pmd.AbstractJavaRule;
 import net.sourceforge.pmd.ast.ASTAssignmentOperator;
 import net.sourceforge.pmd.ast.ASTClassOrInterfaceDeclaration;
+import net.sourceforge.pmd.ast.ASTCompilationUnit;
+import net.sourceforge.pmd.ast.ASTFieldDeclaration;
 import net.sourceforge.pmd.ast.ASTIfStatement;
 import net.sourceforge.pmd.ast.ASTLiteral;
 import net.sourceforge.pmd.ast.ASTMethodDeclaration;
@@ -18,11 +23,9 @@ import net.sourceforge.pmd.ast.ASTReturnStatement;
 import net.sourceforge.pmd.ast.ASTStatementExpression;
 import net.sourceforge.pmd.ast.ASTSynchronizedStatement;
 import net.sourceforge.pmd.ast.ASTType;
+import net.sourceforge.pmd.ast.ASTVariableDeclaratorId;
 import net.sourceforge.pmd.ast.Node;
 
-import java.util.ArrayList;
-import java.util.List;
-
 /**
  * void method() {
  * if(x == null) {
@@ -44,12 +47,33 @@ import java.util.List;
  */
 public class DoubleCheckedLocking extends AbstractJavaRule {
 
+    List<String> volatileFields;
+
+    public Object visit(ASTCompilationUnit compilationUnit, Object data) {
+        if ( volatileFields == null ) {
+            volatileFields = new ArrayList<String>(0);
+        } else {
+            volatileFields.clear();
+        }
+        return super.visit(compilationUnit, data);
+    }
+
     public Object visit(ASTClassOrInterfaceDeclaration node, Object data) {
         if (node.isInterface()) {
             return data;
         }
         return super.visit(node, data);
     }
+ 
+    public Object visit(ASTFieldDeclaration fieldDeclaration, Object data) {
+        if ( fieldDeclaration.isVolatile() ) {
+        	List<ASTVariableDeclaratorId> declarators = fieldDeclaration.findChildrenOfType(ASTVariableDeclaratorId.class);
+        	for (ASTVariableDeclaratorId declarator : declarators ) {
+                this.volatileFields.add(declarator.getImage());
+        	}
+        }
+        return super.visit(fieldDeclaration, data);
+    }
 
     public Object visit(ASTMethodDeclaration node, Object data) {
         if (node.getResultType().isVoid()) {
@@ -76,7 +100,8 @@ public class DoubleCheckedLocking extends AbstractJavaRule {
         if (lastChild instanceof ASTPrimaryPrefix) {
             returnVariableName = getNameFromPrimaryPrefix((ASTPrimaryPrefix) lastChild);
         }
-        if (returnVariableName == null) {
+        // With Java5 and volatile keyword, DCL is no longer an issue
+        if (returnVariableName == null || this.volatileFields.contains(returnVariableName)) {
             return super.visit(node, data);
         }
         List<ASTIfStatement> isl = new ArrayList<ASTIfStatement>();