[thread-safety] don't count accesses in methods of builder classes

Summary:
Although the Builder pattern is not actually thread-safe, Builder's are not expected to be shared between threads.
Handle this by ignoring all unprotected accesses in classes the end with "Builder".
We might be able to soften this heuristic in the future by ensuring rather than assuming that Builder are not shared between methods (or, ideally, between threads).

Reviewed By: peterogithub

Differential Revision: D4280761

fbshipit-source-id: a4e6738

infer/src/checkers/ThreadSafety.ml

@@ -23,6 +23,17 @@ module Summary = Summary.Make (struct
       payload.Specs.threadsafety
   end)
 
+(* we want to consider Builder classes and other safe immutablility-ensuring patterns as
+   thread-safe. we are overly friendly about this for now; any class whose name ends with `Builder`
+   is assumed to be thread-safe. in the future, we can ask for builder classes to be annotated with
+   @Builder and verify that annotated classes satisfy the expected invariants. *)
+let is_builder_class class_name =
+  string_is_suffix "Builder" class_name
+
+let is_call_to_builder_class_method = function
+  | Procname.Java java_pname -> is_builder_class (Procname.java_get_class_name java_pname)
+  | _ -> false
+
 let is_initializer tenv proc_name =
   Procname.is_constructor proc_name ||
   Procname.is_class_initializer proc_name ||
@@ -92,7 +103,9 @@ module TransferFunctions (CFG : ProcCfg.S) = struct
                     let _, read_writestate' =
                       (* 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)
+                      if is_unprotected lockstate' &&
+                         not (is_initializer tenv pn) &&
+                         not (is_call_to_builder_class_method pn)
                       then
                         let call_site = CallSite.make pn loc in
                         let callee_readstate =

infer/tests/codetoanalyze/java/threadsafety/Builders.java

@@ -0,0 +1,95 @@
+/*
+ * 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 com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableList.Builder;
+
+@ThreadSafe
+public class Builders {
+
+  static class Obj {
+    final String f;
+    String g;
+
+    public Obj(String f, String g) {
+      this.f = f;
+      this.g = g;
+    }
+
+    public static class Builder {
+      String f;
+      String g;
+
+      public Builder setFromObj(Obj input) {
+        this.f = input.f;
+        this.g = input.g;
+        return this;
+      }
+
+      public Obj build() {
+        return new Obj(f, g);
+      }
+
+      public Builder setF(String f) {
+        this.f = f;
+        return this;
+      }
+
+      public Builder setG(String g) {
+        this.g = g;
+        return this;
+      }
+    }
+  }
+
+  public void guavaBuilderOk(ImmutableList.Builder<String> builder) {
+    builder.add("foo");
+    builder.build();
+  }
+
+  public Obj customBuilderOk1(Obj.Builder builder) {
+    builder.setF("f");
+    builder.setG("g");
+    return builder.build();
+  }
+
+  public Obj customBuilderOk2(Obj.Builder builder) {
+    return builder.setF("f").setG("g").build();
+  }
+
+  public Obj customBuilderOk3(Obj input) {
+    Obj.Builder builder = new Obj.Builder();
+    return builder.setFromObj(input).build();
+  }
+
+  public Obj mutateBad(Obj o) {
+    o.g = "";
+    return o;
+  }
+
+  public Obj buildThenMutateBad(Obj input) {
+    Obj.Builder builder = new Obj.Builder();
+    Obj output = builder.setFromObj(input).build();
+    input.g = "";
+    return output;
+  }
+
+}
+
+@ThreadSafe
+class TopLevelBuilder {
+  public String g;
+
+  public void setG(String g) {
+    this.g = g; // still want to warn if the builder is annotated ThreadSafe
+  }
+
+}

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

@@ -1,3 +1,6 @@
+codetoanalyze/java/threadsafety/Builders.java, Builders$Obj Builders.buildThenMutateBad(Builders$Obj), 3, THREAD_SAFETY_ERROR, [access to codetoanalyze.java.checkers.Builders$Obj.g]
+codetoanalyze/java/threadsafety/Builders.java, Builders$Obj Builders.mutateBad(Builders$Obj), 1, THREAD_SAFETY_ERROR, [access to codetoanalyze.java.checkers.Builders$Obj.g]
+codetoanalyze/java/threadsafety/Builders.java, void TopLevelBuilder.setG(String), 1, THREAD_SAFETY_ERROR, [access to codetoanalyze.java.checkers.TopLevelBuilder.g]
 codetoanalyze/java/threadsafety/Locks.java, void Locks.FP_unlockOneLock(), 4, THREAD_SAFETY_ERROR, [access to codetoanalyze.java.checkers.Locks.f]
 codetoanalyze/java/threadsafety/Locks.java, void Locks.afterReentrantLockUnlockBad(), 3, THREAD_SAFETY_ERROR, [access to codetoanalyze.java.checkers.Locks.f]
 codetoanalyze/java/threadsafety/Locks.java, void Locks.afterUnlockBad(), 3, THREAD_SAFETY_ERROR, [access to codetoanalyze.java.checkers.Locks.f]