[nullsafe][refactor] Consolidate information about the issue to be reported

Summary:
This diff introduces `NullsafeIssue.t`: information about the issue
ready to be reported (and put to `result.json`).

Its notion was already implicitly used in a lot of code.

With this change, the achitechure becomes the following:

Firstly, `TypeErr.err_instance` represents issues at the moment of registration
during the typecheck. At this moment we don't always want to report
them, but it is important to store even non-reportable ones (we use it to calculate mode promotions).

Secondly, given the current `NullsafeMode.t`, we can either report the
issue or not. This logic lives in
`TypeErr.make_nullsafe_issue_if_reportable_lazy` that normally redirects
this decision to Rules (e.g. AssignmentRule or DereferenceRule).

Thirdly, if we want to report the issue, it is time to actually figure
out what to report (e.g. the precise error message, or additional
nullsafe specific `.json` params - see next diffs adding them).

Note that the exact logic of deciding if we want to report / how to
report / what should the message or .json param be is in practice
coupled (otherwise we'd have weird bugs when we decided the issue is
reportable, but don't have a good user facing reason).

In practice such logic for complix issues leaves in Rules.
C.f. `DereferenceRule` and `AssignmentRule` code.

The tricky part is that those rules actually share some common code
responsible for reporting, e.g. when it comes to processing third
parties (so the decision making & reporting is unified). See
`ErrorRenderingUtils.mk_nullsafe_issue_for_untrusted_values` which is
called both from `AssignmentRule` and `DereferenceRule`.

`NullsafeIssue.t` glues together those shared parts of code, and make
dependencies explicit.

Check out the next diff when we add more capabilities to
`NullsafeIssue.t` (e.g. ability to store dependent third party methods).

Without this refactoring, implementing this feature would be rather
tricky.

Reviewed By: skcho

Differential Revision: D23705587

fbshipit-source-id: b5246062a

infer/src/nullsafe/AssignmentRule.ml

@@ -38,8 +38,6 @@ module ReportableViolation = struct
     if is_non_reportable then None else Some {nullsafe_mode; violation}
 
 
-  let get_severity {nullsafe_mode} = NullsafeMode.severity nullsafe_mode
-
   let get_origin_opt assignment_type origin =
     let should_show_origin =
       match assignment_type with
@@ -146,7 +144,7 @@ module ReportableViolation = struct
 
 
   let mk_nullsafe_issue_for_explicitly_nullable_values ~assignment_type ~rhs_origin
-      ~explicit_rhs_nullable_kind ~assignment_location =
+      ~explicit_rhs_nullable_kind ~assignment_location ~nullsafe_mode =
     let nullability_evidence =
       get_origin_opt assignment_type rhs_origin
       |> Option.bind ~f:(fun origin -> TypeOrigin.get_description origin)
@@ -164,7 +162,7 @@ module ReportableViolation = struct
           Format.asprintf " If you don't expect null, use %a instead." MF.pp_monospaced descr )
         ~default:""
     in
-    let error_message =
+    let description =
       match assignment_type with
       | PassingParamToFunction function_info ->
           Format.sprintf "%s%s"
@@ -197,10 +195,11 @@ module ReportableViolation = struct
             return_description nullability_evidence_as_suffix alternative_recommendation
     in
     let issue_type = get_issue_type assignment_type in
-    (error_message, issue_type, assignment_location)
+    NullsafeIssue.make ~description ~issue_type ~loc:assignment_location
+      ~severity:(NullsafeMode.severity nullsafe_mode)
 
 
-  let get_description ~assignment_location assignment_type {nullsafe_mode; violation= {rhs}} =
+  let make_nullsafe_issue ~assignment_location assignment_type {nullsafe_mode; violation= {rhs}} =
     let rhs_origin = InferredNullability.get_origin rhs in
     let user_friendly_nullable =
       ErrorRenderingUtils.UserFriendlyNullable.from_nullability
@@ -219,7 +218,7 @@ module ReportableViolation = struct
           ~bad_usage_location:assignment_location rhs_origin
     | ErrorRenderingUtils.UserFriendlyNullable.ExplainablyNullable explicit_kind ->
         (* Attempt to assigning a value that can be explained to the user as nullable. *)
-        mk_nullsafe_issue_for_explicitly_nullable_values ~assignment_type ~rhs_origin
+        mk_nullsafe_issue_for_explicitly_nullable_values ~assignment_type ~rhs_origin ~nullsafe_mode
           ~explicit_rhs_nullable_kind:explicit_kind ~assignment_location
 end
 

infer/src/nullsafe/AssignmentRule.mli

@@ -37,11 +37,8 @@ module ReportableViolation : sig
     ; param_position: int
     ; function_procname: Procname.Java.t }
 
