[infer][java] only report one annotation reachability issue per end of call stack

Summary: The purpose of the annotation reachability analysis is to report when a method annotated with `X` never calls, directly or indirectly, another method annotated with `Y`. However, there can be different call stacks following different execution paths from `X` to `Y`. Reporting more than one call stack ending with the same annotated procedure does not bring more signal to the end user. So the purpose of this diff is to avoid those duplicated reports and report at most one annotation reachability issue per end of call stack.

Reviewed By: sblackshear

Differential Revision: D4942765

fbshipit-source-id: 46325a7

infer/src/IR/CallSite.ml

@@ -40,5 +40,3 @@ module Set = PrettyPrintable.MakePPSet(struct
     let compare = compare
     let pp = pp
   end)
-
-module SetDomain = AbstractDomain.FiniteSet (Set)

infer/src/IR/CallSite.mli

@@ -26,5 +26,3 @@ val dummy : t
 val pp : F.formatter -> t -> unit
 
 module Set : PrettyPrintable.PPSet with type elt = t
-
-module SetDomain : module type of AbstractDomain.FiniteSet (Set)

infer/src/checkers/AnnotReachabilityDomain.ml

@@ -7,4 +7,8 @@
  * of patent rights can be found in the PATENTS file in the same directory.
  *)
 
-include AbstractDomain.Map (Annot.Map) (CallSite.SetDomain)
+module CallSites = AbstractDomain.FiniteSet (CallSite.Set)
+
+module SinkMap = AbstractDomain.Map (Typ.Procname.Map) (CallSites)
+
+include AbstractDomain.Map (Annot.Map) (SinkMap)

infer/src/checkers/annotationReachability.ml

@@ -46,17 +46,22 @@ module Domain = struct
   module TrackingDomain = AbstractDomain.BottomLifted (TrackingVar)
   include AbstractDomain.Pair (AnnotReachabilityDomain) (TrackingDomain)
 
-  let add_call_site annot call_site ((annot_map, previous_vstate) as astate) =
+  let add_call_site annot sink call_site ((annot_map, previous_vstate) as astate) =
     match previous_vstate with
     | TrackingDomain.Bottom -> astate
     | TrackingDomain.NonBottom _ ->
-        let call_set =
+        let sink_map =
           try AnnotReachabilityDomain.find annot annot_map
-          with Not_found -> CallSite.SetDomain.empty in
+          with Not_found -> AnnotReachabilityDomain.SinkMap.empty in
-        let call_set' = CallSite.SetDomain.add call_site call_set in
+        let sink_map' =
-        if phys_equal call_set' call_set
+          if AnnotReachabilityDomain.SinkMap.mem sink sink_map
+          then sink_map
+          else
+            let singleton = AnnotReachabilityDomain.CallSites.singleton call_site in
+            AnnotReachabilityDomain.SinkMap.singleton sink singleton in
+        if phys_equal sink_map' sink_map
         then astate
