From 233d6a53c0e14ae62019f1f96f9b87d5ebb1771b Mon Sep 17 00:00:00 2001 From: Martino Luca Date: Thu, 27 Apr 2017 02:46:16 -0700 Subject: [PATCH] [Infer] Emit potential exception lines into qualifier's message Summary: Sometimes reports need traces to be fully understood, but sometimes reporting where the exception takes place can save time to developers. Reviewed By: jvillard Differential Revision: D4914037 fbshipit-source-id: 039ab63 --- Makefile | 1 + infer/src/IR/Errlog.ml | 21 +++++++++ infer/src/IR/Errlog.mli | 6 +++ infer/src/backend/InferPrint.re | 22 +++++++++- infer/src/base/Config.ml | 2 + infer/src/base/Config.mli | 6 ++- infer/src/base/IStd.ml | 8 ++++ infer/src/base/MarkupFormatter.ml | 19 +++++++- infer/src/base/MarkupFormatter.mli | 9 ++++ .../resource_leak_exception_lines/Makefile | 43 +++++++++++++++++++ .../SimpleLeak.java | 26 +++++++++++ .../qualifier.exp | 3 ++ 12 files changed, 162 insertions(+), 4 deletions(-) create mode 100644 infer/tests/build_systems/resource_leak_exception_lines/Makefile create mode 100644 infer/tests/build_systems/resource_leak_exception_lines/SimpleLeak.java create mode 100644 infer/tests/build_systems/resource_leak_exception_lines/qualifier.exp diff --git a/Makefile b/Makefile index 6069c01c6..9815274e2 100644 --- a/Makefile +++ b/Makefile @@ -64,6 +64,7 @@ BUILD_SYSTEMS_TESTS += \ differential_skip_duplicated_types_on_filenames_with_renamings \ gradle \ javac \ + resource_leak_exception_lines \ DIRECT_TESTS += \ java_checkers java_eradicate java_infer java_tracing java_quandary java_threadsafety \ diff --git a/infer/src/IR/Errlog.ml b/infer/src/IR/Errlog.ml index 55a1bb6b1..df78dc80a 100644 --- a/infer/src/IR/Errlog.ml +++ b/infer/src/IR/Errlog.ml @@ -27,12 +27,33 @@ type loc_trace_elem = { lt_node_tags : node_tag list (** tags describing the node at the current location *) } +let contains_exception loc_trace_elem = + let pred nt = + match nt with + | Exception _ -> true + | Condition _ | Procedure_start _ | Procedure_end _ -> false in + List.exists ~f:pred loc_trace_elem.lt_node_tags + let make_trace_element lt_level lt_loc lt_description lt_node_tags = { lt_level; lt_loc; lt_description; lt_node_tags } (** Trace of locations *) type loc_trace = loc_trace_elem list +let compute_local_exception_line loc_trace = + let compute_local_exception_line state step = + match state with + | `Stop _ -> state + | `Continue (last_known_step_at_level_zero_opt, line_opt) -> + let last_known_step_at_level_zero_opt' = + if Int.equal step.lt_level 0 then Some step + else last_known_step_at_level_zero_opt in + match last_known_step_at_level_zero_opt' with + | Some step_zero when contains_exception step -> + `Stop (last_known_step_at_level_zero_opt', Some step_zero.lt_loc.line) + | _ -> `Continue (last_known_step_at_level_zero_opt', line_opt) in + snd (List_.fold_until ~init:(`Continue (None, None)) ~f:compute_local_exception_line loc_trace) + type node_id_key = { node_id : int; node_key : int diff --git a/infer/src/IR/Errlog.mli b/infer/src/IR/Errlog.mli index e1acf4163..d60e35f55 100644 --- a/infer/src/IR/Errlog.mli +++ b/infer/src/IR/Errlog.mli @@ -31,6 +31,12 @@ val make_trace_element : int -> Location.t -> string -> node_tag list -> loc_tra (** Trace of locations *) type loc_trace = loc_trace_elem list +(** Look at all the trace steps and find those that are arising any exception, + then bind them to the closest step at level 0. + This extra information adds value to the report itself, and may avoid + digging into the trace to understand the cause of the report. *) +val compute_local_exception_line : loc_trace -> int option + type node_id_key = private { node_id : int; node_key : int diff --git a/infer/src/backend/InferPrint.re b/infer/src/backend/InferPrint.re index 56042d4ae..7c158cd75 100644 --- a/infer/src/backend/InferPrint.re +++ b/infer/src/backend/InferPrint.re @@ -429,6 +429,8 @@ module IssuesCsv = { }; }; +let potential_exception_message = "potential exception at line"; + module IssuesJson = { let is_first_item = ref true; let pp_json_open fmt () => F.fprintf fmt "[@?"; @@ -464,11 +466,26 @@ module IssuesJson = { | _ => None }; let visibility = Exceptions.string_of_visibility err_data.visibility; + let qualifier = { + let base_qualifier = error_desc_to_plain_string key.err_desc; + if (Localise.equal key.err_name Localise.resource_leak) { + switch (Errlog.compute_local_exception_line err_data.loc_trace) { + | None => base_qualifier + | Some line => + let potential_exception_message = + Format.asprintf + "%a: %s %d" MarkupFormatter.pp_bold "Note" potential_exception_message line; + Format.sprintf "%s\n%s" base_qualifier potential_exception_message + } + } else { + base_qualifier + } + }; let bug = { Jsonbug_j.bug_class: Exceptions.err_class_string err_data.err_class, kind, bug_type, - qualifier: error_desc_to_plain_string key.err_desc, + qualifier, severity: key.severity, visibility, line: err_data.loc.Location.line, @@ -538,6 +555,9 @@ let pp_custom_of_report fmt report fields => { Format.fprintf fmt "%s%d" (comma_separator index) (issue.line - issue.procedure_start_line) | `Issue_field_procedure_id_without_crc => Format.fprintf fmt "%s%s" (comma_separator index) (DB.strip_crc issue.procedure_id) + | `Issue_field_qualifier_contains_potential_exception_note => + Format.fprintf + fmt "%B" (String.is_substring issue.qualifier substring::potential_exception_message) }; List.iteri f::pp_field fields; Format.fprintf fmt "@." diff --git a/infer/src/base/Config.ml b/infer/src/base/Config.ml index 937a1d345..ea6bebc82 100644 --- a/infer/src/base/Config.ml +++ b/infer/src/base/Config.ml @@ -94,6 +94,8 @@ let issues_fields_symbols = [ ("hash", `Issue_field_hash); ("line_offset", `Issue_field_line_offset); ("procedure_id_without_crc", `Issue_field_procedure_id_without_crc); + ("qualifier_contains_potential_exception_note", + `Issue_field_qualifier_contains_potential_exception_note); ] type os_type = Unix | Win32 | Cygwin diff --git a/infer/src/base/Config.mli b/infer/src/base/Config.mli index 65edd927b..26b7c226f 100644 --- a/infer/src/base/Config.mli +++ b/infer/src/base/Config.mli @@ -58,7 +58,8 @@ val issues_fields_symbols : | `Issue_field_key | `Issue_field_hash | `Issue_field_line_offset - | `Issue_field_procedure_id_without_crc]) list + | `Issue_field_procedure_id_without_crc + | `Issue_field_qualifier_contains_potential_exception_note]) list type os_type = Unix | Win32 | Cygwin @@ -262,7 +263,8 @@ val issues_fields : [`Issue_field_bug_class | `Issue_field_key | `Issue_field_hash | `Issue_field_line_offset - | `Issue_field_procedure_id_without_crc] list + | `Issue_field_procedure_id_without_crc + | `Issue_field_qualifier_contains_potential_exception_note] list val iterations : int val java_jar_compiler : string option val javac_classes_out : string diff --git a/infer/src/base/IStd.ml b/infer/src/base/IStd.ml index e9d85de6b..1e375e798 100644 --- a/infer/src/base/IStd.ml +++ b/infer/src/base/IStd.ml @@ -46,6 +46,14 @@ module Unix_ = struct end +module List_ = struct + let rec fold_until ~init ~f l = + match l, init with + | _, `Stop init' + | [], `Continue init' -> init' + | h :: t, `Continue _ -> fold_until ~init:(f init h) ~f t +end + (* Use Caml.Set since they are serialized using Marshal, and Core.Std.Set includes the comparison function in its representation, which Marshal cannot (de)serialize. *) module IntSet = Caml.Set.Make(Int) diff --git a/infer/src/base/MarkupFormatter.ml b/infer/src/base/MarkupFormatter.ml index fb076df10..398006c2b 100644 --- a/infer/src/base/MarkupFormatter.ml +++ b/infer/src/base/MarkupFormatter.ml @@ -17,6 +17,10 @@ type 'a formatter = { wrap_code : (Format.formatter -> 'a -> unit) -> Format.formatter -> 'a -> unit; pp_code : Format.formatter -> string -> unit; code_to_string : string -> string; + + wrap_bold : (Format.formatter -> 'a -> unit) -> Format.formatter -> 'a -> unit; + pp_bold : Format.formatter -> string -> unit; + bold_to_string : string -> string; } module NoFormatter : sig @@ -30,7 +34,10 @@ end = struct monospaced_to_string = Fn.id; wrap_code = wrap_simple; pp_code = pp_simple; - code_to_string = Fn.id + code_to_string = Fn.id; + wrap_bold = wrap_simple; + pp_bold = pp_simple; + bold_to_string = Fn.id } end @@ -46,6 +53,10 @@ end = struct let pp_code fmt s = wrap_code Format.pp_print_string fmt s let code_to_string s = Format.asprintf "%a" pp_code s + let wrap_bold pp fmt x = Format.fprintf fmt "**%a**" pp x + let pp_bold fmt s = wrap_bold Format.pp_print_string fmt s + let bold_to_string s = Format.asprintf "%a" pp_bold s + let formatter = { wrap_monospaced; pp_monospaced; @@ -53,6 +64,9 @@ end = struct wrap_code; pp_code; code_to_string; + wrap_bold; + pp_bold; + bold_to_string; } end @@ -66,3 +80,6 @@ let monospaced_to_string = formatter.monospaced_to_string let wrap_code = formatter.wrap_code let pp_code = formatter.pp_code let code_to_string = formatter.code_to_string +let wrap_bold = formatter.wrap_bold +let pp_bold = formatter.pp_bold +let bold_to_string = formatter.bold_to_string diff --git a/infer/src/base/MarkupFormatter.mli b/infer/src/base/MarkupFormatter.mli index 81a0454be..432e0f0ee 100644 --- a/infer/src/base/MarkupFormatter.mli +++ b/infer/src/base/MarkupFormatter.mli @@ -24,3 +24,12 @@ val pp_code : Format.formatter -> string -> unit (* wrap into a code block *) val code_to_string : string -> string + +(** used to combine pp together, wrap content into a bold block *) +val wrap_bold : (Format.formatter -> 'a -> unit) -> Format.formatter -> 'a -> unit + +(** pp to wrap into a bold block *) +val pp_bold : Format.formatter -> string -> unit + +(* wrap into a bold block *) +val bold_to_string : string -> string diff --git a/infer/tests/build_systems/resource_leak_exception_lines/Makefile b/infer/tests/build_systems/resource_leak_exception_lines/Makefile new file mode 100644 index 000000000..0da8ba6c8 --- /dev/null +++ b/infer/tests/build_systems/resource_leak_exception_lines/Makefile @@ -0,0 +1,43 @@ +# 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 = ../.. +include $(TESTS_DIR)/base.make + +INFER_OUT = infer-out +REPORT = $(INFER_OUT)/report.json +EXPECTED_TEST_OUTPUT = qualifier.exp.test + +default: analyze + +.PHONY: analyze +analyze: $(REPORT) + +$(REPORT): + $(QUIET)$(call silent_on_success, Running analysis,\ + $(INFER_BIN) -o $(INFER_OUT) --project-root $(CURDIR) -- javac SimpleLeak.java) + +$(EXPECTED_TEST_OUTPUT): $(REPORT) + $(QUIET)$(INFERPRINT_BIN) \ + --issues-fields "qualifier_contains_potential_exception_note,bug_type,file,procedure,line_offset" \ + --from-json-report $(REPORT) \ + --issues-tests $(EXPECTED_TEST_OUTPUT) + +.PHONY: print +print: $(EXPECTED_TEST_OUTPUT) + +.PHONY: test +test: print + $(QUIET)$(call check_no_diff,qualifier.exp,$(EXPECTED_TEST_OUTPUT)) + +.PHONY: replace +replace: $(EXPECTED_TEST_OUTPUT) + cp $(EXPECTED_TEST_OUTPUT) qualifier.exp + +.PHONY: clean +clean: + $(REMOVE_DIR) *.class *.exp.test $(INFER_OUT) diff --git a/infer/tests/build_systems/resource_leak_exception_lines/SimpleLeak.java b/infer/tests/build_systems/resource_leak_exception_lines/SimpleLeak.java new file mode 100644 index 000000000..c3c4c8068 --- /dev/null +++ b/infer/tests/build_systems/resource_leak_exception_lines/SimpleLeak.java @@ -0,0 +1,26 @@ +/* + * 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. + */ + +import java.io.*; + +public class SimpleLeak { + public void method(boolean v) { + try { + FileInputStream a = new FileInputStream("aaa"); + FileInputStream z = null; + if (v) { + z = new FileInputStream("bbb"); + z.read(); + } + z.close(); + } catch (IOException e) { + // do nothing + } + } +} diff --git a/infer/tests/build_systems/resource_leak_exception_lines/qualifier.exp b/infer/tests/build_systems/resource_leak_exception_lines/qualifier.exp new file mode 100644 index 000000000..28d70c2d8 --- /dev/null +++ b/infer/tests/build_systems/resource_leak_exception_lines/qualifier.exp @@ -0,0 +1,3 @@ +false, RESOURCE_LEAK, SimpleLeak.java, void SimpleLeak.method(boolean), 2 +false, NULL_DEREFERENCE, SimpleLeak.java, void SimpleLeak.method(boolean), 8 +true, RESOURCE_LEAK, SimpleLeak.java, void SimpleLeak.method(boolean), 9