[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

@@ -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
 

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
+        let callee_summary_opt =
-        apply_callee_summary callee_summary caller_pname ret_id_typ actuals astate
+          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;

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 =
+  let satisfies_heuristic ~callee_pname ~callee_summary_opt tenv =
-    (* don't track Litho client -> Litho framework calls; we want to use the summaries *)
+    (* If the method is build() itself or doesn't contain a build() in
-    not (LithoFramework.is_function callee_pname && not (LithoFramework.is_function caller_pname))
+       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

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() {
-  public Component buildPropLithoMissingBothBad_FN() {
     return mMyLithoComponent.create().build();
   }
 
-  // we pick up Component.build() rather than
+  public void buildPropLithoMissingOneBad() {
-  // MyLithoComponent.build(). Hence, we can't detect that it has
-  // missing props at all.
-  public void buildPropLithoMissingOneBad_FN() {
     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() {
-  public Component buildPropLithoOK_FP() {
     Component.Builder layoutBuilder =
         mMyLithoComponent.create().prop1(new Object()).prop2(new Object());
     return layoutBuilder.build();

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.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.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 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()]