-  val get_severity : t -> IssueType.severity
-  (** Severity of the violation to be reported *)
-
-  val get_description :
-    assignment_location:Location.t -> assignment_type -> t -> string * IssueType.t * Location.t
+  val make_nullsafe_issue :
+    assignment_location:Location.t -> assignment_type -> t -> NullsafeIssue.t
   (** Given context around violation, return error message together with the info where to put this
       message *)
 end

infer/src/nullsafe/DereferenceRule.ml

@@ -38,7 +38,7 @@ module ReportableViolation = struct
 
 
   let mk_nullsafe_issue_for_explicitly_nullable_values ~explicit_kind ~dereference_type
-      dereference_location ~nullable_object_descr ~nullable_object_origin =
+      dereference_location ~nullsafe_mode ~nullable_object_descr ~nullable_object_origin =
     let module MF = MarkupFormatter in
     let what_is_dereferred_str =
       match dereference_type with
@@ -95,10 +95,12 @@ module ReportableViolation = struct
           Format.sprintf "%s is nullable and is not locally checked for null when %s%s.%s"
             what_is_dereferred_str action_descr origin_descr alternative_recommendation
     in
-    (description, IssueType.eradicate_nullable_dereference, dereference_location)
+    NullsafeIssue.make ~description ~issue_type:IssueType.eradicate_nullable_dereference
+      ~loc:dereference_location
+      ~severity:(NullsafeMode.severity nullsafe_mode)
 
 
-  let get_description {nullsafe_mode; violation= {nullability}} ~dereference_location
+  let make_nullsafe_issue {nullsafe_mode; violation= {nullability}} ~dereference_location
       dereference_type ~nullable_object_descr =
     let user_friendly_nullable =
       ErrorRenderingUtils.UserFriendlyNullable.from_nullability
@@ -119,10 +121,7 @@ module ReportableViolation = struct
     | ErrorRenderingUtils.UserFriendlyNullable.ExplainablyNullable explicit_kind ->
         (* Attempt to dereference value that can be explained to the user as nullable. *)
         mk_nullsafe_issue_for_explicitly_nullable_values ~explicit_kind ~dereference_type
-          dereference_location ~nullable_object_descr ~nullable_object_origin
-
-
-  let get_severity {nullsafe_mode} = NullsafeMode.severity nullsafe_mode
+          ~nullsafe_mode dereference_location ~nullable_object_descr ~nullable_object_origin
 end
 
 let check nullability =

infer/src/nullsafe/DereferenceRule.mli

@@ -31,15 +31,12 @@ module ReportableViolation : sig
   (** Depending on the mode, violation might or might not be important enough to be reported to the
       user. If it should NOT be reported for that mode, this function will return None. *)
 
-  val get_severity : t -> IssueType.severity
-  (** Severity of the violation to be reported *)
-
-  val get_description :
+  val make_nullsafe_issue :
        t
     -> dereference_location:Location.t
     -> dereference_type
     -> nullable_object_descr:string option
-    -> string * IssueType.t * Location.t
+    -> NullsafeIssue.t
   (** Given context around violation, return error message together with the info where to put this
       message *)
 end

infer/src/nullsafe/EradicateReporting.ml

@@ -7,15 +7,19 @@
 
 open! IStd
 
-let report_error {IntraproceduralAnalysis.proc_desc; tenv; err_log} checker kind loc
-    ?(field_name = None) ~severity description =
+let report_error {IntraproceduralAnalysis.proc_desc; tenv; err_log} checker ?(field_name = None)
+    nullsafe_issue =
   let proc_attrs = Procdesc.get_attributes proc_desc in
-  let suppressed = Reporting.is_suppressed tenv proc_attrs kind ~field_name in
+  let issue_type = NullsafeIssue.get_issue_type nullsafe_issue in
+  let description = NullsafeIssue.get_description nullsafe_issue in
+  let severity = NullsafeIssue.get_severity nullsafe_issue in
+  let loc = NullsafeIssue.get_loc nullsafe_issue in
+  let suppressed = Reporting.is_suppressed tenv proc_attrs issue_type ~field_name in
   if suppressed then Logging.debug Analysis Medium "Reporting is suppressed!@\n"
   else
     let localized_description = Localise.verbatim_desc description in
     let issue_to_report =
-      {IssueToReport.issue_type= kind; description= localized_description; ocaml_pos= None}
+      {IssueToReport.issue_type; description= localized_description; ocaml_pos= None}
     in
     let trace = [Errlog.make_trace_element 0 loc description []] in
     let node = AnalysisState.get_node_exn () in

infer/src/nullsafe/EradicateReporting.mli

@@ -10,9 +10,6 @@ open! IStd
 val report_error :
      IntraproceduralAnalysis.t
   -> Checker.t
-  -> IssueType.t
-  -> Location.t
   -> ?field_name:Fieldname.t option
