[infer][nullable checker] avoid reporting the nullable violations that are already reported by the biabduction analysis

Summary: This is to allow the bi-abduction analysis and the nullable checker for Clang languages to run together without stepping on each other toes.

Reviewed By: sblackshear

Differential Revision: D6567934

fbshipit-source-id: a318c33

Makefile

@@ -49,7 +49,8 @@ BUILD_SYSTEMS_TESTS += \
 
 DIRECT_TESTS += \
   c_biabduction c_bufferoverrun c_errors c_frontend \
-  cpp_bufferoverrun cpp_errors cpp_frontend  cpp_liveness cpp_quandary cpp_racerd cpp_siof cpp_uninit  cpp_nullable \
+  cpp_bufferoverrun cpp_errors cpp_frontend  cpp_liveness cpp_quandary \
+  cpp_racerd cpp_siof cpp_uninit  cpp_nullable cpp_conflicts \
 
 ifneq ($(BUCK),no)
 BUILD_SYSTEMS_TESTS += buck-clang-db buck_flavors buck_flavors_run buck_flavors_deterministic

infer/src/IR/Errlog.ml

@@ -147,7 +147,6 @@ let fold (f: err_key -> err_data -> 'a -> 'a) t acc =
     (fun err_key set acc -> ErrDataSet.fold (fun err_data acc -> f err_key err_data acc) set acc)
     t acc
 
-
 (** Return the number of elements in the error log which satisfy [filter] *)
 let size filter (err_log: t) =
   let count = ref 0 in

infer/src/IR/Localise.ml

@@ -195,6 +195,13 @@ let error_desc_set_bucket err_desc bucket =
   {err_desc with descriptions; tags}
 
 
+let error_desc_is_reportable_bucket err_desc =
+  let issue_bucket = error_desc_get_bucket err_desc in
+  let high_buckets = BucketLevel.([b1; b2]) in
+  Option.value_map issue_bucket ~default:false ~f:(fun b ->
+      List.mem ~equal:String.equal high_buckets b )
+
+
 (** get the value tag, if any *)
 let get_value_line_tag tags =
   try
@@ -988,4 +995,3 @@ let desc_uninitialized_dangling_pointer_deref deref expr_str loc =
       (at_line tags loc)
   in
   {no_desc with descriptions= [description]; tags= !tags}
-

infer/src/IR/Localise.mli

@@ -74,6 +74,9 @@ val error_desc_get_bucket : error_desc -> string option
 val error_desc_set_bucket : error_desc -> string -> error_desc
 (** set the bucket value of an error_desc *)
 
+val error_desc_is_reportable_bucket : error_desc -> bool
+(** check if the report is in a high confidence bucket *)
+
 val error_desc_hash : error_desc -> int
 (** hash function for error_desc *)
 

infer/src/backend/InferPrint.ml

@@ -199,14 +199,7 @@ let should_report (issue_kind: Exceptions.err_kind) issue_type error_desc eclass
         in
         List.mem ~equal:IssueType.equal null_deref_issue_types issue_type
       in
-      if issue_type_is_null_deref then
-        let issue_bucket_is_high =
-          let issue_bucket = Localise.error_desc_get_bucket error_desc in
-          let high_buckets = Localise.BucketLevel.([b1; b2]) in
-          Option.value_map issue_bucket ~default:false ~f:(fun b ->
-              List.mem ~equal:String.equal high_buckets b )
-        in
-        issue_bucket_is_high
+      if issue_type_is_null_deref then Localise.error_desc_is_reportable_bucket error_desc
       else true
 
 

infer/src/checkers/NullabilityCheck.ml

@@ -60,78 +60,93 @@ module TransferFunctions (CFG : ProcCfg.S) = struct
         method_prefixes
 
 
+  let is_conflicting_report summary report_location =
+    if not Config.filtering then false
+    else
+      Errlog.fold
+        (fun {Errlog.err_name; err_desc; in_footprint} {Errlog.loc} found_confict ->
+          found_confict
+          || in_footprint && IssueType.equal err_name IssueType.null_dereference
+             && Location.equal loc report_location
+             && Localise.error_desc_is_reportable_bucket err_desc)
+        (Specs.get_err_log summary) false
+
+
   let report_nullable_dereference ap call_sites {ProcData.pdesc; extras} loc =
