[threadsafety] De-duplicate the reports

Summary: Report at most one read/write race or unprotected write per access path per method

Reviewed By: sblackshear, jvillard

Differential Revision: D4590815

fbshipit-source-id: 3c3a9d9

infer/src/checkers/ThreadSafety.ml

@@ -772,12 +772,20 @@ let combine_conditional_unconditional_writes conditional_writes unconditional_wr
     conditional_writes
     unconditional_writes
 
-let conflicting_accesses (sink1 : ThreadSafetyDomain.TraceElem.t)
+
+let equal_accesses (sink1 : ThreadSafetyDomain.TraceElem.t)
     (sink2 : ThreadSafetyDomain.TraceElem.t) =
   AccessPath.equal_access_list
     (snd (ThreadSafetyDomain.TraceElem.kind sink1))
     (snd (ThreadSafetyDomain.TraceElem.kind sink2))
 
+(* For now equal-access and conflicting-access are equivalent.
+   But that will change when we (soon) consider conficting accesses
+   that are not via assignment, such as add and get for containers*)
+let conflicting_accesses (sink1 : ThreadSafetyDomain.TraceElem.t)
+    (sink2 : ThreadSafetyDomain.TraceElem.t) =
+  equal_accesses sink1 sink2
+
 (* trace is really reads or writes set. Fix terminology later *)
 let filter_conflicting_sinks sink trace =
   let conflicts =
@@ -792,8 +800,8 @@ let filter_conflicting_sinks sink trace =
    write accesses*)
 let collect_conflicting_writes sink tab =
   let procs_and_writes =
-    IList.map
+    List.map
-      (fun (key,(_, _, conditional_writes, unconditional_writes, _)) ->
+      ~f:(fun (key,(_, _, conditional_writes, unconditional_writes, _)) ->
           let conflicting_writes =
             combine_conditional_unconditional_writes
               conditional_writes unconditional_writes
@@ -809,6 +817,27 @@ let collect_conflicting_writes sink tab =
       )
     procs_and_writes
 
+(* keep only the first copy of an access per procedure  *)
+let de_dup trace =
+  let original_sinks = ThreadSafetyDomain.PathDomain.sinks trace in
+  let list_of_original_sinks = ThreadSafetyDomain.PathDomain.Sinks.elements original_sinks in
+  let de_duped_sinks =
+    ThreadSafetyDomain.PathDomain.Sinks.filter
+      (fun sink ->
+         (* for each sink we will keep one in the equivalence class of those
+            with same access path. We select that by using find_exn to get
+            the first element equivalent ot sink in a list of sinks. This
+            first element is the dedup representative, and it happens to
+            typically be the first such access in a method.  *)
+         let first_sink =
+           List.find_exn
+             ~f:(fun sink2 -> equal_accesses sink sink2)
+             list_of_original_sinks in
+         Int.equal (ThreadSafetyDomain.TraceElem.compare sink first_sink) 0
+      )
+      original_sinks in
+  ThreadSafetyDomain.PathDomain.update_sinks trace de_duped_sinks
+
 (*A helper function used in the error reporting*)
 let pp_accesses_sink fmt sink =
   let _, accesses = ThreadSafetyDomain.PathDomain.Sink.kind sink in
@@ -846,7 +875,7 @@ let report_thread_safety_violations ( _, tenv, pname, pdesc) make_description tr
 
   IList.iter
     report_one_path
-    (PathDomain.get_reportable_sink_paths trace ~trace_of_pname)
+    (PathDomain.get_reportable_sink_paths (de_dup trace) ~trace_of_pname)
 
 
 let make_unprotected_write_description
@@ -879,7 +908,6 @@ let make_read_write_race_description tenv pname final_sink_site initial_sink_sit
     conflicts_description
     (calculate_addendum_message tenv pname)
 
-
 (* find those elements of reads which have conflicts
         somewhere else, and report them *)
 let report_reads proc_env reads tab =

infer/tests/codetoanalyze/java/threadsafety/DeDup.java

@@ -0,0 +1,50 @@
+/*
+ * Copyright (c) 2016 - 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 codetoanalyze.java.checkers;
+
+import javax.annotation.concurrent.ThreadSafe;
+
+@ThreadSafe
+class DeDup{
+
+Integer field;
+
+  /*Only the first write should be reported*/
+  void two_writes(){
+   field = 22;
+   field = 84;
+   }
+
+  /*Only the first read should be reported*/
+  void two_reads(){ //parallel reads are OK
+   Integer local;
+   local = field;
+   local = field+1;
+  }
+
+  /*Both accesses should be reported*/
+  void write_read(){ //parallel reads are OK
+   Integer local;
+   field = 87;
+   local = field;
+  }
+
+  /*Should only report the first write, which happens to be interprocedural*/
+  void twoWritesOneInCaller() {
+    writeToField();
+    field = 22;
+  }
+
+  /*Should not report directly as provate method*/
+  private void writeToField() {
+    field = 33;
+  }
+
+}

