[hack][builder pattern] Do not report issues on builders that don't h…

…ave any method calls

Summary: Do not report issues on builders that don't have any method calls

Reviewed By: davidpichardie

Differential Revision:
D65419582

Privacy Context Container: L1208441

fbshipit-source-id: 9bbce9367401f52df2c3e6795d7c55d8290c18ba

infer/src/IR/Procname.ml

@@ -995,6 +995,12 @@ let is_hack_construct = function
 
 let is_hack_xinit = function Hack classname -> Hack.is_xinit classname | _ -> false
 
+let is_hack_internal procname =
+  if is_hack_xinit procname || is_hack_builtins procname || is_hack_construct procname then true
+  else match procname with (* models are implemented as C functions *)
+       | C _ -> true | _ -> false
+
+
 let has_hack_classname = function Hack {class_name= Some _} -> true | _ -> false
 
 let get_global_name_of_initializer t =

infer/src/IR/Procname.mli

@@ -499,3 +499,6 @@ val is_hack_async_name : t -> bool
 val is_hack_construct : t -> bool
 
 val is_hack_xinit : t -> bool
+
+val is_hack_internal : t -> bool
+(* Check if the function is not a model nor one of the internal functions necessary for implementation *)

infer/src/pulse/Pulse.ml

@@ -52,6 +52,17 @@ let is_hack_async tenv pname =
                   Struct.is_hack_interface str ) )
 
 
+let is_hack_builder_procname tenv pname =
+  if Procname.is_hack_internal pname then false
+  else
+    match Procname.get_class_type_name pname with
+    | Some (HackClass _ as tn) ->
+        List.exists Config.hack_builder_patterns ~f:(fun (class_name, _) ->
+            PatternMatch.is_subtype tenv tn (HackClass (HackClassName.make class_name)) )
+    | _ ->
+        false
+
+
 let is_hack_builder_consumer tenv pname =
   match Procname.get_class_type_name pname with
   | Some (HackClass _ as tn) ->
@@ -761,6 +772,12 @@ module PulseTransferFunctions = struct
           L.d_printfln "**it's a builder consumer" ;
           AddressAttributes.set_hack_builder (ValueOrigin.value arg) Attribute.Builder.Discardable
             astate
+      | Some callee_pname, {ProcnameDispatcher.Call.FuncArg.arg_payload= arg} :: _
+        when is_hack_builder_procname tenv callee_pname ->
+          L.d_printfln "**builder is called via %a and is non-discardable now" Procname.pp
+            callee_pname ;
+          AddressAttributes.set_hack_builder (ValueOrigin.value arg)
+            Attribute.Builder.NonDiscardable astate
       | _, _ ->
           astate
     in

infer/src/pulse/PulseAbductiveDomain.mli

@@ -193,7 +193,6 @@ module AddressAttributes : sig
   val remove_hack_builder : AbstractValue.t -> t -> t [@@warning "-unused-value-declaration"]
 
   val set_hack_builder : AbstractValue.t -> Attribute.Builder.t -> t -> t
-  [@@warning "-unused-value-declaration"]
 
   val get_hack_builder : AbstractValue.t -> t -> Attribute.Builder.t option
   [@@warning "-unused-value-declaration"]

infer/src/pulse/PulseModelsDSL.ml

@@ -622,7 +622,10 @@ module Syntax = struct
           match Typ.name typ with
           | Some (HackClass hacktypname) when is_hack_builder_in_config tenv hacktypname ->
               let* () = allocation (Attribute.HackBuilderResource hacktypname) new_obj in
-              AddressAttributes.set_hack_builder (fst new_obj) Attribute.Builder.NonDiscardable
+              (* While it makes sense to set initial builder state to NonDiscardable we'd like
+                 to delay that until the first method call to avoid FPs caused by reporting
+                 on builders without any method calls *)
+              AddressAttributes.set_hack_builder (fst new_obj) Attribute.Builder.Discardable
               |> exec_command
           | _ ->
               ret ()

infer/tests/codetoanalyze/hack/pulse/builder.hack

