From 76177070456f38e2d7c357d4f178217b4660cb4b Mon Sep 17 00:00:00 2001 From: Mitya Lyubarskiy Date: Wed, 18 Mar 2020 08:18:50 -0700 Subject: [PATCH] [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 | 28 +++++++++++++------------- infer/src/nullsafe/AssignmentRule.mli | 8 ++++---- infer/src/nullsafe/DereferenceRule.ml | 10 ++++----- infer/src/nullsafe/DereferenceRule.mli | 8 ++++---- infer/src/nullsafe/InheritanceRule.ml | 26 ++++++++++++------------ infer/src/nullsafe/InheritanceRule.mli | 8 ++++---- infer/src/nullsafe/typeErr.ml | 6 +++--- 7 files changed, 47 insertions(+), 47 deletions(-) diff --git a/infer/src/nullsafe/AssignmentRule.ml b/infer/src/nullsafe/AssignmentRule.ml index 097e8c8c6..40a14837b 100644 --- a/infer/src/nullsafe/AssignmentRule.ml +++ b/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} diff --git a/infer/src/nullsafe/AssignmentRule.mli b/infer/src/nullsafe/AssignmentRule.mli index 3886aa4c4..ec078c4b9 100644 --- a/infer/src/nullsafe/AssignmentRule.mli +++ b/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. *) diff --git a/infer/src/nullsafe/DereferenceRule.ml b/infer/src/nullsafe/DereferenceRule.ml index b10b0082b..206caf4d1 100644 --- a/infer/src/nullsafe/DereferenceRule.ml +++ b/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} diff --git a/infer/src/nullsafe/DereferenceRule.mli b/infer/src/nullsafe/DereferenceRule.mli index 4619487cc..f298b0f77 100644 --- a/infer/src/nullsafe/DereferenceRule.mli +++ b/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. *) diff --git a/infer/src/nullsafe/InheritanceRule.ml b/infer/src/nullsafe/InheritanceRule.ml index 34a1a39e9..64e3fc92d 100644 --- a/infer/src/nullsafe/InheritanceRule.ml +++ b/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} diff --git a/infer/src/nullsafe/InheritanceRule.mli b/infer/src/nullsafe/InheritanceRule.mli index bf3fbc23d..b30c5f0cf 100644 --- a/infer/src/nullsafe/InheritanceRule.mli +++ b/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. *) diff --git a/infer/src/nullsafe/typeErr.ml b/infer/src/nullsafe/typeErr.ml index 6fc670c78..488d295ed 100644 --- a/infer/src/nullsafe/typeErr.ml +++ b/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