[thread-safety] unify @AssumeThreadSafe and @ThreadSafeMethod into @ThreadSafe(enableChecks = ...)

Summary: Rather than having three separate annotations related to checking/assuming thread-safety, let's just have one annotation instead.

Reviewed By: peterogithub

Differential Revision: D4605258

fbshipit-source-id: 17c935b

infer/annotations/com/facebook/infer/annotation/ThreadSafe.java

@@ -14,11 +14,14 @@ import java.lang.annotation.Retention;
 import java.lang.annotation.RetentionPolicy;
 import java.lang.annotation.Target;
 
-/** assume that a method is thread-safe without actually checking it (as opposed to @ThreadSafe,
+/**
-    which does check). useful for suppressing warnings involving benign races */
+ * Similar to @ThreadSafe annotation from javax.concurrent.annotation, but can be applied to
+ * methods. In addition, you can ask Infer to assume thread-safety rather than checking it by using
+ * @ThreadSafe(enableChecks = false).
+ */
 
-@Target({ ElementType.METHOD })
+@Target({ ElementType.METHOD, ElementType.TYPE })
 @Retention(RetentionPolicy.CLASS)
-public @interface AssumeThreadSafe {
+public @interface ThreadSafe {
-  String because(); /** describe why the thread-safety assumption is reasonable */
+  boolean enableChecks() default true;
 }

infer/annotations/com/facebook/infer/annotation/ThreadSafeMethod.java

@@ -1,22 +0,0 @@
-/*
- * Copyright (c) 2017 - 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 com.facebook.infer.annotation;
-
-import java.lang.annotation.ElementType;
-import java.lang.annotation.Retention;
-import java.lang.annotation.RetentionPolicy;
-import java.lang.annotation.Target;
-
-/** like javax.annotation.concurrent.ThreadSafe, but for individual methods rather than classes */
-
-@Target(ElementType.METHOD)
-@Retention(RetentionPolicy.CLASS)
-public @interface ThreadSafeMethod {
-}

infer/src/checkers/ThreadSafety.ml

@@ -669,8 +669,29 @@ let runs_on_ui_thread proc_desc =
                   Annotations.ia_is_on_unbind annot ||
                   Annotations.ia_is_on_unmount annot)
 
-let is_assumed_thread_safe pdesc =
+
-  Annotations.pdesc_return_annot_ends_with pdesc Annotations.assume_thread_safe
+(* returns true if the annotation is @ThreadSafe or @ThreadSafe(enableChecks = true) *)
+let is_thread_safe item_annot =
+  let f (annot, _) =
+    Annotations.annot_ends_with annot Annotations.thread_safe &&
+    match annot.Annot.parameters with
+    | ["false"] -> false
+    | _ -> true in
+  List.exists ~f item_annot
+
+(* returns true if the annotation is @ThreadSafe(enableChecks = false) *)
+let is_assumed_thread_safe item_annot =
+  let f (annot, _) =
+    Annotations.annot_ends_with annot Annotations.thread_safe &&
+    match annot.Annot.parameters with
+    | ["false"] -> true
+    | _ -> false in
+  List.exists ~f item_annot
+
+let pdesc_is_assumed_thread_safe pdesc tenv =
+  is_assumed_thread_safe (Annotations.pdesc_get_return_annot pdesc) ||
+  PatternMatch.check_current_class_attributes
+    is_assumed_thread_safe tenv (Procdesc.get_proc_name pdesc)
 
 (* return true if we should compute a summary for the procedure. if this returns false, we won't
    analyze the procedure or report any warnings on it *)
@@ -684,7 +705,7 @@ let should_analyze_proc pdesc tenv =
   not (is_call_to_immutable_collection_method tenv pn) &&
   not (runs_on_ui_thread pdesc) &&
   not (is_thread_confined_method tenv pdesc) &&
-  not (is_assumed_thread_safe pdesc)
+  not (pdesc_is_assumed_thread_safe pdesc tenv)
 
 (* return true if we should report on unprotected accesses during the procedure *)
 let should_report_on_proc (_, _, proc_name, proc_desc) =
@@ -762,8 +783,9 @@ let get_current_class_and_threadsafe_superclasses tenv pname =
   match pname with
   | Procname.Java java_pname ->
       let current_class = Procname.java_get_class_type_name java_pname in
-      let thread_safe_annotated_classes = PatternMatch.find_superclasses_with_attributes
+      let thread_safe_annotated_classes =
-          Annotations.ia_is_thread_safe tenv current_class
+        PatternMatch.find_superclasses_with_attributes
+          is_thread_safe tenv current_class
       in
       Some (current_class,thread_safe_annotated_classes)
   | _ -> None  (*shouldn't happen*)
@@ -981,7 +1003,7 @@ let process_results_table file_env tab =
          Annotations.pname_has_return_annot
            pn
            ~attrs_of_pname:Specs.proc_resolve_attributes
-           Annotations.ia_is_thread_safe_method)
+           is_thread_safe)
       tenv
       (Procdesc.get_proc_name pdesc) in
   let should_report ((_, tenv, _, pdesc) as proc_env) =

infer/src/checkers/annotations.ml

@@ -15,7 +15,6 @@ module L = Logging
 (** Annotations. *)
 
 let any_thread = "AnyThread"
-let assume_thread_safe = "AssumeThreadSafe"
 let bind = "Bind"
 let bind_view = "BindView"
 let bind_array = "BindArray"
