Merge branch 'bug-1471' into pmd/5.4.x

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/basic/DoubleCheckedLockingRule.java

@@ -10,9 +10,12 @@
 import net.sourceforge.pmd.lang.java.ast.ASTAssignmentOperator;
 import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceDeclaration;
 import net.sourceforge.pmd.lang.java.ast.ASTCompilationUnit;
+import net.sourceforge.pmd.lang.java.ast.ASTEqualityExpression;
+import net.sourceforge.pmd.lang.java.ast.ASTExpression;
 import net.sourceforge.pmd.lang.java.ast.ASTFieldDeclaration;
 import net.sourceforge.pmd.lang.java.ast.ASTIfStatement;
 import net.sourceforge.pmd.lang.java.ast.ASTLiteral;
+import net.sourceforge.pmd.lang.java.ast.ASTLocalVariableDeclaration;
 import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclaration;
 import net.sourceforge.pmd.lang.java.ast.ASTName;
 import net.sourceforge.pmd.lang.java.ast.ASTNullLiteral;
@@ -24,6 +27,7 @@
 import net.sourceforge.pmd.lang.java.ast.ASTSynchronizedStatement;
 import net.sourceforge.pmd.lang.java.ast.ASTType;
 import net.sourceforge.pmd.lang.java.ast.ASTVariableDeclaratorId;
+import net.sourceforge.pmd.lang.java.ast.ASTVariableInitializer;
 import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule;
 
 /**
@@ -106,6 +110,10 @@ public Object visit(ASTMethodDeclaration node, Object data) {
         if (returnVariableName == null || this.volatileFields.contains(returnVariableName)) {
             return super.visit(node, data);
         }
+        // if the return variable is local and only written with the volatile field, then it's ok, too
+        if (checkLocalVariableUsage(node, returnVariableName)) {
+            return super.visit(node, data);
+        }
         List<ASTIfStatement> isl = node.findDescendantsOfType(ASTIfStatement.class);
         if (isl.size() == 2) {
             ASTIfStatement is = isl.get(0);
@@ -142,6 +150,56 @@ public Object visit(ASTMethodDeclaration node, Object data) {
         return super.visit(node, data);
     }
 
+    private boolean checkLocalVariableUsage(ASTMethodDeclaration node, String returnVariableName) {
+        List<ASTLocalVariableDeclaration> locals = node.findDescendantsOfType(ASTLocalVariableDeclaration.class);
+        ASTVariableInitializer initializer = null;
+        for (ASTLocalVariableDeclaration l : locals) {
+            ASTVariableDeclaratorId id = l.getFirstDescendantOfType(ASTVariableDeclaratorId.class);
+            if (id != null && id.hasImageEqualTo(returnVariableName)) {
+                initializer = l.getFirstDescendantOfType(ASTVariableInitializer.class);
+                break;
+            }
+        }
+        // the return variable name doesn't seem to be a local variable
+        if (initializer == null) return false;
+
+        // verify the value with which the local variable is initialized
+        if (initializer.jjtGetNumChildren() > 0 && initializer.jjtGetChild(0) instanceof ASTExpression
+                && initializer.jjtGetChild(0).jjtGetNumChildren() > 0
+                && initializer.jjtGetChild(0).jjtGetChild(0) instanceof ASTPrimaryExpression
+                && initializer.jjtGetChild(0).jjtGetChild(0).jjtGetNumChildren() > 0
+                && initializer.jjtGetChild(0).jjtGetChild(0).jjtGetChild(0) instanceof ASTPrimaryPrefix
+                && initializer.jjtGetChild(0).jjtGetChild(0).jjtGetChild(0).jjtGetNumChildren() > 0
+                && initializer.jjtGetChild(0).jjtGetChild(0).jjtGetChild(0).jjtGetChild(0) instanceof ASTName) {
+            ASTName name = (ASTName)initializer.jjtGetChild(0).jjtGetChild(0).jjtGetChild(0).jjtGetChild(0);
+            if (name == null || !volatileFields.contains(name.getImage())) {
+                return false;
+            }
+        } else {
+            // not a simple assignment
+            return false;
+        }
+
+        // now check every usage/assignment of the variable
+        List<ASTName> names = node.findDescendantsOfType(ASTName.class);
+        for (ASTName n : names) {
+            if (!n.hasImageEqualTo(returnVariableName)) continue;
+
+            Node expression = n.getNthParent(3);
+            if (expression instanceof ASTEqualityExpression) continue;
+            if (expression instanceof ASTStatementExpression) {
+                if (expression.jjtGetNumChildren() > 2 && expression.jjtGetChild(1) instanceof ASTAssignmentOperator) {
+                    ASTName value = expression.jjtGetChild(2).getFirstDescendantOfType(ASTName.class);
+                    if (value == null || !volatileFields.contains(value.getImage())) {
+                        return false;
+                    }
+                }
+            }
+        }
+
+        return true;
+    }
+
     private boolean ifVerify(ASTIfStatement is, String varname) {
         List<ASTPrimaryExpression> finder = is.findDescendantsOfType(ASTPrimaryExpression.class);
         if (finder.size() > 1) {

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

@@ -126,8 +126,10 @@ public class Foo {		// perfect, both methods provided
           externalInfoUrl="${pmd.website.baseurl}/rules/java/basic.html#DoubleCheckedLocking">
       <description>
 Partially created objects can be returned by the Double Checked Locking pattern when used in Java.
-An optimizing JRE may assign a reference to the baz variable before it creates the object the
-reference is intended to point to.
+An optimizing JRE may assign a reference to the baz variable before it calls the constructor of the object the
+reference points to.
+
+Note: With Java 5, you can make Double checked locking work, if you declare the variable to be `volatile`.
 
 For more details refer to: http://www.javaworld.com/javaworld/jw-02-2001/jw-0209-double.html
 or http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html
@@ -136,7 +138,7 @@ or http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html
       <example>
   <![CDATA[
 public class Foo {
-	Object baz;
+	/*volatile */ Object baz = null; // fix for Java5 and later: volatile
 	Object bar() {
 		if (baz == null) { // baz may be non-null yet not fully created
 			synchronized(this) {

pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/basic/xml/DoubleCheckedLocking.xml

@@ -123,5 +123,27 @@ Object bar() {
 }
      ]]></code>
     </test-code>
-
+
+    <test-code>
+        <description>#1471 False positives for DoubleCheckedLocking</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+public class Foo {
+    private static volatile Foo instance;
+
+    public static Foo getInstance() {
+        Foo result = instance;
+        if (result == null) {
+            synchronized (Foo.class) {
+                result = instance;
+                if (result == null) {
+                    result = instance = new Foo();
+                }
+            }
+        }
+        return result;
+    }
+}
+        ]]></code>
+    </test-code>
 </test-data>

src/site/markdown/overview/changelog.md

@@ -30,6 +30,8 @@
 
 **Bugfixes:**
 
+*   java-basic/DoubleCheckedLocking:
+    *   [#1471](https://sourceforge.net/p/pmd/bugs/1471/): False positives for DoubleCheckedLocking
 *   java-basic/SimplifiedTernary:
     *   [#1424](https://sourceforge.net/p/pmd/bugs/1424/): False positive with ternary operator
 *   java-codesize/TooManyMethods: