From f4d351372413b3abee7046b58e57ba6ce9cab0ba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ezgi=20=C3=87i=C3=A7ek?= Date: Tue, 10 Dec 2019 02:46:32 -0800 Subject: [PATCH] Revert "[litho] Check and report on all nodes where return calls" Summary: This reverts commit 4fd6165d190bab32544f9f040b777565432c15b2. We don't need to check for reporting each node anymore. It suffices to just check per function. Reviewed By: skcho Differential Revision: D18883833 fbshipit-source-id: 2591b3af3 --- 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 | 24 +----------- .../litho-required-props/RequiredProps.java | 4 +- .../java/litho-required-props/issues.exp | 3 +- .../java/litho/RequiredProps.java | 1 + 10 files changed, 13 insertions(+), 121 deletions(-) diff --git a/infer/src/absint/LowerHil.ml b/infer/src/absint/LowerHil.ml index f86ebcd50..a6cb1a6b5 100644 --- a/infer/src/absint/LowerHil.ml +++ b/infer/src/absint/LowerHil.ml @@ -84,18 +84,6 @@ 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 @@ -115,11 +103,6 @@ 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 @@ -144,14 +127,6 @@ 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 a9104338e..9c6a4093d 100644 --- a/infer/src/absint/LowerHil.mli +++ b/infer/src/absint/LowerHil.mli @@ -42,13 +42,6 @@ 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 4de01f7b0..781f76da8 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_on_post astate _tenv summary = + let report astate _tenv summary = let report_graphql_getter access_path call_chain = let call_strings = List.map @@ -57,9 +57,6 @@ 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 9166be6a8..e7dc308d2 100644 --- a/infer/src/checkers/LithoDomain.ml +++ b/infer/src/checkers/LithoDomain.ml @@ -187,12 +187,6 @@ 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} @@ -244,16 +238,6 @@ 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 @@ -293,10 +277,6 @@ 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 c7c43eb4e..2bbc4f418 100644 --- a/infer/src/checkers/LithoDomain.mli +++ b/infer/src/checkers/LithoDomain.mli @@ -82,13 +82,6 @@ val call_build_method : ret:LocalAccessPath.t -> receiver:LocalAccessPath.t -> t val check_required_props : check_on_string_set:(Typ.name -> MethodCallPrefix.t list -> String.Set.t -> unit) -> t -> unit -val check_required_props_of_receiver : - pname:Typ.Procname.t - -> check_on_string_set:(Typ.name -> MethodCallPrefix.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 cae7cca17..08196960a 100644 --- a/infer/src/checkers/LithoFramework.ml +++ b/infer/src/checkers/LithoFramework.ml @@ -88,10 +88,7 @@ module type LithoContext = sig val should_report : Procdesc.t -> Tenv.t -> bool - 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 report : t -> Tenv.t -> Summary.t -> unit val session_name : string end @@ -214,40 +211,19 @@ 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 - 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 -> + 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 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 ) + TF.Payload.update_summary payload summary + | None -> + summary end diff --git a/infer/src/checkers/RequiredProps.ml b/infer/src/checkers/RequiredProps.ml index 7d773d26c..d3084ac73 100644 --- a/infer/src/checkers/RequiredProps.ml +++ b/infer/src/checkers/RequiredProps.ml @@ -165,7 +165,7 @@ module LithoContext = struct check_on_string_set tenv summary parent_typename call_chain prop_set - let report_on_post astate tenv summary = + let report astate tenv summary = let check_required_prop_chain _ call_chain = let call_chain = List.drop_while call_chain ~f:(fun Domain.MethodCall.{procname} -> @@ -195,28 +195,6 @@ module LithoContext = struct else 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_prefix = check_on_string_set_prefix 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:check_on_string_set_prefix receiver astate ) ) - - let session_name = "litho required props" end diff --git a/infer/tests/codetoanalyze/java/litho-required-props/RequiredProps.java b/infer/tests/codetoanalyze/java/litho-required-props/RequiredProps.java index d6dfa2349..1ec914f16 100644 --- a/infer/tests/codetoanalyze/java/litho-required-props/RequiredProps.java +++ b/infer/tests/codetoanalyze/java/litho-required-props/RequiredProps.java @@ -220,7 +220,7 @@ public class RequiredProps { return builder2.build(); } - public void buildWithColumnChildBad_FN() { + public void buildWithColumnChildBad() { Column.Builder builder = Column.create(); Component.Builder childBuilder = mMyComponent.create().prop1(new Object()); // forgot prop 3, and builder.child() will invoke build() on childBuilder @@ -317,7 +317,7 @@ public class RequiredProps { return layoutBuilder.build(); } - public void castImpossibleOk_FP(Object o1) { + public void castImpossibleOk(Object o1) { Component.Builder builder = mMyLithoComponent.create(); if (builder instanceof MyComponent.Builder) ((MyComponent.Builder) builder) diff --git a/infer/tests/codetoanalyze/java/litho-required-props/issues.exp b/infer/tests/codetoanalyze/java/litho-required-props/issues.exp index 1822d0826..a0bb0700c 100644 --- a/infer/tests/codetoanalyze/java/litho-required-props/issues.exp +++ b/infer/tests/codetoanalyze/java/litho-required-props/issues.exp @@ -9,10 +9,9 @@ codetoanalyze/java/litho-required-props/RequiredProps.java, codetoanalyze.java.l codetoanalyze/java/litho-required-props/RequiredProps.java, codetoanalyze.java.litho.RequiredProps.buildPropMissingInConditionalBad(boolean):void, 0, MISSING_REQUIRED_PROP, no_bucket, ERROR, [@Prop prop3 is required for component codetoanalyze.java.litho.MyComponent, but is not set before the call to build(),calls MyComponent$Builder MyComponent$Builder.prop1(Object)] codetoanalyze/java/litho-required-props/RequiredProps.java, codetoanalyze.java.litho.RequiredProps.buildPropResMissingBad():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()] codetoanalyze/java/litho-required-props/RequiredProps.java, codetoanalyze.java.litho.RequiredProps.buildPropVarArgMissingBad():void, 0, MISSING_REQUIRED_PROP, no_bucket, ERROR, [Either @Prop props or @Prop(varArg = prop) is required for component codetoanalyze.java.litho.VarArgPropComponent, but is not set before the call to build()] +codetoanalyze/java/litho-required-props/RequiredProps.java, codetoanalyze.java.litho.RequiredProps.buildWithColumnChildBad():void, 0, MISSING_REQUIRED_PROP, no_bucket, ERROR, [@Prop prop3 is required for component codetoanalyze.java.litho.MyComponent, but is not set before the call to build(),calls MyComponent$Builder MyComponent$Builder.prop1(Object)] codetoanalyze/java/litho-required-props/RequiredProps.java, codetoanalyze.java.litho.RequiredProps.buildWithout1Bad():com.facebook.litho.Component, 0, MISSING_REQUIRED_PROP, no_bucket, ERROR, [@Prop prop1 is required for component codetoanalyze.java.litho.MyComponent, but is not set before the call to build(),calls MyComponent$Builder MyComponent$Builder.prop2(Object),calls MyComponent$Builder MyComponent$Builder.prop3(Object)] codetoanalyze/java/litho-required-props/RequiredProps.java, codetoanalyze.java.litho.RequiredProps.buildWithout3Bad():com.facebook.litho.Component, 0, MISSING_REQUIRED_PROP, no_bucket, ERROR, [@Prop prop3 is required for component codetoanalyze.java.litho.MyComponent, but is not set before the call to build(),calls MyComponent$Builder MyComponent$Builder.prop1(Object),calls MyComponent$Builder MyComponent$Builder.prop2(Object)] -codetoanalyze/java/litho-required-props/RequiredProps.java, codetoanalyze.java.litho.RequiredProps.castImpossibleOk_FP(java.lang.Object):void, 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()] -codetoanalyze/java/litho-required-props/RequiredProps.java, codetoanalyze.java.litho.RequiredProps.castImpossibleOk_FP(java.lang.Object):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()] codetoanalyze/java/litho-required-props/RequiredProps.java, codetoanalyze.java.litho.RequiredProps.doubleSetMissingBad():com.facebook.litho.Component, 0, MISSING_REQUIRED_PROP, no_bucket, ERROR, [@Prop prop1 is required for component codetoanalyze.java.litho.MyComponent, but is not set before the call to build(),calls Component$Builder Component$Builder.commonProp(Object),calls MyComponent$Builder MyComponent$Builder.prop3(Object)] codetoanalyze/java/litho-required-props/RequiredProps.java, codetoanalyze.java.litho.RequiredProps.generalTypeForgot3Bad():java.lang.Object, 0, MISSING_REQUIRED_PROP, no_bucket, ERROR, [@Prop prop3 is required for component codetoanalyze.java.litho.MyComponent, but is not set before the call to build(),calls MyComponent$Builder MyComponent$Builder.prop1(Object)] codetoanalyze/java/litho-required-props/RequiredProps.java, codetoanalyze.java.litho.RequiredProps.missingProp1InBothBranchesBeforeBuildBad(boolean):com.facebook.litho.Component, 0, MISSING_REQUIRED_PROP, no_bucket, ERROR, [@Prop prop1 is required for component codetoanalyze.java.litho.MyComponent, but is not set before the call to build(),calls MyComponent$Builder MyComponent$Builder.prop3(Object)] diff --git a/infer/tests/codetoanalyze/java/litho/RequiredProps.java b/infer/tests/codetoanalyze/java/litho/RequiredProps.java index 5dd4f3ab5..68225adb1 100644 --- a/infer/tests/codetoanalyze/java/litho/RequiredProps.java +++ b/infer/tests/codetoanalyze/java/litho/RequiredProps.java @@ -414,4 +414,5 @@ public class RequiredProps { : mMyLithoComponent.create().prop1(new Object()).prop2(new Object()); return builder.build(); } + }