From 08f9cd4eb8d211d070820e4502df8435e5d582fa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ezgi=20=C3=87i=C3=A7ek?= Date: Wed, 9 Oct 2019 03:14:11 -0700 Subject: [PATCH] [required-props] Improve the heuristic to check build() for summaries Summary: Before, we didn't track litho framework callees on client code which was wrong. Now, we replace this with the following: If the callee is `build()` itself or doesn't contain a `build()` in its summary, then we want to track it in the domain. The former makes sense since we always want to track `build()` methods. The latter also makes sense since such a method could be a setter for a prop (as in the case of `prop1` in `buildPropLithoOK` which we were missing before due to the imprecise heuristic that prevented picking up callees in litho). Reviewed By: ngorogiannis Differential Revision: D17810704 fbshipit-source-id: 87d88e921 --- infer/src/checkers/GraphQLFieldAccess.ml | 2 +- infer/src/checkers/LithoFramework.ml | 17 ++++++++++------- infer/src/checkers/RequiredProps.ml | 18 +++++++++++++++--- .../java/litho/RequiredProps.java | 11 +++-------- .../tests/codetoanalyze/java/litho/issues.exp | 5 +++-- 5 files changed, 32 insertions(+), 21 deletions(-) diff --git a/infer/src/checkers/GraphQLFieldAccess.ml b/infer/src/checkers/GraphQLFieldAccess.ml index 75d1181f8..781f76da8 100644 --- a/infer/src/checkers/GraphQLFieldAccess.ml +++ b/infer/src/checkers/GraphQLFieldAccess.ml @@ -32,7 +32,7 @@ module LithoContext = struct let check_callee ~callee_pname ~tenv:_ = is_graphql_function callee_pname - let satisfies_heuristic ~callee_pname:_ ~caller_pname:_ = true + let satisfies_heuristic ~callee_pname:_ ~callee_summary_opt:_ _ = true let field = Payloads.Fields.litho_graphql_field_access diff --git a/infer/src/checkers/LithoFramework.ml b/infer/src/checkers/LithoFramework.ml index fa5d55222..9702dee17 100644 --- a/infer/src/checkers/LithoFramework.ml +++ b/infer/src/checkers/LithoFramework.ml @@ -53,7 +53,8 @@ module type LithoContext = sig val check_callee : callee_pname:Typ.Procname.t -> tenv:Tenv.t -> t option -> bool - val satisfies_heuristic : callee_pname:Typ.Procname.t -> caller_pname:Typ.Procname.t -> bool + val satisfies_heuristic : + callee_pname:Typ.Procname.t -> callee_summary_opt:t option -> Tenv.t -> bool val should_report : Procdesc.t -> Tenv.t -> bool @@ -104,19 +105,19 @@ struct , (HilExp.AccessExpression receiver_ae :: _ as actuals) , _ , location ) -> - let domain_summary = Payload.read ~caller_summary:summary ~callee_pname in + 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 in if - ( LithoContext.check_callee ~callee_pname ~tenv domain_summary + ( LithoContext.check_callee ~callee_pname ~tenv callee_summary_opt || (* track callee in order to report respective errors *) Domain.mem receiver astate (* track anything called on a receiver we're already tracking *) ) && (not (Typ.Procname.Java.is_static java_callee_procname)) - && LithoContext.satisfies_heuristic ~callee_pname ~caller_pname + && LithoContext.satisfies_heuristic ~callee_pname ~callee_summary_opt tenv then let return_access_path = Domain.LocalAccessPath.make (return_base, []) caller_pname in let return_calls = @@ -127,10 +128,12 @@ struct Domain.add return_access_path return_calls astate else (* treat it like a normal call *) - apply_callee_summary domain_summary caller_pname return_base actuals astate + apply_callee_summary callee_summary_opt caller_pname return_base actuals astate | Call (ret_id_typ, Direct callee_procname, actuals, _, _) -> - let callee_summary = Payload.read ~caller_summary:summary ~callee_pname:callee_procname in - apply_callee_summary callee_summary caller_pname ret_id_typ actuals astate + let callee_summary_opt = + Payload.read ~caller_summary:summary ~callee_pname:callee_procname + in + apply_callee_summary callee_summary_opt caller_pname ret_id_typ actuals astate | Assign (lhs_ae, HilExp.AccessExpression rhs_ae, _) -> ( (* creating an alias for the rhs binding; assume all reads will now occur through the alias. this helps us keep track of chains in cases like tmp = getFoo(); x = tmp; diff --git a/infer/src/checkers/RequiredProps.ml b/infer/src/checkers/RequiredProps.ml index 0bc36c911..cf1ed2675 100644 --- a/infer/src/checkers/RequiredProps.ml +++ b/infer/src/checkers/RequiredProps.ml @@ -118,9 +118,21 @@ module LithoContext = struct let check_callee ~callee_pname ~tenv _ = LithoFramework.is_component_builder callee_pname tenv - let satisfies_heuristic ~callee_pname ~caller_pname = - (* don't track Litho client -> Litho framework calls; we want to use the summaries *) - not (LithoFramework.is_function callee_pname && not (LithoFramework.is_function caller_pname)) + let satisfies_heuristic ~callee_pname ~callee_summary_opt tenv = + (* If the method is build() itself or doesn't contain a build() in + its summary, we want to track it in the domain. *) + LithoFramework.is_component_build_method callee_pname tenv + || + (* check if build() exists in callees *) + let build_exists_in_callees = + Option.value_map callee_summary_opt ~default:[] ~f:Domain.bindings + |> List.exists ~f:(fun (_, call_set) -> + LithoDomain.CallSet.exists + (fun LithoDomain.MethodCall.{procname} -> + LithoFramework.is_component_build_method procname tenv ) + call_set ) + in + not build_exists_in_callees let field = Payloads.Fields.litho_required_props diff --git a/infer/tests/codetoanalyze/java/litho/RequiredProps.java b/infer/tests/codetoanalyze/java/litho/RequiredProps.java index 6b0713b9f..117a46306 100644 --- a/infer/tests/codetoanalyze/java/litho/RequiredProps.java +++ b/infer/tests/codetoanalyze/java/litho/RequiredProps.java @@ -179,22 +179,17 @@ public class RequiredProps { mVarArgPropComponent.create().build(); } - // can't track MyLithoComponent.build() because it is on litho, hence we miss the error - public Component buildPropLithoMissingBothBad_FN() { + public Component buildPropLithoMissingBothBad() { return mMyLithoComponent.create().build(); } - // we pick up Component.build() rather than - // MyLithoComponent.build(). Hence, we can't detect that it has - // missing props at all. - public void buildPropLithoMissingOneBad_FN() { + public void buildPropLithoMissingOneBad() { Column.create() .child(mMyLithoComponent.create().prop1(new Object()).commonProp(new Object())) .build(); } - // We can't track prop1 and prop2 because they are on litho - public Component buildPropLithoOK_FP() { + public Component buildPropLithoOK() { Component.Builder layoutBuilder = mMyLithoComponent.create().prop1(new Object()).prop2(new Object()); return layoutBuilder.build(); diff --git a/infer/tests/codetoanalyze/java/litho/issues.exp b/infer/tests/codetoanalyze/java/litho/issues.exp index d07720a5f..debaf661c 100644 --- a/infer/tests/codetoanalyze/java/litho/issues.exp +++ b/infer/tests/codetoanalyze/java/litho/issues.exp @@ -1,6 +1,7 @@ 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.buildPropLithoOK_FP():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 MyLithoComponent$Builder.build()] -codetoanalyze/java/litho/RequiredProps.java, codetoanalyze.java.litho.RequiredProps.buildPropLithoOK_FP():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 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 prop2 is required for component com.facebook.litho.MyLithoComponent, but is not set before the call to build(),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 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$Builder.prop1(Object),calls Component$Builder Component$Builder.commonProp(Object),calls Component Component$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()]