diff --git a/infer/src/checkers/inefficientKeysetIterator.ml b/infer/src/checkers/inefficientKeysetIterator.ml index 2165285bf..ef7a49bed 100644 --- a/infer/src/checkers/inefficientKeysetIterator.ml +++ b/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 diff --git a/infer/tests/codetoanalyze/java/inefficientKeysetIterator/Test.java b/infer/tests/codetoanalyze/java/inefficientKeysetIterator/Test.java index 0260374fd..58f29fb07 100644 --- a/infer/tests/codetoanalyze/java/inefficientKeysetIterator/Test.java +++ b/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 testMap) { + void inefficient_loop_itr_heur_bad_FN(HashMap 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 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 testMap) { + + Set 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); diff --git a/infer/tests/codetoanalyze/java/inefficientKeysetIterator/issues.exp b/infer/tests/codetoanalyze/java/inefficientKeysetIterator/issues.exp index 1f3e0d379..ce0b6d014 100644 --- a/infer/tests/codetoanalyze/java/inefficientKeysetIterator/issues.exp +++ b/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.]