Fix FP of UnusedAssignment with synchronized

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/internal/JavaAstUtils.java

@@ -39,6 +39,7 @@ import net.sourceforge.pmd.lang.java.ast.ASTCastExpression;
 import net.sourceforge.pmd.lang.java.ast.ASTCatchClause;
 import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceType;
 import net.sourceforge.pmd.lang.java.ast.ASTConstructorCall;
+import net.sourceforge.pmd.lang.java.ast.ASTExplicitConstructorInvocation;
 import net.sourceforge.pmd.lang.java.ast.ASTExpression;
 import net.sourceforge.pmd.lang.java.ast.ASTExpressionStatement;
 import net.sourceforge.pmd.lang.java.ast.ASTFieldAccess;
@@ -46,6 +47,7 @@ import net.sourceforge.pmd.lang.java.ast.ASTForStatement;
 import net.sourceforge.pmd.lang.java.ast.ASTFormalParameter;
 import net.sourceforge.pmd.lang.java.ast.ASTFormalParameters;
 import net.sourceforge.pmd.lang.java.ast.ASTInfixExpression;
+import net.sourceforge.pmd.lang.java.ast.ASTInitializer;
 import net.sourceforge.pmd.lang.java.ast.ASTLabeledStatement;
 import net.sourceforge.pmd.lang.java.ast.ASTList;
 import net.sourceforge.pmd.lang.java.ast.ASTLocalVariableDeclaration;
@@ -64,6 +66,7 @@ import net.sourceforge.pmd.lang.java.ast.ASTThrowStatement;
 import net.sourceforge.pmd.lang.java.ast.ASTUnaryExpression;
 import net.sourceforge.pmd.lang.java.ast.ASTVariableAccess;
 import net.sourceforge.pmd.lang.java.ast.ASTVariableDeclaratorId;
+import net.sourceforge.pmd.lang.java.ast.AccessNode;
 import net.sourceforge.pmd.lang.java.ast.AccessNode.Visibility;
 import net.sourceforge.pmd.lang.java.ast.Annotatable;
 import net.sourceforge.pmd.lang.java.ast.BinaryOp;
@@ -773,4 +776,12 @@ public final class JavaAstUtils {
         }
         return false;
     }
+
+    public static boolean isInStaticCtx(JavaNode node) {
+        return node.ancestors()
+                   .any(it -> it instanceof AccessNode && ((AccessNode) it).isStatic()
+                       || it instanceof ASTInitializer && ((ASTInitializer) it).isStatic()
+                       || it instanceof ASTExplicitConstructorInvocation
+                   );
+    }
 }

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/internal/DataflowPass.java

@@ -64,6 +64,7 @@ import net.sourceforge.pmd.lang.java.ast.ASTSwitchExpression;
 import net.sourceforge.pmd.lang.java.ast.ASTSwitchFallthroughBranch;
 import net.sourceforge.pmd.lang.java.ast.ASTSwitchLike;
 import net.sourceforge.pmd.lang.java.ast.ASTSwitchStatement;
+import net.sourceforge.pmd.lang.java.ast.ASTSynchronizedStatement;
 import net.sourceforge.pmd.lang.java.ast.ASTThisExpression;
 import net.sourceforge.pmd.lang.java.ast.ASTThrowStatement;
 import net.sourceforge.pmd.lang.java.ast.ASTTryStatement;
@@ -315,9 +316,6 @@ public final class DataflowPass {
 
         // The class scope for the "this" reference, used to find fields
         // of this class
-        // null if we're not processing instance/static initializers,
-        // so in methods we don't care about fields
-        // If not null, fields are effectively treated as locals
         private final @NonNull JClassSymbol enclosingClassScope;
         private final boolean inStaticCtx;
 
@@ -509,6 +507,16 @@ public final class DataflowPass {
             return cur;
         }
 
+        @Override
+        public SpanInfo visit(ASTSynchronizedStatement node, SpanInfo data) {
+            // visit lock expr and child block
+            SpanInfo body = super.visit(node, data);
+            // We should assume that all assignments may be observed by other threads
+            // at the end of the critical section.
+            useAllSelfFields(body, JavaAstUtils.isInStaticCtx(node),
+                             enclosingClassScope, enclosingClassScope.tryGetNode());
+            return body;
+        }
 
         @Override
         public SpanInfo visit(ASTTryStatement node, final SpanInfo before) {

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

@@ -3607,4 +3607,74 @@ public class UnusedAssignmentUnusedVariableFP {
         }
         ]]></code>
     </test-code>
+    <test-code>
+        <description>FP with synchronized</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+        class Foo {
+            private static boolean someField;
+            private static final Object lock = new Object();
+            static void method() {
+                synchronized (lock) {
+                    processPendingActive = true;
+                }
+                doSomething();
+                synchronized (lock) {
+                    processPendingActive = false;
+                }
+            }
+            static void doSomething() {
+
+            }
+        }
+        ]]></code>
+    </test-code>
+    <test-code>
+        <description>FP with synchronized 2</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+        class Foo {
+            private static native void waitForReferencePendingList();
+
+            private static final Object processPendingLock = new Object();
+            private static boolean processPendingActive = false;
+
+            private static void processPendingReferences() {
+                // Only the singleton reference processing thread calls
+                // waitForReferencePendingList() and getAndClearReferencePendingList().
+                // These are separate operations to avoid a race with other threads
+                // that are calling waitForReferenceProcessing().
+                waitForReferencePendingList();
+                Reference<Object> pendingList;
+                synchronized (processPendingLock) {
+                    pendingList = getAndClearReferencePendingList();
+                    processPendingActive = true;
+                }
+                while (pendingList != null) {
+                    Reference<Object> ref = pendingList;
+                    pendingList = ref.discovered;
+                    ref.discovered = null;
+
+                    if (ref instanceof Cleaner) {
+                        ((Cleaner)ref).clean();
+                        // Notify any waiters that progress has been made.
+                        // This improves latency for nio.Bits waiters, which
+                        // are the only important ones.
+                        synchronized (processPendingLock) {
+                            processPendingLock.notifyAll();
+                        }
+                    } else {
+                        ReferenceQueue<? super Object> q = ref.queue;
+                        if (q != ReferenceQueue.NULL) q.enqueue(ref);
+                    }
+                }
+                // Notify any waiters of completion of current round.
+                synchronized (processPendingLock) {
+                    processPendingActive = false;
+                    processPendingLock.notifyAll();
+                }
+            }
+        }
+        ]]></code>
+    </test-code>
 </test-data>