diff --git a/Makefile b/Makefile index 96a8efbe0..6bfb7450d 100644 --- a/Makefile +++ b/Makefile @@ -139,6 +139,7 @@ DIRECT_TESTS += \ java_eradicate \ java_hoisting \ java_hoistingExpensive \ + java_inefficientKeysetIterator \ java_infer \ java_performance \ java_purity \ diff --git a/infer/man/man1/infer-analyze.txt b/infer/man/man1/infer-analyze.txt index f376897ab..e56b402b7 100644 --- a/infer/man/man1/infer-analyze.txt +++ b/infer/man/man1/infer-analyze.txt @@ -141,6 +141,15 @@ OPTIONS Activates: Enable --immutable-cast and disable all other checkers (Conversely: --no-immutable-cast-only) + --inefficient-keyset-iterator + Activates: Check for inefficient uses of keySet iterator instead + of entrySet iterator. (Conversely: + --no-inefficient-keyset-iterator) + + --inefficient-keyset-iterator-only + Activates: Enable --inefficient-keyset-iterator and disable all + other checkers (Conversely: --no-inefficient-keyset-iterator-only) + --jobs,-j int Run the specified number of analysis jobs simultaneously (default: ) diff --git a/infer/man/man1/infer-full.txt b/infer/man/man1/infer-full.txt index 402385e84..469197488 100644 --- a/infer/man/man1/infer-full.txt +++ b/infer/man/man1/infer-full.txt @@ -389,6 +389,7 @@ OPTIONS (disabled by default), GRAPHQL_FIELD_ACCESS (enabled by default), GUARDEDBY_VIOLATION (enabled by default), + INEFFICIENT_KEYSET_ITERATOR (enabled by default), INFERBO_ALLOC_IS_BIG (enabled by default), INFERBO_ALLOC_IS_NEGATIVE (enabled by default), INFERBO_ALLOC_IS_ZERO (enabled by default), @@ -614,6 +615,16 @@ OPTIONS Activates: Enable --immutable-cast and disable all other checkers (Conversely: --no-immutable-cast-only) See also infer-analyze(1). + --inefficient-keyset-iterator + Activates: Check for inefficient uses of keySet iterator instead + of entrySet iterator. (Conversely: + --no-inefficient-keyset-iterator) See also infer-analyze(1). + + --inefficient-keyset-iterator-only + Activates: Enable --inefficient-keyset-iterator and disable all + other checkers (Conversely: --no-inefficient-keyset-iterator-only) + See also infer-analyze(1). + --iphoneos-target-sdk-version string Specify the target SDK version to use for iphoneos See also infer-capture(1). diff --git a/infer/man/man1/infer-report.txt b/infer/man/man1/infer-report.txt index 1e2f15c3c..ee7aca90b 100644 --- a/infer/man/man1/infer-report.txt +++ b/infer/man/man1/infer-report.txt @@ -129,6 +129,7 @@ OPTIONS (disabled by default), GRAPHQL_FIELD_ACCESS (enabled by default), GUARDEDBY_VIOLATION (enabled by default), + INEFFICIENT_KEYSET_ITERATOR (enabled by default), INFERBO_ALLOC_IS_BIG (enabled by default), INFERBO_ALLOC_IS_NEGATIVE (enabled by default), INFERBO_ALLOC_IS_ZERO (enabled by default), diff --git a/infer/man/man1/infer.txt b/infer/man/man1/infer.txt index c072f899f..bf7cba45f 100644 --- a/infer/man/man1/infer.txt +++ b/infer/man/man1/infer.txt @@ -389,6 +389,7 @@ OPTIONS (disabled by default), GRAPHQL_FIELD_ACCESS (enabled by default), GUARDEDBY_VIOLATION (enabled by default), + INEFFICIENT_KEYSET_ITERATOR (enabled by default), INFERBO_ALLOC_IS_BIG (enabled by default), INFERBO_ALLOC_IS_NEGATIVE (enabled by default), INFERBO_ALLOC_IS_ZERO (enabled by default), @@ -614,6 +615,16 @@ OPTIONS Activates: Enable --immutable-cast and disable all other checkers (Conversely: --no-immutable-cast-only) See also infer-analyze(1). + --inefficient-keyset-iterator + Activates: Check for inefficient uses of keySet iterator instead + of entrySet iterator. (Conversely: + --no-inefficient-keyset-iterator) See also infer-analyze(1). + + --inefficient-keyset-iterator-only + Activates: Enable --inefficient-keyset-iterator and disable all + other checkers (Conversely: --no-inefficient-keyset-iterator-only) + See also infer-analyze(1). + --iphoneos-target-sdk-version string Specify the target SDK version to use for iphoneos See also infer-capture(1). diff --git a/infer/src/IR/Instrs.ml b/infer/src/IR/Instrs.ml index 5b37fa4e5..5f9be774f 100644 --- a/infer/src/IR/Instrs.ml +++ b/infer/src/IR/Instrs.ml @@ -56,6 +56,8 @@ type not_reversed_t = not_reversed t let of_array instrs = NotReversed instrs +let get_underlying_not_reversed = function NotReversed instrs -> instrs + let empty = of_array [||] let singleton instr = of_array [|instr|] diff --git a/infer/src/IR/Instrs.mli b/infer/src/IR/Instrs.mli index a973d819a..8e115d735 100644 --- a/infer/src/IR/Instrs.mli +++ b/infer/src/IR/Instrs.mli @@ -62,3 +62,5 @@ val pp : Pp.env -> Format.formatter -> _ t -> unit val fold : (_ t, Sil.instr, 'a) Container.fold val iter : (_ t, Sil.instr) Container.iter + +val get_underlying_not_reversed : not_reversed t -> Sil.instr array diff --git a/infer/src/base/Config.ml b/infer/src/base/Config.ml index dad5100ea..c5da916f0 100644 --- a/infer/src/base/Config.ml +++ b/infer/src/base/Config.ml @@ -608,6 +608,7 @@ and ( annotation_reachability , eradicate , fragment_retains_view , immutable_cast + , inefficient_keyset_iterator , linters , litho , liveness @@ -654,6 +655,9 @@ and ( annotation_reachability mk_checker ~long:"immutable-cast" ~default:false "the detection of object cast from immutable type to mutable type. For instance, it will \ detect cast from ImmutableList to List, ImmutableMap to Map, and ImmutableSet to Set." + and inefficient_keyset_iterator = + mk_checker ~long:"inefficient-keyset-iterator" ~default:false + "Check for inefficient uses of keySet iterator instead of entrySet iterator." and linters = mk_checker ~long:"linters" ~default:true "syntactic linters" and litho = mk_checker ~long:"litho" "Experimental checkers supporting the Litho framework" and liveness = @@ -727,6 +731,7 @@ and ( annotation_reachability , eradicate , fragment_retains_view , immutable_cast + , inefficient_keyset_iterator , linters , litho , liveness @@ -2823,6 +2828,8 @@ and icfg_dotty_outfile = !icfg_dotty_outfile and immutable_cast = !immutable_cast +and inefficient_keyset_iterator = !inefficient_keyset_iterator + and iphoneos_target_sdk_version = !iphoneos_target_sdk_version and iphoneos_target_sdk_version_path_regex = diff --git a/infer/src/base/Config.mli b/infer/src/base/Config.mli index 47d0b5c32..19b6712ff 100644 --- a/infer/src/base/Config.mli +++ b/infer/src/base/Config.mli @@ -404,6 +404,8 @@ val icfg_dotty_outfile : string option val immutable_cast : bool +val inefficient_keyset_iterator : bool + val infer_is_clang : bool val infer_is_javac : bool diff --git a/infer/src/base/IssueType.ml b/infer/src/base/IssueType.ml index f26ced994..f5af546dd 100644 --- a/infer/src/base/IssueType.ml +++ b/infer/src/base/IssueType.ml @@ -277,6 +277,8 @@ let graphql_field_access = from_string "GRAPHQL_FIELD_ACCESS" let guardedby_violation_racerd = from_string "GUARDEDBY_VIOLATION" ~hum:"GuardedBy Violation" +let inefficient_keyset_iterator = from_string "INEFFICIENT_KEYSET_ITERATOR" + let inferbo_alloc_is_big = from_string "INFERBO_ALLOC_IS_BIG" let inferbo_alloc_is_negative = from_string "INFERBO_ALLOC_IS_NEGATIVE" diff --git a/infer/src/base/IssueType.mli b/infer/src/base/IssueType.mli index cb61cc447..a9bcafa41 100644 --- a/infer/src/base/IssueType.mli +++ b/infer/src/base/IssueType.mli @@ -181,6 +181,8 @@ val graphql_field_access : t val guardedby_violation_racerd : t +val inefficient_keyset_iterator : t + val inferbo_alloc_is_big : t val inferbo_alloc_is_negative : t diff --git a/infer/src/checkers/cost.ml b/infer/src/checkers/cost.ml index ae61b5d4d..78c5bfa38 100644 --- a/infer/src/checkers/cost.ml +++ b/infer/src/checkers/cost.ml @@ -824,10 +824,7 @@ let checker {Callbacks.tenv; proc_desc; integer_type_widths; summary} : Summary. (* computes reaching defs: node -> (var -> node set) *) let reaching_defs_invariant_map = ReachingDefs.compute_invariant_map proc_desc tenv in (* collect all prune nodes that occur in loop guards, needed for ControlDepAnalyzer *) - let control_maps, loop_head_to_loop_nodes = - let loop_head_to_source_nodes_map = Loop_control.get_loop_head_to_source_nodes node_cfg in - Loop_control.get_control_maps loop_head_to_source_nodes_map - in + let control_maps, loop_head_to_loop_nodes = Loop_control.get_loop_control_maps node_cfg in (* computes the control dependencies: node -> var set *) let control_dep_invariant_map = Control.compute_invariant_map proc_desc tenv control_maps in (* compute loop invariant map for control var analysis *) diff --git a/infer/src/checkers/inefficientKeysetIterator.ml b/infer/src/checkers/inefficientKeysetIterator.ml new file mode 100644 index 000000000..a27baee03 --- /dev/null +++ b/infer/src/checkers/inefficientKeysetIterator.ml @@ -0,0 +1,95 @@ +(* + * Copyright (c) 2019-present, Facebook, Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + *) +open! IStd +module F = Format +module CFG = ProcCfg.Normal +module LoopNodes = AbstractDomain.FiniteSet (Procdesc.Node) + +let find_loaded_pvar id = function + | Sil.Load (lhs_id, Exp.Lvar rhs_pvar, _, _) when Ident.equal lhs_id id -> + Some rhs_pvar + | _ -> + None + + +let find_first_arg_id ~fun_name ~lhs_f = function + | Sil.Call ((lhs_id, _), Exp.Const (Const.Cfun pname), (Exp.Var rhs_id, _) :: _, _, _) + when lhs_f lhs_id && String.equal fun_name (Typ.Procname.get_method pname) -> + Some rhs_id + | _ -> + None + + +(** If given a node that has 4 instructions and calls fun_name, + pickup bcvarY, i.e. variable for the first argument + n$X = *&$bcvarY + _ = *n$X + n$X+1 = fun_name(n$X,....) + *&$irvarZ = n$X+1 + *) +let find_first_arg_pvar node ~fun_name = + let instrs = Procdesc.Node.get_instrs node in + if Int.equal (Instrs.count instrs) 4 then + let instr_arr = Instrs.get_underlying_not_reversed instrs in + match instr_arr.(3) with + | Sil.Store (Exp.Lvar _, _, Exp.Var rhs_id, _) -> + find_first_arg_id ~fun_name ~lhs_f:(Ident.equal rhs_id) instr_arr.(2) + |> Option.bind ~f:(fun arg_id -> find_loaded_pvar arg_id instr_arr.(0)) + | _ -> + None + else None + + +let report_matching_get summary pvar loop_nodes : unit = + LoopNodes.iter + (fun node -> + let instrs = Procdesc.Node.get_instrs node in + if Int.equal (Instrs.count instrs) 5 then + let instr_arr = Instrs.get_underlying_not_reversed instrs in + find_first_arg_id ~fun_name:"get" ~lhs_f:(fun _ -> true) instr_arr.(3) + |> Option.iter ~f:(fun arg_id -> + find_loaded_pvar arg_id instr_arr.(0) + |> Option.iter ~f:(fun arg_pvar -> + if Pvar.equal arg_pvar pvar then + let pp_m = MarkupFormatter.pp_monospaced in + let exp_desc = + F.asprintf + "Accessing a value using a key that was retrieved from a %a iterator. \ + It is more efficient to use an iterator on the %a of the map, \ + avoiding the extra %a lookup." + pp_m "keySet" pp_m "entrySet" pp_m "HashMap.get(key)" + in + let loc = Procdesc.Node.get_loc node in + let ltr = [Errlog.make_trace_element 0 loc exp_desc []] in + Reporting.log_error summary ~loc ~ltr IssueType.inefficient_keyset_iterator + exp_desc ) ) ) + loop_nodes + + +let when_dominating_pred_satisfies idom my_node ~f = + let preds = + Procdesc.Node.get_preds my_node + |> List.filter ~f:(fun node -> Dominators.dominates idom node my_node) + in + match preds with [pred_node] -> f pred_node | _ -> () + + +let checker Callbacks.{summary; proc_desc} : Summary.t = + let cfg = CFG.from_pdesc proc_desc in + let _, loop_head_to_loop_nodes = Loop_control.get_loop_control_maps cfg in + let idom = Dominators.get_idoms proc_desc in + Procdesc.NodeMap.iter + (fun loop_head loop_nodes -> + if find_first_arg_pvar loop_head ~fun_name:"hasNext" |> 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") then + when_dominating_pred_satisfies idom itr_node ~f:(fun keySet_node -> + find_first_arg_pvar keySet_node ~fun_name:"keySet" + |> Option.iter ~f:(fun get_pvar -> + report_matching_get summary get_pvar loop_nodes ) ) ) ) + loop_head_to_loop_nodes ; + summary diff --git a/infer/src/checkers/loop_control.ml b/infer/src/checkers/loop_control.ml index a2354cd6e..c13728b3b 100644 --- a/infer/src/checkers/loop_control.ml +++ b/infer/src/checkers/loop_control.ml @@ -184,3 +184,8 @@ let get_control_maps loop_head_to_source_nodes_map = { exit_map= Control.ExitNodeToLoopHeads.empty ; loop_head_to_guard_nodes= Control.LoopHeadToGuardNodes.empty } , LoopInvariant.LoopHeadToLoopNodes.empty ) + + +let get_loop_control_maps cfg = + let loop_head_to_source_nodes_map = get_loop_head_to_source_nodes cfg in + get_control_maps loop_head_to_source_nodes_map diff --git a/infer/src/checkers/loop_control.mli b/infer/src/checkers/loop_control.mli index a1700468f..88dfcb3ef 100644 --- a/infer/src/checkers/loop_control.mli +++ b/infer/src/checkers/loop_control.mli @@ -20,9 +20,8 @@ val get_loop_head_to_source_nodes : Procdesc.t -> Procdesc.Node.t list Procdesc. loop_head (target of back-edges) --> source nodes *) -val get_control_maps : - Procdesc.Node.t list Procdesc.NodeMap.t - -> Control.loop_control_maps * Control.GuardNodes.t Procdesc.NodeMap.t +val get_loop_control_maps : + Procdesc.t -> Control.loop_control_maps * Control.GuardNodes.t Procdesc.NodeMap.t (** Get a pair of maps (exit_map, loop_head_to_guard_map) where exit_map : exit_node -> loop_head set (i.e. target of the back edges) diff --git a/infer/src/checkers/registerCheckers.ml b/infer/src/checkers/registerCheckers.ml index 0ffebdad9..c343928d2 100644 --- a/infer/src/checkers/registerCheckers.ml +++ b/infer/src/checkers/registerCheckers.ml @@ -67,6 +67,9 @@ let all_checkers = ; { name= "immutable cast" ; active= Config.immutable_cast ; callbacks= [(Procedure ImmutableChecker.callback_check_immutable_cast, Language.Java)] } + ; { name= "inefficient keyset iterator" + ; active= Config.inefficient_keyset_iterator + ; callbacks= [(Procedure InefficientKeysetIterator.checker, Language.Java)] } ; { name= "liveness" ; active= Config.liveness ; callbacks= [(Procedure Liveness.checker, Language.Clang)] } diff --git a/infer/tests/codetoanalyze/java/inefficientKeysetIterator/Makefile b/infer/tests/codetoanalyze/java/inefficientKeysetIterator/Makefile new file mode 100644 index 000000000..b450e41d5 --- /dev/null +++ b/infer/tests/codetoanalyze/java/inefficientKeysetIterator/Makefile @@ -0,0 +1,12 @@ +# Copyright (c) 2019-present, Facebook, Inc. +# +# This source code is licensed under the MIT license found in the +# LICENSE file in the root directory of this source tree. + +TESTS_DIR = ../../.. + +INFER_OPTIONS = --inefficient-keyset-iterator-only --debug-exceptions +INFERPRINT_OPTIONS = --issues-tests +SOURCES = $(wildcard *.java) + +include $(TESTS_DIR)/javac.make diff --git a/infer/tests/codetoanalyze/java/inefficientKeysetIterator/Test.java b/infer/tests/codetoanalyze/java/inefficientKeysetIterator/Test.java new file mode 100644 index 000000000..7b5e5d887 --- /dev/null +++ b/infer/tests/codetoanalyze/java/inefficientKeysetIterator/Test.java @@ -0,0 +1,60 @@ +/* + * Copyright (c) 2019-present, Facebook, Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ +import java.util.HashMap; +import java.util.Iterator; +import java.util.Map; + +class Test { + + void inefficient_loop_bad(HashMap testMap) { + for (String key : testMap.keySet()) { + testMap.get(key); + } + } + + void inefficient_loop_itr_bad(HashMap testMap) { + + Iterator itr2 = testMap.keySet().iterator(); + while (itr2.hasNext()) { + String key = (String) itr2.next(); + testMap.get(key); + } + } + + void inefficient_loop_itr_bad_FN(HashMap testMap) { + + Iterator itr2 = testMap.keySet().iterator(); + int i = 0; + while (itr2.hasNext()) { + String key = (String) itr2.next(); + testMap.get(key); + } + } + + void efficient_loop_itr_ok(HashMap testMap) { + + Iterator> itr1 = testMap.entrySet().iterator(); + while (itr1.hasNext()) { + Map.Entry entry = itr1.next(); + entry.getKey(); + entry.getValue(); + } + } + + void efficient_loop_ok(HashMap testMap) { + for (Map.Entry entry : testMap.entrySet()) { + entry.getKey(); + entry.getValue(); + } + } + + void negative_loop_ok(HashMap testMap1, HashMap testMap2) { + for (String key : testMap1.keySet()) { + testMap2.get(key); + } + } +} diff --git a/infer/tests/codetoanalyze/java/inefficientKeysetIterator/issues.exp b/infer/tests/codetoanalyze/java/inefficientKeysetIterator/issues.exp new file mode 100644 index 000000000..1f3e0d379 --- /dev/null +++ b/infer/tests/codetoanalyze/java/inefficientKeysetIterator/issues.exp @@ -0,0 +1,2 @@ +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.]