Reporting cleanup 3: NodeKey

Summary:
- abstracted the type for a node key
- moved it to its own module with an ugly `compute` to avoid cyclic dependencies...
- renamed `node_id` to `node_id_key` where needed
- moved key computation from `State` to `Procdesc.Node`

Reviewed By: jvillard

Differential Revision: D9332803

fbshipit-source-id: fe1ae8c1c

infer/src/IR/Errlog.ml

@@ -60,7 +60,7 @@ let compute_local_exception_line loc_trace =
   List.fold_until ~init:(None, None) ~f:compute_local_exception_line ~finish:snd loc_trace
 
 
-type node_id_key = {node_id: int; node_key: Caml.Digest.t}
+type node_id_key = {node_id: int; node_key: NodeKey.t}
 
 type err_key =
   { severity: Exceptions.severity
@@ -213,8 +213,8 @@ let update errlog_old errlog_new =
   ErrLogHash.iter (fun err_key l -> ignore (add_issue errlog_old err_key l)) errlog_new
 
 
-let log_issue procname ~clang_method_kind severity err_log ~loc ~node_id:(node_id, node_key)
+let log_issue procname ~clang_method_kind severity err_log ~loc ~node_id_key ~session ~ltr
-    ~session ~ltr ~linters_def_file ~doc_url ~access ~extras exn =
+    ~linters_def_file ~doc_url ~access ~extras exn =
   let lang = Typ.Procname.get_language procname in
   let error = Exceptions.recognize_exception exn in
   let severity = Option.value error.severity ~default:severity in
@@ -254,7 +254,6 @@ let log_issue procname ~clang_method_kind severity err_log ~loc ~node_id:(node_i
       EventLogger.log issue ) ;
   if should_report && not hide_java_loc_zero && not hide_memory_error then
     let added =
-      let node_id_key = {node_id; node_key} in
       let err_data =
         { node_id_key
         ; session

infer/src/IR/Errlog.mli

@@ -34,7 +34,7 @@ val compute_local_exception_line : loc_trace -> int option
     This extra information adds value to the report itself, and may avoid
     digging into the trace to understand the cause of the report. *)
 
-type node_id_key = private {node_id: int; node_key: Caml.Digest.t}
+type node_id_key = {node_id: int; node_key: NodeKey.t}
 
 type err_key = private
   { severity: Exceptions.severity
@@ -94,5 +94,5 @@ val update : t -> t -> unit
 
 val log_issue :
   Typ.Procname.t -> clang_method_kind:string option -> Exceptions.severity -> t -> loc:Location.t
-  -> node_id:int * Caml.Digest.t -> session:int -> ltr:loc_trace -> linters_def_file:string option
+  -> node_id_key:node_id_key -> session:int -> ltr:loc_trace -> linters_def_file:string option
   -> doc_url:string option -> access:string option -> extras:Jsonbug_t.extra option -> exn -> unit

infer/src/IR/NodeKey.ml

@@ -0,0 +1,19 @@
+(*
+ * Copyright (c) 2018-present, Facebook, Inc.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ *)
+
+open! IStd
+
+type t = Caml.Digest.t
+
+let to_string = Caml.Digest.to_hex
+
+let compute node ~simple_key ~succs ~preds =
+  let v = (simple_key node, List.rev_map ~f:simple_key succs, List.rev_map ~f:simple_key preds) in
+  Utils.better_hash v
+
+
+let of_frontend_node_key = Utils.better_hash

infer/src/IR/NodeKey.mli

@@ -0,0 +1,17 @@
+(*
+ * Copyright (c) 2018-present, Facebook, Inc.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ *)
+
+open! IStd
+
+type t
+
+val to_string : t -> string
+
+val compute :
+  'node -> simple_key:('node -> 'simple_key) -> succs:'node list -> preds:'node list -> t
+
+val of_frontend_node_key : string -> t

infer/src/IR/Procdesc.ml

@@ -331,6 +331,41 @@ module Node = struct
     in
     let pp fmt = F.fprintf fmt "%s@.%a" str (pp_instrs pe ~instro:None ~sub_instrs:true) node in
     F.asprintf "%t" pp
+
+
+  (** simple key for a node: just look at the instructions *)
+  let simple_key node =
+    let add_instr instr =
+      if Sil.instr_is_auxiliary instr then None
+      else
+        let instr_key =
+          match instr with
+          | Sil.Load _ ->
+              1
+          | Sil.Store _ ->
+              2
+          | Sil.Prune _ ->
+              3
+          | Sil.Call _ ->
+              4
+          | Sil.Nullify _ ->
+              5
+          | Sil.Abstract _ ->
+              6
+          | Sil.Remove_temps _ ->
+              7
+        in
+        Some instr_key
+    in
+    get_instrs node |> IContainer.rev_filter_map_to_list ~fold:Instrs.fold ~f:add_instr
+    |> Utils.better_hash
+
+
+  (** key for a node: look at the current node, successors and predecessors *)
+  let compute_key node =
+    let succs = get_succs node in
+    let preds = get_preds node in
+    NodeKey.compute node ~simple_key ~succs ~preds
 end
 
 (* =============== END of module Node =============== *)

infer/src/IR/Procdesc.mli

@@ -146,6 +146,8 @@ module Node : sig
     Pp.env -> sub_instrs:bool -> instro:Sil.instr option -> Format.formatter -> t -> unit
   (** Print extended instructions for the node,
       highlighting the given subinstruction if present *)
+
+  val compute_key : t -> NodeKey.t
 end
 
 (** Map with node id keys. *)

infer/src/backend/InferPrint.ml

@@ -315,7 +315,7 @@ module JsonIssuePrinter = MakeJsonListPrinter (struct
         ; procedure_start_line
         ; file
         ; bug_trace= loc_trace_to_jsonbug_record err_data.loc_trace err_key.severity
-        ; node_key= err_data.node_id_key.node_key |> Caml.Digest.to_hex
+        ; node_key= err_data.node_id_key.node_key |> NodeKey.to_string
         ; key= compute_key bug_type proc_name file
         ; hash= compute_hash severity bug_type proc_name file qualifier
         ; dotty= error_desc_to_dotty_string err_key.err_desc

infer/src/backend/reporting.ml

@@ -9,27 +9,27 @@ open! IStd
 module L = Logging
 
 type log_t =
-  ?loc:Location.t -> ?node_id:int * Caml.Digest.t -> ?session:int -> ?ltr:Errlog.loc_trace
+  ?loc:Location.t -> ?node_id_key:Errlog.node_id_key -> ?session:int -> ?ltr:Errlog.loc_trace
   -> ?linters_def_file:string -> ?doc_url:string -> ?access:string -> ?extras:Jsonbug_t.extra
   -> exn -> unit
 
-let log_issue_from_errlog_internal procname ~clang_method_kind severity err_log ~loc ~node_id
+let log_issue_from_errlog_internal procname ~clang_method_kind severity err_log ~loc ~node_id_key
     ~session ~ltr ~linters_def_file ~doc_url ~access ~extras exn =
   let issue_type = (Exceptions.recognize_exception exn).name in
   if not Config.filtering (* no-filtering takes priority *) || issue_type.IssueType.enabled then
-    Errlog.log_issue procname ~clang_method_kind severity err_log ~loc ~node_id ~session ~ltr
+    Errlog.log_issue procname ~clang_method_kind severity err_log ~loc ~node_id_key ~session ~ltr
       ~linters_def_file ~doc_url ~access ~extras exn
 
 
-let log_issue_from_errlog procname severity errlog ~loc ~node_id ~ltr ~linters_def_file ~doc_url
+let log_issue_from_errlog procname severity errlog ~loc ~node_id_key ~ltr ~linters_def_file
-    exn =
+    ~doc_url exn =
   let session = (State.get_session () :> int) in
-  log_issue_from_errlog_internal procname ~clang_method_kind:None severity errlog ~loc ~node_id
+  log_issue_from_errlog_internal procname ~clang_method_kind:None severity errlog ~loc ~node_id_key
     ~session ~ltr ~linters_def_file ~doc_url ~access:None ~extras:None exn
 
 
-let log_issue_from_summary severity summary ?loc ?node_id ?session ?ltr ?linters_def_file ?doc_url
+let log_issue_from_summary severity summary ?loc ?node_id_key ?session ?ltr ?linters_def_file
-    ?access ?extras exn =
+    ?doc_url ?access ?extras exn =
   let attrs = Summary.get_attributes summary in
   let procname = attrs.proc_name in
   let is_java_generated_method =
@@ -51,26 +51,22 @@ let log_issue_from_summary severity summary ?loc ?node_id ?session ?ltr ?linters
       Some (ProcAttributes.string_of_clang_method_kind attrs.clang_method_kind)
     in
     let loc = match loc with None -> State.get_loc () | Some loc -> loc in
-    let node_id =
+    let node_id_key =
-      match node_id with
+      match node_id_key with None -> State.get_node_id_key () | Some node_id_key -> node_id_key
-      | None ->
-          (State.get_node_id_key () :> int * Caml.Digest.t)
-      | Some node_id ->
-          node_id
     in
     let session =
       match session with None -> (State.get_session () :> int) | Some session -> session
     in
     let ltr = match ltr with None -> State.get_loc_trace () | Some ltr -> ltr in
-    log_issue_from_errlog_internal procname ~clang_method_kind severity err_log ~loc ~node_id
+    log_issue_from_errlog_internal procname ~clang_method_kind severity err_log ~loc ~node_id_key
       ~session ~ltr ~linters_def_file ~doc_url ~access ~extras exn
 
 
-let log_issue_deprecated severity proc_name ?loc ?node_id ?session ?ltr ?linters_def_file ?doc_url
+let log_issue_deprecated severity proc_name ?loc ?node_id_key ?session ?ltr ?linters_def_file
-    ?access ?extras:_ exn =
+    ?doc_url ?access ?extras:_ exn =
   match Summary.get proc_name with
   | Some summary ->
-      log_issue_from_summary severity summary ?loc ?node_id ?session ?ltr ?linters_def_file
+      log_issue_from_summary severity summary ?loc ?node_id_key ?session ?ltr ?linters_def_file
         ?doc_url ?access exn
   | None ->
       L.(die InternalError)
@@ -85,9 +81,9 @@ let log_warning = log_issue_from_summary Exceptions.Warning
 
 let log_issue_external procname severity ~loc ~ltr ?access exn =
   let errlog = IssueLog.get_errlog procname in
-  let node_id = (State.get_node_id_key () :> int * Caml.Digest.t) in
+  let node_id_key = State.get_node_id_key () in
   let session = (State.get_session () :> int) in
-  log_issue_from_errlog_internal procname ~clang_method_kind:None severity errlog ~loc ~node_id
+  log_issue_from_errlog_internal procname ~clang_method_kind:None severity errlog ~loc ~node_id_key
     ~session ~ltr ~linters_def_file:None ~doc_url:None ~access ~extras:None exn
 
 

infer/src/backend/reporting.mli

@@ -10,7 +10,7 @@ open! IStd
 (** Type of functions to report issues to the error_log in a spec. *)
 
 type log_t =
-  ?loc:Location.t -> ?node_id:int * Caml.Digest.t -> ?session:int -> ?ltr:Errlog.loc_trace
+  ?loc:Location.t -> ?node_id_key:Errlog.node_id_key -> ?session:int -> ?ltr:Errlog.loc_trace
   -> ?linters_def_file:string -> ?doc_url:string -> ?access:string -> ?extras:Jsonbug_t.extra
   -> exn -> unit
 
@@ -21,7 +21,7 @@ val log_issue_deprecated : Exceptions.severity -> Typ.Procname.t -> log_t
 
 val log_issue_from_errlog :
   Typ.Procname.t -> Exceptions.severity -> Errlog.t -> loc:Location.t
-  -> node_id:int * Caml.Digest.t -> ltr:Errlog.loc_trace -> linters_def_file:string option
+  -> node_id_key:Errlog.node_id_key -> ltr:Errlog.loc_trace -> linters_def_file:string option
   -> doc_url:string option -> exn -> unit
 (** Report an issue of a given kind  in the given error log. *)
 

infer/src/biabduction/State.ml

@@ -20,8 +20,7 @@ type failure_stats =
   ; (* number of node failures (i.e. at least one instruction failure) *)
     mutable node_ok: int
   ; (* number of node successes (i.e. no instruction failures) *)
-    mutable first_failure:
+    mutable first_failure: (Location.t * Errlog.node_id_key * int * Errlog.loc_trace * exn) option
-      (Location.t * (int * Caml.Digest.t) * int * Errlog.loc_trace * exn) option
   (* exception at the first failure *) }
 
 module NodeHash = Procdesc.NodeHash
@@ -95,46 +94,6 @@ let get_loc () =
 
 let get_node () = !gs.last_node
 
-(** simple key for a node: just look at the instructions *)
-let node_simple_key node =
-  let add_instr instr =
-    if Sil.instr_is_auxiliary instr then None
-    else
-      let instr_key =
-        match instr with
-        | Sil.Load _ ->
-            1
-        | Sil.Store _ ->
-            2
-        | Sil.Prune _ ->
-            3
-        | Sil.Call _ ->
-            4
-        | Sil.Nullify _ ->
-            5
-        | Sil.Abstract _ ->
-            6
-        | Sil.Remove_temps _ ->
-            7
-      in
-      Some instr_key
-  in
-  Procdesc.Node.get_instrs node |> IContainer.rev_filter_map_to_list ~fold:Instrs.fold ~f:add_instr
-  |> Utils.better_hash
-
-
-(** key for a node: look at the current node, successors and predecessors *)
-let node_key node =
-  let succs = Procdesc.Node.get_succs node in
-  let preds = Procdesc.Node.get_preds node in
-  let v =
-    ( node_simple_key node
-    , List.rev_map ~f:node_simple_key succs
-    , List.rev_map ~f:node_simple_key preds )
-  in
-  Utils.better_hash v
-
-
 (** normalize the list of instructions by renaming let-bound ids *)
 let instrs_normalize instrs =
   let bound_ids =
@@ -205,7 +164,10 @@ let mk_find_duplicate_nodes : Procdesc.t -> Procdesc.Node.t -> Procdesc.NodeSet.
 
 let get_node_id () = Procdesc.Node.get_id !gs.last_node
 
-let get_node_id_key () = (Procdesc.Node.get_id !gs.last_node, node_key !gs.last_node)
+let get_node_id_key () =
+  { Errlog.node_id= (Procdesc.Node.get_id !gs.last_node :> int)
+  ; node_key= Procdesc.Node.compute_key !gs.last_node }
+
 
 let get_inst_update pos =
   let loc = get_loc () in
@@ -289,17 +251,17 @@ let mark_instr_ok () =
 
 let mark_instr_fail exn =
   let loc = get_loc () in
-  let key = (get_node_id_key () :> int * Caml.Digest.t) in
+  let node_id_key = get_node_id_key () in
   let session = get_session () in
   let loc_trace = get_loc_trace () in
   let fs = get_failure_stats (get_node ()) in
   if is_none fs.first_failure then
-    fs.first_failure <- Some (loc, key, (session :> int), loc_trace, exn) ;
+    fs.first_failure <- Some (loc, node_id_key, (session :> int), loc_trace, exn) ;
   fs.instr_fail <- fs.instr_fail + 1
 
 
 type log_issue =
-  Typ.Procname.t -> ?loc:Location.t -> ?node_id:int * Caml.Digest.t -> ?session:int
+  Typ.Procname.t -> ?loc:Location.t -> ?node_id_key:Errlog.node_id_key -> ?session:int
   -> ?ltr:Errlog.loc_trace -> ?linters_def_file:string -> ?doc_url:string -> ?access:string
   -> ?extras:Jsonbug_t.extra -> exn -> unit
 
@@ -307,11 +269,11 @@ let process_execution_failures (log_issue: log_issue) pname =
   let do_failure _ fs =
     (* L.out "Node:%a node_ok:%d node_fail:%d@." Procdesc.Node.pp node fs.node_ok fs.node_fail; *)
     match (fs.node_ok, fs.first_failure) with
-    | 0, Some (loc, key, _, loc_trace, exn) when not Config.debug_exceptions ->
+    | 0, Some (loc, node_id_key, _, loc_trace, exn) when not Config.debug_exceptions ->
         let error = Exceptions.recognize_exception exn in
         let desc' = Localise.verbatim_desc ("exception: " ^ error.name.IssueType.unique_id) in
         let exn' = Exceptions.Analysis_stops (desc', error.ocaml_pos) in
-        log_issue pname ~loc ~node_id:key ~ltr:loc_trace exn'
+        log_issue pname ~loc ~node_id_key ~ltr:loc_trace exn'
     | _ ->
         ()
   in

infer/src/biabduction/State.mli

@@ -37,7 +37,7 @@ val get_loc_trace : unit -> Errlog.loc_trace
 val get_node : unit -> Procdesc.Node.t
 (** Get last node seen in symbolic execution *)
 
-val get_node_id_key : unit -> Procdesc.Node.id * Caml.Digest.t
+val get_node_id_key : unit -> Errlog.node_id_key
 (** Get id and key of last node seen in symbolic execution *)
 
 val get_normalized_pre :
@@ -75,7 +75,7 @@ val mk_find_duplicate_nodes : Procdesc.t -> Procdesc.Node.t -> Procdesc.NodeSet.
     and normalized (w.r.t. renaming of let - bound ids) list of instructions. *)
 
 type log_issue =
