From 574335190682c0a5d302227d7466f9ae2310e82e Mon Sep 17 00:00:00 2001
From: Sam Blackshear <shb@fb.com>
Date: Fri, 2 Dec 2016 09:30:14 -0800
Subject: [PATCH] [thread-safety] don't report thread safety violations due to
 calls to constructors

Summary:
`o.<init>` cannot be called in parallel with other methods of `o` from outside, so it's less likely to have thread safety violations in `o.<init>`.
This diff suppresses reporting of thread safety violations for fields touched (transitively) by a constructor.
We can do better than this in the future (t14842325).

Reviewed By: peterogithub

Differential Revision: D4259719

fbshipit-source-id: 20db71f
---
 infer/src/checkers/ThreadSafety.ml                 | 14 +++++++++-----
 .../java/threadsafety/ThreadSafeExample.java       | 13 +++++++++++--
 2 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/infer/src/checkers/ThreadSafety.ml b/infer/src/checkers/ThreadSafety.ml
index f08030826..f33c02871 100644
--- a/infer/src/checkers/ThreadSafety.ml
+++ b/infer/src/checkers/ThreadSafety.ml
@@ -23,6 +23,10 @@ module Summary = Summary.Make (struct
       payload.Specs.threadsafety
   end)
 
+let is_initializer tenv proc_name =
+  Procname.is_constructor proc_name ||
+  Procname.is_class_initializer proc_name ||
+  FbThreadSafety.is_custom_init tenv proc_name
 
 module TransferFunctions (CFG : ProcCfg.S) = struct
   module CFG = CFG
@@ -63,7 +67,7 @@ module TransferFunctions (CFG : ProcCfg.S) = struct
       path_state
       (AccessPath.of_exp exp typ ~f_resolve_id:(fun _ -> None))
 
-  let exec_instr ((lockstate, (readstate,writestate)) as astate) { ProcData.pdesc; } _ =
+  let exec_instr ((lockstate, (readstate,writestate)) as astate) { ProcData.pdesc; tenv; } _ =
     let is_unprotected is_locked =
       not is_locked && not (Procdesc.is_java_synchronized pdesc) in
     function
@@ -81,7 +85,9 @@ module TransferFunctions (CFG : ProcCfg.S) = struct
                 | Some (callee_lockstate, (callee_reads, callee_writes)) ->
                     let lockstate' = callee_lockstate || lockstate in
                     let _, read_writestate' =
-                      if is_unprotected lockstate'
+                      (* TODO (14842325): report on constructors that aren't threadsafe
+                         (e.g., constructors that access static fields) *)
+                      if is_unprotected lockstate' && not (is_initializer tenv pn)
                       then
                         let call_site = CallSite.make pn loc in
                         let callee_readstate =
@@ -152,10 +158,8 @@ module ResultsTableType = Map.Make (struct
   end)
 
 let should_report_on_proc (_,tenv,proc_name,proc_desc) =
-  not (FbThreadSafety.is_custom_init tenv proc_name) &&
+  not (is_initializer tenv proc_name) &&
   not (Procname.java_is_autogen_method proc_name) &&
-  not (Procname.is_constructor proc_name) &&
-  not (Procname.is_class_initializer proc_name) &&
   Procdesc.get_access proc_desc <> PredSymb.Private
 
 (* creates a map from proc_envs to postconditions *)
diff --git a/infer/tests/codetoanalyze/java/threadsafety/ThreadSafeExample.java b/infer/tests/codetoanalyze/java/threadsafety/ThreadSafeExample.java
index a92270b43..e0a7297a4 100644
--- a/infer/tests/codetoanalyze/java/threadsafety/ThreadSafeExample.java
+++ b/infer/tests/codetoanalyze/java/threadsafety/ThreadSafeExample.java
@@ -108,8 +108,17 @@ public class ThreadSafeExample{
     synchronizedCallerOk();
   }
 
-  // doesn't work because we don't model lock
-  public void FP_tsWithLockOk() {
+  // although the constructor touches f, we shouldn't complain here
+  public void callConstructorOk() {
+    new ThreadSafeExample();
+  }
+
+  private Object returnConstructorOk() {
+    return new ThreadSafeExample();
+  }
+
+  public void transitivelyCallConstructorOk() {
+    returnConstructorOk();
   }
 
   public void withLockBothBranchesOk(boolean b) {