-    let pname = Procdesc.get_proc_name pdesc in
-    let annotation = Localise.nullable_annotation_name pname in
-    let call_site =
-      try CallSites.min_elt call_sites with Not_found ->
-        L.(die InternalError)
-          "Expecting a least one element in the set of call sites when analyzing %a"
-          Typ.Procname.pp pname
-    in
-    let simplified_pname =
-      Typ.Procname.to_simplified_string ~withclass:true (CallSite.pname call_site)
-    in
-    let is_direct_dereference =
-      match ap with
-      | (Var.LogicalVar _, _), _ ->
-          true
-      | (Var.ProgramVar pvar, _), _ ->
-          Pvar.is_frontend_tmp pvar
-    in
-    let message =
-      if is_direct_dereference then
-        (* direct dereference without intermediate variable *)
-        F.asprintf
-          "The return value of %s is annotated with %a and is dereferenced without being checked for null at %a"
-          (MF.monospaced_to_string simplified_pname)
-          MF.pp_monospaced annotation Location.pp loc
-      else
-        (* dereference with intermediate variable *)
-        F.asprintf
-          "Variable %a is indirectly annotated with %a (source %a) and is dereferenced without being checked for null at %a"
-          (MF.wrap_monospaced AccessPath.pp)
-          ap MF.pp_monospaced annotation (MF.wrap_monospaced CallSite.pp) call_site Location.pp loc
-    in
-    let exn =
-      Exceptions.Checkers (IssueType.nullable_dereference, Localise.verbatim_desc message)
-    in
     let summary = extras in
-    let trace =
-      let with_origin_site =
-        let callee_pname = CallSite.pname call_site in
-        match Specs.proc_resolve_attributes callee_pname with
-        | None ->
-            []
-        | Some attributes ->
-            let description =
-              F.asprintf "definition of %s" (Typ.Procname.get_method callee_pname)
-            in
-            let trace_element =
-              Errlog.make_trace_element 1 attributes.ProcAttributes.loc description []
-            in
-            [trace_element]
+    if is_conflicting_report summary loc then ()
+    else
+      let pname = Procdesc.get_proc_name pdesc in
+      let annotation = Localise.nullable_annotation_name pname in
+      let call_site =
+        try CallSites.min_elt call_sites with Not_found ->
+          L.(die InternalError)
+            "Expecting a least one element in the set of call sites when analyzing %a"
+            Typ.Procname.pp pname
+      in
+      let simplified_pname =
+        Typ.Procname.to_simplified_string ~withclass:true (CallSite.pname call_site)
+      in
+      let is_direct_dereference =
+        match ap with
+        | (Var.LogicalVar _, _), _ ->
+            true
+        | (Var.ProgramVar pvar, _), _ ->
+            Pvar.is_frontend_tmp pvar
       in
-      let with_assignment_site =
-        let call_site_loc = CallSite.loc call_site in
-        if Location.equal call_site_loc loc then with_origin_site
+      let message =
+        if is_direct_dereference then
+          (* direct dereference without intermediate variable *)
+          F.asprintf
+            "The return value of %s is annotated with %a and is dereferenced without being checked for null at %a"
+            (MF.monospaced_to_string simplified_pname)
+            MF.pp_monospaced annotation Location.pp loc
         else
-          let trace_element =
-            Errlog.make_trace_element 0 call_site_loc "assignment of the nullable value" []
-          in
-          trace_element :: with_origin_site
+          (* dereference with intermediate variable *)
+          F.asprintf
+            "Variable %a is indirectly annotated with %a (source %a) and is dereferenced without being checked for null at %a"
+            (MF.wrap_monospaced AccessPath.pp)
+            ap MF.pp_monospaced annotation (MF.wrap_monospaced CallSite.pp) call_site Location.pp
+            loc
+      in
+      let exn =
+        Exceptions.Checkers (IssueType.nullable_dereference, Localise.verbatim_desc message)
       in
-      let dereference_site =
-        let description =
-          if is_direct_dereference then
-            F.asprintf "dereferencing the return of %s" simplified_pname
-          else F.asprintf "dereference of %a" AccessPath.pp ap
+      let trace =
+        let with_origin_site =
+          let callee_pname = CallSite.pname call_site in
+          match Specs.proc_resolve_attributes callee_pname with
+          | None ->
+              []
+          | Some attributes ->
+              let description =
+                F.asprintf "definition of %s" (Typ.Procname.get_method callee_pname)
+              in
+              let trace_element =
+                Errlog.make_trace_element 1 attributes.ProcAttributes.loc description []
+              in
+              [trace_element]
         in
