[litho] Check and report on all nodes where return calls

Summary: This diff check and report on every nodes. Problem of the previous design is that it has to report alarms only with the abstract memory of the exit node. However, the new abstract value becomes imprecise at every join points on the path to the exit node, since it is using inverted map, i.e., under-approximation on collecting called methods. As a solution, this diff report on every nodes where `.build` is called with the abstract memory at that node.

Reviewed By: ezgicicek

Differential Revision: D18809449

fbshipit-source-id: 4fd6165d1

infer/src/absint/LowerHil.ml

@@ -84,6 +84,18 @@ module Make (TransferFunctions : TransferFunctions.HIL) (HilConfig : HilConfig)
         (Some instr, bindings)
 
 
+  let hil_instrs_of_sil bindings instrs =
+    let rev_hil_instrs, _bindings =
+      Instrs.fold instrs ~init:([], bindings) ~f:(fun (rev_hil_instrs, bindings) instr ->
+          let hil_instr_opt, bindings = hil_instr_of_sil bindings instr in
+          let rev_hil_instrs =
+            Option.fold hil_instr_opt ~init:rev_hil_instrs ~f:(fun acc hil_instr -> hil_instr :: acc)
+          in
+          (rev_hil_instrs, bindings) )
+    in
+    List.rev rev_hil_instrs
+
+
   let exec_instr ((actual_state, bindings) as astate) extras node instr =
     let actual_state', bindings' =
       match hil_instr_of_sil bindings instr with
@@ -103,6 +115,11 @@ module type S = sig
 
   val compute_post :
     Interpreter.TransferFunctions.extras ProcData.t -> initial:domain -> domain option
+
+  val exec_pdesc :
+    Interpreter.TransferFunctions.extras ProcData.t -> initial:domain -> Interpreter.invariant_map
+
+  val hil_instrs_of_sil : Bindings.t -> Instrs.not_reversed_t -> HilInstr.t list
 end
 
 module MakeAbstractInterpreterWithConfig
@@ -127,6 +144,14 @@ module MakeAbstractInterpreterWithConfig
           None
     in
     Interpreter.compute_post ~pp_instr proc_data ~initial:initial' |> Option.map ~f:fst
+
+
+  let exec_pdesc proc_data ~initial =
+    let initial' = (initial, Bindings.empty) in
+    Interpreter.exec_pdesc proc_data ~initial:initial'
+
+
+  let hil_instrs_of_sil = LowerHilInterpreter.hil_instrs_of_sil
 end
 
 module MakeAbstractInterpreter (TransferFunctions : TransferFunctions.HIL) =

infer/src/absint/LowerHil.mli

@@ -42,6 +42,13 @@ module type S = sig
   val compute_post :
     Interpreter.TransferFunctions.extras ProcData.t -> initial:domain -> domain option
   (** compute and return the postcondition for the given procedure starting from [initial]. *)
