[nullsafe] Decouple nullsafe mode from issue calculation, Inheritance Rule

Summary:
This diff continues work in D20491716.
This time for Inheritance Rule.

Reviewed By: jvillard

Differential Revision: D20492889

fbshipit-source-id: c4dfd95c3

infer/src/nullsafe/InheritanceRule.ml

@@ -6,22 +6,75 @@
  *)
 open! IStd
 
-type violation = {nullsafe_mode: NullsafeMode.t; base: Nullability.t; overridden: Nullability.t}
+type violation = {base: Nullability.t; overridden: Nullability.t} [@@deriving compare]
-[@@deriving compare]
 
 type type_role = Param | Ret
 
-let is_whitelisted_violation ~subtype ~supertype =
+module ReportableViolation = struct
-  (* When both nullabilities are kind-of non-nullable we don't want to raise the
+  type t = {nullsafe_mode: NullsafeMode.t; violation: violation}
-     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. *)
-  Nullability.is_nonnullish subtype && Nullability.is_nonnullish supertype
 
+  type violation_type =
+    | InconsistentParam of {param_description: string; param_position: int}
+    | InconsistentReturn
+  [@@deriving compare]
 
-let check ~nullsafe_mode type_role ~base ~overridden =
+  let is_java_lang_object_equals = function
+    | Procname.Java java_procname -> (
+      match
+        (Procname.Java.get_class_name java_procname, Procname.Java.get_method java_procname)
+      with
+      | "java.lang.Object", "equals" ->
+          true
+      | _ ->
+          false )
+    | _ ->
+        false
+
+
+  let get_description _ violation_type ~base_proc_name ~overridden_proc_name =
+    let module MF = MarkupFormatter in
+    let base_method_descr = Procname.to_simplified_string ~withclass:true base_proc_name in
+    let overridden_method_descr =
+      Procname.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 *)
+          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
+end
+
+let check type_role ~base ~overridden =
   let subtype, supertype =
     match type_role with
     | Ret ->
@@ -31,65 +84,17 @@ let check ~nullsafe_mode type_role ~base ~overridden =
         (* contravariance for param *)
         (base, overridden)
   in
-  Result.ok_if_true
+  Result.ok_if_true (Nullability.is_subtype ~subtype ~supertype) ~error:{base; overridden}
-    (Nullability.is_subtype ~subtype ~supertype || is_whitelisted_violation ~subtype ~supertype)
-    ~error:{nullsafe_mode; base; overridden}
-
-
-type violation_type =
-  | InconsistentParam of {param_description: string; param_position: int}
-  | InconsistentReturn
-[@@deriving compare]
-
-let is_java_lang_object_equals = function
-  | Procname.Java java_procname -> (
-    match (Procname.Java.get_class_name java_procname, Procname.Java.get_method java_procname) with
-    | "java.lang.Object", "equals" ->
-        true
-    | _ ->
-        false )
-  | _ ->
-      false
-
-
-let violation_description _ violation_type ~base_proc_name ~overridden_proc_name =
-  let module MF = MarkupFormatter in
-  let base_method_descr = Procname.to_simplified_string ~withclass:true base_proc_name in
-  let overridden_method_descr =
-    Procname.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 *)
-        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 violation_severity {nullsafe_mode} = NullsafeMode.severity nullsafe_mode
+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

@@ -19,25 +19,30 @@ open! IStd
 
 type violation [@@deriving compare]
 
-type violation_type =
-  | InconsistentParam of {param_description: string; param_position: int}
-  | InconsistentReturn
-[@@deriving compare]
-
 type type_role = Param | Ret
 