-  Typ.Procname.t -> ?loc:Location.t -> ?node_id:int * Caml.Digest.t -> ?session:int
+  Typ.Procname.t -> ?loc:Location.t -> ?node_id_key:Errlog.node_id_key -> ?session:int
   -> ?ltr:Errlog.loc_trace -> ?linters_def_file:string -> ?doc_url:string -> ?access:string
   -> ?extras:Jsonbug_t.extra -> exn -> unit
 

infer/src/clang/cFrontend_errors.ml

@@ -456,9 +456,10 @@ let log_frontend_issue method_decl_opt (node: Ctl_parser_types.ast_node)
     | Stmt st ->
         CAst_utils.generate_key_stmt st
   in
-  let key = Utils.better_hash key_str in
+  let node_key = NodeKey.of_frontend_node_key key_str in
   Reporting.log_issue_from_errlog procname issue_desc.severity errlog exn ~loc:issue_desc.loc
-    ~ltr:trace ~node_id:(0, key) ~linters_def_file ~doc_url:issue_desc.doc_url
+    ~ltr:trace ~node_id_key:{Errlog.node_id= 0; node_key} ~linters_def_file
+    ~doc_url:issue_desc.doc_url
 
 
 let fill_issue_desc_info_and_log context ~witness ~current_node (issue_desc: CIssue.issue_desc)