From 3a5a0413bb52fed11f21169758a51daa1fba0713 Mon Sep 17 00:00:00 2001
From: Sam Blackshear <shb@fb.com>
Date: Fri, 13 Oct 2017 15:08:55 -0700
Subject: [PATCH] [thread-safety] remove special treatment for immutable
 collections

Summary:
This was a crutch from the days before ownership analysis.
We shouldn't need it anymore, and it was actually causing FP's because we were skipping analysis of `ImmutableList.builder()` and not understanding that the return value is owned.

Reviewed By: jeremydubreil

Differential Revision: D6035631

fbshipit-source-id: afa0ade
---
 infer/src/checkers/ThreadSafety.ml            | 18 ---------------
 .../java/threadsafety/Builders.java           |  6 -----
 .../java/threadsafety/MyImmutableList.java    | 22 -------------------
 3 files changed, 46 deletions(-)
 delete mode 100644 infer/tests/codetoanalyze/java/threadsafety/MyImmutableList.java

diff --git a/infer/src/checkers/ThreadSafety.ml b/infer/src/checkers/ThreadSafety.ml
index 1d6c650d1..89a7d1000 100644
--- a/infer/src/checkers/ThreadSafety.ml
+++ b/infer/src/checkers/ThreadSafety.ml
@@ -932,23 +932,6 @@ end
 module Analyzer =
   AbstractInterpreter.Make (ProcCfg.Normal) (LowerHil.MakeDefault (TransferFunctions))
 
-(* similarly, we assume that immutable classes safely encapsulate their state *)
-let is_immutable_collection_class class_name tenv =
-  let immutable_collections =
-    [ "com.google.common.collect.ImmutableCollection"
-    ; "com.google.common.collect.ImmutableMap"
-    ; "com.google.common.collect.ImmutableTable" ]
-  in
-  PatternMatch.supertype_exists tenv
-    (fun typename _ -> List.mem ~equal:String.equal immutable_collections (Typ.Name.name typename))
-    class_name
-
-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
-  | _
-   -> false
-
 (* Methods in @ThreadConfined classes and methods annotated with @ThreadConfined are assumed to all
    run on the same thread. For the moment we won't warn on accesses resulting from use of such
    methods at all. In future we should account for races between these methods and methods from
@@ -1005,7 +988,6 @@ let pdesc_is_assumed_thread_safe pdesc tenv =
 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_immutable_collection_method tenv pn)
   && not (pdesc_is_assumed_thread_safe pdesc tenv)
 
 let get_current_class_and_threadsafe_superclasses tenv pname =
diff --git a/infer/tests/codetoanalyze/java/threadsafety/Builders.java b/infer/tests/codetoanalyze/java/threadsafety/Builders.java
index b8f1a52aa..5a954d770 100644
--- a/infer/tests/codetoanalyze/java/threadsafety/Builders.java
+++ b/infer/tests/codetoanalyze/java/threadsafety/Builders.java
@@ -12,7 +12,6 @@ package codetoanalyze.java.checkers;
 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;
 
 public class Builders {
@@ -80,11 +79,6 @@ public class Builders {
     return builder.setFromObj(obj).build();
   }
 
-  @ThreadSafe
-  public void writeImmutableListFieldOk(MyImmutableList<Object> list) {
-    list.writeFld();
-  }
-
   @ThreadSafe
   public Obj mutateBad(Obj o) {
     o.g = "";
diff --git a/infer/tests/codetoanalyze/java/threadsafety/MyImmutableList.java b/infer/tests/codetoanalyze/java/threadsafety/MyImmutableList.java
deleted file mode 100644
index 94125d6e0..000000000
--- a/infer/tests/codetoanalyze/java/threadsafety/MyImmutableList.java
+++ /dev/null
@@ -1,22 +0,0 @@
-/*
- * 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.
- */
-
-// have to pretend we're in the same package to access protected constructor
-package com.google.common.collect;
-
-import com.google.common.collect.ImmutableList;
-
-abstract public class MyImmutableList<T> extends ImmutableList<T> {
-  private Object mFld;
-
-  public void writeFld() {
-    mFld = new Object();
-  }
-
-}