diff --git a/infer/src/nullsafe/InheritanceRule.ml b/infer/src/nullsafe/InheritanceRule.ml index 2dd0c056c..34a1a39e9 100644 --- a/infer/src/nullsafe/InheritanceRule.ml +++ b/infer/src/nullsafe/InheritanceRule.ml @@ -6,22 +6,75 @@ *) open! IStd -type violation = {nullsafe_mode: NullsafeMode.t; base: Nullability.t; overridden: Nullability.t} -[@@deriving compare] +type violation = {base: Nullability.t; overridden: Nullability.t} [@@deriving compare] type type_role = Param | Ret -let is_whitelisted_violation ~subtype ~supertype = - (* 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. *) - Nullability.is_nonnullish subtype && Nullability.is_nonnullish supertype +module ReportableViolation = struct + type t = {nullsafe_mode: NullsafeMode.t; violation: violation} + 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 - (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 + Result.ok_if_true (Nullability.is_subtype ~subtype ~supertype) ~error:{base; overridden} -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} diff --git a/infer/src/nullsafe/InheritanceRule.mli b/infer/src/nullsafe/InheritanceRule.mli index a80ef473b..bf3fbc23d 100644 --- a/infer/src/nullsafe/InheritanceRule.mli +++ b/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 : - nullsafe_mode:NullsafeMode.t - -> type_role - -> base:Nullability.t - -> overridden:Nullability.t - -> (unit, violation) result - -val violation_description : - violation - -> violation_type - -> base_proc_name:Procname.t - -> overridden_proc_name:Procname.t - -> string - -val violation_severity : violation -> Exceptions.severity +val check : type_role -> base:Nullability.t -> overridden:Nullability.t -> (unit, violation) result +(** See description of the rule in the header of the file. Note that formal fact of violation might + or might not be reported to the user, depending on the mode. See [to_reportable_violation] *) + +(** Violation that needs to be reported to the user. *) +module ReportableViolation : sig + type t + + type violation_type = + | InconsistentParam of {param_description: string; param_position: int} + | InconsistentReturn + [@@deriving compare] + + val get_severity : t -> 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. *) diff --git a/infer/src/nullsafe/eradicateChecks.ml b/infer/src/nullsafe/eradicateChecks.ml index 866857a7a..300885543 100644 --- a/infer/src/nullsafe/eradicateChecks.ml +++ b/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 }) diff --git a/infer/src/nullsafe/typeErr.ml b/infer/src/nullsafe/typeErr.ml index b4d662066..ab80a6736 100644 --- a/infer/src/nullsafe/typeErr.ml +++ b/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 - ( InheritanceRule.violation_description inheritance_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.violation_severity inheritance_violation ) + let+ reportable_violation = + InheritanceRule.to_reportable_violation nullsafe_mode inheritance_violation + in + ( 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 ) let report_now_if_reportable (st_report_error : st_report_error) err_instance ~nullsafe_mode loc diff --git a/infer/src/nullsafe/typeErr.mli b/infer/src/nullsafe/typeErr.mli index 983cb7d8d..f1b38e218 100644 --- a/infer/src/nullsafe/typeErr.mli +++ b/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} diff --git a/infer/tests/codetoanalyze/java/nullsafe-default/issues.exp b/infer/tests/codetoanalyze/java/nullsafe-default/issues.exp index 29295a845..67e8804a7 100644 --- a/infer/tests/codetoanalyze/java/nullsafe-default/issues.exp +++ b/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, []