-  -> severity:IssueType.severity
-  -> string
+  -> NullsafeIssue.t
   -> unit

infer/src/nullsafe/ErrorRenderingUtils.ml

@@ -253,7 +253,8 @@ let mk_nullsafe_issue_for_untrusted_values ~nullsafe_mode ~untrusted_kind ~bad_u
       offending_object NullsafeMode.pp nullsafe_mode coming_from_explanation what_is_used
       bad_usage_location.Location.line recommendation
   in
-  (description, issue_type, object_loc)
+  NullsafeIssue.make ~description ~issue_type ~loc:object_loc
+    ~severity:(NullsafeMode.severity nullsafe_mode)
 
 
 let find_alternative_nonnull_method_description nullable_origin =

infer/src/nullsafe/ErrorRenderingUtils.mli

@@ -40,13 +40,13 @@ val mk_nullsafe_issue_for_untrusted_values :
   -> untrusted_kind:UserFriendlyNullable.untrusted_kind
   -> bad_usage_location:Location.t
   -> TypeOrigin.t
-  -> string * IssueType.t * Location.t
+  -> NullsafeIssue.t
 (** Situation when we tried to use nonnull values in a nullsafe mode that does not trust them to be
     non-nullable: [untrusted_kind]. From the user perspective, this case is different from normal
     nullable assignment or dereference violation: what needs to be described is why does not this
-    mode trust this value (and what are possible actions). Returns a tuple (error message, issue
-    type, error location). NOTE: Location of the error will be NOT in the place when the value is
-    used (that is [bad_usage_location]), but where the value is first obtained from. *)
+    mode trust this value (and what are possible actions). NOTE: Location of the error will be NOT
+    in the place when the value is used (that is [bad_usage_location]), but where the value is first
+    obtained from. *)
 
 val find_alternative_nonnull_method_description : TypeOrigin.t -> string option
 (** If type origin is the result of a nullable method call that have a known nonnullable alternative

infer/src/nullsafe/InheritanceRule.ml

@@ -39,47 +39,57 @@ module ReportableViolation = struct
         false
 
 
-  let get_description _ violation_type ~base_proc_name ~overridden_proc_name =
+  let make_nullsafe_issue _ violation_type ~nullsafe_mode ~loc ~base_proc_name ~overridden_proc_name
+      =
     let module MF = MarkupFormatter in
     let base_method_descr = Procname.Java.to_simplified_string ~withclass:true base_proc_name in
     let overridden_method_descr =
       Procname.Java.to_simplified_string ~withclass:true overridden_proc_name
     in
-    match violation_type with
-    | InconsistentReturn ->
-        Format.asprintf
-          "Child method %a is not substitution-compatible with its parent: the return type is \
-           declared as nullable, but parent method %a is missing `@Nullable` declaration. Either \
-           mark the parent as `@Nullable` or ensure the child does not return `null`."
-          MF.pp_monospaced overridden_method_descr MF.pp_monospaced base_method_descr
-    | InconsistentParam {param_description; param_position} ->
-        if is_java_lang_object_equals base_proc_name then
-          (* This is a popular enough case to make error message specific *)
+    let description =
+      match violation_type with
+      | InconsistentReturn ->
           Format.asprintf
-            "Parameter %a is missing `@Nullable` declaration: according to the Java Specification, \
-             for any object `x` call `x.equals(null)` should properly return false."
-            MF.pp_monospaced param_description
-        else
-          let translate_position = function
-            | 1 ->
-                "First"
-            | 2 ->
-                "Second"
-            | 3 ->
-                "Third"
-            | n ->
-                string_of_int n ^ "th"
-          in
-          Format.asprintf
-            "%s parameter %a of method %a is missing `@Nullable` declaration when overriding %a. \
-             The parent method declared it can handle `null` for this param, so the child should \
-             also declare that."
-            (translate_position param_position)
-            MF.pp_monospaced param_description MF.pp_monospaced overridden_method_descr
-            MF.pp_monospaced base_method_descr
-
-
-  let get_severity {nullsafe_mode} = NullsafeMode.severity nullsafe_mode
+            "Child method %a is not substitution-compatible with its parent: the return type is \
+             declared as nullable, but parent method %a is missing `@Nullable` declaration. Either \
+             mark the parent as `@Nullable` or ensure the child does not return `null`."
+            MF.pp_monospaced overridden_method_descr MF.pp_monospaced base_method_descr
+      | InconsistentParam {param_description; param_position} ->
+          if is_java_lang_object_equals base_proc_name then
+            (* This is a popular enough case to make error message specific *)
+            Format.asprintf
+              "Parameter %a is missing `@Nullable` declaration: according to the Java \
+               Specification, for any object `x` call `x.equals(null)` should properly return \
+               false."
+              MF.pp_monospaced param_description
+          else
+            let translate_position = function
+              | 1 ->
+                  "First"
+              | 2 ->
+                  "Second"
+              | 3 ->
+                  "Third"
+              | n ->
+                  string_of_int n ^ "th"
+            in
+            Format.asprintf
+              "%s parameter %a of method %a is missing `@Nullable` declaration when overriding %a. \
+               The parent method declared it can handle `null` for this param, so the child should \
+               also declare that."
+              (translate_position param_position)
+              MF.pp_monospaced param_description MF.pp_monospaced overridden_method_descr
+              MF.pp_monospaced base_method_descr
+    in
+    let severity = NullsafeMode.severity nullsafe_mode in
+    let issue_type =
+      match violation_type with
+      | InconsistentReturn ->
+          IssueType.eradicate_inconsistent_subclass_return_annotation
+      | InconsistentParam _ ->
+          IssueType.eradicate_inconsistent_subclass_parameter_annotation
+    in
+    NullsafeIssue.make ~description ~loc ~issue_type ~severity
 end
 
 let check type_role ~base ~overridden =

infer/src/nullsafe/InheritanceRule.mli

@@ -38,15 +38,14 @@ module ReportableViolation : sig
   (** Depending on the mode, violation might or might not be important enough to be reported to the
       user. If it should NOT be reported for that mode, this function will return None. *)
 
-  val get_severity : t -> IssueType.severity
-  (** Severity of the violation to be reported *)
-
-  val get_description :
+  val make_nullsafe_issue :
        t
     -> violation_type
+    -> nullsafe_mode:NullsafeMode.t
+    -> loc:Location.t
     -> base_proc_name:Procname.Java.t
     -> overridden_proc_name:Procname.Java.t
-    -> string
+    -> NullsafeIssue.t
   (** Given context around violation, return error message together with the info where to put this
       message *)
 end

infer/src/nullsafe/NullsafeIssue.ml

@@ -0,0 +1,24 @@
+(*
+ * 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.
+ *)
+
+open! IStd
+
+type t =
+  { issue_type: IssueType.t
+  ; description: string  (** Human-readable description *)
+  ; loc: Location.t  (** Where to report the error *)
+  ; severity: IssueType.severity }
+
+let make ~issue_type ~description ~loc ~severity = {issue_type; description; loc; severity}
+
+let get_issue_type {issue_type} = issue_type
+
+let get_description {description} = description
+
+let get_loc {loc} = loc
+
+let get_severity {severity} = severity

infer/src/nullsafe/NullsafeIssue.mli

@@ -0,0 +1,22 @@
+(*
+ * 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.
+ *)
+open! IStd
+
+(** Information about the nullsafe issue to be reported to the user / put into the result json *)
+
+type t
+
+val make :
+  issue_type:IssueType.t -> description:string -> loc:Location.t -> severity:IssueType.severity -> t
+
+val get_issue_type : t -> IssueType.t
+
+val get_description : t -> string
+
+val get_loc : t -> Location.t
+
+val get_severity : t -> IssueType.severity

infer/src/nullsafe/eradicateChecks.ml

@@ -35,7 +35,7 @@ let check_object_dereference ({IntraproceduralAnalysis.tenv; _} as analysis_data
           {dereference_violation; dereference_location= loc; nullable_object_descr; dereference_type}
       in
       TypeErr.register_error analysis_data find_canonical_duplicate type_error (Some instr_ref)
-        ~nullsafe_mode loc )
+        ~nullsafe_mode )
 
 
 (** [expr] is an expression that was explicitly compared with `null`. At the same time, [expr] had
@@ -106,8 +106,8 @@ let check_condition_for_redundancy
     let condition_descr = explain_expr tenv node expr in
     let nonnull_origin = InferredNullability.get_origin inferred_nullability in
     TypeErr.register_error analysis_data find_canonical_duplicate
-      (TypeErr.Condition_redundant {is_always_true; condition_descr; nonnull_origin})
-      (Some instr_ref) ~nullsafe_mode loc
+      (TypeErr.Condition_redundant {is_always_true; loc; condition_descr; nonnull_origin})
+      (Some instr_ref) ~nullsafe_mode
 
 
 (** Check an assignment to a field. *)
@@ -154,7 +154,7 @@ let check_field_assignment
                  { assignment_violation
                  ; assignment_location= loc
                  ; assignment_type= AssignmentRule.ReportableViolation.AssigningToField fname })
