[ConfigImpact] Run reportdiff on config impact json results

Summary:
This diff runs `infer reportdiff` on config impact results, ie previous and current
`config-impact-report.json`s.  Ungated and added/removed callees are reported at `introduced.json`.

Reviewed By: ezgicicek

Differential Revision: D26723054

fbshipit-source-id: efabd0d5f

.gitignore

@@ -47,6 +47,7 @@ duplicates.txt
 /infer/tests/build_systems/differential_*/**/*.class
 /infer/tests/build_systems/differential_*/**/Diff*.java
 /infer/tests/build_systems/differential_*/**/Diff*.m
+/infer/tests/build_systems/fb_differential_*/**/Diff*.java
 /infer/tests/build_systems/incremental_analysis_remove_file/src
 /infer/tests/build_systems/incremental_analysis_cost_change/src
 /infer/tests/build_systems/incremental_analysis_add_procedure/src

Makefile

@@ -178,6 +178,8 @@ DIRECT_TESTS += \
   java_topl \
 
 ifeq ($(IS_FACEBOOK_TREE),yes)
+BUILD_SYSTEMS_TESTS += \
+  fb_differential_of_config_impact_report_java
 DIRECT_TESTS += \
   java_fb-config-impact \
   java_fb-gk-interaction \

infer/man/man1/infer-full.txt

@@ -268,6 +268,9 @@ OPTIONS
            checkers (Conversely: --no-config-impact-analysis-only)       
     See also infer-analyze(1).
 
+       --config-impact-current path
+           Config impact report of the latest revision           See also infer-reportdiff(1).
+
        --config-impact-data-file file
            [ConfigImpact] Specify the file containing the config data       
     See also infer-report(1).
@@ -280,6 +283,10 @@ OPTIONS
            Specify the maximum number of unchecked callees to print in the
            config impact checker           See also infer-report(1) and infer-reportdiff(1).
 
