From 5f77a3c1a5611666dc21d829bde6965915f144d0 Mon Sep 17 00:00:00 2001 From: Sungkeun Cho Date: Fri, 6 Dec 2019 03:19:37 -0800 Subject: [PATCH] [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 | 25 +++++++++++ infer/src/absint/LowerHil.mli | 7 +++ infer/src/checkers/GraphQLFieldAccess.ml | 5 ++- infer/src/checkers/LithoDomain.ml | 20 +++++++++ infer/src/checkers/LithoDomain.mli | 7 +++ infer/src/checkers/LithoFramework.ml | 38 +++++++++++++--- infer/src/checkers/RequiredProps.ml | 45 +++++++++++++------ .../java/litho/RequiredProps.java | 8 ++++ .../tests/codetoanalyze/java/litho/issues.exp | 1 + 9 files changed, 135 insertions(+), 21 deletions(-) diff --git a/infer/src/absint/LowerHil.ml b/infer/src/absint/LowerHil.ml index a6cb1a6b5..f86ebcd50 100644 --- a/infer/src/absint/LowerHil.ml +++ b/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) = diff --git a/infer/src/absint/LowerHil.mli b/infer/src/absint/LowerHil.mli index 9c6a4093d..a9104338e 100644 --- a/infer/src/absint/LowerHil.mli +++ b/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. diff --git a/infer/src/checkers/GraphQLFieldAccess.ml b/infer/src/checkers/GraphQLFieldAccess.ml index 781f76da8..4de01f7b0 100644 --- a/infer/src/checkers/GraphQLFieldAccess.ml +++ b/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 diff --git a/infer/src/checkers/LithoDomain.ml b/infer/src/checkers/LithoDomain.ml index e15c4fa89..3c586d3b5 100644 --- a/infer/src/checkers/LithoDomain.ml +++ b/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) diff --git a/infer/src/checkers/LithoDomain.mli b/infer/src/checkers/LithoDomain.mli index a1fe12678..dd6338ee4 100644 --- a/infer/src/checkers/LithoDomain.mli +++ b/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 *) diff --git a/infer/src/checkers/LithoFramework.ml b/infer/src/checkers/LithoFramework.ml index 0d0769b43..69f463fa7 100644 --- a/infer/src/checkers/LithoFramework.ml +++ b/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 diff --git a/infer/src/checkers/RequiredProps.ml b/infer/src/checkers/RequiredProps.ml index 19d1542cb..8829e0122 100644 --- a/infer/src/checkers/RequiredProps.ml +++ b/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" diff --git a/infer/tests/codetoanalyze/java/litho/RequiredProps.java b/infer/tests/codetoanalyze/java/litho/RequiredProps.java index 8161472cd..ddfd80f13 100644 --- a/infer/tests/codetoanalyze/java/litho/RequiredProps.java +++ b/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(); + } + } } diff --git a/infer/tests/codetoanalyze/java/litho/issues.exp b/infer/tests/codetoanalyze/java/litho/issues.exp index 09a7a18cb..add97473c 100644 --- a/infer/tests/codetoanalyze/java/litho/issues.exp +++ b/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()]