-              (Some instr_ref) ~nullsafe_mode loc ) )
+              (Some instr_ref) ~nullsafe_mode ) )
 
 
 (* Check if the field declared as not nullable (implicitly or explicitly). If the field is
@@ -297,8 +297,8 @@ let check_constructor_initialization
                   ()
                 else
                   TypeErr.register_error analysis_data find_canonical_duplicate
-                    (TypeErr.Field_not_initialized {field_name})
-                    None ~nullsafe_mode loc ;
+                    (TypeErr.Field_not_initialized {field_name; loc})
+                    None ~nullsafe_mode ;
               (* Check if field is over-annotated. *)
               match annotated_field with
               | None ->
@@ -315,8 +315,9 @@ let check_constructor_initialization
                         TypeErr.register_error analysis_data find_canonical_duplicate
                           (TypeErr.Over_annotation
                              { over_annotated_violation
+                             ; loc
                              ; violation_type= OverAnnotatedRule.FieldOverAnnoted field_name })
-                          ~nullsafe_mode None loc ) )
+                          ~nullsafe_mode None ) )
           in
           List.iter ~f:do_field fields
       | None ->
@@ -336,7 +337,7 @@ let check_return_not_nullable analysis_data ~nullsafe_mode ~java_pname find_cano
            { assignment_violation
            ; assignment_location= loc
            ; assignment_type= ReturningFromFunction java_pname })
