[thread-safety] skip reporting on truncated traces

Summary:
Due to limitations in our Buck integration, the thread-safety analysis cannot create a trace that bottoms out in a Buck target that is not a direct dependency of the current target.
These truncated traces are confusing and tough to act on.
Until we can address these limitations, let's avoid reporting on truncated traces.

Reviewed By: jeremydubreil

Differential Revision: D5969840

fbshipit-source-id: 877b9de

infer/src/concurrency/RacerD.ml

@@ -346,7 +346,7 @@ module TransferFunctions (CFG : ProcCfg.S) = struct
     then
       let open Domain in
       let pre = AccessPrecondition.make locks threads proc_data.pdesc in
-      AccessDomain.add_access pre (make_unannotated_call_access pname loc) attribute_map
+      AccessDomain.add_access pre (TraceElem.make_unannotated_call_access pname loc) attribute_map
     else attribute_map
 
 
@@ -380,7 +380,9 @@ module TransferFunctions (CFG : ProcCfg.S) = struct
                || is_safe_access access prefix_path proc_data.tenv
             then access_acc
             else
-              AccessDomain.add_access pre (make_field_access access_path ~is_write loc) access_acc
+              AccessDomain.add_access pre
+                (TraceElem.make_field_access access_path ~is_write loc)
+                access_acc
           in
           add_field_accesses pre access_path access_acc' access_list'
     in
@@ -525,7 +527,7 @@ module TransferFunctions (CFG : ProcCfg.S) = struct
       if is_synchronized_container callee_pname receiver_ap tenv then AccessDomain.empty
       else
         let container_access =
-          make_container_access receiver_ap ~is_write callee_pname callee_loc
+          TraceElem.make_container_access receiver_ap ~is_write callee_pname callee_loc
         in
         AccessDomain.add_access (Unprotected (IntSet.singleton 0)) container_access
           AccessDomain.empty
@@ -1323,21 +1325,25 @@ let report_thread_safety_violation tenv pdesc issue_type ~make_description ~repo
   let open RacerDDomain in
   let pname = Procdesc.get_proc_name pdesc in
   let report_one_path ((_, sinks) as path) =
-    let initial_sink, _ = List.last_exn sinks in
     let final_sink, _ = List.hd_exn sinks in
-    let initial_sink_site = PathDomain.Sink.call_site initial_sink in
+    let is_full_trace = TraceElem.is_direct final_sink in
-    let final_sink_site = PathDomain.Sink.call_site final_sink in
+    (* Traces can be truncated due to limitations of our Buck integration. If we have a truncated
-    let loc = CallSite.loc initial_sink_site in
+       trace, it's probably going to be too confusing to be actionable. Skip it. *)
-    let ltr = make_trace ~report_kind path pdesc in
+    if is_full_trace || not Config.filtering then
-    (* what the potential bug is *)
+      let final_sink_site = PathDomain.Sink.call_site final_sink in
-    let description = make_description pname final_sink_site initial_sink_site final_sink in
+      let initial_sink, _ = List.last_exn sinks in
-    (* why we are reporting it *)
+      let initial_sink_site = PathDomain.Sink.call_site initial_sink in
-    let explanation = get_reporting_explanation report_kind tenv pname thread in
+      let loc = CallSite.loc initial_sink_site in
-    let error_message = F.sprintf "%s%s" description explanation in
+      let ltr = make_trace ~report_kind path pdesc in
-    let exn =
+      (* what the potential bug is *)
-      Exceptions.Checkers (issue_type.IssueType.unique_id, Localise.verbatim_desc error_message)
+      let description = make_description pname final_sink_site initial_sink_site final_sink in
-    in
+      (* why we are reporting it *)
-    Reporting.log_error_deprecated ~store_summary:true pname ~loc ~ltr exn
+      let explanation = get_reporting_explanation report_kind tenv pname thread in
+      let error_message = F.sprintf "%s%s" description explanation in
+      let exn =
+        Exceptions.Checkers (issue_type.IssueType.unique_id, Localise.verbatim_desc error_message)
+      in
+      Reporting.log_error_deprecated ~store_summary:true pname ~loc ~ltr exn
   in
   let trace_of_pname = trace_of_pname access pdesc in
   Option.iter ~f:report_one_path (PathDomain.get_reportable_sink_path access ~trace_of_pname)

infer/src/concurrency/RacerDDomain.ml

@@ -124,26 +124,34 @@ module TraceElem = struct
 
     let pp = pp
   end)