@@ -118,10 +118,6 @@ class BuilderTester {
   // now check builders recognised from the .inferconfig file
   public static function testConfigBad(): void {
     $b = new NoBuilderSuffix();
-  }
-
-  public static function testConfigBad2(): void {
-    $b = new NoBuilderSuffix();
     $b->setFoo(42);
   }
 

infer/tests/codetoanalyze/hack/pulse/issues.exp

@@ -28,9 +28,7 @@ booleans.hack, $root.Booleans::testBoolBad, 3, TAINT_ERROR, no_bucket, ERROR, [s
 booleans.hack, $root.Booleans::testNullBad, 3, TAINT_ERROR, no_bucket, ERROR, [source of the taint here: value passed as argument `#0` to `$root.Booleans::testNullBad` with kind `Simple`,flows to this sink: value passed as argument `#0` to `$root.Level1::taintSink` with kind `Simple`], source: $root.Booleans::testNullBad, sink: $root.Level1::taintSink, tainted expression: $sc
 booleans.hack, $root.Booleans::testNotNullBad, 3, TAINT_ERROR, no_bucket, ERROR, [source of the taint here: value passed as argument `#0` to `$root.Booleans::testNotNullBad` with kind `Simple`,flows to this sink: value passed as argument `#0` to `$root.Level1::taintSink` with kind `Simple`], source: $root.Booleans::testNotNullBad, sink: $root.Level1::taintSink, tainted expression: $sc
 builder.hack, BuilderTester$static.builderUserBad, 3, PULSE_UNFINISHED_BUILDER, no_bucket, ERROR, [allocation part of the trace starts here,allocated by constructor `MyBuilder()` here,builder object becomes unreachable here]
-builder.hack, BuilderTester$static.FP_builderWithoutCallsOk, 9, PULSE_UNFINISHED_BUILDER, no_bucket, ERROR, [allocation part of the trace starts here,allocated by constructor `MyBuilder()` here,builder object becomes unreachable here]
-builder.hack, BuilderTester$static.testConfigBad, 2, PULSE_UNFINISHED_BUILDER, no_bucket, ERROR, [allocation part of the trace starts here,allocated by constructor `NoBuilderSuffix()` here,builder object becomes unreachable here]
-builder.hack, BuilderTester$static.testConfigBad2, 3, PULSE_UNFINISHED_BUILDER, no_bucket, ERROR, [allocation part of the trace starts here,allocated by constructor `NoBuilderSuffix()` here,builder object becomes unreachable here]
+builder.hack, BuilderTester$static.testConfigBad, 3, PULSE_UNFINISHED_BUILDER, no_bucket, ERROR, [allocation part of the trace starts here,allocated by constructor `NoBuilderSuffix()` here,builder object becomes unreachable here]
 builder.hack, BuilderTester2$static.testCreateBad, 3, PULSE_UNFINISHED_BUILDER, no_bucket, ERROR, [allocation part of the trace starts here,when calling `BuilderTester2$static.create` here,when calling `MyBuilder$static.__factory` here,allocated by constructor `MyBuilder()` here,builder object becomes unreachable here]
 builder.hack, BuilderTester3$static.testCreateBad, 3, PULSE_UNFINISHED_BUILDER, no_bucket, ERROR, [allocation part of the trace starts here,when calling `MutatorTrait$static.create` here,when calling `MyBuilder$static.__factory` here,allocated by constructor `MyBuilder()` here,builder object becomes unreachable here]
 call_variadic.hack, CallVariadic::CallVariadic$static.callVariadicWith2ArgsBad, 1, TAINT_ERROR, no_bucket, ERROR, [source of the taint here: value returned from `$root.Level1::taintSource` with kind `Simple`,when calling `Variadic::Variadic$static.variadicArgInSink` here,flows to this sink: value passed as argument `#0` to `$root.Level1::taintSink` with kind `Simple`], source: $root.Level1::taintSource, sink: $root.Level1::taintSink, tainted expression: $root.Level1::taintSource()