External calls handled + output printing changed

slither/detectors/operations/cache_array_length.py

@@ -7,7 +7,7 @@ from slither.core.solidity_types import ArrayType
 from slither.core.source_mapping.source_mapping import SourceMapping
 from slither.core.variables import StateVariable
 from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification
-from slither.slithir.operations import Length, Delete
+from slither.slithir.operations import Length, Delete, HighLevelCall
 
 
 class CacheArrayLength(AbstractDetector):
@@ -119,8 +119,10 @@ contract C
         # - when `push` is called
         # - when `pop` is called
         # - when `delete` is called on the entire array
+        # - when external function call is made (instructions from internal function calls are already in
+        #   `node.all_slithir_operations()`, so we don't need to handle internal calls separately)
         if node.type == NodeType.EXPRESSION:
-            for op in node.irs:
+            for op in node.all_slithir_operations():
                 if isinstance(op, Length) and op.value == array:
                     # op accesses array.length, not necessarily modifying it
                     return True
@@ -132,6 +134,8 @@ contract C
                         and op.expression.expression.value == array
                     ):
                         return True
+                if isinstance(op, HighLevelCall) and not op.function.view and not op.function.pure:
+                    return True
 
         for son in node.sons:
             if son not in visited:
@@ -173,7 +177,7 @@ contract C
                 if not CacheArrayLength._is_loop_referencing_array_length(
                     if_node, visited, array, 1
                 ):
-                    non_optimal_array_len_usages.append(if_node.expression)
+                    non_optimal_array_len_usages.append(if_node)
 
     @staticmethod
     def _get_non_optimal_array_len_usages_for_function(f: Function) -> List[SourceMapping]:
@@ -207,8 +211,10 @@ contract C
         )
         for usage in non_optimal_array_len_usages:
             info = [
-                f"Loop condition at {usage.source_mapping} should use cached array length instead of referencing "
-                f"`length` member of the storage array.\n "
+                "Loop condition at ",
+                usage,
+                " should use cached array length instead of referencing `length` member "
+                "of the storage array.\n ",
             ]
             res = self.generate_result(info)
             results.append(res)

tests/e2e/detectors/snapshots/detectors__detector_CacheArrayLength_0_8_17_CacheArrayLength_sol__0.txt

@@ -1,14 +1,18 @@
-Loop condition at tests/e2e/detectors/test_data/cache-array-length/0.8.17/CacheArrayLength.sol#47 should use cached array length instead of referencing `length` member of the storage array.
+Loop condition at i < array.length (tests/e2e/detectors/test_data/cache-array-length/0.8.17/CacheArrayLength.sol#36) should use cached array length instead of referencing `length` member of the storage array.
  
-Loop condition at tests/e2e/detectors/test_data/cache-array-length/0.8.17/CacheArrayLength.sol#78 should use cached array length instead of referencing `length` member of the storage array.
+Loop condition at i_scope_22 < array.length (tests/e2e/detectors/test_data/cache-array-length/0.8.17/CacheArrayLength.sol#166) should use cached array length instead of referencing `length` member of the storage array.
  
-Loop condition at tests/e2e/detectors/test_data/cache-array-length/0.8.17/CacheArrayLength.sol#16 should use cached array length instead of referencing `length` member of the storage array.
+Loop condition at j_scope_11 < array.length (tests/e2e/detectors/test_data/cache-array-length/0.8.17/CacheArrayLength.sol#108) should use cached array length instead of referencing `length` member of the storage array.
  
-Loop condition at tests/e2e/detectors/test_data/cache-array-length/0.8.17/CacheArrayLength.sol#88 should use cached array length instead of referencing `length` member of the storage array.
+Loop condition at i_scope_6 < array.length (tests/e2e/detectors/test_data/cache-array-length/0.8.17/CacheArrayLength.sol#79) should use cached array length instead of referencing `length` member of the storage array.
  
-Loop condition at tests/e2e/detectors/test_data/cache-array-length/0.8.17/CacheArrayLength.sol#105 should use cached array length instead of referencing `length` member of the storage array.
+Loop condition at i_scope_21 < array.length (tests/e2e/detectors/test_data/cache-array-length/0.8.17/CacheArrayLength.sol#160) should use cached array length instead of referencing `length` member of the storage array.
  
-Loop condition at tests/e2e/detectors/test_data/cache-array-length/0.8.17/CacheArrayLength.sol#112 should use cached array length instead of referencing `length` member of the storage array.
+Loop condition at k_scope_9 < array2.length (tests/e2e/detectors/test_data/cache-array-length/0.8.17/CacheArrayLength.sol#98) should use cached array length instead of referencing `length` member of the storage array.
  
-Loop condition at tests/e2e/detectors/test_data/cache-array-length/0.8.17/CacheArrayLength.sol#59 should use cached array length instead of referencing `length` member of the storage array.
+Loop condition at k_scope_17 < array2.length (tests/e2e/detectors/test_data/cache-array-length/0.8.17/CacheArrayLength.sol#132) should use cached array length instead of referencing `length` member of the storage array.
+ 
+Loop condition at j_scope_15 < array.length (tests/e2e/detectors/test_data/cache-array-length/0.8.17/CacheArrayLength.sol#125) should use cached array length instead of referencing `length` member of the storage array.
+ 
+Loop condition at i_scope_4 < array.length (tests/e2e/detectors/test_data/cache-array-length/0.8.17/CacheArrayLength.sol#67) should use cached array length instead of referencing `length` member of the storage array.
  

tests/e2e/detectors/test_data/cache-array-length/0.8.17/CacheArrayLength.sol

@@ -10,6 +10,26 @@ contract CacheArrayLength
     S[] array;
     S[] array2;
 
+    function h() external
+    {
+        
+    }
+
+    function g() internal
+    {
+        this.h();
+    }
+
+    function h_view() external view
+    {
+
+    }
+
+    function g_view() internal view
+    {
+        this.h_view();
+    }
+
     function f() public
     {
         // array accessed but length doesn't change
@@ -123,5 +143,29 @@ contract CacheArrayLength
         {
 
         }
+
+        // array not modified, but it may potentially change in an internal function call
+        for (uint i = 0; i < array.length; i++)
+        {
+            g();
+        }
+
+        // array not modified, but it may potentially change in an external function call
+        for (uint i = 0; i < array.length; i++)
+        {
+            this.h();
+        }
+
+        // array not modified and it cannot be changed in a function call since g_view is a view function
+        for (uint i = 0; i < array.length; i++) // warning should appear
+        {
+            g_view();
+        }
+
+        // array not modified and it cannot be changed in a function call since h_view is a view function
+        for (uint i = 0; i < array.length; i++) // warning should appear
+        {
+            this.h_view();
+        }
     }   
 }