From 2e129a5abe764ca5ccaad7369c343a7ad07eb390 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ezgi=20=C3=87i=C3=A7ek?= Date: Tue, 8 Oct 2019 09:41:36 -0700 Subject: [PATCH] [required-props] Add call chain to trace Summary: Improve the trace by incorporating the callees and their locations in the call chain (i.e. chain of methods starting from `build()` call) - extend the domain to contain the callee location - replace the test results with the new traces This makes our job much easier to debug FPs in a big codebase. Reviewed By: skcho Differential Revision: D17788996 fbshipit-source-id: 31938b5fe --- infer/src/checkers/GraphQLFieldAccess.ml | 5 +++- infer/src/checkers/LithoDomain.ml | 13 +++++----- infer/src/checkers/LithoDomain.mli | 9 ++++--- infer/src/checkers/LithoFramework.ml | 4 +-- infer/src/checkers/RequiredProps.ml | 25 +++++++++++++------ .../tests/codetoanalyze/java/litho/issues.exp | 20 +++++++-------- 6 files changed, 46 insertions(+), 30 deletions(-) diff --git a/infer/src/checkers/GraphQLFieldAccess.ml b/infer/src/checkers/GraphQLFieldAccess.ml index d1a9305e6..75d1181f8 100644 --- a/infer/src/checkers/GraphQLFieldAccess.ml +++ b/infer/src/checkers/GraphQLFieldAccess.ml @@ -43,7 +43,10 @@ module LithoContext = struct let report astate _tenv summary = let report_graphql_getter access_path call_chain = let call_strings = - List.map ~f:(Typ.Procname.to_simplified_string ~withclass:false) call_chain + List.map + ~f:(fun Domain.MethodCall.{procname} -> + Typ.Procname.to_simplified_string ~withclass:false procname ) + call_chain in let call_string = String.concat ~sep:"." call_strings in let message = F.asprintf "%a.%s" AccessPath.pp access_path call_string in diff --git a/infer/src/checkers/LithoDomain.ml b/infer/src/checkers/LithoDomain.ml index 6e52318d6..36fe9dc9a 100644 --- a/infer/src/checkers/LithoDomain.ml +++ b/infer/src/checkers/LithoDomain.ml @@ -27,9 +27,10 @@ module LocalAccessPath = struct end module MethodCall = struct - type t = {receiver: LocalAccessPath.t; procname: Typ.Procname.t} [@@deriving compare] + type t = {receiver: LocalAccessPath.t; procname: Typ.Procname.t; location: Location.t} + [@@deriving compare] - let make receiver procname = {receiver; procname} + let make receiver procname location = {receiver; procname; location} let pp fmt {receiver; procname} = F.fprintf fmt "%a.%a" LocalAccessPath.pp receiver Typ.Procname.pp procname @@ -50,11 +51,11 @@ let substitute ~(f_sub : LocalAccessPath.t -> LocalAccessPath.t option) astate = in let call_set' = CallSet.fold - (fun ({procname} as call) call_set_acc -> + (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} call_set_acc ) + CallSet.add {receiver; procname; location} call_set_acc ) call_set CallSet.empty in add access_path' call_set' acc ) @@ -65,12 +66,12 @@ let substitute ~(f_sub : LocalAccessPath.t -> LocalAccessPath.t option) astate = maximal chain. For example, if the domain encodes the chains foo().bar().goo() and foo().baz(), [f] will be called once on foo().bar().goo() and once on foo().baz() *) let iter_call_chains_with_suffix ~f call_suffix astate = - let rec unroll_call_ ({receiver; procname} : MethodCall.t) (acc, visited) = + let rec unroll_call_ ({receiver; procname} as call : MethodCall.t) (acc, visited) = let is_cycle (call : MethodCall.t) = (* detect direct cycles and cycles due to mutual recursion *) LocalAccessPath.equal call.receiver receiver || Typ.Procname.Set.mem call.procname visited in - let acc' = procname :: acc in + let acc' = call :: acc in let visited' = Typ.Procname.Set.add procname visited in try let calls' = find receiver astate in diff --git a/infer/src/checkers/LithoDomain.mli b/infer/src/checkers/LithoDomain.mli index cb3fcf032..2d3a1ea89 100644 --- a/infer/src/checkers/LithoDomain.mli +++ b/infer/src/checkers/LithoDomain.mli @@ -19,11 +19,12 @@ module LocalAccessPath : sig val pp : F.formatter -> t -> unit end -(** Called procedure + its receiver *) +(** Called procedure & location + its receiver *) module MethodCall : sig - type t = private {receiver: LocalAccessPath.t; procname: Typ.Procname.t} [@@deriving compare] + type t = private {receiver: LocalAccessPath.t; procname: Typ.Procname.t; location: Location.t} + [@@deriving compare] - val make : LocalAccessPath.t -> Typ.Procname.t -> t + val make : LocalAccessPath.t -> Typ.Procname.t -> Location.t -> t val pp : F.formatter -> t -> unit end @@ -36,5 +37,5 @@ 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 *) -val iter_call_chains : f:(AccessPath.t -> Typ.Procname.t list -> unit) -> t -> unit +val iter_call_chains : f:(AccessPath.t -> MethodCall.t list -> unit) -> t -> unit (** Apply [f] to each maximal call chain encoded in [t] *) diff --git a/infer/src/checkers/LithoFramework.ml b/infer/src/checkers/LithoFramework.ml index ec5187782..fa5d55222 100644 --- a/infer/src/checkers/LithoFramework.ml +++ b/infer/src/checkers/LithoFramework.ml @@ -103,7 +103,7 @@ struct , Direct (Typ.Procname.Java java_callee_procname as callee_pname) , (HilExp.AccessExpression receiver_ae :: _ as actuals) , _ - , _ ) -> + , location ) -> let domain_summary = Payload.read ~caller_summary:summary ~callee_pname in let receiver = Domain.LocalAccessPath.make @@ -122,7 +122,7 @@ struct let return_calls = ( try Domain.find return_access_path astate with Caml.Not_found -> Domain.CallSet.empty ) - |> Domain.CallSet.add (Domain.MethodCall.make receiver callee_pname) + |> Domain.CallSet.add (Domain.MethodCall.make receiver callee_pname location) in Domain.add return_access_path return_calls astate else diff --git a/infer/src/checkers/RequiredProps.ml b/infer/src/checkers/RequiredProps.ml index 595f703ab..0bc36c911 100644 --- a/infer/src/checkers/RequiredProps.ml +++ b/infer/src/checkers/RequiredProps.ml @@ -58,7 +58,7 @@ let get_required_props typename tenv = [] -let report_missing_required_prop summary prop parent_typename loc = +let report_missing_required_prop summary prop parent_typename loc call_chain = let message = let prop_string = match prop with @@ -70,7 +70,12 @@ let report_missing_required_prop summary prop parent_typename loc = F.asprintf "%s is required for component %s, but is not set before the call to build()" prop_string (Typ.Name.name parent_typename) in - let ltr = [Errlog.make_trace_element 0 loc message []] in + let ltr = + Errlog.make_trace_element 0 loc message [] + :: List.map call_chain ~f:(fun Domain.MethodCall.{procname; location} -> + let call_msg = F.asprintf "calls %a" Typ.Procname.pp procname in + Errlog.make_trace_element 0 location call_msg [] ) + in Reporting.log_error summary ~loc ~ltr IssueType.missing_required_prop message @@ -78,8 +83,8 @@ let report_missing_required_prop summary prop parent_typename loc = the Litho framework (i.e., is client code) *) let find_client_component_type call_chain = List.find_map - ~f:(fun pname -> - match pname with + ~f:(fun Domain.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) | _ -> @@ -131,19 +136,25 @@ module LithoContext = struct let check_required_prop_chain _ call_chain = let rev_chain = List.rev call_chain in match rev_chain with - | pname :: _ when LithoFramework.is_component_build_method pname tenv -> ( + | Domain.MethodCall.{procname} :: _ + when LithoFramework.is_component_build_method procname tenv -> ( (* Here, we'll have a type name like MyComponent$Builder in hand. Truncate the $Builder part from the typename, then look at the fields of MyComponent to figure out which 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:Typ.Procname.get_method call_chain |> String.Set.of_list in + let prop_set = + List.map + ~f:(fun Domain.MethodCall.{procname} -> Typ.Procname.get_method procname) + 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) ) + (Summary.get_loc summary) call_chain ) required_props | _ -> () ) diff --git a/infer/tests/codetoanalyze/java/litho/issues.exp b/infer/tests/codetoanalyze/java/litho/issues.exp index b1bc18ab3..0bc4ce95d 100644 --- a/infer/tests/codetoanalyze/java/litho/issues.exp +++ b/infer/tests/codetoanalyze/java/litho/issues.exp @@ -1,13 +1,13 @@ -codetoanalyze/java/litho/RequiredProps.java, codetoanalyze.java.litho.RequiredProps.FP_setRequiredOnBothBranchesOk(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()] -codetoanalyze/java/litho/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/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/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()] -codetoanalyze/java/litho/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()] -codetoanalyze/java/litho/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()] -codetoanalyze/java/litho/RequiredProps.java, codetoanalyze.java.litho.RequiredProps.callBuildSuffixWithoutRequiredBad():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()] -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()] -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()] -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()] +codetoanalyze/java/litho/RequiredProps.java, codetoanalyze.java.litho.RequiredProps.FP_setRequiredOnBothBranchesOk(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.prop2(Object),calls MyComponent$Builder MyComponent$Builder.prop3(Object),calls MyComponent MyComponent$Builder.build()] +codetoanalyze/java/litho/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(),calls ResPropComponent ResPropComponent$Builder.build()] +codetoanalyze/java/litho/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(),calls VarArgPropComponent VarArgPropComponent$Builder.build()] +codetoanalyze/java/litho/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),calls Component Component$Builder.build()] +codetoanalyze/java/litho/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),calls MyComponent MyComponent$Builder.build()] +codetoanalyze/java/litho/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),calls MyComponent MyComponent$Builder.build()] +codetoanalyze/java/litho/RequiredProps.java, codetoanalyze.java.litho.RequiredProps.callBuildSuffixWithoutRequiredBad():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),calls MyComponent MyComponent$Builder.build()] +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$Builder.prop1(Object),calls MyComponent 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$Builder.prop3(Object),calls MyComponent$Builder MyComponent$Builder.prop2(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$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()] codetoanalyze/java/litho/ShouldUpdate.java, com.facebook.graphql.model.LithoTest.onCreateLayout():void, 0, GRAPHQL_FIELD_ACCESS, no_bucket, ERROR, [story.getActors().get(...).toString()] codetoanalyze/java/litho/ShouldUpdate.java, com.facebook.graphql.model.LithoTest.onCreateLayout():void, 0, GRAPHQL_FIELD_ACCESS, no_bucket, ERROR, [story.getActors().get(...)]