From 47790ed4965fc8ed13f7311ee40fe3319bb40a73 Mon Sep 17 00:00:00 2001 From: Sungkeun Cho Date: Thu, 5 Dec 2019 05:45:11 -0800 Subject: [PATCH] [litho] Add prop check in the new domain Summary: This diff checks litho condition on the new abstract value. This is triggered with `--new-litho-domain`, but it is intra-procedural as of now. Reviewed By: ezgicicek Differential Revision: D18783203 fbshipit-source-id: 98570104e --- infer/man/man1/infer-full.txt | 4 + infer/src/base/Config.ml | 4 + infer/src/base/Config.mli | 2 + infer/src/checkers/LithoDomain.ml | 122 +++++++++++++----- infer/src/checkers/LithoDomain.mli | 13 +- infer/src/checkers/LithoFramework.ml | 23 +++- infer/src/checkers/RequiredProps.ml | 25 ++-- .../java/litho/RequiredProps.java | 4 +- .../tests/codetoanalyze/java/litho/issues.exp | 2 - 9 files changed, 148 insertions(+), 51 deletions(-) diff --git a/infer/man/man1/infer-full.txt b/infer/man/man1/infer-full.txt index 2d028401d..b09939f87 100644 --- a/infer/man/man1/infer-full.txt +++ b/infer/man/man1/infer-full.txt @@ -1539,6 +1539,10 @@ INTERNAL OPTIONS --never-returning-null json Matcher or list of matchers for functions that never return null. + --new-litho-domain + Activates: [EXPERIMENTAL] Use new litho domain (Conversely: + --no-new-litho-domain) + --nullable-annotation-name string Specify custom nullable annotation name diff --git a/infer/src/base/Config.ml b/infer/src/base/Config.ml index a4c9a1bf3..4a592c9d2 100644 --- a/infer/src/base/Config.ml +++ b/infer/src/base/Config.ml @@ -1740,6 +1740,8 @@ and monitor_prop_size = and nelseg = CLOpt.mk_bool ~deprecated:["nelseg"] ~long:"nelseg" "Use only nonempty lsegs" +and new_litho_domain = CLOpt.mk_bool ~long:"new-litho-domain" "[EXPERIMENTAL] Use new litho domain" + and nullable_annotation = CLOpt.mk_string_opt ~long:"nullable-annotation-name" "Specify custom nullable annotation name" @@ -3019,6 +3021,8 @@ and monitor_prop_size = !monitor_prop_size and nelseg = !nelseg +and new_litho_domain = !new_litho_domain + and nullable_annotation = !nullable_annotation and nullsafe_optimistic_third_party_params_in_non_strict = diff --git a/infer/src/base/Config.mli b/infer/src/base/Config.mli index a60df4a20..88cafaab1 100644 --- a/infer/src/base/Config.mli +++ b/infer/src/base/Config.mli @@ -492,6 +492,8 @@ val monitor_prop_size : bool val nelseg : bool +val new_litho_domain : bool + val no_translate_libs : bool val nullable_annotation : string option diff --git a/infer/src/checkers/LithoDomain.ml b/infer/src/checkers/LithoDomain.ml index 22a1b6d62..600309564 100644 --- a/infer/src/checkers/LithoDomain.ml +++ b/infer/src/checkers/LithoDomain.ml @@ -15,6 +15,10 @@ module LocalAccessPath = struct let make access_path parent = {access_path; parent} + let make_from_access_expression ae parent = + make (HilExp.AccessExpression.to_access_path ae) parent + + let to_formal_option {access_path= ((_, base_typ) as base), accesses; parent} formal_map = match FormalMap.get_formal_index base formal_map with | Some formal_index -> @@ -34,6 +38,9 @@ module MethodCall = struct let pp fmt {receiver; procname} = F.fprintf fmt "%a.%a" LocalAccessPath.pp receiver Typ.Procname.pp procname + + + let procname_to_string {procname} = Typ.Procname.get_method procname end module CallSet = AbstractDomain.FiniteSet (MethodCall) @@ -65,6 +72,7 @@ module NewDomain = struct if r <> 0 then r else Typ.Procname.compare x.procname y.procname end) + (* TODO: add return type && and do not add return method to the set *) type t = {is_build_method_called: IsBuildMethodCalled.t; method_calls: S.t} let pp fmt {is_build_method_called; method_calls} = @@ -98,7 +106,43 @@ module NewDomain = struct if is_build_method_called then x else {x with method_calls= S.add e method_calls} - let set_build_method_called x = {x with is_build_method_called= true} + (* TODO do not add callee to the set *) + let set_build_method_called callee x = + let x = add callee x in + {x with is_build_method_called= true} + + + let find_client_component_type method_calls = + let exception Found of Typ.name in + let f MethodCall.{procname} = + match procname with + | Typ.Procname.Java java_pname -> + Typ.Name.Java.get_outer_class (Typ.Procname.Java.get_class_type_name java_pname) + |> Option.iter ~f:(fun typ -> raise (Found typ)) + | _ -> + () + in + match S.iter f method_calls with () -> None | exception Found typ -> Some typ + + + let to_string_set method_calls = + let accum_as_string method_call acc = + String.Set.add acc (MethodCall.procname_to_string method_call) + in + S.fold accum_as_string method_calls String.Set.empty + + + let get_call_chain method_calls = + (* TODO: sort chain by inserted order *) + S.elements method_calls + + + let check_required_props ~check_on_string_set {is_build_method_called; method_calls} = + if is_build_method_called then + Option.iter (find_client_component_type method_calls) ~f:(fun parent_typename -> + let prop_set = to_string_set method_calls in + let call_chain = get_call_chain method_calls in + check_on_string_set parent_typename call_chain prop_set ) end module MethodCalled = struct @@ -120,16 +164,21 @@ module NewDomain = struct created_locations x - let build_method_called_one created_location x = + let build_method_called_one callee created_location x = let f v = let method_calls = Option.value v ~default:MethodCalls.empty in - Some (MethodCalls.set_build_method_called method_calls) + Some (MethodCalls.set_build_method_called callee method_calls) in update created_location f x - let build_method_called created_locations x = - CreatedLocations.fold build_method_called_one created_locations x + let build_method_called created_locations callee x = + CreatedLocations.fold (build_method_called_one callee) created_locations x + + + let check_required_props ~check_on_string_set x = + let f _ method_calls = MethodCalls.check_required_props ~check_on_string_set method_calls in + iter f x end type t = {created: Created.t; method_called: MethodCalled.t} @@ -171,10 +220,14 @@ module NewDomain = struct ; method_called= MethodCalled.add_all created_locations callee method_called } - let call_build_method ~ret ~receiver {created; method_called} = + let call_build_method ~ret ~receiver callee {created; method_called} = let created_locations = Created.lookup receiver created in { created= Created.add ret created_locations created - ; method_called= MethodCalled.build_method_called created_locations method_called } + ; method_called= MethodCalled.build_method_called created_locations callee method_called } + + + let check_required_props ~check_on_string_set {method_called} = + MethodCalled.check_required_props ~check_on_string_set method_called end include struct @@ -182,6 +235,8 @@ include struct let lift_old f (o, _) = f o + let lift_new f (_, n) = f n + let map_old f (o, n) = (f o, n) let map_new f (o, n) = (o, f n) @@ -208,30 +263,39 @@ include struct let call_builder ~ret ~receiver callee = map_new (NewDomain.call_builder ~ret ~receiver callee) - let call_build_method ~ret ~receiver = map_new (NewDomain.call_build_method ~ret ~receiver) + let call_build_method ~ret ~receiver callee = + map_new (NewDomain.call_build_method ~ret ~receiver callee) + + + let check_required_props ~check_on_string_set = + lift_new (NewDomain.check_required_props ~check_on_string_set) end -let substitute ~(f_sub : LocalAccessPath.t -> LocalAccessPath.t option) astate = - fold - (fun original_access_path call_set acc -> - let access_path' = - match f_sub original_access_path with - | Some access_path -> - access_path - | None -> - original_access_path - in - let call_set' = - CallSet.fold - (fun ({procname; location} as call) call_set_acc -> - let receiver = - match f_sub call.receiver with Some receiver' -> receiver' | None -> call.receiver - in - CallSet.add {receiver; procname; location} call_set_acc ) - call_set CallSet.empty - in - add access_path' call_set' acc ) - astate empty +let substitute ~(f_sub : LocalAccessPath.t -> LocalAccessPath.t option) ((_, new_astate) as astate) + = + let old_astate, _ = + fold + (fun original_access_path call_set acc -> + let access_path' = + match f_sub original_access_path with + | Some access_path -> + access_path + | None -> + original_access_path + in + let call_set' = + CallSet.fold + (fun ({procname; location} as call) call_set_acc -> + let receiver = + match f_sub call.receiver with Some receiver' -> receiver' | None -> call.receiver + in + CallSet.add {receiver; procname; location} call_set_acc ) + call_set CallSet.empty + in + add access_path' call_set' acc ) + astate empty + in + (old_astate, new_astate) (** Unroll the domain to enumerate all the call chains ending in [call] and apply [f] to each diff --git a/infer/src/checkers/LithoDomain.mli b/infer/src/checkers/LithoDomain.mli index 90d5295fb..915cb77e9 100644 --- a/infer/src/checkers/LithoDomain.mli +++ b/infer/src/checkers/LithoDomain.mli @@ -14,6 +14,8 @@ module LocalAccessPath : sig val make : AccessPath.t -> Typ.Procname.t -> t + val make_from_access_expression : HilExp.AccessExpression.t -> Typ.Procname.t -> t + val to_formal_option : t -> FormalMap.t -> t option val pp : F.formatter -> t -> unit @@ -27,6 +29,8 @@ module MethodCall : sig val make : LocalAccessPath.t -> Typ.Procname.t -> Location.t -> t val pp : F.formatter -> t -> unit + + val procname_to_string : t -> string end module CallSet : module type of AbstractDomain.FiniteSet (MethodCall) @@ -52,10 +56,17 @@ val bindings : t -> (LocalAccessPath.t * CallSet.t) list val assign : lhs:LocalAccessPath.t -> rhs:LocalAccessPath.t -> t -> t val call_create : LocalAccessPath.t -> Location.t -> t -> t +(** Semantics of builder creation method *) val call_builder : ret:LocalAccessPath.t -> receiver:LocalAccessPath.t -> MethodCall.t -> t -> t +(** Semantics of builder's methods, e.g. [prop] *) + +val call_build_method : + ret:LocalAccessPath.t -> receiver:LocalAccessPath.t -> MethodCall.t -> t -> t +(** Semantics of builder's final build method *) -val call_build_method : ret:LocalAccessPath.t -> receiver:LocalAccessPath.t -> t -> t +val check_required_props : + check_on_string_set:(Typ.name -> MethodCall.t list -> String.Set.t -> unit) -> 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 diff --git a/infer/src/checkers/LithoFramework.ml b/infer/src/checkers/LithoFramework.ml index 9545709c8..2bbaa17de 100644 --- a/infer/src/checkers/LithoFramework.ml +++ b/infer/src/checkers/LithoFramework.ml @@ -41,6 +41,14 @@ let is_component procname tenv = false +let is_call_build_inside procname tenv = + match Typ.Procname.get_method procname with + | "child" -> + is_component_builder procname tenv + | _ -> + false + + let is_component_build_method procname tenv = match Typ.Procname.get_method procname with | "build" -> @@ -121,9 +129,7 @@ struct , location ) -> let callee_summary_opt = Payload.read ~caller_summary:summary ~callee_pname in let receiver = - Domain.LocalAccessPath.make - (HilExp.AccessExpression.to_access_path receiver_ae) - caller_pname + Domain.LocalAccessPath.make_from_access_expression receiver_ae caller_pname in if ( LithoContext.check_callee ~callee_pname ~tenv callee_summary_opt @@ -142,7 +148,14 @@ struct if is_component_create_method callee_pname tenv then Domain.call_create return_access_path location astate else if is_component_build_method callee_pname tenv then - Domain.call_build_method ~ret:return_access_path ~receiver astate + Domain.call_build_method ~ret:return_access_path ~receiver callee astate + else if is_call_build_inside callee_pname tenv then + match actuals with + | _ :: HilExp.AccessExpression ae :: _ -> + let receiver = Domain.LocalAccessPath.make_from_access_expression ae caller_pname in + Domain.call_build_method ~ret:return_access_path ~receiver callee astate + | _ -> + astate else if is_component_builder callee_pname tenv then Domain.call_builder ~ret:return_access_path ~receiver callee astate else astate @@ -179,7 +192,7 @@ struct end module MakeAnalyzer (LithoContext : LithoContext with type t = Domain.t) = struct - module TF = TransferFunctions (ProcCfg.Exceptional) (LithoContext) + module TF = TransferFunctions (ProcCfg.Normal) (LithoContext) module A = LowerHil.MakeAbstractInterpreter (TF) let checker {Callbacks.summary; exe_env} = diff --git a/infer/src/checkers/RequiredProps.ml b/infer/src/checkers/RequiredProps.ml index e68a96087..d540bd2d1 100644 --- a/infer/src/checkers/RequiredProps.ml +++ b/infer/src/checkers/RequiredProps.ml @@ -154,6 +154,15 @@ module LithoContext = struct 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_required_prop_chain _ call_chain = let call_chain = List.drop_while call_chain ~f:(fun Domain.MethodCall.{procname} -> @@ -168,25 +177,17 @@ module LithoContext = struct ones are annotated with @Prop *) match find_client_component_type call_chain with | Some parent_typename -> - let required_props = get_required_props parent_typename tenv in let prop_set = - List.map - ~f:(fun Domain.MethodCall.{procname} -> Typ.Procname.get_method procname) - call_chain - |> String.Set.of_list + List.map ~f:Domain.MethodCall.procname_to_string call_chain |> String.Set.of_list 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 + check_on_string_set parent_typename call_chain prop_set | _ -> () ) | _ -> () in - Domain.iter_call_chains ~f:check_required_prop_chain astate + 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 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 949eec1d7..34cb8ffa4 100644 --- a/infer/tests/codetoanalyze/java/litho/RequiredProps.java +++ b/infer/tests/codetoanalyze/java/litho/RequiredProps.java @@ -80,7 +80,7 @@ public class RequiredProps { return builder.prop2(new Object()).prop3(new Object()).build(); } - public Component setRequiredOnBothBranchesOk_FP(boolean b) { + public Component setRequiredOnBothBranchesOk(boolean b) { MyComponent.Builder builder = mMyComponent.create(); if (b) { builder = builder.prop1(new Object()); @@ -102,7 +102,7 @@ public class RequiredProps { } // gets confused at cyclic dependency to builder when setting prop1 - public Component setRequiredOk_FP(boolean b) { + public Component setRequiredOk(boolean b) { MyComponent.Builder builder = mMyComponent.create(); builder = builder.prop1(new Object()); return builder.prop2(new Object()).prop3(new Object()).build(); diff --git a/infer/tests/codetoanalyze/java/litho/issues.exp b/infer/tests/codetoanalyze/java/litho/issues.exp index 7a5b63b17..280e1b526 100644 --- a/infer/tests/codetoanalyze/java/litho/issues.exp +++ b/infer/tests/codetoanalyze/java/litho/issues.exp @@ -14,11 +14,9 @@ codetoanalyze/java/litho/RequiredProps.java, codetoanalyze.java.litho.RequiredPr codetoanalyze/java/litho/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.create(),calls MyComponent$Builder MyComponent$Builder.prop1(Object),calls Component MyComponent$Builder.build()] codetoanalyze/java/litho/RequiredProps.java, codetoanalyze.java.litho.RequiredProps.setProp3InCalleeButForgetProp1Bad():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.create(),calls MyComponent$Builder MyComponent$Builder.prop3(Object),calls MyComponent$Builder MyComponent$Builder.prop2(Object),calls MyComponent MyComponent$Builder.build()] codetoanalyze/java/litho/RequiredProps.java, codetoanalyze.java.litho.RequiredProps.setRequiredEffectful_FP(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.create(),calls MyComponent$Builder MyComponent$Builder.prop2(Object),calls MyComponent$Builder MyComponent$Builder.prop3(Object),calls MyComponent MyComponent$Builder.build()] -codetoanalyze/java/litho/RequiredProps.java, codetoanalyze.java.litho.RequiredProps.setRequiredOk_FP(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.create(),calls MyComponent$Builder MyComponent$Builder.prop2(Object),calls MyComponent$Builder MyComponent$Builder.prop3(Object),calls MyComponent MyComponent$Builder.build()] codetoanalyze/java/litho/RequiredProps.java, codetoanalyze.java.litho.RequiredProps.setRequiredOnBothBranchesMissingProp3Bad(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.create(),calls MyComponent$Builder MyComponent$Builder.prop2(Object),calls MyComponent MyComponent$Builder.build()] codetoanalyze/java/litho/RequiredProps.java, codetoanalyze.java.litho.RequiredProps.setRequiredOnBothBranchesMissingProp3Bad(boolean):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.create(),calls MyComponent$Builder MyComponent$Builder.prop2(Object),calls MyComponent MyComponent$Builder.build()] codetoanalyze/java/litho/RequiredProps.java, codetoanalyze.java.litho.RequiredProps.setRequiredOnBothBranchesNoAssignOk_FP(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.create(),calls MyComponent$Builder MyComponent$Builder.prop2(Object),calls MyComponent$Builder MyComponent$Builder.prop3(Object),calls MyComponent MyComponent$Builder.build()] -codetoanalyze/java/litho/RequiredProps.java, codetoanalyze.java.litho.RequiredProps.setRequiredOnBothBranchesOk_FP(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.create(),calls MyComponent$Builder MyComponent$Builder.prop2(Object),calls MyComponent$Builder MyComponent$Builder.prop3(Object),calls MyComponent MyComponent$Builder.build()] codetoanalyze/java/litho/RequiredProps.java, codetoanalyze.java.litho.RequiredProps.setRequiredOnOneBranchBad(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.create(),calls MyComponent$Builder MyComponent$Builder.prop2(Object),calls MyComponent$Builder MyComponent$Builder.prop3(Object),calls MyComponent MyComponent$Builder.build()] codetoanalyze/java/litho/RequiredProps.java, codetoanalyze.java.litho.RequiredProps.setRequiredOnOneBranchEffectfulBad(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.create(),calls MyComponent$Builder MyComponent$Builder.prop2(Object),calls MyComponent$Builder MyComponent$Builder.prop3(Object),calls MyComponent MyComponent$Builder.build()] codetoanalyze/java/litho/ShouldUpdate.java, com.facebook.graphql.model.LithoTest.onCreateLayout():void, 0, GRAPHQL_FIELD_ACCESS, no_bucket, ERROR, [story.getActors()]