Rename "interesting" fields and methods

PatrikScully · PatrikScully · commit 543b24e827ee · 2024-07-03T15:00:39.000+02:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -10,18 +10,18 @@ Currently the versioning policy of this project follows [Semantic Versioning v2.
 - Do not consider Records as Singletons ([#2981](https://github.com/spotbugs/spotbugs/issues/2981))
 - Keep a maximum of 10000 cached analysis entries for plugin's analysis engines ([#3025](https://github.com/spotbugs/spotbugs/pull/3025))
 
+### Added
+- New detector `AtomicOperationsCombinedDetector` and introduced new bug types:
+  - `AT_COMBINED_ATOMIC_OPERATIONS_ARE_NOT_ATOMIC` is reported in case of combined atomic operations are not synchronized.
+  - `AT_ATOMIC_OPERATION_NEEDS_SYNCHRONIZATION` is reported when an atomic operation is not synchronized, but should be because of thread safety.
+
 ## 4.8.6 - 2024-06-17
 ### Fixed
 - Do not report BC_UNCONFIRMED_CAST for Java 21's type switches when the switch instruction is TABLESWITCH ([#2782](https://github.com/spotbugs/spotbugs/issues/2782))
 - Do not throw exception when inspecting empty switch statements ([#2995](https://github.com/spotbugs/spotbugs/issues/2995))
 - Adjust priority since relaxed mode reports even `IGNORED_PRIORITY` ([#2994](https://github.com/spotbugs/spotbugs/issues/2994))
 - Fix duplicated log4j2 jar in distribution ([#3001](https://github.com/spotbugs/spotbugs/issues/3001))
 
-### Added
-- New detector `AtomicOperationsCombinedDetector` and introduced new bug types:
-  - `AT_COMBINED_ATOMIC_OPERATIONS_ARE_NOT_ATOMIC` is reported in case of combined atomic operations are not synchronized.
-  - `AT_ATOMIC_OPERATION_NEEDS_SYNCHRONIZATION` is reported when an atomic operation is not synchronized, but should be because of thread safety.
-
 ## 4.8.5 - 2024-05-03
 ### Fixed
 - Fix FP `SING_SINGLETON_GETTER_NOT_SYNCHRONIZED` with eager instances ([#2932](https://github.com/spotbugs/spotbugs/issues/2932))
diff --git a/spotbugs/src/main/java/edu/umd/cs/findbugs/detect/AtomicOperationsCombinedDetector.java b/spotbugs/src/main/java/edu/umd/cs/findbugs/detect/AtomicOperationsCombinedDetector.java
@@ -101,11 +101,11 @@ private BugInstance toBugInstance(Detector detector, String type) {
     private final BugReporter bugReporter;
     private final BugAccumulator bugAccumulator;
 
-    private final Set<XField> interestingFields = new HashSet<>();
+    private final Set<XField> fieldsForAtomicityCheck = new HashSet<>();
     private final Set<XField> combinedAtomicFields = new HashSet<>();
 
-    private final Map<Method, Map<XField, List<BugPrototype>>> interestingFieldCalls = new HashMap<>();
-    private final Map<Method, List<BugPrototype>> interestingLocalVariableCalls = new HashMap<>();
+    private final Map<Method, Map<XField, List<BugPrototype>>> fieldAccessBugPrototypes = new HashMap<>();
+    private final Map<Method, List<BugPrototype>> localVariableInvocations = new HashMap<>();
 
     private final Set<XMethod> unsynchronizedPrivateMethods = new HashSet<>();
 
@@ -118,13 +118,13 @@ public AtomicOperationsCombinedDetector(BugReporter bugReporter) {
     public void visitClassContext(ClassContext classContext) {
         try {
             for (Method method : classContext.getMethodsInCallOrder()) {
-                collectInterestingFields(classContext, method);
+                collectFieldsForAtomicityCheck(classContext, method);
             }
             for (Method method : classContext.getMethodsInCallOrder()) {
-                analyzeInterestingFields(classContext, method);
+                analyzeFieldsForAtomicityViolations(classContext, method);
             }
             removePresynchronizedPrivateMethodCalls();
-            accumulateInterestingFields();
+            accumulateFieldsForAtomicityAnalysis();
             accumulateLocalVariables();
         } catch (CheckedAnalysisException e) {
             bugReporter.logError(String.format("Detector %s caught exception while analyzing class %s",
@@ -134,7 +134,7 @@ public void visitClassContext(ClassContext classContext) {
         }
     }
 
-    private void collectInterestingFields(ClassContext classContext, Method method) throws CFGBuilderException {
+    private void collectFieldsForAtomicityCheck(ClassContext classContext, Method method) throws CFGBuilderException {
         CFG cfg = classContext.getCFG(method);
         ConstantPoolGen cpg = classContext.getConstantPoolGen();
 
@@ -145,22 +145,22 @@ private void collectInterestingFields(ClassContext classContext, Method method)
             if (instruction instanceof PUTFIELD) {
                 OpcodeStack stack = OpcodeStackScanner.getStackAt(classContext.getJavaClass(), method, handle.getPosition());
                 OpcodeStack.Item stackItem = stack.getStackItem(0);
-                if (isInterestingField(stackItem.getReturnValueOf())) {
-                    interestingFields.add(XFactory.createXField((FieldInstruction) instruction, cpg));
+                if (isAtomicField(stackItem.getReturnValueOf())) {
+                    fieldsForAtomicityCheck.add(XFactory.createXField((FieldInstruction) instruction, cpg));
                 }
             }
         }
     }
 
-    private static boolean isInterestingField(ClassMember classMember) {
+    private static boolean isAtomicField(ClassMember classMember) {
         if (classMember == null) {
             return false;
         }
         return CollectionAnalysis.isSynchronizedCollection(classMember)
                 || (classMember.getClassName().startsWith("java.util.concurrent.atomic") && classMember.getSignature().endsWith(")V"));
     }
 
-    private void analyzeInterestingFields(ClassContext classContext, Method method) throws CheckedAnalysisException {
+    private void analyzeFieldsForAtomicityViolations(ClassContext classContext, Method method) throws CheckedAnalysisException {
         if (Const.CONSTRUCTOR_NAME.equals(method.getName()) || Const.STATIC_INITIALIZER_NAME.equals(method.getName()) || method.isSynchronized()) {
             return;
         }
@@ -184,57 +184,57 @@ private void analyzeInterestingFields(ClassContext classContext, Method method)
                 synchronizedBlock = false;
             } else if (instruction instanceof PUTFIELD && !synchronizedBlock && !MethodAnalysis.isDuplicatedLocation(methodDescriptor, pc)) {
                 XField xField = XFactory.createXField((FieldInstruction) instruction, cpg);
-                if (interestingFields.contains(xField)) {
+                if (fieldsForAtomicityCheck.contains(xField)) {
                     bugPrototype.invokedField = xField;
-                    interestingFieldCalls.computeIfAbsent(method, value -> new HashMap<>())
+                    fieldAccessBugPrototypes.computeIfAbsent(method, value -> new HashMap<>())
                             .computeIfAbsent(xField, value -> new LinkedList<>())
                             .add(bugPrototype);
                 }
             } else if (instruction instanceof InvokeInstruction && !(instruction instanceof INVOKEDYNAMIC)
                     && !synchronizedBlock && !MethodAnalysis.isDuplicatedLocation(methodDescriptor, pc)) {
                 OpcodeStack stack = OpcodeStackScanner.getStackAt(javaClass, method, pc);
-                Optional<XField> interestingField = getInterestingField(stack);
+                Optional<XField> fieldRequiringAtomicityCheck = findFieldRequiringAtomicityCheck(stack);
                 XMethod xMethod = XFactory.createXMethod((InvokeInstruction) instruction, cpg);
-                if (interestingField.isPresent()) {
-                    bugPrototype.invokedField = interestingField.get();
+                if (fieldRequiringAtomicityCheck.isPresent()) {
+                    bugPrototype.invokedField = fieldRequiringAtomicityCheck.get();
                     bugPrototype.invokedMethod = xMethod;
-                    interestingFieldCalls.computeIfAbsent(method, value -> new HashMap<>())
-                            .computeIfAbsent(interestingField.get(), value -> new LinkedList<>())
+                    fieldAccessBugPrototypes.computeIfAbsent(method, value -> new HashMap<>())
+                            .computeIfAbsent(fieldRequiringAtomicityCheck.get(), value -> new LinkedList<>())
                             .add(bugPrototype);
 
                     if (javaClass.getClassName().equals(xMethod.getClassName())) {
                         unsynchronizedPrivateMethods.add(xMethod);
                     }
                 } else if (stack.getStackDepth() > 0) {
                     OpcodeStack.Item stackItem = stack.getStackItem(0);
-                    if (isInterestingLocalVariable(stackItem)) {
+                    if (isLocalVariableRequiringAtomicityCheck(stackItem)) {
                         LocalVariableAnnotation annotation = LocalVariableAnnotation.getLocalVariableAnnotation(method, stackItem, pc);
                         if (annotation != null) {
                             bugPrototype.localVariableAnnotation = annotation;
                             bugPrototype.invokedMethod = xMethod;
-                            interestingLocalVariableCalls.computeIfAbsent(method, value -> new LinkedList<>())
+                            localVariableInvocations.computeIfAbsent(method, value -> new LinkedList<>())
                                     .add(bugPrototype);
                         }
                     } else if (isAtomicOperationsCombined(stack)) {
-                        combinedAtomicFields.addAll(getInterestingFieldsInCall(location, callListDataflow));
+                        combinedAtomicFields.addAll(findFieldsInvolvedInAtomicOperations(location, callListDataflow));
                     }
                 }
             }
         }
     }
 
-    private Optional<XField> getInterestingField(OpcodeStack stack) {
+    private Optional<XField> findFieldRequiringAtomicityCheck(OpcodeStack stack) {
         if (stack.getStackDepth() > 1 && stack.getStackItem(0).getReturnValueOf() != null) {
             return Optional.empty();
         }
         return IntStream.range(0, stack.getStackDepth())
                 .mapToObj(stack::getStackItem)
                 .map(OpcodeStack.Item::getXField)
-                .filter(interestingFields::contains)
+                .filter(fieldsForAtomicityCheck::contains)
                 .findFirst();
     }
 
-    private static boolean isInterestingLocalVariable(OpcodeStack.Item stackItem) {
+    private static boolean isLocalVariableRequiringAtomicityCheck(OpcodeStack.Item stackItem) {
         return isAtomicSignature(stackItem.getSignature())
                 && (stackItem.getReturnValueOf() == null || !stackItem.getReturnValueOf().getName().contains(Const.CONSTRUCTOR_NAME));
     }
@@ -253,21 +253,21 @@ private static boolean isAtomicReference(OpcodeStack.Item stackItem) {
         return stackItem.getReturnValueOf() != null && stackItem.getReturnValueOf().getClassName().startsWith("java.util.concurrent.atomic");
     }
 
-    private Set<XField> getInterestingFieldsInCall(Location location, CallListDataflow callListDataflow) throws DataflowAnalysisException {
-        Set<XField> interestingFieldsInCallList = new HashSet<>();
+    private Set<XField> findFieldsInvolvedInAtomicOperations(Location location, CallListDataflow callListDataflow) throws DataflowAnalysisException {
+        Set<XField> involvedFields = new HashSet<>();
         CallList factAtLocation = callListDataflow.getFactAtLocation(location);
         Iterator<Call> callIterator = factAtLocation.callIterator();
         while (callIterator.hasNext()) {
             Call call = callIterator.next();
             call.getAttributes().stream()
-                    .filter(interestingFields::contains)
-                    .forEach(interestingFieldsInCallList::add);
+                    .filter(fieldsForAtomicityCheck::contains)
+                    .forEach(involvedFields::add);
         }
-        return interestingFieldsInCallList;
+        return involvedFields;
     }
 
     private void removePresynchronizedPrivateMethodCalls() {
-        interestingLocalVariableCalls.entrySet().removeIf(entry -> entry.getKey().isPrivate()
+        localVariableInvocations.entrySet().removeIf(entry -> entry.getKey().isPrivate()
                 && unsynchronizedPrivateMethods.stream().noneMatch(privMethod -> equalMethods(entry.getKey(), privMethod)));
     }
 
@@ -280,44 +280,44 @@ public void report() {
         bugAccumulator.reportAccumulatedBugs();
     }
 
-    private void accumulateInterestingFields() {
-        Set<XField> interestingFieldsWithMultipleCalls = interestingFieldCalls.values().stream()
+    private void accumulateFieldsForAtomicityAnalysis() {
+        Set<XField> fieldsToProcess = fieldAccessBugPrototypes.values().stream()
                 .flatMap(map -> map.entrySet().stream())
                 .filter(entry -> entry.getValue().size() > 1)
                 .map(Map.Entry::getKey)
                 .collect(Collectors.toSet());
 
-        interestingFieldCalls.entrySet().stream()
+        fieldAccessBugPrototypes.entrySet().stream()
                 .flatMap(entry -> entry.getValue().entrySet().stream())
-                .forEach(entry -> processFieldData(entry.getKey(), entry.getValue(), interestingFieldsWithMultipleCalls));
+                .forEach(entry -> processFieldData(entry.getKey(), entry.getValue(), fieldsToProcess));
     }
 
     private void accumulateLocalVariables() {
-        interestingLocalVariableCalls.values().forEach(this::processFieldData);
+        localVariableInvocations.values().forEach(this::processFieldData);
     }
 
-    private void processFieldData(List<BugPrototype> interestingFieldCallData) {
-        processFieldData(null, interestingFieldCallData, new HashSet<>());
+    private void processFieldData(List<BugPrototype> fieldCallData) {
+        processFieldData(null, fieldCallData, new HashSet<>());
     }
 
-    private void processFieldData(XField field, List<BugPrototype> interestingFieldCallData, Set<XField> interestingFieldsWithMultipleCalls) {
-        if (!interestingFieldCallData.isEmpty()) {
-            BugPrototype bugPrototype = interestingFieldCallData.get(interestingFieldCallData.size() - 1);
-            if (interestingFieldCallData.size() > 1 || combinedAtomicFields.contains(field)) {
+    private void processFieldData(XField field, List<BugPrototype> fieldCallData, Set<XField> fieldsWithMultipleCalls) {
+        if (!fieldCallData.isEmpty()) {
+            BugPrototype bugPrototype = fieldCallData.get(fieldCallData.size() - 1);
+            if (fieldCallData.size() > 1 || combinedAtomicFields.contains(field)) {
                 BugInstance bugInstance = bugPrototype.toBugInstance(this, "AT_COMBINED_ATOMIC_OPERATIONS_ARE_NOT_ATOMIC");
                 bugAccumulator.accumulateBug(bugInstance, bugInstance.getPrimarySourceLineAnnotation());
-            } else if (interestingFieldsWithMultipleCalls.contains(field)) {
+            } else if (fieldsWithMultipleCalls.contains(field)) {
                 BugInstance bugInstance = bugPrototype.toBugInstance(this, "AT_ATOMIC_OPERATION_NEEDS_SYNCHRONIZATION");
                 bugAccumulator.accumulateBug(bugInstance, bugInstance.getPrimarySourceLineAnnotation());
             }
         }
     }
 
     private void clearProperties() {
-        interestingFields.clear();
+        fieldsForAtomicityCheck.clear();
         combinedAtomicFields.clear();
-        interestingFieldCalls.clear();
-        interestingLocalVariableCalls.clear();
+        fieldAccessBugPrototypes.clear();
+        localVariableInvocations.clear();
         unsynchronizedPrivateMethods.clear();
     }
 }