+
+  val exec_pdesc :
+    Interpreter.TransferFunctions.extras ProcData.t -> initial:domain -> Interpreter.invariant_map
+  (** compute and return the invariant map for the given procedure starting from [initial]. *)
+
+  val hil_instrs_of_sil : Bindings.t -> Instrs.not_reversed_t -> HilInstr.t list
+  (** construct HIL instructions from SIL instructions with initial bindings. *)
 end
 
 (** Wrapper around Interpreter to prevent clients from having to deal with IdAccessPathMapDomain.

infer/src/checkers/GraphQLFieldAccess.ml

@@ -40,7 +40,7 @@ module LithoContext = struct
     LithoFramework.is_on_create_layout (Procdesc.get_proc_name proc_desc)
 
 
-  let report astate _tenv summary =
+  let report_on_post astate _tenv summary =
     let report_graphql_getter access_path call_chain =
       let call_strings =
         List.map
@@ -57,6 +57,9 @@ module LithoContext = struct
     Domain.iter_call_chains ~f:report_graphql_getter astate
 
 
+  (* This checker will be removed later. *)
+  let report_on_inv_map ~inv_map_iter:_ _tenv _summary = ()
+
   let session_name = "litho graphql field access"
 end
 

infer/src/checkers/LithoDomain.ml

@@ -197,6 +197,12 @@ module NewDomain = struct
         MethodCalls.check_required_props ~check_on_string_set typ_name method_calls
       in
       iter f x
+
+
+    let check_required_props_of_created ~check_on_string_set ({CreatedLocation.typ_name} as created)
+        x =
+      Option.iter (find_opt created x) ~f:(fun method_calls ->
+          MethodCalls.check_required_props ~check_on_string_set typ_name method_calls )
   end
 
   type t = {created: Created.t; method_called: MethodCalled.t}
@@ -246,6 +252,16 @@ module NewDomain = struct
 
   let check_required_props ~check_on_string_set {method_called} =
     MethodCalled.check_required_props ~check_on_string_set method_called
+
+
+  let check_required_props_of_receiver ~pname ~check_on_string_set receiver {created; method_called}
+      =
+    let check_on_created_location created =
+      MethodCalled.check_required_props_of_created ~check_on_string_set created method_called
+    in
+    let receiver_path = LocalAccessPath.make_from_access_expression receiver pname in
+    let created_locations = Created.lookup receiver_path created in
+    CreatedLocations.iter check_on_created_location created_locations
 end
 
 include struct
@@ -285,6 +301,10 @@ include struct
 
   let check_required_props ~check_on_string_set =
     lift_new (NewDomain.check_required_props ~check_on_string_set)
+
+
+  let check_required_props_of_receiver ~pname ~check_on_string_set receiver =
+    lift_new (NewDomain.check_required_props_of_receiver ~pname ~check_on_string_set receiver)
 end
 
 let substitute ~(f_sub : LocalAccessPath.t -> LocalAccessPath.t option) ((_, new_astate) as astate)

infer/src/checkers/LithoDomain.mli

@@ -69,6 +69,13 @@ val call_build_method : ret:LocalAccessPath.t -> receiver:LocalAccessPath.t -> t
 val check_required_props :
   check_on_string_set:(Typ.name -> MethodCall.t list -> String.Set.t -> unit) -> t -> unit
 
+val check_required_props_of_receiver :
+     pname:Typ.Procname.t
+  -> check_on_string_set:(Typ.name -> MethodCall.t list -> String.Set.t -> unit)
+  -> HilExp.access_expression
+  -> t
+  -> unit
+
 val substitute : f_sub:(LocalAccessPath.t -> LocalAccessPath.t option) -> t -> t
 (** Substitute each access path in the domain using [f_sub]. If [f_sub] returns None, the original
     access path is retained; otherwise, the new one is used *)

infer/src/checkers/LithoFramework.ml

@@ -88,7 +88,10 @@ module type LithoContext = sig
 
   val should_report : Procdesc.t -> Tenv.t -> bool
 
-  val report : t -> Tenv.t -> Summary.t -> unit
+  val report_on_post : t -> Tenv.t -> Summary.t -> unit
+
+  val report_on_inv_map :
+    inv_map_iter:(f:(HilInstr.t list -> Domain.t -> unit) -> unit) -> Tenv.t -> Summary.t -> unit
 
   val session_name : string
 end
@@ -207,19 +210,40 @@ module MakeAnalyzer (LithoContext : LithoContext with type t = Domain.t) = struc
   module TF = TransferFunctions (ProcCfg.Normal) (LithoContext)
   module A = LowerHil.MakeAbstractInterpreter (TF)
 
+  let inv_map_iter proc_desc inv_map ~(f : HilInstr.t list -> Domain.t -> unit) =
+    let f node =
+      let node_id = Procdesc.Node.get_id node in
+      Option.iter (A.Interpreter.extract_state node_id inv_map)
+        ~f:(fun {AbstractInterpreter.State.pre= _, bindings; post= astate, _} ->
+          let sil_instrs = Procdesc.Node.get_instrs node in
+          f (A.hil_instrs_of_sil bindings sil_instrs) astate )
+    in
+    Procdesc.iter_nodes f proc_desc
+
+
   let checker {Callbacks.summary; exe_env} =
     let proc_desc = Summary.get_proc_desc summary in
     let tenv = Exe_env.get_tenv exe_env (Summary.get_proc_name summary) in
     let proc_data = ProcData.make_default summary tenv in
-    match A.compute_post proc_data ~initial:Domain.empty with
-    | Some post ->
-        if LithoContext.should_report proc_desc tenv then LithoContext.report post tenv summary ;
+    let post_opt =
+      if Config.new_litho_domain then (
+        let inv_map = A.exec_pdesc proc_data ~initial:Domain.empty in
+        if LithoContext.should_report proc_desc tenv then
+          LithoContext.report_on_inv_map ~inv_map_iter:(inv_map_iter proc_desc inv_map) tenv summary ;
+        let exit_node_id = Procdesc.Node.get_id (TF.CFG.exit_node proc_desc) in
+        A.Interpreter.extract_post exit_node_id inv_map |> Option.map ~f:fst )
+      else
+        let post_opt = A.compute_post proc_data ~initial:Domain.empty in
+        Option.iter post_opt ~f:(fun post ->
+            if LithoContext.should_report proc_desc tenv then
+              LithoContext.report_on_post post tenv summary ) ;
+        post_opt
+    in
+    Option.value_map post_opt ~default:summary ~f:(fun post ->
         let postprocess astate formal_map : Domain.t =
           let f_sub access_path = Domain.LocalAccessPath.to_formal_option access_path formal_map in
           Domain.substitute ~f_sub astate
         in
         let payload = postprocess post (FormalMap.make proc_desc) in
-        TF.Payload.update_summary payload summary
-    | None ->
-        summary
+        TF.Payload.update_summary payload summary )
 end

infer/src/checkers/RequiredProps.ml

@@ -152,16 +152,15 @@ module LithoContext = struct
     && Procdesc.get_access proc_desc <> PredSymb.Private
 
 
-  let report astate tenv summary =
-    let check_on_string_set parent_typename call_chain prop_set =
-      let required_props = get_required_props parent_typename tenv in
-      List.iter
-        ~f:(fun required_prop ->
-          if not (has_prop prop_set required_prop) then
-            report_missing_required_prop summary required_prop parent_typename
-              (Summary.get_loc summary) call_chain )
-        required_props
-    in
+  let check_on_string_set tenv summary parent_typename call_chain prop_set =
+    let required_props = get_required_props parent_typename tenv in
+    List.iter required_props ~f:(fun required_prop ->
+        if not (has_prop prop_set required_prop) then
+          report_missing_required_prop summary required_prop parent_typename
+            (Summary.get_loc summary) call_chain )
+
+
+  let report_on_post astate tenv summary =
     let check_required_prop_chain _ call_chain =
       let call_chain =
         List.drop_while call_chain ~f:(fun Domain.MethodCall.{procname} ->
@@ -179,14 +178,34 @@ module LithoContext = struct
             let prop_set =
               List.map ~f:Domain.MethodCall.procname_to_string call_chain |> String.Set.of_list
             in
-            check_on_string_set parent_typename call_chain prop_set
+            check_on_string_set tenv summary parent_typename call_chain prop_set
         | _ ->
             () )
       | _ ->
           ()
     in
-    if Config.new_litho_domain then Domain.check_required_props ~check_on_string_set astate
-    else Domain.iter_call_chains ~f:check_required_prop_chain astate
+    Domain.iter_call_chains ~f:check_required_prop_chain astate
+
+
+  let report_on_inv_map ~inv_map_iter tenv summary =
+    let find_return_instr instrs =
+      List.find_map instrs ~f:(fun instr ->
+          match instr with
+          | HilInstr.Call (_, Direct callee_pname, HilExp.AccessExpression receiver_ae :: _, _, _)
+            ->
+              if LithoFramework.is_call_build_inside callee_pname tenv then
+                (* TODO: inter-procedural checking *)
+                None
+              else if LithoFramework.is_component_builder callee_pname tenv then Some receiver_ae
+              else None
+          | _ ->
+              None )
+    in
+    let pname = Summary.get_proc_name summary in
+    let check_on_string_set = check_on_string_set tenv summary in
+    inv_map_iter ~f:(fun instrs astate ->
+        Option.iter (find_return_instr instrs) ~f:(fun receiver ->
+            Domain.check_required_props_of_receiver ~pname ~check_on_string_set receiver astate ) )
 
 
   let session_name = "litho required props"

infer/tests/codetoanalyze/java/litho/RequiredProps.java

@@ -322,4 +322,12 @@ public class RequiredProps {
       return mMyTreeComponent.create().build();
     }
   }
