[threadsafety] Better error message when warning on subclasses of @ThreadSafe classes

Summary:
We issue a thread safety warning on a class not
marked ThreadSafe, when it has a super that is. This makes some sense. But,
it will be nice to remind that a super is so maeked, else the mesg could
seem out of context or surprising

Reviewed By: sblackshear

Differential Revision: D4075145

fbshipit-source-id: ebc2b83

infer/src/checkers/ThreadSafety.ml

@@ -128,13 +128,35 @@ let make_results_table file_env =
   in
   map_post_computation_over_procs compute_post_for_procedure procs_to_analyze
 
+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
+          Annotations.ia_is_thread_safe tenv current_class
+      in
+      Some (current_class,thread_safe_annotated_classes)
+  | _ -> None  (*shouldn't happen*)
+
+(** The addendum message says that a superclass is marked @ThreadSafe,
+    when the current class is not so marked*)
+let calculate_addendum_message tenv pname =
+  match get_current_class_and_threadsafe_superclasses tenv pname with
+  | Some (current_class,thread_safe_annotated_classes) ->
+      if not (IList.mem Typename.equal current_class thread_safe_annotated_classes) then
+        match thread_safe_annotated_classes with
+        | hd::_ -> F.asprintf "\n Note: Superclass %a is marked @ThreadSafe." Typename.pp hd
+        | [] -> ""
+      else ""
+  | _ -> ""
 
 let report_thread_safety_errors ( _, tenv, pname, pdesc) writestate =
   let report_one_error access_path =
     let description =
-      F.asprintf "Method %a writes to field %a outside of synchronization"
+      F.asprintf "Method %a writes to field %a outside of synchronization. %s"
         Procname.pp pname
         AccessPath.pp_access_list access_path
+        (calculate_addendum_message tenv pname)
     in
     Checkers.ST.report_error tenv
       pname
@@ -157,11 +179,14 @@ let process_results_table tab =
    the classes in a file. This might be tightened in future or even broadened in future
    based on other criteria *)
 let should_analyze_file file_env =
-  IList.exists
-    (fun (_, tenv, pname, _) ->
-       PatternMatch.check_class_attributes Annotations.ia_is_thread_safe tenv pname
-    )
-    file_env
+  let current_class_or_super_marked_threadsafe =
+    fun (_, tenv, pname, _) ->
+      match get_current_class_and_threadsafe_superclasses tenv pname with
+      | Some (_, thread_safe_annotated_classes) ->
+          not (thread_safe_annotated_classes = [])
+      | _ -> false
+  in
+  IList.exists current_class_or_super_marked_threadsafe file_env
 
 (*Gathers results by analyzing all the methods in a file, then post-processes
   the results to check (approximation of) thread safety *)

infer/src/checkers/patternMatch.ml

@@ -379,3 +379,16 @@ let check_class_attributes check tenv = function
         check_class_annots
         (Procname.java_get_class_type_name java_pname)
   | _ -> false
+
+(** find superclasss with attributes (e.g., @ThreadSafe), including current class*)
+let rec find_superclasses_with_attributes check tenv tname =
+  match Tenv.lookup tenv tname with
+  | Some (struct_typ) ->
+      let result_from_supers = IList.flatten
+          (IList.map (find_superclasses_with_attributes check tenv) struct_typ.supers)
+      in
+      if check struct_typ.annots then
+        tname ::result_from_supers
+      else
+        result_from_supers
+  | _ -> []

infer/src/checkers/patternMatch.mli

@@ -113,3 +113,7 @@ val is_runtime_exception : Tenv.t -> Typename.t -> bool
 
 (** tests whether any class attributes (e.g., @ThreadSafe) pass check of first argument*)
 val check_class_attributes : (Annot.Item.t -> bool) -> Tenv.t -> Procname.t -> bool
+
+(** find superclasss with attributes (e.g., @ThreadSafe), including current class*)
+val find_superclasses_with_attributes : (Annot.Item.t -> bool) -> Tenv.t
+  -> Typename.t -> Typename.t list

infer/tests/codetoanalyze/java/checkers/ExtendsThreadSafeExample.java

@@ -0,0 +1,24 @@
+/*
+ * 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.
+ */
+class ExtendsThreadSafeExample extends ThreadSafeExample{
+
+  Integer field;
+
+  (* Presently,we will warn not just on overwridden methods from
+  @ThreadSafe class, but potentially on other methods in subclass *)
+  public void newmethodBad() {
+     field = 22;
+  }
+
+  (* Bad now that it's overridden *)
+  public void tsOK() {
+     field = 44;
+  }
+
+}

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

@@ -21,6 +21,8 @@ ExpensiveInheritanceExample.java, void ExpensiveInheritanceExample.doesReportBec
 ExpensiveInheritanceExample.java, void ExpensiveInheritanceExample.reportsAssumingObjectOfTypeA(), 2, CHECKERS_CALLS_EXPENSIVE_METHOD
 ExpensiveInheritanceExample.java, void ExpensiveInheritanceExample.reportsBecauseFooIsExpensiveInA(A), 1, CHECKERS_CALLS_EXPENSIVE_METHOD
 ExpensiveSubtypingExample.java, void ExpensiveSubtypingExample.m3(), 0, CHECKERS_EXPENSIVE_OVERRIDES_UNANNOTATED
+ExtendsThreadSafeExample.java, void ExtendsThreadSafeExample.newmethodBad(), 0, CHECKERS_THREAD_SAFETY_WARNING
+ExtendsThreadSafeExample.java, void ExtendsThreadSafeExample.tsOK(), 0, CHECKERS_THREAD_SAFETY_WARNING
 FragmentRetainsViewExample.java, void FragmentRetainsViewExample.onDestroyView(), 0, CHECKERS_FRAGMENT_RETAINS_VIEW
 FragmentRetainsViewExample.java, void FragmentRetainsViewExample.onDestroyView(), 0, CHECKERS_FRAGMENT_RETAINS_VIEW
 FragmentRetainsViewExample.java, void FragmentRetainsViewExample.onDestroyView(), 0, CHECKERS_FRAGMENT_RETAINS_VIEW