-end
 
-let make_container_access access_path pname ~is_write loc =
+  let dummy_pname = Typ.Procname.empty_block
-  let site = CallSite.make Typ.Procname.empty_block loc in
+
-  let access =
+  let make_dummy_site = CallSite.make dummy_pname
-    if is_write then Access.ContainerWrite (access_path, pname)
+
-    else Access.ContainerRead (access_path, pname)
+  (* all trace elems are created with a dummy call site. any trace elem without a dummy call site
-  in
+     represents a call that leads to an access rather than a direct access *)
-  TraceElem.make access site
+  let is_direct {site} = Typ.Procname.equal (CallSite.pname site) dummy_pname
+
+  let make_container_access access_path pname ~is_write loc =
+    let site = make_dummy_site loc in
+    let access =
+      if is_write then Access.ContainerWrite (access_path, pname)
+      else Access.ContainerRead (access_path, pname)
+    in
+    make access site
 
 
-let make_field_access access_path ~is_write loc =
+  let make_field_access access_path ~is_write loc =
-  let site = CallSite.make Typ.Procname.empty_block loc in
+    let site = make_dummy_site loc in
-  TraceElem.make (Access.make_field_access access_path ~is_write) site
+    make (Access.make_field_access access_path ~is_write) site
 
 
-let make_unannotated_call_access pname loc =
+  let make_unannotated_call_access pname loc =
-  let site = CallSite.make Typ.Procname.empty_block loc in
+    let site = make_dummy_site loc in
-  TraceElem.make (Access.InterfaceCall pname) site
+    make (Access.InterfaceCall pname) site
 