+
+  public Component buildPropLithoMissingInOneBranchBad(boolean b) {
+    if (b) {
+      return mMyLithoComponent.create().prop1(new Object()).build();
+    } else {
+      return mMyLithoComponent.create().prop1(new Object()).prop2(new Object()).build();
+    }
+  }
 }

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

@@ -1,5 +1,6 @@
 codetoanalyze/java/litho/RequiredProps.java, codetoanalyze.java.litho.RequiredProps.buildPropLithoMissingBothBad():com.facebook.litho.Component, 0, MISSING_REQUIRED_PROP, no_bucket, ERROR, [@Prop prop2 is required for component com.facebook.litho.MyLithoComponent, but is not set before the call to build(),calls MyLithoComponent$Builder MyLithoComponent.create(),calls MyLithoComponent MyLithoComponent$Builder.build()]
 codetoanalyze/java/litho/RequiredProps.java, codetoanalyze.java.litho.RequiredProps.buildPropLithoMissingBothBad():com.facebook.litho.Component, 0, MISSING_REQUIRED_PROP, no_bucket, ERROR, [@Prop prop1 is required for component com.facebook.litho.MyLithoComponent, but is not set before the call to build(),calls MyLithoComponent$Builder MyLithoComponent.create(),calls MyLithoComponent MyLithoComponent$Builder.build()]
