M6-5-5: Improve detection of modification by reference

 - No longer consider the taking of a const reference as a modification.
 - Consider taking a non-const reference of a potential loop counter as
   modifying that loop counter.
 - Update tests to reflect changes.

Author: lcartey

change_notes/2023-11-05-m6-5-5-const-refs.md

@@ -0,0 +1,3 @@
+ * `M6-5-5`
+   - Reduce false positives by no longer considering the taking of a const reference as a modification.
+   - Improve detection of non-local modification of loop iteration variables to reduce false positives.

cpp/autosar/test/rules/M6-5-5/LoopControlVariableModifiedInLoopExpression.expected

@@ -1 +1,2 @@
 | test.cpp:24:8:24:15 | testFlag | Loop control variable testFlag is modified in the loop update expression. |
+| test.cpp:47:12:47:12 | y | Loop control variable y is modified in the loop update expression. |

cpp/autosar/test/rules/M6-5-5/test.cpp

@@ -24,3 +24,27 @@ void test_loop_control_variable_modified_in_expression() {
        testFlag = updateFlagWithIncrement(++x)) { // NON_COMPLIANT
   }
 }
+
+#include <vector>
+
+void test_const_refs(std::vector<int> v) {
+  std::vector<int>::iterator first = v.begin();
+  std::vector<int>::iterator last = v.end();
+  // call to operator!= passes a const reference to first
+  for (; first != last; first++) { // COMPLIANT
+  }
+}
+
+void update(std::vector<int>::iterator &f, const int &x, int &y) {}
+
+void test_const_refs_update(std::vector<int> v) {
+  std::vector<int>::iterator last = v.end();
+  int x = 0;
+  int y = 0;
+  // call to operator!= passes a const reference to first
+  for (std::vector<int>::iterator first = v.begin(); first != last; update(
+           first, x, // COMPLIANT - first is a loop counter, so can be modified
+           y)) {     // NON_COMPLIANT - y is modified and is not a loop counter
+    first + 1;
+  }
+}

cpp/common/src/codingstandards/cpp/Loops.qll

@@ -106,6 +106,15 @@ Variable getALoopCounter(ForStmt fs) {
     )
     or
     updateOp = result.getAnAssignedValue()
+    or
+    // updateOp is an access whose address is taken in a non-const way
+    exists(FunctionCall fc, VariableAccess va |
+      fc = updateOp and
+      fc instanceof FunctionCall and
+      fc.getAnArgument() = va and
+      va = result.getAnAccess() and
+      va.isAddressOfAccessNonConst()
+    )
   ) and
   result instanceof Variable and
   // checked or used in the condition
@@ -260,7 +269,7 @@ predicate isLoopControlVarModifiedInLoopCondition(
   loopControlVariableAccess = forLoop.getCondition().getAChild+() and
   (
     loopControlVariableAccess.isModified() or
-    loopControlVariableAccess.isAddressOfAccess()
+    loopControlVariableAccess.isAddressOfAccessNonConst()
   )
 }
 
@@ -277,7 +286,7 @@ predicate isLoopControlVarModifiedInLoopExpr(
   loopControlVariableAccess = forLoop.getUpdate().getAChild() and
   (
     loopControlVariableAccess.isModified() or
-    loopControlVariableAccess.isAddressOfAccess()
+    loopControlVariableAccess.isAddressOfAccessNonConst()
   )
 }