+       --config-impact-previous path
+           Config impact report of the base revision to use for comparison       
+    See also infer-reportdiff(1).
+
        --continue
            Activates: Continue the capture for the reactive analysis,
            increasing the changed files/procedures. (If a procedure was
@@ -1468,12 +1475,18 @@ INTERNAL OPTIONS
        --compilation-database-reset
            Set --compilation-database to the empty list.
 
+       --config-impact-current-reset
+           Cancel the effect of --config-impact-current.
+
        --config-impact-data-file-reset
            Cancel the effect of --config-impact-data-file.
 
        --config-impact-issues-tests-reset
            Cancel the effect of --config-impact-issues-tests.
 
+       --config-impact-previous-reset
+           Cancel the effect of --config-impact-previous.
+
        --cost-issues-tests-reset
            Cancel the effect of --cost-issues-tests.
 

infer/man/man1/infer-reportdiff.txt

@@ -20,10 +20,16 @@ DESCRIPTION
 
 
 OPTIONS
+       --config-impact-current path
+           Config impact report of the latest revision
+
        --config-impact-max-callees-to-print int
            Specify the maximum number of unchecked callees to print in the
            config impact checker
 
+       --config-impact-previous path
+           Config impact report of the base revision to use for comparison
+
        --cost-tests-only-autoreleasepool
            Activates: [EXPERIMENTAL] Report only autoreleasepool size results
            in cost tests (Conversely: --no-cost-tests-only-autoreleasepool)

infer/man/man1/infer.txt

@@ -268,6 +268,9 @@ OPTIONS
            checkers (Conversely: --no-config-impact-analysis-only)       
     See also infer-analyze(1).
 
+       --config-impact-current path
+           Config impact report of the latest revision           See also infer-reportdiff(1).
+
        --config-impact-data-file file
            [ConfigImpact] Specify the file containing the config data       
     See also infer-report(1).
@@ -280,6 +283,10 @@ OPTIONS
            Specify the maximum number of unchecked callees to print in the
            config impact checker           See also infer-report(1) and infer-reportdiff(1).
 
+       --config-impact-previous path
+           Config impact report of the base revision to use for comparison       
+    See also infer-reportdiff(1).
+
        --continue
            Activates: Continue the capture for the reactive analysis,
            increasing the changed files/procedures. (If a procedure was

infer/src/base/Config.ml

@@ -913,6 +913,12 @@ and compilation_database_escaped =
      from Xcode (can be specified multiple times)"
 
 
+and config_impact_current =
+  CLOpt.mk_path_opt ~long:"config-impact-current"
+    ~in_help:InferCommand.[(ReportDiff, manual_generic)]
+    "Config impact report of the latest revision"
+
+
 and config_impact_data_file =
   CLOpt.mk_path_opt ~long:"config-impact-data-file"
     ~in_help:InferCommand.[(Report, manual_generic)]
@@ -933,6 +939,12 @@ and config_impact_max_callees_to_print =
     "Specify the maximum number of unchecked callees to print in the config impact checker"
 
 
+and config_impact_previous =
+  CLOpt.mk_path_opt ~long:"config-impact-previous"
+    ~in_help:InferCommand.[(ReportDiff, manual_generic)]
+    "Config impact report of the base revision to use for comparison"
+
+
 (** Continue the capture for reactive mode: If a procedure was changed beforehand, keep the changed
     marking. *)
 and continue =
@@ -2875,12 +2887,16 @@ and clang_libcxx_include_to_override_regex = !clang_libcxx_include_to_override_r
 
 and classpath = !classpath
 
+and config_impact_current = !config_impact_current
+
 and config_impact_data_file = !config_impact_data_file
 
 and config_impact_issues_tests = !config_impact_issues_tests
 
 and config_impact_max_callees_to_print = !config_impact_max_callees_to_print
 
+and config_impact_previous = !config_impact_previous
+
 and continue_analysis = !continue_analysis
 
 and continue_capture = !continue

infer/src/base/Config.mli

@@ -226,12 +226,16 @@ val clang_libcxx_include_to_override_regex : string option
 
 val command : InferCommand.t
 
+val config_impact_current : string option
+
 val config_impact_data_file : string option
 
 val config_impact_issues_tests : string option
 
 val config_impact_max_callees_to_print : int
 
+val config_impact_previous : string option
+
 val continue_analysis : bool
 
 val continue_capture : bool

infer/src/base/IssueType.mli

@@ -135,7 +135,7 @@ val condition_always_true : t
 
 val config_checks_between_markers : t
 
-val config_impact_analysis : t [@@warning "-32"]
+val config_impact_analysis : t
 
 val constant_address_dereference : t
 

infer/src/checkers/ConfigImpactAnalysis.ml

@@ -47,10 +47,6 @@ module UncheckedCallee = struct
 
   and call_type = Direct | Indirect of t
 
-  let get_callee {callee} = callee
-
-  let get_location {location} = location
-
   let pp_common ~with_location f {callee; location; call_type} =
     F.fprintf f "%a is %scalled" Procname.pp callee
       (match call_type with Direct -> "" | Indirect _ -> "indirectly ") ;
@@ -64,15 +60,15 @@ module UncheckedCallee = struct
   let pp_without_location_list f unchecked_callees =
     IList.pp_print_list ~max:Config.config_impact_max_callees_to_print
       ~pp_sep:(fun f () -> Format.pp_print_string f ", ")
-      (fun f unchecked_callee -> Procname.pp f (get_callee unchecked_callee))
+      (fun f {callee} -> Procname.pp f callee)
       f unchecked_callees
 
 
   let replace_location_by_call location x = {x with location; call_type= Indirect x}
 
-  let[@warning "-32"] rec make_err_trace x =
+  let rec make_err_trace ({location} as x) =
     let desc = F.asprintf "%a" pp_without_location x in
-    let trace_elem = Errlog.make_trace_element 0 (get_location x) desc [] in
+    let trace_elem = Errlog.make_trace_element 0 location desc [] in
     match x.call_type with Direct -> [trace_elem] | Indirect x -> trace_elem :: make_err_trace x
 end
 

infer/src/checkers/ConfigImpactAnalysis.mli

@@ -7,9 +7,17 @@
 
 open! IStd
 
-module UncheckedCallees : sig
+module UncheckedCallee : sig
   type t
 
+  val make_err_trace : t -> Errlog.loc_trace
+
+  val pp_without_location_list : Format.formatter -> t list -> unit
+end
+
+module UncheckedCallees : sig
+  include AbstractDomain.FiniteSetS with type elt = UncheckedCallee.t
+
   val encode : t -> string
 
   val decode : string -> t

infer/src/infer.ml

@@ -206,16 +206,26 @@ let () =
           Option.iter config_impact_out_path ~f:write_from_config_impact_json )
   | ReportDiff ->
       (* at least one report must be passed in input to compute differential *)
-      ( match Config.(report_current, report_previous, costs_current, costs_previous) with
-      | None, None, None, None ->
+      ( match
+          Config.
+            ( report_current
+            , report_previous
+            , costs_current
+            , costs_previous
+            , config_impact_current
+            , config_impact_previous )
+        with
+      | None, None, None, None, None, None ->
           L.die UserError
             "Expected at least one argument among '--report-current', '--report-previous', \
-             '--costs-current', and '--costs-previous'"
+             '--costs-current', '--costs-previous', '--config-impact-current', and \
+             '--config-impact-previous'\n"
       | _ ->
           () ) ;
       ReportDiff.reportdiff ~current_report:Config.report_current
         ~previous_report:Config.report_previous ~current_costs:Config.costs_current
-        ~previous_costs:Config.costs_previous
+        ~previous_costs:Config.costs_previous ~current_config_impact:Config.config_impact_current
+        ~previous_config_impact:Config.config_impact_previous
   | Debug when not Config.(global_tenv || procedures || source_files) ->
       L.die UserError
         "Expected at least one of '--procedures', '--source_files', or '--global-tenv'"

infer/src/integration/Differential.ml

@@ -385,9 +385,106 @@ let of_costs ~(current_costs : Jsonbug_t.costs_report) ~(previous_costs : Jsonbu
     CostIssues.enabled_cost_map ([], [], [])
 
 
+module ConfigImpactItem = struct
+  module UncheckedCallee = ConfigImpactAnalysis.UncheckedCallee
+  module UncheckedCallees = ConfigImpactAnalysis.UncheckedCallees
+
+  type t = {config_impact_item: Jsonbug_t.config_impact_item; unchecked_callees: UncheckedCallees.t}
+
+  type change_type = Added | Removed
+
+  let pp_change_type f x =
+    Format.pp_print_string f (match x with Added -> "added" | Removed -> "removed")
+
+
+  let get_qualifier_trace ~change_type unchecked_callees =
+    let nb_callees = UncheckedCallees.cardinal unchecked_callees in
+    if nb_callees <= 0 then assert false ;
+    let is_singleton = nb_callees <= 1 in
+    let unchecked_callees = UncheckedCallees.elements unchecked_callees in
+    let qualifier =
+      Format.asprintf "Function call%s to %a %s %a without GK/QE."
+        (if is_singleton then "" else "s")
+        UncheckedCallee.pp_without_location_list unchecked_callees
+        (if is_singleton then "is" else "are")
+        pp_change_type change_type
+    in
+    (* Note: It takes only one trace among the callees. *)
+    let trace = List.hd_exn unchecked_callees |> UncheckedCallee.make_err_trace in
+    (qualifier, trace)
+
+
+  let issue_of ~change_type (config_impact_item : Jsonbug_t.config_impact_item) callees =
+    let should_report =
+      ((not Config.filtering) || IssueType.config_impact_analysis.enabled)
+      && not (UncheckedCallees.is_empty callees)
+    in
+    if should_report then
+      let qualifier, trace = get_qualifier_trace ~change_type callees in
+      let file = config_impact_item.loc.file in
+      let line = config_impact_item.loc.lnum in
+      Some
+        { Jsonbug_j.bug_type= IssueType.config_impact_analysis.unique_id
+        ; qualifier
+        ; severity= IssueType.string_of_severity Advice
+        ; line
+        ; column= config_impact_item.loc.cnum
+        ; procedure= config_impact_item.procedure_id
+        ; procedure_start_line= line
+        ; file
+        ; bug_trace= JsonReports.loc_trace_to_jsonbug_record trace Advice
+        ; key= ""
+        ; node_key= None
+        ; hash= config_impact_item.hash
+        ; dotty= None
+        ; infer_source_loc= None
+        ; bug_type_hum= IssueType.config_impact_analysis.hum
+        ; linters_def_file= None
+        ; doc_url= None
+        ; traceview_id= None
+        ; censored_reason=
+            SourceFile.create ~warn_on_error:false file
+            |> JsonReports.censored_reason IssueType.config_impact_analysis
+        ; access= None
+        ; extras= None }
+    else None
+
+
+  let issues_of ~(current_config_impact : Jsonbug_t.config_impact_report)
+      ~(previous_config_impact : Jsonbug_t.config_impact_report) =
+    let fold_aux ~key:_ ~data ((acc_introduced, acc_fixed) as acc) =
+      match data with
+      | `Both (current, previous) ->
+          let introduced =
+            UncheckedCallees.diff current.unchecked_callees previous.unchecked_callees
+            |> issue_of ~change_type:Added current.config_impact_item
+          in
+          let removed =
+            UncheckedCallees.diff previous.unchecked_callees current.unchecked_callees
+            |> issue_of ~change_type:Removed previous.config_impact_item
+          in
+          (Option.to_list introduced @ acc_introduced, Option.to_list removed @ acc_fixed)
+      | `Left _ | `Right _ ->
+          (* Note: The reports available on one side are ignored since we don't want to report on
+             newly added (or freshly removed) functions. *)
+          acc
+    in
+    let map_of_config_impact config_impact =
+      List.fold ~init:String.Map.empty config_impact
+        ~f:(fun acc ({Jsonbug_t.hash= key; unchecked_callees} as config_impact_item) ->
+          let unchecked_callees = UncheckedCallees.decode unchecked_callees in
+          String.Map.add_exn acc ~key ~data:{config_impact_item; unchecked_callees} )
+    in
+    let current_map = map_of_config_impact current_config_impact in
+    let previous_map = map_of_config_impact previous_config_impact in
+    Map.fold2 ~init:([], []) current_map previous_map ~f:fold_aux
+end
+
 (** Set operations should keep duplicated issues with identical hashes *)
 let of_reports ~(current_report : Jsonbug_t.report) ~(previous_report : Jsonbug_t.report)
-    ~(current_costs : Jsonbug_t.costs_report) ~(previous_costs : Jsonbug_t.costs_report) : t =
+    ~(current_costs : Jsonbug_t.costs_report) ~(previous_costs : Jsonbug_t.costs_report)
+    ~(current_config_impact : Jsonbug_t.config_impact_report)
+    ~(previous_config_impact : Jsonbug_t.config_impact_report) : t =
   let fold_aux ~key:_ ~data (left, both, right) =
     match data with
     | `Left left' ->
@@ -404,8 +501,13 @@ let of_reports ~(current_report : Jsonbug_t.report) ~(previous_report : Jsonbug_
   in
   let costs_summary = CostsSummary.to_json ~current_costs ~previous_costs in
   let introduced_costs, preexisting_costs, fixed_costs = of_costs ~current_costs ~previous_costs in
-  { introduced= List.rev_append introduced_costs (dedup introduced)
-  ; fixed= List.rev_append fixed_costs (dedup fixed)
+  let introduced_config_impact, fixed_config_impact =
+    ConfigImpactItem.issues_of ~current_config_impact ~previous_config_impact
+  in
+  { introduced=
+      List.rev_append introduced_costs (dedup introduced)
+      |> List.rev_append introduced_config_impact
+  ; fixed= List.rev_append fixed_costs (dedup fixed) |> List.rev_append fixed_config_impact
   ; preexisting= List.rev_append preexisting_costs (dedup preexisting)
   ; costs_summary }
 

infer/src/integration/Differential.mli

@@ -18,6 +18,8 @@ val of_reports :
   -> previous_report:Jsonbug_t.report
   -> current_costs:Jsonbug_t.costs_report
   -> previous_costs:Jsonbug_t.costs_report
+  -> current_config_impact:Jsonbug_t.config_impact_report
+  -> previous_config_impact:Jsonbug_t.config_impact_report
   -> t
 
 val to_files : t -> string -> unit

infer/src/integration/ReportDiff.ml

@@ -7,7 +7,9 @@
 open! IStd
 
 let reportdiff ~current_report:current_report_fname ~previous_report:previous_report_fname
-    ~current_costs:current_costs_fname ~previous_costs:previous_costs_fname =
+    ~current_costs:current_costs_fname ~previous_costs:previous_costs_fname
+    ~current_config_impact:current_config_impact_fname
+    ~previous_config_impact:previous_config_impact_fname =
   let load_aux ~f filename_opt =
     Option.value_map
       ~f:(fun filename -> Atdgen_runtime.Util.Json.from_file f filename)
@@ -15,13 +17,17 @@ let reportdiff ~current_report:current_report_fname ~previous_report:previous_re
   in
   let load_report = load_aux ~f:Jsonbug_j.read_report in
   let load_costs = load_aux ~f:Jsonbug_j.read_costs_report in
+  let load_config_impact = load_aux ~f:Jsonbug_j.read_config_impact_report in
   let current_report = load_report current_report_fname in
   let previous_report = load_report previous_report_fname in
   let current_costs = load_costs current_costs_fname in
   let previous_costs = load_costs previous_costs_fname in
+  let current_config_impact = load_config_impact current_config_impact_fname in
+  let previous_config_impact = load_config_impact previous_config_impact_fname in
   let diff =
     let unfiltered_diff =
       Differential.of_reports ~current_report ~previous_report ~current_costs ~previous_costs
+        ~current_config_impact ~previous_config_impact
     in
     (* FIXME(T54950303) replace use of filtering with deduplicate *)
     if Config.filtering then

infer/src/integration/ReportDiff.mli

@@ -12,4 +12,6 @@ val reportdiff :
   -> previous_report:string option
   -> current_costs:string option
   -> previous_costs:string option
+  -> current_config_impact:string option
+  -> previous_config_impact:string option
   -> unit

infer/src/unit/DifferentialFiltersTests.ml

@@ -168,8 +168,11 @@ let test_skip_duplicated_types_on_filenames =
   in
   let current_costs = [] in
   let previous_costs = [] in
+  let current_config_impact = [] in
+  let previous_config_impact = [] in
   let diff =
     Differential.of_reports ~current_report ~previous_report ~current_costs ~previous_costs
+      ~current_config_impact ~previous_config_impact
   in
   let diff' =
     DifferentialFilters.VISIBLE_FOR_TESTING_DO_NOT_USE_DIRECTLY.skip_duplicated_types_on_filenames

infer/src/unit/DifferentialTests.ml

@@ -27,7 +27,14 @@ let current_costs = []
 
 let previous_costs = []
 
-let diff = Differential.of_reports ~current_report ~previous_report ~current_costs ~previous_costs
+let current_config_impact = []
+
+let previous_config_impact = []
+
+let diff =
+  Differential.of_reports ~current_report ~previous_report ~current_costs ~previous_costs
+    ~current_config_impact ~previous_config_impact
+
 
 (* Sets operations should keep duplicated issues with identical hashes *)
 let test_diff_keeps_duplicated_hashes =

infer/tests/build_systems/fb_differential_of_config_impact_report_java/Makefile

@@ -0,0 +1,28 @@
+# Copyright (c) Facebook, Inc. and its affiliates.
+#
+# This source code is licensed under the MIT license found in the
+# LICENSE file in the root directory of this source tree.
+
+# E2E test for differential of costs
+
+TESTS_DIR = ../..
+SOURCES = src/DiffExample.current.java src/DiffExample.previous.java
+COPIED = src/DiffExample.java
+CLEAN_EXTRA = $(COPIED) *.class
+DIFFERENTIAL_ARGS = --enable-issue-type CONFIG_IMPACT
+include $(TESTS_DIR)/differential-config-impact.make
+include $(TESTS_DIR)/java.make
+
+$(CURRENT_REPORT) $(PREVIOUS_REPORT): $(JAVA_DEPS)
+
+$(CURRENT_REPORT):
+	$(QUIET)$(COPY) src/DiffExample.current.java src/DiffExample.java
+	$(QUIET)$(call silent_on_success,Testing Config Impact Differential: current,\
+	$(INFER_BIN) --no-filtering --config-impact-analysis-only --config-impact-data-file config_data.json -o $(CURRENT_DIR) \
+	  -- $(JAVAC) -cp $(CLASSPATH)  $(COPIED))
+
+$(PREVIOUS_REPORT):
+	$(QUIET)$(COPY) src/DiffExample.previous.java src/DiffExample.java
+	$(QUIET)$(call silent_on_success,Testing Config Impact Differential: previous,\
+	$(INFER_BIN) --no-filtering --config-impact-analysis-only --config-impact-data-file config_data.json -o $(PREVIOUS_DIR) \
+	  -- $(JAVAC) -cp $(CLASSPATH) $(COPIED))

infer/tests/differential-config-impact.make

@@ -0,0 +1,63 @@
+# Copyright (c) Facebook, Inc. and its affiliates.
+#
+# This source code is licensed under the MIT license found in the
+# LICENSE file in the root directory of this source tree.
+
+# Targets that must be defined: CURRENT_REPORT and PREVIOUS_REPORT
+# Optional variables: DIFFERENTIAL_ARGS, CLEAN_EXTRA
+
+include $(TESTS_DIR)/base.make
+
+INFER_OUT = infer-out
+EXPECTED_TEST_OUTPUT = introduced.exp.test
+INFERPRINT_ISSUES_FIELDS = "bug_type,file,procedure,qualifier"
+
+CURRENT_DIR = infer-out-current
+PREVIOUS_DIR = infer-out-previous
+CURRENT_REPORT = $(CURRENT_DIR)/config-impact-report.json
+PREVIOUS_REPORT = $(PREVIOUS_DIR)/config-impact-report.json
+
+default: analyze
+
+# the following dependency is to guarantee that the computation of
+# PREVIOUS_REPORT and CURRENT_REPORT will be serialized
+$(PREVIOUS_REPORT): $(CURRENT_REPORT)
+
+.PHONY: analyze
+analyze: $(CURRENT_REPORT) $(PREVIOUS_REPORT)
+
+$(CURRENT_REPORT) $(PREVIOUS_REPORT): $(INFER_BIN) $(SOURCES)
+
+$(EXPECTED_TEST_OUTPUT): $(CURRENT_REPORT) $(PREVIOUS_REPORT) $(MODIFIED_FILES_FILE) \
+                         $(INFER_BIN) $(MAKEFILE_LIST)
+	$(QUIET)$(REMOVE_DIR) $(INFER_OUT)
+	$(QUIET)$(call silent_on_success,Computing results difference in $(TEST_REL_DIR),\
+	  $(INFER_BIN) -o $(INFER_OUT) --project-root $(CURDIR) reportdiff \
+		--config-impact-current $(CURRENT_REPORT) --config-impact-previous $(PREVIOUS_REPORT) \
+		$(DIFFERENTIAL_ARGS))
+	$(QUIET)$(INFER_BIN) report -o $(INFER_OUT) \
+		--issues-tests-fields $(INFERPRINT_ISSUES_FIELDS) \
+		--from-json-report $(INFER_OUT)/differential/introduced.json \
+		--issues-tests introduced.exp.test
+	$(QUIET)$(INFER_BIN) report -o $(INFER_OUT) \
+		--issues-tests-fields $(INFERPRINT_ISSUES_FIELDS) \
+		--from-json-report $(INFER_OUT)/differential/fixed.json \
+		--issues-tests fixed.exp.test
+
+.PHONY: print
+print: $(EXPECTED_TEST_OUTPUT)
+
+.PHONY: test
+test: print
+	$(QUIET)$(call check_no_diff,introduced.exp,introduced.exp.test)
+	$(QUIET)$(call check_no_diff,fixed.exp,fixed.exp.test)
+
+.PHONY: replace
+replace: $(EXPECTED_TEST_OUTPUT)
+	cp introduced.exp.test introduced.exp
+	cp fixed.exp.test fixed.exp
+
+.PHONY: clean
+clean:
+	$(REMOVE_DIR) *.exp.test $(INFER_OUT) $(CURRENT_DIR) $(PREVIOUS_DIR) \
+		$(CLEAN_EXTRA)