[thread-safety] warn on private methods marked ThreadSafe

Summary: Fixing a small oversight in the current scheme.

Reviewed By: jeremydubreil

Differential Revision: D4751964

fbshipit-source-id: 086318f

infer/src/checkers/ThreadSafety.ml

@@ -774,11 +774,22 @@ let should_analyze_proc pdesc tenv =
   not (is_thread_confined_method tenv pdesc) &&
   not (pdesc_is_assumed_thread_safe pdesc tenv)
 
+let is_thread_safe_method pdesc tenv =
+  PatternMatch.override_exists
+    (fun pn ->
+       Annotations.pname_has_return_annot
+         pn
+         ~attrs_of_pname:Specs.proc_resolve_attributes
+         is_thread_safe)
+    tenv
+    (Procdesc.get_proc_name pdesc)
+
 (* return true if we should report on unprotected accesses during the procedure *)
-let should_report_on_proc (_, _, proc_name, proc_desc) =
-  not (Typ.Procname.java_is_autogen_method proc_name) &&
-  Procdesc.get_access proc_desc <> PredSymb.Private &&
-  not (Annotations.pdesc_return_annot_ends_with proc_desc Annotations.visibleForTesting)
+let should_report_on_proc (_, tenv, proc_name, proc_desc) =
+  is_thread_safe_method proc_desc tenv ||
+  (not (Typ.Procname.java_is_autogen_method proc_name) &&
+   Procdesc.get_access proc_desc <> PredSymb.Private &&
+   not (Annotations.pdesc_return_annot_ends_with proc_desc Annotations.visibleForTesting))
 
 let analyze_procedure callback =
   let is_initializer tenv proc_name =
@@ -1168,19 +1179,9 @@ let should_report_on_file file_env =
 *)
 let process_results_table file_env tab =
   let should_report_on_all_procs = should_report_on_file file_env in
-  (* TODO (t15588153): clean this up *)
-  let is_thread_safe_method pdesc tenv =
-    PatternMatch.override_exists
-      (fun pn ->
-         Annotations.pname_has_return_annot
-           pn
-           ~attrs_of_pname:Specs.proc_resolve_attributes
-           is_thread_safe)
-      tenv
-      (Procdesc.get_proc_name pdesc) in
   let should_report ((_, tenv, _, pdesc) as proc_env) =
-    (should_report_on_all_procs || is_thread_safe_method pdesc tenv)
-    && should_report_on_proc proc_env in
+    (should_report_on_all_procs && should_report_on_proc proc_env) ||
+    is_thread_safe_method pdesc tenv in
   ResultsTableType.iter (* report errors for each method *)
     (fun proc_env (threaded, _, accesses, _) ->
        if should_report proc_env

infer/tests/codetoanalyze/java/threadsafety/ThreadSafeExample.java

@@ -165,6 +165,17 @@ class NonThreadSafeClass {
     this.field = new Object(); // should warn
   }
 
+  @ThreadSafe
+  private void threadSafePrivateMethod() {
+    this.field = new Object(); // should warn
+  }
+
+  @ThreadSafe
+  @VisibleForTesting
+  public void threadSafeVisibleForTestingMethod() {
+    this.field = new Object(); // should warn
+  }
+
   @ThreadSafe
   public void safeMethod() {
   }

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

@@ -70,6 +70,8 @@ codetoanalyze/java/threadsafety/ReadWriteRaces.java, void ReadWriteRaces.readInC
 codetoanalyze/java/threadsafety/ThreadSafeExample.java, void ExtendsThreadSafeExample.newmethodBad(), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.ExtendsThreadSafeExample.field]
 codetoanalyze/java/threadsafety/ThreadSafeExample.java, void ExtendsThreadSafeExample.tsOK(), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.ExtendsThreadSafeExample.field]
 codetoanalyze/java/threadsafety/ThreadSafeExample.java, void NonThreadSafeClass.threadSafeMethod(), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.NonThreadSafeClass.field]
+codetoanalyze/java/threadsafety/ThreadSafeExample.java, void NonThreadSafeClass.threadSafePrivateMethod(), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.NonThreadSafeClass.field]
+codetoanalyze/java/threadsafety/ThreadSafeExample.java, void NonThreadSafeClass.threadSafeVisibleForTestingMethod(), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.NonThreadSafeClass.field]
 codetoanalyze/java/threadsafety/ThreadSafeExample.java, void NonThreadSafeSubclass.safeMethod(), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.NonThreadSafeClass.field]
 codetoanalyze/java/threadsafety/ThreadSafeExample.java, void ThreadSafeExample.callPublicMethodBad(), 1, THREAD_SAFETY_VIOLATION, [call to void ThreadSafeExample.assignInPrivateMethodOk(),access to codetoanalyze.java.checkers.ThreadSafeExample.f]
 codetoanalyze/java/threadsafety/ThreadSafeExample.java, void ThreadSafeExample.callVisibleForTestingBad(), 1, THREAD_SAFETY_VIOLATION, [call to void ThreadSafeExample.visibleForTestingNotPublicOk(),access to codetoanalyze.java.checkers.ThreadSafeExample.f]