-val check :
+val check : type_role -> base:Nullability.t -> overridden:Nullability.t -> (unit, violation) result
-     nullsafe_mode:NullsafeMode.t
+(** See description of the rule in the header of the file. Note that formal fact of violation might
-  -> type_role
+    or might not be reported to the user, depending on the mode. See [to_reportable_violation] *)
-  -> base:Nullability.t
+
-  -> overridden:Nullability.t
+(** Violation that needs to be reported to the user. *)
-  -> (unit, violation) result
+module ReportableViolation : sig
-
+  type t
-val violation_description :
+
-     violation
+  type violation_type =
-  -> violation_type
+    | InconsistentParam of {param_description: string; param_position: int}
-  -> base_proc_name:Procname.t
+    | InconsistentReturn
-  -> overridden_proc_name:Procname.t
+  [@@deriving compare]
-  -> string
+
-
+  val get_severity : t -> Exceptions.severity
-val violation_severity : violation -> Exceptions.severity
+  (** Severity of the violation to be reported *)
+
+  val get_description :
+    t -> violation_type -> base_proc_name:Procname.t -> overridden_proc_name:Procname.t -> string
+  (** 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/eradicateChecks.ml

@@ -460,12 +460,12 @@ let check_inheritance_rule_for_return find_canonical_duplicate tenv loc ~nullsaf
     ~base_proc_name ~overridden_proc_name ~overridden_proc_desc ~base_nullability
     ~overridden_nullability =
   Result.iter_error
-    (InheritanceRule.check ~nullsafe_mode InheritanceRule.Ret ~base:base_nullability
+    (InheritanceRule.check InheritanceRule.Ret ~base:base_nullability
        ~overridden:overridden_nullability) ~f:(fun inheritance_violation ->
       register_error tenv find_canonical_duplicate
         (TypeErr.Inconsistent_subclass
            { inheritance_violation
-           ; violation_type= InheritanceRule.InconsistentReturn
+           ; violation_type= InheritanceRule.ReportableViolation.InconsistentReturn
            ; overridden_proc_name
            ; base_proc_name })
         None ~nullsafe_mode loc overridden_proc_desc )
@@ -475,13 +475,13 @@ let check_inheritance_rule_for_param find_canonical_duplicate tenv loc ~nullsafe
     ~overridden_param_name ~base_proc_name ~overridden_proc_name ~overridden_proc_desc
     ~param_position ~base_nullability ~overridden_nullability =
   Result.iter_error
-    (InheritanceRule.check ~nullsafe_mode InheritanceRule.Param ~base:base_nullability
+    (InheritanceRule.check InheritanceRule.Param ~base:base_nullability
        ~overridden:overridden_nullability) ~f:(fun inheritance_violation ->
       register_error tenv find_canonical_duplicate
         (TypeErr.Inconsistent_subclass
            { inheritance_violation
            ; violation_type=
-               InheritanceRule.InconsistentParam
+               InheritanceRule.ReportableViolation.InconsistentParam
                  {param_position; param_description= Mangled.to_string overridden_param_name}
            ; base_proc_name
            ; overridden_proc_name })

infer/src/nullsafe/typeErr.ml

@@ -62,7 +62,7 @@ type err_instance =
       {is_always_true: bool; condition_descr: string option; nonnull_origin: TypeOrigin.t}
   | Inconsistent_subclass of
       { inheritance_violation: InheritanceRule.violation
-      ; violation_type: InheritanceRule.violation_type
+      ; violation_type: InheritanceRule.ReportableViolation.violation_type
       ; base_proc_name: Procname.t
       ; overridden_proc_name: Procname.t }
   | Field_not_initialized of {nullsafe_mode: NullsafeMode.t; field_name: Fieldname.t}
@@ -273,16 +273,18 @@ let get_error_info_if_reportable ~nullsafe_mode err_instance =
       (description, issue_type, Some error_location, severity)
   | Inconsistent_subclass
       {inheritance_violation; violation_type; base_proc_name; overridden_proc_name} ->
-      Some
+      let+ reportable_violation =
-        ( InheritanceRule.violation_description inheritance_violation violation_type ~base_proc_name
+        InheritanceRule.to_reportable_violation nullsafe_mode inheritance_violation
-            ~overridden_proc_name
+      in
-        , ( match violation_type with
+      ( InheritanceRule.ReportableViolation.get_description reportable_violation violation_type
-          | InconsistentReturn ->
+          ~base_proc_name ~overridden_proc_name
-              IssueType.eradicate_inconsistent_subclass_return_annotation
+      , ( match violation_type with
-          | InconsistentParam _ ->
+        | InconsistentReturn ->
-              IssueType.eradicate_inconsistent_subclass_parameter_annotation )
+            IssueType.eradicate_inconsistent_subclass_return_annotation
-        , None
+        | InconsistentParam _ ->
-        , InheritanceRule.violation_severity inheritance_violation )
+            IssueType.eradicate_inconsistent_subclass_parameter_annotation )
+      , None
+      , InheritanceRule.ReportableViolation.get_severity reportable_violation )
 
 
 let report_now_if_reportable (st_report_error : st_report_error) err_instance ~nullsafe_mode loc

infer/src/nullsafe/typeErr.mli

@@ -38,7 +38,7 @@ type err_instance =
       {is_always_true: bool; condition_descr: string option; nonnull_origin: TypeOrigin.t}
   | Inconsistent_subclass of
       { inheritance_violation: InheritanceRule.violation
-      ; violation_type: InheritanceRule.violation_type
+      ; violation_type: InheritanceRule.ReportableViolation.violation_type
       ; base_proc_name: Procname.t
       ; overridden_proc_name: Procname.t }
   | Field_not_initialized of {nullsafe_mode: NullsafeMode.t; field_name: Fieldname.t}

infer/tests/codetoanalyze/java/nullsafe-default/issues.exp

@@ -372,7 +372,7 @@ codetoanalyze/java/nullsafe-default/TrueFalseOnNull.java, Linters_dummy_method,
 codetoanalyze/java/nullsafe-default/TrueFalseOnNull.java, Linters_dummy_method, 1, ERADICATE_META_CLASS_IS_CLEAN, no_bucket, INFO, []
 codetoanalyze/java/nullsafe-default/TrueFalseOnNull.java, Linters_dummy_method, 1, ERADICATE_META_CLASS_IS_CLEAN, no_bucket, INFO, []
 codetoanalyze/java/nullsafe-default/TrueFalseOnNull.java, Linters_dummy_method, 1, ERADICATE_META_CLASS_IS_CLEAN, no_bucket, INFO, []
-codetoanalyze/java/nullsafe-default/TrueFalseOnNull.java, Linters_dummy_method, 1, ERADICATE_META_CLASS_IS_CLEAN, no_bucket, INFO, []
+codetoanalyze/java/nullsafe-default/TrueFalseOnNull.java, Linters_dummy_method, 1, ERADICATE_META_CLASS_NEEDS_FIXING, no_bucket, INFO, []
 codetoanalyze/java/nullsafe-default/TrueFalseOnNull.java, Linters_dummy_method, 1, ERADICATE_META_CLASS_NEEDS_FIXING, no_bucket, INFO, []
 codetoanalyze/java/nullsafe-default/TrueFalseOnNull.java, Linters_dummy_method, 1, ERADICATE_META_CLASS_NEEDS_FIXING, no_bucket, INFO, []
 codetoanalyze/java/nullsafe-default/TrueFalseOnNull.java, Linters_dummy_method, 1, ERADICATE_META_CLASS_NEEDS_FIXING, no_bucket, INFO, []