From 58e897edd0d37676cfcb3fc561725bd5c4364dd5 Mon Sep 17 00:00:00 2001 From: Mitya Lyubarskiy Date: Thu, 17 Sep 2020 05:08:13 -0700 Subject: [PATCH] [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 | 13 +- infer/src/nullsafe/AssignmentRule.mli | 7 +- infer/src/nullsafe/DereferenceRule.ml | 13 +- infer/src/nullsafe/DereferenceRule.mli | 7 +- infer/src/nullsafe/EradicateReporting.ml | 12 +- infer/src/nullsafe/EradicateReporting.mli | 5 +- infer/src/nullsafe/ErrorRenderingUtils.ml | 3 +- infer/src/nullsafe/ErrorRenderingUtils.mli | 8 +- infer/src/nullsafe/InheritanceRule.ml | 80 ++++++---- infer/src/nullsafe/InheritanceRule.mli | 9 +- infer/src/nullsafe/NullsafeIssue.ml | 24 +++ infer/src/nullsafe/NullsafeIssue.mli | 22 +++ infer/src/nullsafe/eradicateChecks.ml | 30 ++-- infer/src/nullsafe/immutableChecker.ml | 3 +- infer/src/nullsafe/typeCheck.ml | 3 +- infer/src/nullsafe/typeErr.ml | 150 +++++++++--------- infer/src/nullsafe/typeErr.mli | 14 +- .../nullsafe/unit/AggregatedSummariesTest.ml | 1 + 18 files changed, 229 insertions(+), 175 deletions(-) create mode 100644 infer/src/nullsafe/NullsafeIssue.ml create mode 100644 infer/src/nullsafe/NullsafeIssue.mli diff --git a/infer/src/nullsafe/AssignmentRule.ml b/infer/src/nullsafe/AssignmentRule.ml index 761e45284..cbe578614 100644 --- a/infer/src/nullsafe/AssignmentRule.ml +++ b/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 diff --git a/infer/src/nullsafe/AssignmentRule.mli b/infer/src/nullsafe/AssignmentRule.mli index bd7fea9f9..780f57529 100644 --- a/infer/src/nullsafe/AssignmentRule.mli +++ b/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 diff --git a/infer/src/nullsafe/DereferenceRule.ml b/infer/src/nullsafe/DereferenceRule.ml index 647a57d5f..08db26772 100644 --- a/infer/src/nullsafe/DereferenceRule.ml +++ b/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 = diff --git a/infer/src/nullsafe/DereferenceRule.mli b/infer/src/nullsafe/DereferenceRule.mli index 732ae357d..0f92e2a74 100644 --- a/infer/src/nullsafe/DereferenceRule.mli +++ b/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 diff --git a/infer/src/nullsafe/EradicateReporting.ml b/infer/src/nullsafe/EradicateReporting.ml index 54175cb50..bcff37fd5 100644 --- a/infer/src/nullsafe/EradicateReporting.ml +++ b/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 diff --git a/infer/src/nullsafe/EradicateReporting.mli b/infer/src/nullsafe/EradicateReporting.mli index 6c3d30c1f..b5eaf9423 100644 --- a/infer/src/nullsafe/EradicateReporting.mli +++ b/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 diff --git a/infer/src/nullsafe/ErrorRenderingUtils.ml b/infer/src/nullsafe/ErrorRenderingUtils.ml index 3e9e83543..8bcdcc1d9 100644 --- a/infer/src/nullsafe/ErrorRenderingUtils.ml +++ b/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 = diff --git a/infer/src/nullsafe/ErrorRenderingUtils.mli b/infer/src/nullsafe/ErrorRenderingUtils.mli index 6e6d40033..911eb5bc6 100644 --- a/infer/src/nullsafe/ErrorRenderingUtils.mli +++ b/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 diff --git a/infer/src/nullsafe/InheritanceRule.ml b/infer/src/nullsafe/InheritanceRule.ml index c1ad3d1f2..4eda4fdf6 100644 --- a/infer/src/nullsafe/InheritanceRule.ml +++ b/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 = diff --git a/infer/src/nullsafe/InheritanceRule.mli b/infer/src/nullsafe/InheritanceRule.mli index 3a7175ade..123297755 100644 --- a/infer/src/nullsafe/InheritanceRule.mli +++ b/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 diff --git a/infer/src/nullsafe/NullsafeIssue.ml b/infer/src/nullsafe/NullsafeIssue.ml new file mode 100644 index 000000000..37cc56e43 --- /dev/null +++ b/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 diff --git a/infer/src/nullsafe/NullsafeIssue.mli b/infer/src/nullsafe/NullsafeIssue.mli new file mode 100644 index 000000000..c8cb5e100 --- /dev/null +++ b/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 diff --git a/infer/src/nullsafe/eradicateChecks.ml b/infer/src/nullsafe/eradicateChecks.ml index 5ea4de977..8f9f35961 100644 --- a/infer/src/nullsafe/eradicateChecks.ml +++ b/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 diff --git a/infer/src/nullsafe/immutableChecker.ml b/infer/src/nullsafe/immutableChecker.ml index 9ceac3267..eb3cd62d2 100644 --- a/infer/src/nullsafe/immutableChecker.ml +++ b/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 -> diff --git a/infer/src/nullsafe/typeCheck.ml b/infer/src/nullsafe/typeCheck.ml index dbdf89355..38b534de5 100644 --- a/infer/src/nullsafe/typeCheck.ml +++ b/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 diff --git a/infer/src/nullsafe/typeErr.ml b/infer/src/nullsafe/typeErr.ml index 920681c48..34c7922c2 100644 --- a/infer/src/nullsafe/typeErr.ml +++ b/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 diff --git a/infer/src/nullsafe/typeErr.mli b/infer/src/nullsafe/typeErr.mli index 04a7c67a4..e793847fd 100644 --- a/infer/src/nullsafe/typeErr.mli +++ b/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. *) diff --git a/infer/src/nullsafe/unit/AggregatedSummariesTest.ml b/infer/src/nullsafe/unit/AggregatedSummariesTest.ml index b5f3ad886..781e7205f 100644 --- a/infer/src/nullsafe/unit/AggregatedSummariesTest.ml +++ b/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