From 7b44236874bb78be282a41f9b8cbfb7b2558fb83 Mon Sep 17 00:00:00 2001 From: Sam Blackshear Date: Mon, 1 May 2017 11:39:31 -0700 Subject: [PATCH] [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 | 12 ---------- .../java/threadsafety/Builders.java | 24 +++++++++++++------ .../java/threadsafety/issues.exp | 2 ++ 3 files changed, 19 insertions(+), 19 deletions(-) diff --git a/infer/src/checkers/ThreadSafety.ml b/infer/src/checkers/ThreadSafety.ml index 5a364f585..db03494fd 100644 --- a/infer/src/checkers/ThreadSafety.ml +++ b/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) diff --git a/infer/tests/codetoanalyze/java/threadsafety/Builders.java b/infer/tests/codetoanalyze/java/threadsafety/Builders.java index eb4e01acb..b8f1a52aa 100644 --- a/infer/tests/codetoanalyze/java/threadsafety/Builders.java +++ b/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 builder) { + @ThreadSafe + public void guavaBuilderOk() { + ImmutableList.Builder 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 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(); diff --git a/infer/tests/codetoanalyze/java/threadsafety/issues.exp b/infer/tests/codetoanalyze/java/threadsafety/issues.exp index 0652a16db..b4bb95408 100644 --- a/infer/tests/codetoanalyze/java/threadsafety/issues.exp +++ b/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, [,access to `codetoanalyze.java.checkers.Annotations.f`,,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, [,call to Builders$Obj$Builder Builders$Obj$Builder.setFromObj(Builders$Obj),access to `codetoanalyze.java.checkers.Builders$Obj.g`,,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.(Object),access to `Constructors.staticField`] codetoanalyze/java/threadsafety/Constructors.java, Constructors.(), 1, THREAD_SAFETY_VIOLATION, [access to `Constructors.staticField`] codetoanalyze/java/threadsafety/Constructors.java, Constructors.(Constructors), 1, THREAD_SAFETY_VIOLATION, [access to `Constructors.field`]