-        else (AnnotReachabilityDomain.add annot call_set' annot_map, previous_vstate)
+        else (AnnotReachabilityDomain.add annot sink_map' annot_map, previous_vstate)
 
   let stop_tracking (annot_map, _ : astate) =
     (annot_map, TrackingDomain.Bottom)
@@ -149,16 +154,17 @@ let method_has_annot annot tenv pname =
 let method_overrides_annot annot tenv pname =
   method_overrides (method_has_annot annot) tenv pname
 
-let lookup_annotation_calls caller_pdesc annot pname : CallSite.SetDomain.t =
+let lookup_annotation_calls caller_pdesc annot pname =
   match Ondemand.analyze_proc_name ~propagate_exceptions:false caller_pdesc pname with
   | Some { Specs.payload = { Specs.annot_map = Some annot_map; }; } ->
       begin
         try
           Annot.Map.find annot annot_map
         with Not_found ->
-          CallSite.SetDomain.empty
+          AnnotReachabilityDomain.SinkMap.empty
       end
-  | _ -> CallSite.SetDomain.empty
+  | _ ->
+      AnnotReachabilityDomain.SinkMap.empty
 
 let update_trace loc trace =
   if Location.equal loc Location.dummy then trace
@@ -207,7 +213,7 @@ let report_annotation_stack src_annot snk_annot src_summary loc trace stack_str
       Exceptions.Checkers (msg, Localise.verbatim_desc description) in
     Reporting.log_error_from_summary src_summary ~loc ~ltr:final_trace exn
 
-let report_call_stack summary end_of_stack lookup_next_calls report call_site calls =
+let report_call_stack summary end_of_stack lookup_next_calls report call_site sink_map =
   (* TODO: stop using this; we can use the call site instead *)
   let lookup_location pname =
     match Specs.get_summary pname with
@@ -222,23 +228,29 @@ let report_call_stack summary end_of_stack lookup_next_calls report call_site ca
       let callee_pname_str = string_of_pname callee_pname in
       let new_stack_str = stack_str ^ callee_pname_str ^ " -> " in
       let new_trace = update_trace call_loc trace |> update_trace callee_def_loc in
-      let unseen_callees, visited_callees =
+      let unseen_callees, updated_callees =
-        CallSite.SetDomain.fold
+        AnnotReachabilityDomain.SinkMap.fold
-          (fun call_site ((unseen, visited) as accu) ->
+          (fun _ call_sites ((unseen, visited) as accu) ->
-             let p = CallSite.pname call_site in
+             try
-             let loc = CallSite.loc call_site in
+               let call_site = AnnotReachabilityDomain.CallSites.choose call_sites in
-             if Typ.Procname.Set.mem p visited then accu
+               let p = CallSite.pname call_site in
-             else ((p, loc) :: unseen, Typ.Procname.Set.add p visited))
+               let loc = CallSite.loc call_site in
+               if Typ.Procname.Set.mem p visited then accu
+               else ((p, loc) :: unseen, Typ.Procname.Set.add p visited)
+             with Not_found -> accu)
           next_calls
           ([], visited_pnames) in
-      List.iter ~f:(loop fst_call_loc visited_callees (new_trace, new_stack_str)) unseen_callees in
+      List.iter ~f:(loop fst_call_loc updated_callees (new_trace, new_stack_str)) unseen_callees in
-  CallSite.SetDomain.iter
+  AnnotReachabilityDomain.SinkMap.iter
-    (fun fst_call_site ->
+    (fun _ call_sites ->
-       let fst_callee_pname = CallSite.pname fst_call_site in
+       try
-       let fst_call_loc = CallSite.loc fst_call_site in
+         let fst_call_site = AnnotReachabilityDomain.CallSites.choose call_sites in
-       let start_trace = update_trace (CallSite.loc call_site) [] in
+         let fst_callee_pname = CallSite.pname fst_call_site in
-       loop fst_call_loc Typ.Procname.Set.empty (start_trace, "") (fst_callee_pname, fst_call_loc))
+         let fst_call_loc = CallSite.loc fst_call_site in
-    calls
+         let start_trace = update_trace (CallSite.loc call_site) [] in
+         loop fst_call_loc Typ.Procname.Set.empty (start_trace, "") (fst_callee_pname, fst_call_loc)
+       with Not_found -> ())
+    sink_map
 
 module TransferFunctions (CFG : ProcCfg.S) = struct
   module CFG = CFG
@@ -285,7 +297,7 @@ module TransferFunctions (CFG : ProcCfg.S) = struct
       ~f:(fun astate (_, annot) ->
           if method_has_annot annot tenv callee_pname &&
              not (method_is_sanitizer annot tenv caller_pname)
-          then Domain.add_call_site annot call_site astate
+          then Domain.add_call_site annot callee_pname call_site astate
           else astate)
       src_snk_pairs
 
@@ -293,11 +305,16 @@ module TransferFunctions (CFG : ProcCfg.S) = struct
     match Summary.read_summary pdesc callee_pname with
     | None -> astate
     | Some callee_call_map ->
+        let add_call_site annot sink calls astate =
+          if AnnotReachabilityDomain.CallSites.is_empty calls
+          then astate
+          else Domain.add_call_site annot sink call_site astate in
         Annot.Map.fold
-          (fun annot calls astate ->
+          (fun annot sink_map astate ->
-             if CallSite.SetDomain.is_empty calls
+             AnnotReachabilityDomain.SinkMap.fold
-             then astate
+               (add_call_site annot)
-             else Domain.add_call_site annot call_site astate)
+               sink_map
+               astate)
           callee_call_map
           astate
 
@@ -359,7 +376,7 @@ module Interprocedural = struct
       PatternMatch.override_iter check_expensive_subtyping_rules tenv proc_name;
 
     let report_src_snk_paths annot_map (src_annot_list, (snk_annot: Annot.t)) =
-      let report_src_snk_path (calls : CallSite.SetDomain.t) (src_annot: Annot.t) =
+      let report_src_snk_path sink_map (src_annot: Annot.t) =
         if method_overrides_annot src_annot tenv proc_name
         then
           let f_report =
@@ -370,7 +387,7 @@ module Interprocedural = struct
             (lookup_annotation_calls proc_desc snk_annot)
             f_report
             (CallSite.make proc_name loc)
-            calls in
+            sink_map in
       try
         let sink_map = Annot.Map.find snk_annot annot_map in
         List.iter ~f:(report_src_snk_path sink_map) src_annot_list

infer/tests/codetoanalyze/java/checkers/AnnotationReachabilityDuplicatesExample.java

@@ -0,0 +1,54 @@
+/*
+ * Copyright (c) 2013 - 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 codetoanalyze.java.checkers;
+
+import com.facebook.infer.annotation.Expensive;
+import com.facebook.infer.annotation.PerformanceCritical;
+
+public class AnnotationReachabilityDuplicatesExample {
+
+
+  @Expensive
+  native void expensive();
+
+  void callsExpensive1() {
+    expensive();
+  }
+
+  void callsExpensive2() {
+    expensive();
+  }
+
+  void callsExpensiveTwice() {
+    callsExpensive1();
+    callsExpensive2();
+  }
+
+  @PerformanceCritical
+  void perfCriticalBad2() {
+      callsExpensiveTwice();  // should report here only once
+  }
+
+  native boolean star();
+
+  void callsEitherExpensive() {
+    if (star()) {
+      callsExpensive1();
+    } else {
+      callsExpensive2();
+    }
+  }
+
+  @PerformanceCritical
+  void perfCriticalBad1() {
+    callsEitherExpensive();  // should report here only once
+  }
+
+}

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

@@ -1,3 +1,5 @@
+codetoanalyze/java/checkers/AnnotationReachabilityDuplicatesExample.java, void AnnotationReachabilityDuplicatesExample.perfCriticalBad1(), 1, CHECKERS_CALLS_EXPENSIVE_METHOD, []
+codetoanalyze/java/checkers/AnnotationReachabilityDuplicatesExample.java, void AnnotationReachabilityDuplicatesExample.perfCriticalBad2(), 1, CHECKERS_CALLS_EXPENSIVE_METHOD, []
 codetoanalyze/java/checkers/CustomAnnotations.java, void CustomAnnotations.source1Bad(), 1, CHECKERS_ANNOTATION_REACHABILITY_ERROR, []
 codetoanalyze/java/checkers/CustomAnnotations.java, void CustomAnnotations.source2Bad(), 1, CHECKERS_ANNOTATION_REACHABILITY_ERROR, []
 codetoanalyze/java/checkers/ExpensiveCallExample.java, View ExpensiveCallExample.callsFindViewByIdFromActivity(FragmentActivity,int), 1, CHECKERS_CALLS_EXPENSIVE_METHOD, []