@@ -56,7 +55,6 @@ let suppress_lint = "SuppressLint"
 let suppress_view_nullability = "SuppressViewNullability"
 let thread_confined = "ThreadConfined"
 let thread_safe = "ThreadSafe"
-let thread_safe_method = "ThreadSafeMethod"
 let true_on_null = "TrueOnNull"
 let ui_thread = "UiThread"
 let verify_annotation = "com.facebook.infer.annotation.Verify"
@@ -122,15 +120,6 @@ let struct_typ_has_annot (struct_typ : StructTyp.t) predicate =
 let ia_is_not_thread_safe ia =
   ia_ends_with ia not_thread_safe
 
-let ia_is_thread_safe ia =
-  ia_ends_with ia thread_safe
-
-let ia_is_thread_safe_method ia =
-  ia_ends_with ia thread_safe_method
-
-let ia_is_assume_thread_safe ia =
-  ia_ends_with ia assume_thread_safe
-
 let ia_is_nullable ia =
   ia_ends_with ia nullable
 

infer/src/checkers/annotations.mli

@@ -12,7 +12,6 @@ open! IStd
 (** Annotations. *)
 
 val any_thread : string
-val assume_thread_safe : string
 val expensive : string
 val no_allocation : string
 val nullable : string
@@ -26,7 +25,6 @@ val strict : string
 val suppress_lint : string
 val thread_confined : string
 val thread_safe : string
-val thread_safe_method : string
 val ui_thread : string
 val visibleForTesting : string
 
@@ -77,9 +75,6 @@ val ia_is_integrity_source : Annot.Item.t -> bool
 val ia_is_integrity_sink : Annot.Item.t -> bool
 val ia_is_guarded_by : Annot.Item.t -> bool
 val ia_is_not_thread_safe : Annot.Item.t -> bool
-val ia_is_thread_safe : Annot.Item.t -> bool
-val ia_is_thread_safe_method : Annot.Item.t -> bool
-val ia_is_assume_thread_safe : Annot.Item.t -> bool
 val ia_is_ui_thread : Annot.Item.t -> bool
 val ia_is_thread_confined : Annot.Item.t -> bool
 val ia_is_returns_ownership : Annot.Item.t -> bool

infer/tests/codetoanalyze/java/threadsafety/Annotations.java

@@ -14,11 +14,9 @@ import java.lang.annotation.Retention;
 import java.lang.annotation.RetentionPolicy;
 import java.lang.annotation.Target;
 
-import javax.annotation.concurrent.ThreadSafe;
-
 import android.support.annotation.UiThread;
 
-import com.facebook.infer.annotation.AssumeThreadSafe;
+import com.facebook.infer.annotation.ThreadSafe;
 import com.facebook.infer.annotation.Functional;
 import com.facebook.infer.annotation.ThreadConfined;
 import com.facebook.infer.annotation.ReturnsOwnership;
@@ -56,6 +54,16 @@ interface Interface {
   @ReturnsOwnership Obj returnsOwnershipMethod();
 }
 
+@ThreadSafe(enableChecks = false)
+class AssumedThreadSafe {
+
+  Object field;
+
+  public void writeOk() {
+    this.field = new Object();
+  }
+}
+
 @ThreadSafe
 class Annotations implements Interface {
   Object f;
@@ -138,7 +146,7 @@ class Annotations implements Interface {
     this.f = new Object();
   }
 
-  @AssumeThreadSafe(because = "it's a test")
+  @ThreadSafe(enableChecks = false)
   public void assumeThreadSafeOk() {
     this.f = new Object();
   }
@@ -293,4 +301,8 @@ class Annotations implements Interface {
     owned.f = new Object();
   }
 
+  public void writeToAssumedThreadSafeClassOk(AssumedThreadSafe c) {
+    c.writeOk();
+  }
+
 }

infer/tests/codetoanalyze/java/threadsafety/ThreadSafeExample.java

@@ -10,11 +10,10 @@
 package codetoanalyze.java.checkers;
 
 import javax.annotation.concurrent.NotThreadSafe;
-import javax.annotation.concurrent.ThreadSafe;
 
-import com.google.common.annotations.VisibleForTesting;
+import com.facebook.infer.annotation.ThreadSafe;
 
-import com.facebook.infer.annotation.ThreadSafeMethod;
+import com.google.common.annotations.VisibleForTesting;
 
 @ThreadSafe
 public class ThreadSafeExample{
@@ -161,12 +160,12 @@ class NonThreadSafeClass {
 
   Object field;
 
-  @ThreadSafeMethod
+  @ThreadSafe
   public void threadSafeMethod() {
     this.field = new Object(); // should warn
   }
 
-  @ThreadSafeMethod
+  @ThreadSafe
   public void safeMethod() {
   }
 
@@ -175,12 +174,12 @@ class NonThreadSafeClass {
 class NonThreadSafeSubclass extends NonThreadSafeClass {
 
   @Override
-  // overrides method annotated with @ThreadSafeMethod, should warn
+  // overrides method annotated with @ThreadSafe, should warn
   public void safeMethod() {
     this.field = new Object();
   }
 
-  // won't report this now, but should in the future. if a method annotated with @ThreadSafeMethod
+  // won't report this now, but should in the future. if a method annotated with @ThreadSafe
   // in class C touches field f, then all other accesses to f in C must also be thread-safe
   public void FN_touchesSameFieldAsThreadSafeMethod() {
     this.field = new Object();