refactor around FindCompoundOperationsOnSharedVariables

spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/FindCompoundOperationsOnSharedVariablesTest.java

@@ -59,22 +59,25 @@ void reportsBugFor_compoundPostIncrementation_onSharedVariable() {
     // &=
     @Test
     void reportsBugFor_compoundIAND_onSharedVariable() {
+        // considered multithreaded because it has a volatile field (not the problematic)
         performAnalysis("multithreaded/compoundOperationOnSharedVariables/CompoundIANDOperationOnSharedVariable.class");
         assertBugTypeCount(BUG_TYPE, 1);
-        assertBugInMethodAtLine(BUG_TYPE, "CompoundIANDOperationOnSharedVariable", "getNum", 11);
+        assertBugInMethodAtLine(BUG_TYPE, "CompoundIANDOperationOnSharedVariable", "getNum", 12);
     }
 
     // |=
     @Test
     void reportsBugFor_compoundIOR_onSharedVariable() {
+        // considered multithreaded because it has a field (not the problematic) from the java.util.concurrent.atomic package
         performAnalysis("multithreaded/compoundOperationOnSharedVariables/CompoundIOROperationOnSharedVariable.class");
         assertBugTypeCount(BUG_TYPE, 1);
-        assertBugInMethodAtLine(BUG_TYPE, "CompoundIOROperationOnSharedVariable", "getNum", 11);
+        assertBugInMethodAtLine(BUG_TYPE, "CompoundIOROperationOnSharedVariable", "getNum", 14);
     }
 
     // >>>=
     @Test
     void reportsBugFor_compoundLogicalRightShifting_onSharedVariable() {
+        // considered multithreaded because it extends Thread
         performAnalysis("multithreaded/compoundOperationOnSharedVariables/CompoundLogicalRightShiftingOnSharedVariable.class");
         assertBugTypeCount(BUG_TYPE, 1);
         assertBugInMethodAtLine(BUG_TYPE, "CompoundLogicalRightShiftingOnSharedVariable", "getNum", 11);
@@ -83,6 +86,7 @@ void reportsBugFor_compoundLogicalRightShifting_onSharedVariable() {
     // >>=
     @Test
     void reportsBugFor_compoundRightShifting_onSharedVariable() {
+        // considered multithreaded because it has a method with synchronized block
         performAnalysis("multithreaded/compoundOperationOnSharedVariables/CompoundRightShiftingOnSharedVariable.class");
         assertBugTypeCount(BUG_TYPE, 1);
         assertBugInMethodAtLine(BUG_TYPE, "CompoundRightShiftingOnSharedVariable", "getNum", 11);
@@ -91,6 +95,7 @@ void reportsBugFor_compoundRightShifting_onSharedVariable() {
     // <<=
     @Test
     void reportsBugFor_compoundLeftShifting_onSharedVariable() {
+        // considered multithreaded because it has synchronized method
         performAnalysis("multithreaded/compoundOperationOnSharedVariables/CompoundLeftShiftingOnSharedVariable.class");
         assertBugTypeCount(BUG_TYPE, 1);
         assertBugInMethodAtLine(BUG_TYPE, "CompoundLeftShiftingOnSharedVariable", "getNum", 11);
@@ -99,6 +104,7 @@ void reportsBugFor_compoundLeftShifting_onSharedVariable() {
     // %=
     @Test
     void reportsBugFor_compoundModulo_onSharedVariable() {
+        // considered multithreaded because it implements Runnable
         performAnalysis("multithreaded/compoundOperationOnSharedVariables/CompoundModuloOnSharedVariable.class");
         assertBugTypeCount(BUG_TYPE, 1);
         assertBugInMethodAtLine(BUG_TYPE, "CompoundModuloOnSharedVariable", "getNum", 12);
@@ -132,6 +138,7 @@ void reportsBugFor_compoundSubtraction_onSharedVariable() {
     // +=
     @Test
     void reportsBugFor_compoundAddition_onSharedVolatileVariable() {
+        // simply defining the field as volatile is not enough
         performAnalysis("multithreaded/compoundOperationOnSharedVariables/CompoundAdditionOnSharedVolatileVariable.class");
         assertBugTypeCount(BUG_TYPE, 1);
         assertBugInMethodAtLine(BUG_TYPE, "CompoundAdditionOnSharedVolatileVariable", "getNum", 11);
@@ -140,8 +147,9 @@ void reportsBugFor_compoundAddition_onSharedVolatileVariable() {
     // ^=
     @Test
     void reportsBugFor_compoundXOROperation_onSharedVariable() {
+        // considered multithreaded because it has a field (not the problematic) with synchronized assignment
         performAnalysis("multithreaded/compoundOperationOnSharedVariables/CompoundXOROperationOnSharedVariable.class");
         assertBugTypeCount(BUG_TYPE, 1);
-        assertBugInMethodAtLine(BUG_TYPE, "CompoundXOROperationOnSharedVariable", "getFlag", 11);
+        assertBugInMethodAtLine(BUG_TYPE, "CompoundXOROperationOnSharedVariable", "getFlag", 16);
     }
 }

spotbugs/etc/messages.xml

@@ -9110,8 +9110,8 @@ Using floating-point variables should not be used as loop counters, as they are
     </Details>
   </BugPattern>
   <BugPattern type="COSV_COMPOUND_OPERATIONS_ON_SHARED_VARIABLES">
-    <ShortDescription>Compound operation on shared variables</ShortDescription>
-    <LongDescription>A compound operation was used on a shared variable in a multithreaded class</LongDescription>
+    <ShortDescription>Operations on shared variable is not atomic</ShortDescription>
+    <LongDescription>Operations on the "{2.name}" shared variable in "{0.simpleName}" class is not atomic</LongDescription>
     <Details>
       <![CDATA[
         <p>

spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindCompoundOperationsOnSharedVariables.java

@@ -29,6 +29,7 @@
 
 import java.util.ArrayList;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -70,87 +71,95 @@ public class FindCompoundOperationsOnSharedVariables extends OpcodeStackDetector
     private final BugAccumulator bugAccumulator;
     private final Map<XMethod, List<XField>> compoundlyWrittenNotSecuredFieldsByMethods = new HashMap<>();
     private final Map<XMethod, List<XField>> readNotSecuredFieldsByMethods = new HashMap<>();
+    private final Set<XField> possiblyRelevantFields = new HashSet<>();
     private boolean isInsideSynchronizedOrLockingMethod = false;
-    private XField maybeCompoundlyOperatedField;
-    private int stepsMadeInCompoundOperationProcess = Const.UNDEFINED;
+    private State detectionState = State.START;
+
+    private enum State {
+        START,
+        SEEN_FIRST_OP_GET,
+        SEEN_OPERAND_GET_OR_PUSH,
+        SEEN_OPERATION
+    }
 
     public FindCompoundOperationsOnSharedVariables(BugReporter bugReporter) {
         this.bugAccumulator = new BugAccumulator(bugReporter);
     }
 
     @Override
     public void visit(Method method) {
-        ClassContext currentClassContext = getClassContext();
-        isInsideSynchronizedOrLockingMethod = MultiThreadedCodeIdentifierUtils.isMethodMultiThreaded(method, currentClassContext);
+        isInsideSynchronizedOrLockingMethod = MultiThreadedCodeIdentifierUtils.isMethodMultiThreaded(method, getClassContext());
+        possiblyRelevantFields.clear();
+        detectionState = State.START;
     }
 
     @Override
     public void visitAfter(JavaClass obj) {
         bugAccumulator.reportAccumulatedBugs();
         compoundlyWrittenNotSecuredFieldsByMethods.clear();
         readNotSecuredFieldsByMethods.clear();
+        possiblyRelevantFields.clear();
     }
 
     @Override
     public void visitClassContext(ClassContext classContext) {
         if (MultiThreadedCodeIdentifierUtils.isPartOfMultiThreadedCode(classContext)) {
             isInsideSynchronizedOrLockingMethod = false;
-            maybeCompoundlyOperatedField = null;
-            stepsMadeInCompoundOperationProcess = Const.UNDEFINED;
+            possiblyRelevantFields.clear();
+            detectionState = State.START;
 
             super.visitClassContext(classContext);
         }
     }
 
     @Override
     public void sawOpcode(int seen) {
-        if (!isInsideSynchronizedOrLockingMethod) {
-            if (seen == Const.GETFIELD || seen == Const.GETSTATIC) {
-                XField maybeFieldToRead = getXFieldOperand();
-                lookForUnsecuredOperationsOnFieldInOtherMethods(maybeFieldToRead, getXMethod(),
-                        compoundlyWrittenNotSecuredFieldsByMethods, readNotSecuredFieldsByMethods);
-                maybeCompoundlyOperatedField = maybeFieldToRead;
-                stepsMadeInCompoundOperationProcess = 1;
-                return;
-            }
+        if (isInsideSynchronizedOrLockingMethod) {
+            return;
+        }
 
-            // Possible invoke virtual first if it is a wrapper type
-            if (stepsMadeInCompoundOperationProcess == 1 && seen == Const.INVOKEVIRTUAL) {
-                return;
-            }
+        if (seen == Const.GETFIELD || seen == Const.GETSTATIC) {
+            XField readField = getXFieldOperand();
+            lookForUnsecuredOperationsOnFieldInOtherMethods(readField, getXMethod(),
+                    compoundlyWrittenNotSecuredFieldsByMethods, readNotSecuredFieldsByMethods);
+            possiblyRelevantFields.add(readField);
+            detectionState = State.SEEN_FIRST_OP_GET;
+            return;
+        }
 
-            if (stepsMadeInCompoundOperationProcess == 1 && (isReadOpCode(seen) || isPushConstant(seen))) {
-                stepsMadeInCompoundOperationProcess = 2;
-                return;
-            }
+        // Possible invoke virtual first if it is a wrapper type
+        if (detectionState == State.SEEN_FIRST_OP_GET && seen == Const.INVOKEVIRTUAL) {
+            return;
+        }
 
-            // In case of the field's type is a wrapper type
-            if (stepsMadeInCompoundOperationProcess == 2 && seen == Const.INVOKEVIRTUAL) {
-                return;
-            }
+        if (detectionState == State.SEEN_FIRST_OP_GET && (isReadOpCode(seen) || isPushConstant(seen))) {
+            detectionState = State.SEEN_OPERAND_GET_OR_PUSH;
+            return;
+        }
 
-            if (stepsMadeInCompoundOperationProcess == 2 && isPossibleCompoundOperation(seen)) {
-                stepsMadeInCompoundOperationProcess = 3;
-                return;
-            }
+        // In case of the field's type is a wrapper type
+        if (detectionState == State.SEEN_OPERAND_GET_OR_PUSH && seen == Const.INVOKEVIRTUAL) {
+            return;
+        }
 
-            // In case of wrapper types valueOf() method
-            if (stepsMadeInCompoundOperationProcess == 3 && seen == Const.INVOKESTATIC) {
-                return;
-            }
+        if (detectionState == State.SEEN_OPERAND_GET_OR_PUSH && isPossibleCompoundOperation(seen)) {
+            detectionState = State.SEEN_OPERATION;
+            return;
+        }
 
-            if (stepsMadeInCompoundOperationProcess == 3 && (seen == Const.PUTFIELD || seen == Const.PUTSTATIC)) {
-                if (maybeCompoundlyOperatedField != null && maybeCompoundlyOperatedField.equals(getXFieldOperand())) {
-                    lookForUnsecuredOperationsOnFieldInOtherMethods(getXFieldOperand(), getXMethod(),
-                            readNotSecuredFieldsByMethods, compoundlyWrittenNotSecuredFieldsByMethods);
-                }
+        // In case of wrapper types valueOf() method
+        if (detectionState == State.SEEN_OPERATION && seen == Const.INVOKESTATIC) {
+            return;
+        }
 
-                maybeCompoundlyOperatedField = null;
-                stepsMadeInCompoundOperationProcess = Const.UNDEFINED;
+        if (detectionState == State.SEEN_OPERATION && (seen == Const.PUTFIELD || seen == Const.PUTSTATIC)) {
+            if (!possiblyRelevantFields.isEmpty() && possiblyRelevantFields.contains(getXFieldOperand())) {
+                lookForUnsecuredOperationsOnFieldInOtherMethods(getXFieldOperand(), getXMethod(),
+                        readNotSecuredFieldsByMethods, compoundlyWrittenNotSecuredFieldsByMethods);
             }
-        } else {
-            maybeCompoundlyOperatedField = null;
-            stepsMadeInCompoundOperationProcess = Const.UNDEFINED;
+
+            possiblyRelevantFields.clear();
+            detectionState = State.START;
         }
     }
 

spotbugs/src/main/java/edu/umd/cs/findbugs/util/CollectionAnalysis.java

@@ -18,11 +18,10 @@
 
 package edu.umd.cs.findbugs.util;
 
-import java.util.Arrays;
-import java.util.HashSet;
 import java.util.Set;
 
 import edu.umd.cs.findbugs.ba.ClassMember;
+import edu.umd.cs.findbugs.internalAnnotations.DottedClassName;
 
 /**
  * Utility class for analyzing collections.
@@ -42,10 +41,21 @@ private CollectionAnalysis() {
      * @return {@code true} if the class member is a synchronized collection, {@code false} otherwise
      */
     public static boolean isSynchronizedCollection(ClassMember classMember) {
-        Set<String> interestingCollectionMethodNames = new HashSet<>(Arrays.asList(
+        return isSynchronizedCollection(classMember.getClassName(), classMember.getName());
+    }
+
+    /**
+     * Checks if a method is a synchronized collection creating one.
+     *
+     * @param className name of the class containing the method
+     * @param methodName the name of the method
+     * @return {@code true} if it's a synchronized collection creating method, {@code false} otherwise
+     */
+    public static boolean isSynchronizedCollection(@DottedClassName String className, String methodName) {
+        final Set<String> interestingCollectionMethodNames = Set.of(
                 "synchronizedCollection", "synchronizedSet", "synchronizedSortedSet",
                 "synchronizedNavigableSet", "synchronizedList", "synchronizedMap",
-                "synchronizedSortedMap", "synchronizedNavigableMap"));
-        return "java.util.Collections".equals(classMember.getClassName()) && interestingCollectionMethodNames.contains(classMember.getName());
+                "synchronizedSortedMap", "synchronizedNavigableMap");
+        return "java.util.Collections".equals(className) && interestingCollectionMethodNames.contains(methodName);
     }
 }

spotbugs/src/main/java/edu/umd/cs/findbugs/util/MultiThreadedCodeIdentifierUtils.java

@@ -22,14 +22,16 @@
 import java.util.stream.Stream;
 
 import edu.umd.cs.findbugs.ba.ch.Subtypes2;
+import edu.umd.cs.findbugs.internalAnnotations.DottedClassName;
 import org.apache.bcel.classfile.Field;
 import org.apache.bcel.classfile.JavaClass;
 import org.apache.bcel.classfile.LocalVariableTable;
 import org.apache.bcel.classfile.Method;
 import org.apache.bcel.generic.ConstantPoolGen;
-import org.apache.bcel.generic.INVOKEVIRTUAL;
+import org.apache.bcel.generic.INVOKEDYNAMIC;
 import org.apache.bcel.generic.Instruction;
 import org.apache.bcel.generic.InstructionHandle;
+import org.apache.bcel.generic.InvokeInstruction;
 import org.apache.bcel.generic.MONITORENTER;
 import org.apache.bcel.generic.MethodGen;
 
@@ -48,7 +50,7 @@ private MultiThreadedCodeIdentifierUtils() {
     public static boolean isPartOfMultiThreadedCode(ClassContext classContext) {
         JavaClass javaClass = classContext.getJavaClass();
         if (Subtypes2.instanceOf(javaClass, JAVA_LANG_RUNNABLE) ||
-                Stream.of(javaClass.getFields()).anyMatch(MultiThreadedCodeIdentifierUtils::isFieldMultiThreaded)) {
+                Stream.of(javaClass.getFields()).anyMatch(MultiThreadedCodeIdentifierUtils::isFieldIndicatingMultiThreadedContainer)) {
             return true;
         }
 
@@ -81,11 +83,11 @@ private static boolean hasMultiThreadedInstruction(MethodGen methodGen) {
             Instruction ins = handle.getInstruction();
             if (ins instanceof MONITORENTER) {
                 return true;
-            } else if (ins instanceof INVOKEVIRTUAL) {
-                INVOKEVIRTUAL invokevirtual = ((INVOKEVIRTUAL) ins);
-                String className = invokevirtual.getClassName(cpg);
-                String methodName = invokevirtual.getMethodName(cpg);
-                if (isConcurrentLockInterfaceCall(className, methodName)) {
+            } else if (ins instanceof InvokeInstruction && !(ins instanceof INVOKEDYNAMIC)) {
+                InvokeInstruction iins = (InvokeInstruction) ins;
+                String className = iins.getClassName(cpg);
+                String methodName = iins.getMethodName(cpg);
+                if (isConcurrentLockInterfaceCall(className, methodName) || CollectionAnalysis.isSynchronizedCollection(className, methodName)) {
                     return true;
                 }
             }
@@ -95,16 +97,21 @@ private static boolean hasMultiThreadedInstruction(MethodGen methodGen) {
         return false;
     }
 
-    private static boolean isConcurrentLockInterfaceCall(String className, String methodName) {
-        return Subtypes2.instanceOf(className, "java.util.concurrent.locks.Lock")
+    private static boolean isConcurrentLockInterfaceCall(@DottedClassName String className, String methodName) {
+        return isInstanceOfLock(className)
                 && ("lock".equals(methodName)
                         || "unlock".equals(methodName)
                         || "tryLock".equals(methodName)
                         || "lockInterruptibly".equals(methodName));
     }
 
-    private static boolean isFieldMultiThreaded(Field field) {
-        return field.isVolatile() || isFromAtomicPackage(field.getSignature());
+    private static boolean isInstanceOfLock(@DottedClassName String className) {
+        return className != null && Subtypes2.instanceOf(className, "java.util.concurrent.locks.Lock");
+    }
+
+    private static boolean isFieldIndicatingMultiThreadedContainer(Field field) {
+        return field.isVolatile() || isFromAtomicPackage(field.getSignature())
+                || isInstanceOfLock(ClassName.fromFieldSignatureToDottedClassName(field.getSignature()));
     }
 
     public static boolean isFromAtomicPackage(String signature) {

spotbugsTestCases/src/java/multithreaded/compoundOperationOnSharedVariables/CompoundIANDOperationOnSharedVariable.java

@@ -1,7 +1,8 @@
 package multithreaded.compoundOperationOnSharedVariables;
 
-public class CompoundIANDOperationOnSharedVariable extends Thread {
+public class CompoundIANDOperationOnSharedVariable {
     private int num = 1;
+    private volatile int volatileField; // just to signal that its multithreaded
 
     public void toggle() {
         num &= 3;

spotbugsTestCases/src/java/multithreaded/compoundOperationOnSharedVariables/CompoundIOROperationOnSharedVariable.java

@@ -1,6 +1,9 @@
 package multithreaded.compoundOperationOnSharedVariables;
 
-public class CompoundIOROperationOnSharedVariable extends Thread {
+import java.util.concurrent.atomic.AtomicInteger;
+
+public class CompoundIOROperationOnSharedVariable {
+    private AtomicInteger atomicInteger; // just to signal that its multithreaded
     private int num = 1;
 
     public void toggle() {

spotbugsTestCases/src/java/multithreaded/compoundOperationOnSharedVariables/CompoundXOROperationOnSharedVariable.java

@@ -1,6 +1,11 @@
 package multithreaded.compoundOperationOnSharedVariables;
 
-public class CompoundXOROperationOnSharedVariable extends Thread {
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+
+public class CompoundXOROperationOnSharedVariable {
+    private final List<Integer> nums = Collections.synchronizedList(new ArrayList<>()); // just to signal that its multithreaded
     private Boolean flag = true;
 
     public void toggle(Boolean tmp) {