infer/tests/codetoanalyze/java/threadsafety/issues.exp

@@ -26,6 +26,11 @@ codetoanalyze/java/threadsafety/Containers.java, void Containers.mapPutAllBad(Ma
 codetoanalyze/java/threadsafety/Containers.java, void Containers.mapPutBad(String,String), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.Containers.mMap.put]
 codetoanalyze/java/threadsafety/Containers.java, void Containers.mapRemoveBad(String), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.Containers.mMap.remove]
 codetoanalyze/java/threadsafety/Containers.java, void Containers.mapSubclassWriteBad(HashMap,String), 1, THREAD_SAFETY_VIOLATION, [access to remove]
+codetoanalyze/java/threadsafety/DeDup.java, void DeDup.twoWritesOneInCaller(), 1, THREAD_SAFETY_VIOLATION, [call to void DeDup.writeToField(),access to codetoanalyze.java.checkers.DeDup.field]
+codetoanalyze/java/threadsafety/DeDup.java, void DeDup.two_reads(), 2, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.DeDup.field]
+codetoanalyze/java/threadsafety/DeDup.java, void DeDup.two_writes(), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.DeDup.field]
+codetoanalyze/java/threadsafety/DeDup.java, void DeDup.write_read(), 2, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.DeDup.field]
+codetoanalyze/java/threadsafety/DeDup.java, void DeDup.write_read(), 3, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.DeDup.field]
 codetoanalyze/java/threadsafety/Locks.java, void Locks.FP_unlockOneLock(), 4, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.Locks.f]
 codetoanalyze/java/threadsafety/Locks.java, void Locks.afterReentrantLockUnlockBad(), 3, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.Locks.f]
 codetoanalyze/java/threadsafety/Locks.java, void Locks.afterUnlockBad(), 3, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.Locks.f]
@@ -53,7 +58,6 @@ codetoanalyze/java/threadsafety/ThreadSafeExample.java, void ThreadSafeExample.c
 codetoanalyze/java/threadsafety/ThreadSafeExample.java, void ThreadSafeExample.callVisibleForTestingBad(), 1, THREAD_SAFETY_VIOLATION, [call to void ThreadSafeExample.visibleForTestingNotPublicOk(),access to codetoanalyze.java.checkers.ThreadSafeExample.f]
 codetoanalyze/java/threadsafety/ThreadSafeExample.java, void ThreadSafeExample.deeperTraceBad(), 1, THREAD_SAFETY_VIOLATION, [call to void ThreadSafeExample.callAssignInPrivateMethod(),call to void ThreadSafeExample.assignInPrivateMethodOk(),access to codetoanalyze.java.checkers.ThreadSafeExample.f]
 codetoanalyze/java/threadsafety/ThreadSafeExample.java, void ThreadSafeExample.oddBad(), 1, THREAD_SAFETY_VIOLATION, [call to void ThreadSafeExample.evenOk(),access to codetoanalyze.java.checkers.ThreadSafeExample.f]
-codetoanalyze/java/threadsafety/ThreadSafeExample.java, void ThreadSafeExample.recursiveBad(), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.ThreadSafeExample.f]
 codetoanalyze/java/threadsafety/ThreadSafeExample.java, void ThreadSafeExample.recursiveBad(), 2, THREAD_SAFETY_VIOLATION, [call to void ThreadSafeExample.recursiveBad(),access to codetoanalyze.java.checkers.ThreadSafeExample.f]
 codetoanalyze/java/threadsafety/ThreadSafeExample.java, void ThreadSafeExample.tsBad(), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.ThreadSafeExample.f]
 codetoanalyze/java/threadsafety/ThreadSafeExample.java, void YesThreadSafeExtendsNotThreadSafeExample.subsubmethodBad(), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.YesThreadSafeExtendsNotThreadSafeExample.subsubfield]