[thread-safety] remove hacky special case for Builder's

Summary:
Before we understood ownership, we needed this to avoid a mountain of Builder-related FP's.
Now that we have fairly sophisticated understanding of ownership, we can kill this hack.

Reviewed By: jaegs

Differential Revision: D4940238

fbshipit-source-id: 8d86e57

infer/src/checkers/ThreadSafety.ml

@@ -676,13 +676,6 @@ module Analyzer = AbstractInterpreter.Make (ProcCfg.Normal) (TransferFunctions)
 
 module Interprocedural = AbstractInterpreter.Interprocedural (Summary)
 
-(* 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 ~suffix:"Builder" class_name
-
 (* similarly, we assume that immutable classes safely encapsulate their state *)
 let is_immutable_collection_class class_name tenv =
   let immutable_collections = [
@@ -696,10 +689,6 @@ let is_immutable_collection_class class_name tenv =
        List.mem ~equal:String.equal immutable_collections (Typ.Name.name typename))
     class_name
 
-let is_call_to_builder_class_method = function
-  | Typ.Procname.Java java_pname -> is_builder_class (Typ.Procname.java_get_class_name java_pname)
-  | _ -> false
-
 let is_call_to_immutable_collection_method tenv = function
   | Typ.Procname.Java java_pname ->
       is_immutable_collection_class (Typ.Procname.java_get_class_type_name java_pname) tenv
@@ -770,7 +759,6 @@ let should_analyze_proc pdesc tenv =
   let pn = Procdesc.get_proc_name pdesc in
   not (Typ.Procname.is_class_initializer pn) &&
   not (FbThreadSafety.is_logging_method pn) &&
-  not (is_call_to_builder_class_method pn) &&
   not (is_call_to_immutable_collection_method tenv pn) &&
   not (pdesc_is_assumed_thread_safe pdesc tenv)
 

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

@@ -9,13 +9,12 @@
 
 package codetoanalyze.java.checkers;
 
-import javax.annotation.concurrent.ThreadSafe;
+import com.facebook.infer.annotation.ThreadSafe;
 
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.MyImmutableList;
 import com.google.common.collect.ImmutableList.Builder;
 
-@ThreadSafe
 public class Builders {
 
   static class Obj {
@@ -53,35 +52,46 @@ public class Builders {
     }
   }
 
-  public void guavaBuilderOk(ImmutableList.Builder<String> builder) {
+  @ThreadSafe
+  public void guavaBuilderOk() {
+    ImmutableList.Builder<String> builder = new ImmutableList.Builder();
     builder.add("foo");
     builder.build();
   }
 
-  public Obj customBuilderOk1(Obj.Builder builder) {
+  @ThreadSafe
+  public Obj customBuilderOk1() {
+    Obj.Builder builder = new Obj.Builder();
     builder.setF("f");
     builder.setG("g");
     return builder.build();
   }
 
-  public Obj customBuilderOk2(Obj.Builder builder) {
+  @ThreadSafe
+  public Obj customBuilderOk2() {
+    Obj.Builder builder = new Obj.Builder();
     return builder.setF("f").setG("g").build();
   }
 
-  public Obj customBuilderOk3(Obj input) {
+  @ThreadSafe
+  public Obj customBuilderOk3() {
+    Obj obj = new Obj("a", "b");
     Obj.Builder builder = new Obj.Builder();
-    return builder.setFromObj(input).build();
+    return builder.setFromObj(obj).build();
   }
 
+  @ThreadSafe
   public void writeImmutableListFieldOk(MyImmutableList<Object> list) {
     list.writeFld();
   }
 
+  @ThreadSafe
   public Obj mutateBad(Obj o) {
     o.g = "";
     return o;
   }
 
+  @ThreadSafe
   public Obj buildThenMutateBad(Obj input) {
     Obj.Builder builder = new Obj.Builder();
     Obj output = builder.setFromObj(input).build();

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

@@ -11,8 +11,10 @@ codetoanalyze/java/threadsafety/Annotations.java, void Annotations.read_from_non
 codetoanalyze/java/threadsafety/Annotations.java, void Annotations.read_off_UI_thread_Bad(), 1, THREAD_SAFETY_VIOLATION, [<Beginning of read trace>,access to `codetoanalyze.java.checkers.Annotations.f`,<Beginning of write trace>,access to `codetoanalyze.java.checkers.Annotations.f`]
 codetoanalyze/java/threadsafety/Annotations.java, void ThreadSafeAlias.threadSafeAliasBad1(), 1, THREAD_SAFETY_VIOLATION, [access to `codetoanalyze.java.checkers.ThreadSafeAlias.field`]
 codetoanalyze/java/threadsafety/Annotations.java, void ThreadSafeAlias.threadSafeAliasBad2(), 1, THREAD_SAFETY_VIOLATION, [access to `codetoanalyze.java.checkers.ThreadSafeAlias.field`]
+codetoanalyze/java/threadsafety/Builders.java, Builders$Obj Builders.buildThenMutateBad(Builders$Obj), 2, THREAD_SAFETY_VIOLATION, [<Beginning of read trace>,call to Builders$Obj$Builder Builders$Obj$Builder.setFromObj(Builders$Obj),access to `codetoanalyze.java.checkers.Builders$Obj.g`,<Beginning of write trace>,access to `codetoanalyze.java.checkers.Builders$Obj.g`]
 codetoanalyze/java/threadsafety/Builders.java, Builders$Obj Builders.buildThenMutateBad(Builders$Obj), 3, THREAD_SAFETY_VIOLATION, [access to `codetoanalyze.java.checkers.Builders$Obj.g`]
 codetoanalyze/java/threadsafety/Builders.java, Builders$Obj Builders.mutateBad(Builders$Obj), 1, THREAD_SAFETY_VIOLATION, [access to `codetoanalyze.java.checkers.Builders$Obj.g`]
+codetoanalyze/java/threadsafety/Builders.java, void TopLevelBuilder.setG(String), 1, THREAD_SAFETY_VIOLATION, [access to `codetoanalyze.java.checkers.TopLevelBuilder.g`]
 codetoanalyze/java/threadsafety/Constructors.java, Constructors Constructors.singletonBad(), 2, THREAD_SAFETY_VIOLATION, [call to Constructors.<init>(Object),access to `Constructors.staticField`]
 codetoanalyze/java/threadsafety/Constructors.java, Constructors.<init>(), 1, THREAD_SAFETY_VIOLATION, [access to `Constructors.staticField`]
 codetoanalyze/java/threadsafety/Constructors.java, Constructors.<init>(Constructors), 1, THREAD_SAFETY_VIOLATION, [access to `Constructors.field`]