diff --git a/infer/src/checkers/ThreadSafety.ml b/infer/src/checkers/ThreadSafety.ml index f4f4d6e98..1ffed9c9c 100644 --- a/infer/src/checkers/ThreadSafety.ml +++ b/infer/src/checkers/ThreadSafety.ml @@ -810,6 +810,11 @@ let combine_conditional_unconditional_writes conditional_writes unconditional_wr conditional_writes unconditional_writes +let equal_locs (sink1 : ThreadSafetyDomain.TraceElem.t) + (sink2 : ThreadSafetyDomain.TraceElem.t) = + Location.equal + (CallSite.loc (ThreadSafetyDomain.TraceElem.call_site sink1)) + (CallSite.loc (ThreadSafetyDomain.TraceElem.call_site sink2)) let equal_accesses (sink1 : ThreadSafetyDomain.TraceElem.t) (sink2 : ThreadSafetyDomain.TraceElem.t) = @@ -855,26 +860,35 @@ let collect_conflicting_writes sink tab = ) procs_and_writes -(* keep only the first copy of an access per procedure *) +(* keep only the first copy of an access per procedure, + and keep at most one warning per line (they are usually interprocedural accesses + to different fields generated by the same call) *) 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 = + let select_representatives original_sinks predicate = + let list_of_original_sinks = ThreadSafetyDomain.PathDomain.Sinks.elements original_sinks in 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 + satisfying predicate. 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) + ~f:(fun sink2 -> predicate 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 + let de_duped_sinks_by_accesses = select_representatives + (ThreadSafetyDomain.PathDomain.sinks trace) + equal_accesses in + let de_duped_sinks_by_locs_and_accesses = select_representatives + de_duped_sinks_by_accesses + equal_locs in + ThreadSafetyDomain.PathDomain.update_sinks trace de_duped_sinks_by_locs_and_accesses + + (*A helper function used in the error reporting*) let pp_accesses_sink fmt ~is_write_access sink = diff --git a/infer/tests/codetoanalyze/java/threadsafety/DeDup.java b/infer/tests/codetoanalyze/java/threadsafety/DeDup.java index ce0a97a33..a4abdad15 100644 --- a/infer/tests/codetoanalyze/java/threadsafety/DeDup.java +++ b/infer/tests/codetoanalyze/java/threadsafety/DeDup.java @@ -15,6 +15,18 @@ import javax.annotation.concurrent.ThreadSafe; class DeDup{ Integer field; +Integer fielda, fieldb; + + /* Only want one rather than two reports */ + void two_fields(){ + foo(); + } + + private void foo() { + fielda = 88; + fieldb = 99; + } + /*Only the first write should be reported*/ void two_writes(){ diff --git a/infer/tests/codetoanalyze/java/threadsafety/issues.exp b/infer/tests/codetoanalyze/java/threadsafety/issues.exp index b3cf58f4b..44ab4ee5c 100644 --- a/infer/tests/codetoanalyze/java/threadsafety/issues.exp +++ b/infer/tests/codetoanalyze/java/threadsafety/issues.exp @@ -27,6 +27,7 @@ codetoanalyze/java/threadsafety/Containers.java, void Containers.mapPutBad(Strin codetoanalyze/java/threadsafety/Containers.java, void Containers.mapRemoveBad(String), 1, THREAD_SAFETY_VIOLATION, [access to container codetoanalyze.java.checkers.Containers.mMap] codetoanalyze/java/threadsafety/Containers.java, void Containers.mapSubclassWriteBad(HashMap,String), 1, THREAD_SAFETY_VIOLATION, [access to container ] 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_fields(), 1, THREAD_SAFETY_VIOLATION, [call to void DeDup.foo(),access to codetoanalyze.java.checkers.DeDup.fielda] 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]