-        Errlog.make_trace_element 0 loc description []
+        let with_assignment_site =
+          let call_site_loc = CallSite.loc call_site in
+          if Location.equal call_site_loc loc then with_origin_site
+          else
+            let trace_element =
+              Errlog.make_trace_element 0 call_site_loc "assignment of the nullable value" []
+            in
+            trace_element :: with_origin_site
+        in
+        let dereference_site =
+          let description =
+            if is_direct_dereference then
+              F.asprintf "dereferencing the return of %s" simplified_pname
+            else F.asprintf "dereference of %a" AccessPath.pp ap
+          in
+          Errlog.make_trace_element 0 loc description []
+        in
+        dereference_site :: with_assignment_site
       in
-      dereference_site :: with_assignment_site
-    in
-    Reporting.log_error summary ~loc ~ltr:trace exn
+      Reporting.log_error summary ~loc ~ltr:trace exn
 
 
   let add_nullable_ap ap call_sites (aps, pnames) = (NullableAP.add ap call_sites aps, pnames)

infer/src/checkers/registerCheckers.ml

@@ -27,9 +27,16 @@ type callback = callback_fun * Config.language
 type checker = {name: string; active: bool; callbacks: callback list}
 
 let all_checkers =
+  (* TODO (T24393492): the checkers should run in the order from this list.
+     Currently, the checkers are run in the reverse order *)
   [ { name= "annotation reachability"
     ; active= Config.annotation_reachability
     ; callbacks= [(Procedure AnnotationReachability.checker, Config.Java)] }
+  ; { name= "nullable checks"
+    ; active= Config.check_nullable
+    ; callbacks=
+        [ (Procedure NullabilityCheck.checker, Config.Clang)
+        ; (Procedure NullabilityCheck.checker, Config.Java) ] }
   ; { name= "biabduction"
     ; active= Config.biabduction
     ; callbacks=
@@ -59,11 +66,6 @@ let all_checkers =
   ; { name= "printf args"
     ; active= Config.printf_args
     ; callbacks= [(Procedure PrintfArgs.callback_printf_args, Config.Java)] }
-  ; { name= "nullable checks"
-    ; active= Config.check_nullable
-    ; callbacks=
-        [ (Procedure NullabilityCheck.checker, Config.Clang)
-        ; (Procedure NullabilityCheck.checker, Config.Java) ] }
   ; { name= "nullable suggestion"
     ; active= Config.suggest_nullable
     ; callbacks=

infer/tests/codetoanalyze/cpp/conflicts/Makefile

@@ -0,0 +1,20 @@
+# 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.
+
+
+TESTS_DIR = ../../..
+
+ANALYZER = checkers
+CLANG_OPTIONS = -c -x c++ -std=c++14
+INFER_OPTIONS = --no-default-checkers --biabduction --check-nullable --project-root $(TESTS_DIR)
+INFERPRINT_OPTIONS = --issues-tests
+
+SOURCES = $(wildcard *.cpp)
+
+include $(TESTS_DIR)/clang.make
+
+infer-out/report.json: $(MAKEFILE_LIST)

infer/tests/codetoanalyze/cpp/conflicts/issues.exp

@@ -0,0 +1,3 @@
+codetoanalyze/cpp/conflicts/test.cpp, test1_bad, 2, NULL_DEREFERENCE, [start of procedure test1_bad(),Skipping nullableMethod(): function or method not found]
+codetoanalyze/cpp/conflicts/test.cpp, test2_bad, 1, NULLABLE_DEREFERENCE, [dereferencing the return of nullableMethod(),definition of nullableMethod]
+codetoanalyze/cpp/conflicts/test.cpp, test3_bad, 5, NULLABLE_DEREFERENCE, [dereference of &p,assignment of the nullable value,definition of nullableMethod]

infer/tests/codetoanalyze/cpp/conflicts/test.cpp

@@ -0,0 +1,34 @@
+/*
+ * 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.
+ */
+class C {
+ private:
+  int f;
+
+ public:
+  int get() { return f; }
+};
+
+C* _Nullable nullableMethod();
+
+int test1_bad() {
+  C* p = nullableMethod();
+  return p->get(); // reported by the biabduction analysis
+}
+
+int test2_bad() {
+  return nullableMethod()->get(); // not reported by the biabduction analysis
+}
+
+int test3_bad() {
+  C* p = nullableMethod();
+  for (int i = 0; i < 10; i++) {
+    p = nullableMethod();
+  }
+  return p->get(); // not reported by the biabduction analysis
+}