[hoisting] Don't report functions modeled as VariantForHoisting but consider them invariant

Reviewed By: ddino

Differential Revision: D13025250

fbshipit-source-id: ba1e39591

infer/src/checkers/hoisting.ml

@@ -75,8 +75,17 @@ let do_report summary Call.({pname; loc}) loop_head_loc =
   Reporting.log_error summary ~loc ~ltr IssueType.invariant_call message
 
 
-let should_report proc_desc Call.({pname; node; params}) inferbo_invariant_map =
-  (not Config.hoisting_report_only_expensive)
+let should_report tenv proc_desc Call.({pname; node; params}) inferbo_invariant_map =
+  (* If a function is modeled as variant for hoisting (like
+     List.size or __cast ), we don't want to report it *)
+  let is_variant_for_hoisting =
+    match InvariantModels.Call.dispatch tenv pname [] with
+    | Some inv ->
+        InvariantModels.is_variant_for_hoisting inv
+    | None ->
+        false
+  in
+  ((not is_variant_for_hoisting) && not Config.hoisting_report_only_expensive)
   ||
   (* only report if function call has expensive/symbolic cost *)
   match Ondemand.analyze_proc_name pname with
@@ -120,7 +129,7 @@ let checker ({Callbacks.tenv; summary; proc_desc} as callback_args) : Summary.t
       let loop_head_loc = Procdesc.Node.get_loc loop_head in
       HoistCalls.iter
         (fun call ->
-          if should_report proc_desc call inferbo_invariant_map then
+          if should_report tenv proc_desc call inferbo_invariant_map then
             do_report summary call loop_head_loc )
         inv_instrs )
     loop_head_to_inv_instrs ;

infer/src/checkers/invariantModels.ml

@@ -6,27 +6,32 @@
  *)
 
 open! IStd
+module BuiltinInvariantSet = Caml.Set.Make (String)
 
 type model = Invariant | Variant | VariantForHoisting
 
-(* Even though functions marked with VariantForHoisting are pure, we
-   don't want to report them in hoisting. Hence, we model them as
-   Variant. *)
-let is_invariant = function
-  | Invariant ->
-      true
-  | VariantForHoisting ->
-      not Config.loop_hoisting
-  | Variant ->
-      false
+let is_invariant = function Invariant -> true | VariantForHoisting -> true | Variant -> false
 
+let is_variant_for_hoisting = function VariantForHoisting -> true | _ -> false
+
+let invariants =
+  BuiltinInvariantSet.of_list
+    [ "__instanceof"
+    ; "__cast"
+    ; "__get_array_length"
+    ; "__get_type_of"
+    ; "__infer_assume"
+    ; "__infer_skip"
+    ; "__infer_fail" ]
+
+
+let invariant_builtins _ s = BuiltinInvariantSet.mem s invariants
 
 module Call = struct
   let dispatch : (Tenv.t, model) ProcnameDispatcher.Call.dispatcher =
     let open ProcnameDispatcher.Call in
     make_dispatcher
-      [ -"__cast" <>--> VariantForHoisting
-      ; +PatternMatch.implements_collection &:: "iterator" <>--> Variant
+      [ +PatternMatch.implements_collection &:: "iterator" <>--> Variant
       ; +PatternMatch.implements_iterator &:: "hasNext" <>--> Variant
       ; +PatternMatch.implements_enumeration &:: "hasMoreElements" <>--> Variant
       ; +PatternMatch.implements_enumeration &:: "nextElement" <>--> Variant
@@ -63,7 +68,7 @@ module Call = struct
       ; +PatternMatch.implements_io "Reader" &:: "read" <>--> Variant
       ; +PatternMatch.implements_io "BufferedReader" &:: "readLine" <>--> Variant
       ; +PatternMatch.implements_inject "Provider" &:: "get" <>--> Invariant
-      ; -"__new" <>--> Variant
+      ; +invariant_builtins <>--> VariantForHoisting
       ; +(fun _ name -> BuiltinDecl.is_declared (Typ.Procname.from_string_c_fun name))
-        <>--> VariantForHoisting ]
+        <>--> Variant ]
 end

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

@@ -6,6 +6,8 @@ codetoanalyze/java/hoisting/Hoist.java, Hoist.clash_function_calls_hoist(int):vo
 codetoanalyze/java/hoisting/Hoist.java, Hoist.dep_not_invariant_dont_hoist(int,int[]):void, 0, PURE_FUNCTION, no_bucket, ERROR, [Side-effect free function void Hoist.dep_not_invariant_dont_hoist(int,int[])]
 codetoanalyze/java/hoisting/Hoist.java, Hoist.dumb_foo():void, 0, PURE_FUNCTION, no_bucket, ERROR, [Side-effect free function void Hoist.dumb_foo()]
 codetoanalyze/java/hoisting/Hoist.java, Hoist.foo(int,int):int, 0, PURE_FUNCTION, no_bucket, ERROR, [Side-effect free function int Hoist.foo(int,int)]
+codetoanalyze/java/hoisting/Hoist.java, Hoist.get_array_length_dont_hoist(int[]):void, 0, PURE_FUNCTION, no_bucket, ERROR, [Side-effect free function void Hoist.get_array_length_dont_hoist(int[])]
+codetoanalyze/java/hoisting/Hoist.java, Hoist.instanceof_dont_hoist(Hoist$EmptyFoo):boolean, 0, PURE_FUNCTION, no_bucket, ERROR, [Side-effect free function boolean Hoist.instanceof_dont_hoist(Hoist$EmptyFoo)]
 codetoanalyze/java/hoisting/Hoist.java, Hoist.legit_hoist(int,int[]):void, 5, INVARIANT_CALL, no_bucket, ERROR, [Loop-invariant call to int Hoist.foo(int,int) at line 73]
 codetoanalyze/java/hoisting/Hoist.java, Hoist.loop_guard_hoist(int,int[]):void, 0, PURE_FUNCTION, no_bucket, ERROR, [Side-effect free function void Hoist.loop_guard_hoist(int,int[])]
 codetoanalyze/java/hoisting/Hoist.java, Hoist.loop_guard_hoist(int,int[]):void, 4, INVARIANT_CALL, no_bucket, ERROR, [Loop-invariant call to int Hoist.foo(int,int) at line 65]