+codetoanalyze/java/litho/RequiredProps.java, codetoanalyze.java.litho.RequiredProps.buildPropLithoMissingInOneBranchBad(boolean):com.facebook.litho.Component, 0, MISSING_REQUIRED_PROP, no_bucket, ERROR, [@Prop prop2 is required for component com.facebook.litho.MyLithoComponent, but is not set before the call to build(),calls MyLithoComponent$Builder MyLithoComponent.create(),calls MyLithoComponent$Builder MyLithoComponent$Builder.prop1(Object),calls MyLithoComponent MyLithoComponent$Builder.build()]
 codetoanalyze/java/litho/RequiredProps.java, codetoanalyze.java.litho.RequiredProps.buildPropLithoMissingOneBad():void, 0, MISSING_REQUIRED_PROP, no_bucket, ERROR, [@Prop prop2 is required for component com.facebook.litho.MyLithoComponent, but is not set before the call to build(),calls MyLithoComponent$Builder MyLithoComponent.create(),calls MyLithoComponent$Builder MyLithoComponent$Builder.prop1(Object),calls Component$Builder Component$Builder.commonProp(Object),calls Component Component$Builder.build()]
 codetoanalyze/java/litho/RequiredProps.java, codetoanalyze.java.litho.RequiredProps.buildPropLithoMissingOneInLoopBad(int):void, 0, MISSING_REQUIRED_PROP, no_bucket, ERROR, [@Prop prop2 is required for component com.facebook.litho.MyLithoComponent, but is not set before the call to build(),calls MyLithoComponent$Builder MyLithoComponent.create(),calls MyLithoComponent$Builder MyLithoComponent$Builder.prop1(Object),calls Component$Builder Component$Builder.commonProp(Object),calls Component Component$Builder.build()]
 codetoanalyze/java/litho/RequiredProps.java, codetoanalyze.java.litho.RequiredProps.buildPropResInCondOk_FP(boolean):void, 0, MISSING_REQUIRED_PROP, no_bucket, ERROR, [@Prop prop is required for component codetoanalyze.java.litho.ResPropComponent, but is not set before the call to build(),calls ResPropComponent$Builder ResPropComponent.create(),calls ResPropComponent ResPropComponent$Builder.build()]