-        None ~nullsafe_mode loc )
+        None ~nullsafe_mode )
 
 
 let check_return_overrannotated ~java_pname analysis_data find_canonical_duplicate loc
@@ -351,8 +352,9 @@ let check_return_overrannotated ~java_pname analysis_data find_canonical_duplica
   Result.iter_error (OverAnnotatedRule.check ~what ~by_rhs_upper_bound)
     ~f:(fun over_annotated_violation ->
       TypeErr.register_error analysis_data find_canonical_duplicate
-        (Over_annotation {over_annotated_violation; violation_type= ReturnOverAnnotated java_pname})
-        None ~nullsafe_mode loc )
+        (Over_annotation
+           {over_annotated_violation; loc; violation_type= ReturnOverAnnotated java_pname})
+        None ~nullsafe_mode )
 
 
 (** Check the annotations when returning from a method. *)
@@ -424,7 +426,7 @@ let check_call_parameters ({IntraproceduralAnalysis.tenv; _} as analysis_data) ~
                  ; actual_param_expression
                  ; param_position
                  ; function_procname= callee_pname } })
-        (Some instr_ref) ~nullsafe_mode loc
+        (Some instr_ref) ~nullsafe_mode
     in
     if PatternMatch.type_is_class formal.param_annotated_type.typ then
       (* Passing a param to a function is essentially an assignment the actual param value
@@ -444,10 +446,11 @@ let check_inheritance_rule_for_return analysis_data find_canonical_duplicate loc
       TypeErr.register_error analysis_data find_canonical_duplicate
         (Inconsistent_subclass
            { inheritance_violation
+           ; loc
            ; violation_type= InconsistentReturn
            ; overridden_proc_name
            ; base_proc_name })
-        None ~nullsafe_mode loc )
+        None ~nullsafe_mode )
 
 
 let check_inheritance_rule_for_param analysis_data find_canonical_duplicate loc ~nullsafe_mode
@@ -463,8 +466,9 @@ let check_inheritance_rule_for_param analysis_data find_canonical_duplicate loc
                InconsistentParam
                  {param_position; param_description= Mangled.to_string overridden_param_name}
            ; base_proc_name
+           ; loc
            ; overridden_proc_name })
-        None ~nullsafe_mode loc )
+        None ~nullsafe_mode )
 
 
 let check_inheritance_rule_for_params analysis_data find_canonical_duplicate loc ~nullsafe_mode

infer/src/nullsafe/immutableChecker.ml

@@ -35,8 +35,9 @@ let check_immutable_cast analysis_data proc_desc typ_expected typ_found_opt loc
                 (Procname.to_simplified_string (Procdesc.get_proc_name proc_desc))
                 Typ.Name.pp name_given Typ.Name.pp name_expected
             in
+            let issue_type = IssueType.checkers_immutable_cast in
             EradicateReporting.report_error analysis_data ImmutableCast
-              IssueType.checkers_immutable_cast loc description ~severity:Warning
+              (NullsafeIssue.make ~loc ~description ~severity:Warning ~issue_type)
       | _ ->
           () )
   | None ->

infer/src/nullsafe/typeCheck.ml

@@ -524,9 +524,10 @@ let do_preconditions_check_not_null
           TypeErr.register_error analysis_data find_canonical_duplicate
             (Condition_redundant
                { is_always_true= true
+               ; loc
                ; condition_descr= EradicateChecks.explain_expr tenv node cond
                ; nonnull_origin= InferredNullability.get_origin nullability })
-            (Some instr_ref) ~nullsafe_mode loc ) ;
+            (Some instr_ref) ~nullsafe_mode ) ;
         let previous_origin = InferredNullability.get_origin nullability in
         let new_origin = TypeOrigin.InferredNonnull {previous_origin} in
         TypeState.add pvar

infer/src/nullsafe/typeErr.ml