+end
 
 (* In this domain true<=false. The intended denotations [[.]] are
     [[true]] = the set of all states where we know according, to annotations

infer/src/concurrency/RacerDDomain.mli

@@ -41,6 +41,16 @@ module TraceElem : sig
   val is_container_write : t -> bool
 
   val map : f:(AccessPath.t -> AccessPath.t) -> t -> t
+
+  val is_direct : t -> bool
+  (** return true if the given trace elem represents a direct access, not a call that eventually
+      leads to an access *)
+
+  val make_container_access : AccessPath.t -> Typ.Procname.t -> is_write:bool -> Location.t -> t
+
+  val make_field_access : AccessPath.t -> is_write:bool -> Location.t -> t
+
+  val make_unannotated_call_access : Typ.Procname.t -> Location.t -> t
 end
 
 (** A bool that is true if a lock is definitely held. Note that this is unsound because it assumes
@@ -201,11 +211,4 @@ type summary =
 
 include AbstractDomain.WithBottom with type astate := astate
 
-val make_container_access :
-  AccessPath.t -> Typ.Procname.t -> is_write:bool -> Location.t -> TraceElem.t
-
-val make_field_access : AccessPath.t -> is_write:bool -> Location.t -> TraceElem.t
-
-val make_unannotated_call_access : Typ.Procname.t -> Location.t -> TraceElem.t
-
 val pp_summary : F.formatter -> summary -> unit

infer/tests/build_systems/threadsafety_traces/Makefile

@@ -0,0 +1,30 @@
+# Copyright (c) 2017 - present Facebook, Inc.
+# All rights reserved.
+#
+# This source code is licensed under the BSD style license found in the
+# LICENSE file in the root directory of this source tree. An additional grant
+# of patent rights can be found in the PATENTS file in the same directory.
+
+TESTS_DIR = ../..
+ROOT_DIR = $(TESTS_DIR)/../..
+
+ANALYZER = checkers
+BUCK_TARGET = //infer/tests/build_systems/threadsafety_traces/module3:module3
+INFERPRINT_OPTIONS = --project-root $(ROOT_DIR) --issues-tests
+CLEAN_EXTRA = $(ROOT_DIR)/buck-out
+
+include $(TESTS_DIR)/infer.make
+
+$(OBJECTS): $(JAVA_SOURCE_FILES)
+	$(QUIET)cd $(ROOT_DIR) && \
+	$(call silent_on_success,Compiling thread-safety trace tests,\
+	INFER_BIN=$(INFER_BIN) NO_BUCKD=1 \
+	$(BUCK) build --no-cache $(BUCK_TARGET))
+
+infer-out/report.json: $(JAVA_DEPS) $(JAVA_SOURCE_FILES)
+	$(QUIET)cd $(ROOT_DIR) && \
+	$(REMOVE_DIR) buck-out && \
+	$(call silent_on_success,Testing thread-safety trace tests with Buck,\
+	INFER_BIN=$(INFER_BIN) NO_BUCKD=1 \
+	$(INFER_BIN) -a $(ANALYZER) --results-dir $(CURDIR)/infer-out -- \
+	  $(BUCK) build --no-cache $(BUCK_TARGET))

infer/tests/build_systems/threadsafety_traces/module1/BUCK

@@ -0,0 +1,8 @@
+java_library(
+    name='module1',
+    srcs=['Class1.java'],
+    deps=[],
+    visibility=[
+        'PUBLIC'
+    ],
+)

infer/tests/build_systems/threadsafety_traces/module1/Class1.java

@@ -0,0 +1,20 @@
+/*
+ * Copyright (c) 2017 - present Facebook, Inc.
+ * All rights reserved.
+ *
+ * This source code is licensed under the BSD style license found in the
+ * LICENSE file in the root directory of this source tree. An additional grant
+ * of patent rights can be found in the PATENTS file in the same directory.
+ */
+
+package threadsafety_traces.module1;
+
+public class Class1 {
+
+  static int sField;
+
+  public static void method() {
+    sField++;
+  }
+
+}

infer/tests/build_systems/threadsafety_traces/module2/BUCK

@@ -0,0 +1,10 @@
+java_library(
+    name='module2',
+    srcs=['Class2.java'],
+    deps=[
+        '//infer/tests/build_systems/threadsafety_traces/module1:module1'
+    ],
+    visibility=[
+        'PUBLIC'
+    ],
+)

infer/tests/build_systems/threadsafety_traces/module2/Class2.java

@@ -0,0 +1,20 @@
+/*
+ * Copyright (c) 2017 - present Facebook, Inc.
+ * All rights reserved.
+ *
+ * This source code is licensed under the BSD style license found in the
+ * LICENSE file in the root directory of this source tree. An additional grant
+ * of patent rights can be found in the PATENTS file in the same directory.
+ */
+
+package threadsafety_traces.module2;
+
+import threadsafety_traces.module1.Class1;
+
+public class Class2 {
+
+  public static void method() {
+    Class1.method();
+  }
+
+}

infer/tests/build_systems/threadsafety_traces/module3/BUCK

@@ -0,0 +1,7 @@
+java_library(
+    name='module3',
+    srcs=['Class3.java'],
+    deps=[
+        '//infer/tests/build_systems/threadsafety_traces/module2:module2',
+    ]
+)

infer/tests/build_systems/threadsafety_traces/module3/Class3.java

@@ -0,0 +1,27 @@
+/*
+ * Copyright (c) 2017 - present Facebook, Inc.
+ * All rights reserved.
+ *
+ * This source code is licensed under the BSD style license found in the
+ * LICENSE file in the root directory of this source tree. An additional grant
+ * of patent rights can be found in the PATENTS file in the same directory.
+ */
+
+package threadsafety_traces.module3;
+
+import threadsafety_traces.module2.Class2;
+
+@interface ThreadSafe {
+}
+
+@ThreadSafe
+public class Class3 {
+
+  /** this will produce a truncated trace that should bottom out in Class1.method, but will stop
+      short due to limitations in our Buck integration. test that we don't report a truncated
+      trace in this situation. */
+  public void callClass2() {
+    Class2.method();
+  }
+
+}