[hack][builder pattern] Configure immediately discardable builder and…

… handle the accordingly

Summary: Configure immediately discardable builder and handle the accordingly

Reviewed By: jvillard

Differential Revision: D65600269

fbshipit-source-id: 03822ae64a217d2d2ebfdce2004864e072321694

infer/src/base/Config.ml

@@ -62,7 +62,10 @@ type pulse_taint_config =
   ; policies: Pulse_config_t.taint_policies
   ; data_flow_kinds: string list }
 
-type pulse_hack_builder_pattern = {class_name: string [@yojson.key "class"]; finalizers: string list}
+type pulse_hack_builder_pattern =
+  { class_name: string [@yojson.key "class"]
+  ; finalizers: string list
+  ; immediately_non_discardable_class: string option [@yojson.option] }
 [@@deriving of_yojson]
 
 type pulse_hack_builder_patterns = pulse_hack_builder_pattern list [@@deriving of_yojson]

infer/src/base/Config.mli

@@ -415,7 +415,8 @@ val global_tenv : bool
 
 val hackc_binary : string option
 
-type pulse_hack_builder_pattern = {class_name: string; finalizers: string list}
+type pulse_hack_builder_pattern =
+  {class_name: string; finalizers: string list; immediately_non_discardable_class: string option}
 
 type pulse_hack_builder_patterns = pulse_hack_builder_pattern list
 

infer/src/pulse/PulseModelsDSL.ml

@@ -604,14 +604,25 @@ module Syntax = struct
     |> lift_model
 
 
-  let is_hack_builder_in_config tenv hacktypname =
-    L.d_printfln "typname = %a" HackClassName.pp hacktypname ;
-    (* TODO: deal with namespaces properly! *)
-    List.exists Config.hack_builder_patterns ~f:(fun Config.{class_name} ->
-        let builder_class_name = Typ.HackClass (HackClassName.make class_name) in
-        PatternMatch.is_subtype tenv (HackClass hacktypname) builder_class_name )
+  module HackBuilder = struct
+    let is_hack_builder tenv hacktypname builder_class_name =
+      (* TODO: deal with namespaces properly! *)
+      let builder_class_name = Typ.HackClass (HackClassName.make builder_class_name) in
+      PatternMatch.is_subtype tenv (HackClass hacktypname) builder_class_name
 
 
+    let is_hack_builder_in_config tenv hacktypname =
+      List.exists Config.hack_builder_patterns ~f:(fun Config.{class_name} ->
+          is_hack_builder tenv hacktypname class_name )
+
+
+    let is_hack_builder_immediately_non_discardable tenv hacktypname =
+      List.exists Config.hack_builder_patterns ~f:(fun Config.{immediately_non_discardable_class} ->
+          Option.exists immediately_non_discardable_class
+            ~f:(fun immediately_non_discardable_class ->
+              is_hack_builder tenv hacktypname immediately_non_discardable_class ) )
+  end
+
   let new_ type_name_exp =
     let* {analysis_data= {tenv}} = get_data in
     let* new_obj = lift_to_monad_and_get_result (internal_new_ type_name_exp) in
@@ -621,13 +632,19 @@ module Syntax = struct
         let* () = and_dynamic_type_is new_obj typ in
         let* () =
           match Typ.name typ with
-          | Some (HackClass hacktypname) when is_hack_builder_in_config tenv hacktypname ->
+          | Some (HackClass hacktypname) when HackBuilder.is_hack_builder_in_config tenv hacktypname
+            ->
               let* () = allocation (Attribute.HackBuilderResource hacktypname) new_obj in
               (* 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
+                  to delay that until the first method call to avoid FPs caused by reporting
+                  on builders without any method calls unless builder is explicitly configured
+                 as non discardable *)
+              let builder_state =
+                if HackBuilder.is_hack_builder_immediately_non_discardable tenv hacktypname then
+                  Attribute.Builder.NonDiscardable
+                else Attribute.Builder.Discardable
+              in
+              AddressAttributes.set_hack_builder (fst new_obj) builder_state |> exec_command
           | _ ->
               ret ()
         in

infer/tests/codetoanalyze/hack/pulse/.inferconfig

@@ -5,8 +5,19 @@
     "models/test_models.hack"
   ],
   "hack-builder-patterns": [
-    { "class": "TestBuilderBase", "finalizers": ["doFinalize", "altFinalize"]},
-    { "class": "MyBuilder", "finalizers": ["saveX"]}
+    {
+      "class": "TestBuilderBase",
+      "finalizers": [
+        "doFinalize",
+        "altFinalize"
+        ],
+      "immediately_non_discardable_class": "MyImmediatelyDiscardableBuilder"
+      },
+    { "class": "MyBuilder",
+      "finalizers": [
+        "saveX"
+      ]
+    }
   ],
   "pulse-specialization-partial": true,
   "pulse-taint-short-traces": true,

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

@@ -23,6 +23,18 @@ class NoBuilderSuffix implements TestBuilderBase {
   }
 }
 
+class MyImmediatelyDiscardableBuilder implements TestBuilderBase {
+  public function __construct() {}
+
+  public function setFoo(int $a): this {
+    return $this;
+  }
+
+  public function doFinalize(): void {
+    return;
+  }
+}
+
 <<__ConsistentConstruct>>
 abstract class AbstractBuilder {
   abstract public function __construct(int $foo);
@@ -101,7 +113,7 @@ class BuilderTester {
     }
   }
 
-  public static function FP_builderWithoutCallsOk(bool $flag): void {
+  public static function builderWithoutCallsOk(bool $flag): void {
     $changes_made = false;
     $b = new MyBuilder(0);
 
@@ -126,6 +138,12 @@ class BuilderTester {
     $b->setFoo(42);
     $b->doFinalize();
   }
+
+  // check that we can recongnise immediately discardable builders
+  // from the .inferconfig and handle them accordingly
+  public static function testImmediatelyDiscardableBuilderBad(): void {
+    $b = new MyImmediatelyDiscardableBuilder();
+  }
 }
 
 // In real code, builders are often created via some icky reflection

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

@@ -29,6 +29,7 @@ booleans.hack, $root.Booleans::testNullBad, 3, TAINT_ERROR, no_bucket, ERROR, [s
 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.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, BuilderTester$static.testImmediatelyDiscardableBuilderBad, 2, PULSE_UNFINISHED_BUILDER, no_bucket, ERROR, [allocation part of the trace starts here,allocated by constructor `MyImmediatelyDiscardableBuilder()` 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()