[nullsafe] Encapsulate creation of ReportableViolation.t inside ReportableViolation

Summary:
`to_reportable_violation` is responsible for identifying if the
violation is reportable in this mode or not.

This logic is higly coupled with other functions in
`ReportableViolation`, such as `get_description`: You can not have
sensible description on the violation that is NOT reportable in a given
mode.

So from logical perpsective, creation of `ReportableViolation.t` should
belong to this module itself, not to the parent `Rule` module.

This change will make design clearer.

Reviewed By: artempyanykh

Differential Revision: D20511756

fbshipit-source-id: ef27b5057

infer/src/nullsafe/AssignmentRule.ml

@@ -24,6 +24,20 @@ module ReportableViolation = struct
     ; param_position: int
     ; function_procname: Procname.t }
 
+  let from nullsafe_mode ({lhs; rhs} as violation) =
+    let falls_under_optimistic_third_party =
+      Config.nullsafe_optimistic_third_party_params_in_non_strict
+      && NullsafeMode.equal nullsafe_mode Default
+      && Nullability.equal lhs ThirdPartyNonnull
+    in
+    let is_non_reportable =
+      falls_under_optimistic_third_party
+      || (* In certain modes, we trust rhs to be non-nullable and don't report violation *)
+      Nullability.is_considered_nonnull ~nullsafe_mode rhs
+    in
+    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 =
@@ -214,17 +228,3 @@ end
 let check ~lhs ~rhs =
   let is_subtype = Nullability.is_subtype ~supertype:lhs ~subtype:rhs in
   Result.ok_if_true is_subtype ~error:{lhs; rhs}
-
-
-let to_reportable_violation nullsafe_mode ({lhs; rhs} as violation) =
-  let falls_under_optimistic_third_party =
-    Config.nullsafe_optimistic_third_party_params_in_non_strict
-    && NullsafeMode.equal nullsafe_mode Default
-    && Nullability.equal lhs ThirdPartyNonnull
-  in
-  let is_non_reportable =
-    falls_under_optimistic_third_party
-    || (* In certain modes, we trust rhs to be non-nullable and don't report violation *)
-    Nullability.is_considered_nonnull ~nullsafe_mode rhs
-  in
-  if is_non_reportable then None else Some ReportableViolation.{nullsafe_mode; violation}

infer/src/nullsafe/AssignmentRule.mli

@@ -20,6 +20,10 @@ val check : lhs:Nullability.t -> rhs:Nullability.t -> (unit, violation) result
 module ReportableViolation : sig
   type t
 
+  val from : NullsafeMode.t -> violation -> t option
+  (** 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. *)
+
   type assignment_type =
     | PassingParamToFunction of function_info
     | AssigningToField of Fieldname.t
@@ -45,7 +49,3 @@ module ReportableViolation : sig
   (** Given context around violation, return error message together with the info where to put this
       message *)
 end
-
-val to_reportable_violation : NullsafeMode.t -> violation -> ReportableViolation.t option
-(** 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. *)

infer/src/nullsafe/DereferenceRule.ml

@@ -18,6 +18,11 @@ module ReportableViolation = struct
     | ArrayLengthAccess
   [@@deriving compare]
 
+  let from nullsafe_mode ({nullability} as violation) =
+    if Nullability.is_considered_nonnull ~nullsafe_mode nullability then None
+    else Some {nullsafe_mode; violation}
+
+
   let get_origin_opt ~nullable_object_descr origin =
     let should_show_origin =
       match nullable_object_descr with
@@ -116,8 +121,3 @@ let check nullability =
       Ok ()
   | _ ->
       Error {nullability}
-
-
-let to_reportable_violation nullsafe_mode ({nullability} as violation) =
-  if Nullability.is_considered_nonnull ~nullsafe_mode nullability then None
-  else Some ReportableViolation.{nullsafe_mode; violation}

infer/src/nullsafe/DereferenceRule.mli

@@ -27,6 +27,10 @@ module ReportableViolation : sig
     | ArrayLengthAccess
   [@@deriving compare]
 
+  val from : NullsafeMode.t -> violation -> t option
+  (** 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 -> Exceptions.severity
   (** Severity of the violation to be reported *)
 
@@ -40,7 +44,3 @@ module ReportableViolation : sig
   (** Given context around violation, return error message together with the info where to put this
       message *)
 end
-
-val to_reportable_violation : NullsafeMode.t -> violation -> ReportableViolation.t option
-(** 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. *)

infer/src/nullsafe/InheritanceRule.ml

@@ -18,6 +18,19 @@ module ReportableViolation = struct
     | InconsistentReturn
   [@@deriving compare]
 
+  let from nullsafe_mode ({base; overridden} as violation) =
+    if
+      Nullability.is_nonnullish base && Nullability.is_nonnullish overridden
+      (* When both nullabilities are kind-of non-nullable we don't want to raise the
+         issue. Without this suppression there will be a lot of non-actionable issues
+         raised for classes in one [NullsafeMode] inheriting from classes in the other
+         [NullsafeMode]. *)
+      (* TODO(T62521386): consider using caller context when determining nullability to get
+         rid of white-lists. *)
+    then None
+    else Some {nullsafe_mode; violation}
+
+
   let is_java_lang_object_equals = function
     | Procname.Java java_procname -> (
       match
@@ -85,16 +98,3 @@ let check type_role ~base ~overridden =
         (base, overridden)
   in
   Result.ok_if_true (Nullability.is_subtype ~subtype ~supertype) ~error:{base; overridden}
-
-
-let to_reportable_violation nullsafe_mode ({base; overridden} as violation) =
-  if
-    Nullability.is_nonnullish base && Nullability.is_nonnullish overridden
-    (* When both nullabilities are kind-of non-nullable we don't want to raise the
-       issue. Without this suppression there will be a lot of non-actionable issues
-       raised for classes in one [NullsafeMode] inheriting from classes in the other
-       [NullsafeMode]. *)
-    (* TODO(T62521386): consider using caller context when determining nullability to get
-       rid of white-lists. *)
-  then None
-  else Some ReportableViolation.{nullsafe_mode; violation}

infer/src/nullsafe/InheritanceRule.mli

@@ -34,6 +34,10 @@ module ReportableViolation : sig
     | InconsistentReturn
   [@@deriving compare]
 
+  val from : NullsafeMode.t -> violation -> t option
+  (** 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 -> Exceptions.severity
   (** Severity of the violation to be reported *)
 
@@ -42,7 +46,3 @@ module ReportableViolation : sig
   (** Given context around violation, return error message together with the info where to put this
       message *)
 end
-
-val to_reportable_violation : NullsafeMode.t -> violation -> ReportableViolation.t option
-(** 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. *)

infer/src/nullsafe/typeErr.ml

@@ -248,7 +248,7 @@ let get_error_info_if_reportable ~nullsafe_mode err_instance =
         , NullsafeMode.severity nullsafe_mode )
   | Bad_assignment {rhs_origin; assignment_location; assignment_type; assignment_violation} ->
       let+ reportable_violation =
-        AssignmentRule.to_reportable_violation nullsafe_mode assignment_violation
+        AssignmentRule.ReportableViolation.from nullsafe_mode assignment_violation
       in
       let description, issue_type, error_location =
         AssignmentRule.ReportableViolation.get_description ~assignment_location assignment_type
@@ -263,7 +263,7 @@ let get_error_info_if_reportable ~nullsafe_mode err_instance =
       ; dereference_type
       ; nullable_object_origin } ->
       let+ reportable_violation =
-        DereferenceRule.to_reportable_violation nullsafe_mode dereference_violation
+        DereferenceRule.ReportableViolation.from nullsafe_mode dereference_violation
       in
       let description, issue_type, error_location =
         DereferenceRule.ReportableViolation.get_description reportable_violation
@@ -274,7 +274,7 @@ let get_error_info_if_reportable ~nullsafe_mode err_instance =
   | Inconsistent_subclass
       {inheritance_violation; violation_type; base_proc_name; overridden_proc_name} ->
       let+ reportable_violation =
-        InheritanceRule.to_reportable_violation nullsafe_mode inheritance_violation
+        InheritanceRule.ReportableViolation.from nullsafe_mode inheritance_violation
       in
       ( InheritanceRule.ReportableViolation.get_description reportable_violation violation_type
           ~base_proc_name ~overridden_proc_name