@@ -61,15 +61,20 @@ end
 (** Instance of an error *)
 type err_instance =
   | Condition_redundant of
-      {is_always_true: bool; condition_descr: string option; nonnull_origin: TypeOrigin.t}
+      { loc: Location.t
+      ; is_always_true: bool
+      ; condition_descr: string option
+      ; nonnull_origin: TypeOrigin.t }
   | Inconsistent_subclass of
-      { inheritance_violation: InheritanceRule.violation
+      { loc: Location.t
+      ; inheritance_violation: InheritanceRule.violation
       ; violation_type: InheritanceRule.ReportableViolation.violation_type
       ; base_proc_name: Procname.Java.t
       ; overridden_proc_name: Procname.Java.t }
-  | Field_not_initialized of {field_name: Fieldname.t}
+  | Field_not_initialized of {loc: Location.t; field_name: Fieldname.t}
   | Over_annotation of
-      { over_annotated_violation: OverAnnotatedRule.violation
+      { loc: Location.t
+      ; over_annotated_violation: OverAnnotatedRule.violation
       ; violation_type: OverAnnotatedRule.violation_type }
   | Nullable_dereference of
       { dereference_violation: DereferenceRule.violation
@@ -135,9 +140,7 @@ module H = Hashtbl.Make (struct
 end
 (* H *))
 
-type err_state =
-  { loc: Location.t  (** location of the error *)
-  ; mutable always: bool  (** always fires on its associated node *) }
+type err_state = {mutable always: bool  (** always fires on its associated node *)}
 
 let err_tbl : err_state H.t = H.create 1
 
@@ -176,7 +179,7 @@ let node_reset_forall node =
 
 
 (** Add an error to the error table and return whether it should be printed now. *)
-let add_err find_canonical_duplicate err_instance instr_ref_opt loc =
+let add_err find_canonical_duplicate err_instance instr_ref_opt =
   let is_forall = get_forall err_instance in
   if H.mem err_tbl (err_instance, instr_ref_opt) then false (* don't print now *)
   else
@@ -192,7 +195,7 @@ let add_err find_canonical_duplicate err_instance instr_ref_opt loc =
           instr_ref_opt
     in
     Logging.debug Analysis Medium "Registering an issue: %a@\n" pp_err_instance err_instance ;
-    H.add err_tbl (err_instance, instr_ref_opt_deduplicate) {loc; always= true} ;
+    H.add err_tbl (err_instance, instr_ref_opt_deduplicate) {always= true} ;
     not is_forall
 
 
@@ -242,61 +245,64 @@ let get_nonnull_explanation_for_condition_redudant (nonnull_origin : TypeOrigin.
 
 
 (** If error is reportable to the user, return its information. Otherwise return [None]. *)
-let get_error_info_if_reportable_lazy ~nullsafe_mode err_instance =
+let make_nullsafe_issue_if_reportable_lazy ~nullsafe_mode err_instance =
   let open IOption.Let_syntax in
   if is_synthetic_err err_instance then None
   else
     match err_instance with
-    | Condition_redundant {is_always_true; condition_descr; nonnull_origin} ->
+    | Condition_redundant {is_always_true; loc; condition_descr; nonnull_origin} ->
         Some
           ( lazy
-            ( P.sprintf "The condition %s might be always %b%s."
-                (Option.value condition_descr ~default:"")
-                is_always_true
-                (get_nonnull_explanation_for_condition_redudant nonnull_origin)
-            , IssueType.eradicate_condition_redundant
-            , None
-            , (* Condition redundant is a very non-precise issue. Depending on the origin of what is compared with null,
-                 this can have a lot of reasons to be actually nullable.
-                 Until it is made non-precise, it is recommended to not turn this warning on.
-                 But even when it is on, this should not be more than advice.
-              *)
-              IssueType.Advice ) )
-    | Over_annotation {over_annotated_violation; violation_type} ->
+            (NullsafeIssue.make
+               ~description:
+                 (P.sprintf "The condition %s might be always %b%s."
+                    (Option.value condition_descr ~default:"")
+                    is_always_true
+                    (get_nonnull_explanation_for_condition_redudant nonnull_origin))
+               ~issue_type:IssueType.eradicate_condition_redundant
+               ~loc
+                 (* Condition redundant is a very non-precise issue. Depending on the origin of what is compared with null,
+                      this can have a lot of reasons to be actually nullable.
+                      Until it is made non-precise, it is recommended to not turn this warning on.
+                      But even when it is on, this should not be more than advice.
+                 *) ~severity:IssueType.Advice) )
+    | Over_annotation {over_annotated_violation; loc; violation_type} ->
         Some
           ( lazy
-            ( OverAnnotatedRule.violation_description over_annotated_violation violation_type
-            , ( match violation_type with
-              | OverAnnotatedRule.FieldOverAnnoted _ ->
-                  IssueType.eradicate_field_over_annotated
-              | OverAnnotatedRule.ReturnOverAnnotated _ ->
-                  IssueType.eradicate_return_over_annotated )
-            , None
-            , (* Very non-precise issue. Should be actually turned off unless for experimental purposes. *)
-              IssueType.Advice ) )
-    | Field_not_initialized {field_name} ->
+            (let issue_type =
+               match violation_type with
+               | OverAnnotatedRule.FieldOverAnnoted _ ->
+                   IssueType.eradicate_field_over_annotated
+               | OverAnnotatedRule.ReturnOverAnnotated _ ->
+                   IssueType.eradicate_return_over_annotated
+             in
+             let description =
+               OverAnnotatedRule.violation_description over_annotated_violation violation_type
+             in
+             NullsafeIssue.make ~description ~issue_type ~loc
+               ~severity:
+                 (* Very non-precise issue. Should be actually turned off unless for experimental purposes. *)
+                 IssueType.Advice ) )
+    | Field_not_initialized {field_name; loc} ->
         Some
           ( lazy
-            ( Format.asprintf
-                "Field %a is declared non-nullable, so it should be initialized in the constructor \
-                 or in an `@Initializer` method"
-                MF.pp_monospaced
-                (Fieldname.get_field_name field_name)
-            , IssueType.eradicate_field_not_initialized
-            , None
-            , NullsafeMode.severity nullsafe_mode ) )
+            (NullsafeIssue.make
+               ~description:
+                 (Format.asprintf
+                    "Field %a is declared non-nullable, so it should be initialized in the \
+                     constructor or in an `@Initializer` method"
+                    MF.pp_monospaced
+                    (Fieldname.get_field_name field_name))
+               ~issue_type:IssueType.eradicate_field_not_initialized ~loc
+               ~severity:(NullsafeMode.severity nullsafe_mode)) )
     | Bad_assignment {assignment_location; assignment_type; assignment_violation} ->
         (* If violation is reportable, create tuple, otherwise None *)
         let+ reportable_violation =
           AssignmentRule.ReportableViolation.from nullsafe_mode assignment_violation
         in
         lazy
-          (let description, issue_type, error_location =
-             AssignmentRule.ReportableViolation.get_description ~assignment_location assignment_type
-               reportable_violation
-           in
-           let severity = AssignmentRule.ReportableViolation.get_severity reportable_violation in
-           (description, issue_type, Some error_location, severity) )
+          (AssignmentRule.ReportableViolation.make_nullsafe_issue ~assignment_location
+             assignment_type reportable_violation)
     | Nullable_dereference
         {dereference_violation; dereference_location; nullable_object_descr; dereference_type} ->
         (* If violation is reportable, create tuple, otherwise None *)
@@ -304,58 +310,46 @@ let get_error_info_if_reportable_lazy ~nullsafe_mode err_instance =
           DereferenceRule.ReportableViolation.from nullsafe_mode dereference_violation
         in
         lazy
-          (let description, issue_type, error_location =
-             DereferenceRule.ReportableViolation.get_description reportable_violation
-               ~dereference_location dereference_type ~nullable_object_descr
-           in
-           let severity = DereferenceRule.ReportableViolation.get_severity reportable_violation in
-           (description, issue_type, Some error_location, severity) )
+          (DereferenceRule.ReportableViolation.make_nullsafe_issue reportable_violation
+             ~dereference_location dereference_type ~nullable_object_descr)
     | Inconsistent_subclass
-        {inheritance_violation; violation_type; base_proc_name; overridden_proc_name} ->
+        {inheritance_violation; violation_type; loc; base_proc_name; overridden_proc_name} ->
         (* If violation is reportable, create tuple, otherwise None *)
         let+ reportable_violation =
           InheritanceRule.ReportableViolation.from nullsafe_mode inheritance_violation
         in
         lazy
-          ( InheritanceRule.ReportableViolation.get_description reportable_violation violation_type
-              ~base_proc_name ~overridden_proc_name
-          , ( match violation_type with
-            | InconsistentReturn ->
-                IssueType.eradicate_inconsistent_subclass_return_annotation
-            | InconsistentParam _ ->
-                IssueType.eradicate_inconsistent_subclass_parameter_annotation )
-          , None
-          , InheritanceRule.ReportableViolation.get_severity reportable_violation )
+          (InheritanceRule.ReportableViolation.make_nullsafe_issue ~nullsafe_mode
+             reportable_violation violation_type ~loc ~base_proc_name ~overridden_proc_name)
 
 
 (** If error is reportable to the user, return description, severity etc. Otherwise return None. *)
-let get_error_info_if_reportable ~nullsafe_mode err_instance =
-  get_error_info_if_reportable_lazy ~nullsafe_mode err_instance |> Option.map ~f:Lazy.force
+let make_nullsafe_issue_if_reportable ~nullsafe_mode err_instance =
+  make_nullsafe_issue_if_reportable_lazy ~nullsafe_mode err_instance |> Option.map ~f:Lazy.force
 
 
 let is_reportable ~nullsafe_mode err_instance =
   (* Note: we don't fetch the whole info because the the class-level analysis breaks some
      assumptions of this function, and also for optimization purposes (saving some string
      manipulations). *)
-  get_error_info_if_reportable_lazy ~nullsafe_mode err_instance |> Option.is_some
+  make_nullsafe_issue_if_reportable_lazy ~nullsafe_mode err_instance |> Option.is_some
 
 
-let report_now_if_reportable analysis_data err_instance ~nullsafe_mode loc =
-  get_error_info_if_reportable ~nullsafe_mode err_instance
-  |> Option.iter ~f:(fun (err_description, infer_issue_type, updated_location, severity) ->
-         Logging.debug Analysis Medium "About to report: %s" err_description ;
+let report_now_if_reportable analysis_data err_instance ~nullsafe_mode =
+  make_nullsafe_issue_if_reportable ~nullsafe_mode err_instance
+  |> Option.iter ~f:(fun nullsafe_issue ->
+         Logging.debug Analysis Medium "About to report: %s"
+           (NullsafeIssue.get_description nullsafe_issue) ;
          let field_name = get_field_name_for_error_suppressing err_instance in
-         let error_location = Option.value updated_location ~default:loc in
-         EradicateReporting.report_error analysis_data Eradicate infer_issue_type error_location
-           ~field_name ~severity err_description )
+         EradicateReporting.report_error analysis_data Eradicate nullsafe_issue ~field_name )
 
 
 (** Register issue (unless exactly the same issue was already registered). If needed, report this
     error immediately. *)
 let register_error analysis_data find_canonical_duplicate err_instance ~nullsafe_mode instr_ref_opt
-    loc =
-  let should_report_now = add_err find_canonical_duplicate err_instance instr_ref_opt loc in
-  if should_report_now then report_now_if_reportable analysis_data err_instance ~nullsafe_mode loc
+    =
+  let should_report_now = add_err find_canonical_duplicate err_instance instr_ref_opt in
+  if should_report_now then report_now_if_reportable analysis_data err_instance ~nullsafe_mode
 
 
 let report_forall_issues_and_reset analysis_data ~nullsafe_mode =
@@ -365,7 +359,7 @@ let report_forall_issues_and_reset analysis_data ~nullsafe_mode =
         let node = InstrRef.get_node instr_ref in
         AnalysisState.set_node node ;
         if is_forall && err_state.always then
-          report_now_if_reportable analysis_data err_instance err_state.loc ~nullsafe_mode
+          report_now_if_reportable analysis_data err_instance ~nullsafe_mode
     | None, _ ->
         ()
   in

infer/src/nullsafe/typeErr.mli

@@ -35,15 +35,20 @@ module InstrRef : InstrRefT
 (** Instance of an error *)
 type err_instance =
   | Condition_redundant of
-      {is_always_true: bool; condition_descr: string option; nonnull_origin: TypeOrigin.t}
+      { loc: Location.t
+      ; is_always_true: bool
+      ; condition_descr: string option
+      ; nonnull_origin: TypeOrigin.t }
   | Inconsistent_subclass of
-      { inheritance_violation: InheritanceRule.violation
+      { loc: Location.t
+      ; inheritance_violation: InheritanceRule.violation
       ; violation_type: InheritanceRule.ReportableViolation.violation_type
       ; base_proc_name: Procname.Java.t
       ; overridden_proc_name: Procname.Java.t }
-  | Field_not_initialized of {field_name: Fieldname.t}
+  | Field_not_initialized of {loc: Location.t; field_name: Fieldname.t}
   | Over_annotation of
-      { over_annotated_violation: OverAnnotatedRule.violation
+      { loc: Location.t
+      ; over_annotated_violation: OverAnnotatedRule.violation
       ; violation_type: OverAnnotatedRule.violation_type }
   | Nullable_dereference of
       { dereference_violation: DereferenceRule.violation
@@ -66,7 +71,6 @@ val register_error :
   -> err_instance
   -> nullsafe_mode:NullsafeMode.t
   -> InstrRef.t option
-  -> Location.t
   -> unit
 (** Register the fact that issue happened. Depending on the error and mode, this error might or
     might not be reported to the user. *)

infer/src/nullsafe/unit/AggregatedSummariesTest.ml

@@ -17,6 +17,7 @@ let mock_summary id =
   let mock_issue =
     TypeErr.Condition_redundant
       { is_always_true= true
+      ; loc= Location.dummy
       ; condition_descr= Some (Int.to_string id)
       ; nonnull_origin= TypeOrigin.New }
   in