[inefficientKeysetIterator] Chase predecessors 4 more nodes to check the pattern

Summary: There were FNs caused by only looking for the immediate predecessors when we were checking the pattern. This diff heuristically chases 4 more predecessors to reduce the number of FNs.

Reviewed By: ngorogiannis

Differential Revision: D16149983

fbshipit-source-id: f65c57a0a

infer/src/checkers/inefficientKeysetIterator.ml

@@ -75,12 +75,26 @@ let report_matching_get tenv summary pvar loop_nodes : unit =
     loop_nodes
 
 
-let when_dominating_pred_satisfies idom my_node ~f =
-  let preds =
-    Procdesc.Node.get_preds my_node
+(** Heuristic: check up to 4 direct predecessor nodes *)
+let when_dominating_preds_satisfy idom my_node ~fun_name ~class_name_f ~f =
+  let preds node =
+    Procdesc.Node.get_preds node
     |> List.filter ~f:(fun node -> Dominators.dominates idom node my_node)
   in
-  match preds with [pred_node] -> f pred_node | _ -> ()
+  let rec aux node (counter : int) =
+    if Int.equal counter 0 then ()
+    else
+      match preds node with
+      | [pred_node] -> (
+        match find_first_arg_pvar pred_node ~fun_name ~class_name_f with
+        | Some pvar ->
+            f pred_node pvar
+        | None ->
+            aux pred_node (counter - 1) )
+      | _ ->
+          ()
+  in
+  aux my_node 4
 
 
 let checker Callbacks.{summary; tenv} : Summary.t =
@@ -95,16 +109,10 @@ let checker Callbacks.{summary; tenv} : Summary.t =
           ~class_name_f:(PatternMatch.implements_iterator tenv)
         |> Option.is_some
       then
-        when_dominating_pred_satisfies idom loop_head ~f:(fun itr_node ->
-            if
-              Option.is_some
-                (find_first_arg_pvar itr_node ~fun_name:"iterator"
-                   ~class_name_f:(PatternMatch.implements_set tenv))
-            then
-              when_dominating_pred_satisfies idom itr_node ~f:(fun keySet_node ->
-                  find_first_arg_pvar keySet_node ~fun_name:"keySet"
-                    ~class_name_f:(PatternMatch.implements_map tenv)
-                  |> Option.iter ~f:(fun get_pvar ->
-                         report_matching_get tenv summary get_pvar loop_nodes ) ) ) )
+        when_dominating_preds_satisfy idom loop_head ~fun_name:"iterator"
+          ~class_name_f:(PatternMatch.implements_set tenv) ~f:(fun itr_node _ ->
+            when_dominating_preds_satisfy idom itr_node ~fun_name:"keySet"
+              ~class_name_f:(PatternMatch.implements_map tenv) ~f:(fun _keySet_node get_pvar ->
+                report_matching_get tenv summary get_pvar loop_nodes ) ) )
     loop_head_to_loop_nodes ;
   summary

infer/tests/codetoanalyze/java/inefficientKeysetIterator/Test.java

@@ -8,6 +8,7 @@ import android.os.Bundle;
 import java.util.HashMap;
 import java.util.Iterator;
 import java.util.Map;
+import java.util.Set;
 
 class Test {
 
@@ -26,10 +27,36 @@ class Test {
     }
   }
 
-  void inefficient_loop_itr_bad_FN(HashMap<String, Integer> testMap) {
+  void inefficient_loop_itr_heur_bad_FN(HashMap<String, Integer> testMap) {
 
     Iterator itr2 = testMap.keySet().iterator();
     int i = 0;
+    int j = 1;
+    int k = 2;
+    int l = 3;
+    while (itr2.hasNext()) {
+      String key = (String) itr2.next();
+      testMap.get(key);
+    }
+  }
+
+  void inefficient_loop_itr_heur_bad(HashMap<String, Integer> testMap) {
+
+    Iterator itr2 = testMap.keySet().iterator();
+    int i = 0;
+    while (itr2.hasNext()) {
+      String key = (String) itr2.next();
+      testMap.get(key);
+    }
+  }
+
+  void inefficient_loop_itr_heur_btw_bad(HashMap<String, Integer> testMap) {
+
+    Set<String> keySet = testMap.keySet();
+    int i = 0;
+    int j = 1;
+    int l = 3;
+    Iterator itr2 = keySet.iterator();
     while (itr2.hasNext()) {
       String key = (String) itr2.next();
       testMap.get(key);

infer/tests/codetoanalyze/java/inefficientKeysetIterator/issues.exp

@@ -1,2 +1,4 @@
 codetoanalyze/java/inefficientKeysetIterator/Test.java, Test.inefficient_loop_bad(java.util.HashMap):void, 2, INEFFICIENT_KEYSET_ITERATOR, no_bucket, ERROR, [Accessing a value using a key that was retrieved from a `keySet` iterator. It is more efficient to use an iterator on the `entrySet` of the map, avoiding the extra `HashMap.get(key)` lookup.]
 codetoanalyze/java/inefficientKeysetIterator/Test.java, Test.inefficient_loop_itr_bad(java.util.HashMap):void, 5, INEFFICIENT_KEYSET_ITERATOR, no_bucket, ERROR, [Accessing a value using a key that was retrieved from a `keySet` iterator. It is more efficient to use an iterator on the `entrySet` of the map, avoiding the extra `HashMap.get(key)` lookup.]
+codetoanalyze/java/inefficientKeysetIterator/Test.java, Test.inefficient_loop_itr_heur_bad(java.util.HashMap):void, 6, INEFFICIENT_KEYSET_ITERATOR, no_bucket, ERROR, [Accessing a value using a key that was retrieved from a `keySet` iterator. It is more efficient to use an iterator on the `entrySet` of the map, avoiding the extra `HashMap.get(key)` lookup.]
+codetoanalyze/java/inefficientKeysetIterator/Test.java, Test.inefficient_loop_itr_heur_btw_bad(java.util.HashMap):void, 9, INEFFICIENT_KEYSET_ITERATOR, no_bucket, ERROR, [Accessing a value using a key that was retrieved from a `keySet` iterator. It is more efficient to use an iterator on the `entrySet` of the map, avoiding the extra `HashMap.get(key)` lookup.]