From 92f765a94852d8e1418bfea19cd203298de4b97f Mon Sep 17 00:00:00 2001 From: Mitya Lyubarskiy Date: Mon, 21 Oct 2019 04:40:24 -0700 Subject: [PATCH] [nullsafe] Unify issue types Summary: 1. For each Nullsafe Rule, lets have a dedicated IssueType. 2. Split error reporting to three subfunctions: description, field type, and infer issue. This allows to introduce additional capabilities in a consolidated manner. Example of such capabilities is should we hide or show an error, what should be error severity depending on strictness mode, and how exactly the error should be reported depending on how exactly nullability is violated. Reviewed By: artempyanykh Differential Revision: D17977887 fbshipit-source-id: 860d67d2f --- infer/src/nullsafe/eradicateChecks.ml | 44 ++-- infer/src/nullsafe/typeErr.ml | 347 +++++++++++++------------- infer/src/nullsafe/typeErr.mli | 40 +-- 3 files changed, 226 insertions(+), 205 deletions(-) diff --git a/infer/src/nullsafe/eradicateChecks.ml b/infer/src/nullsafe/eradicateChecks.ml index b8e0e6536..a7c318e62 100644 --- a/infer/src/nullsafe/eradicateChecks.ml +++ b/infer/src/nullsafe/eradicateChecks.ml @@ -156,9 +156,9 @@ let check_field_assignment tenv find_canonical_duplicate curr_pdesc node instr_r && not (field_is_in_cleanup_context ()) in if should_report_nullable then - let origin_descr = InferredNullability.descr_origin inferred_nullability_rhs in + let rhs_origin_descr = InferredNullability.descr_origin inferred_nullability_rhs in report_error tenv find_canonical_duplicate - (TypeErr.Field_annotation_inconsistent (fname, origin_descr)) + (TypeErr.Bad_assignment {rhs_origin_descr; assignment_type= TypeErr.AssigningToAField fname}) (Some instr_ref) loc curr_pdesc @@ -298,7 +298,7 @@ let check_constructor_initialization tenv find_canonical_duplicate curr_construc ~rhs_upper_bound:(field_nullability_upper_bound_over_all_typestates ()) then report_error tenv find_canonical_duplicate - (TypeErr.Field_over_annotated (field_name, curr_constructor_pname)) + (TypeErr.Over_annotation (TypeErr.FieldOverAnnotedAsNullable field_name)) None loc curr_constructor_pdesc ) in List.iter ~f:do_field fields @@ -314,9 +314,10 @@ let check_return_not_nullable tenv find_canonical_duplicate loc curr_pname curr_ let lhs = AnnotatedNullability.get_nullability ret_signature.ret_annotated_type.nullability in let rhs = InferredNullability.get_nullability ret_inferred_nullability in if not (NullsafeRules.passes_assignment_rule ~lhs ~rhs) then + let rhs_origin_descr = InferredNullability.descr_origin ret_inferred_nullability in report_error tenv find_canonical_duplicate - (TypeErr.Return_annotation_inconsistent - (curr_pname, InferredNullability.descr_origin ret_inferred_nullability)) + (TypeErr.Bad_assignment + {rhs_origin_descr; assignment_type= TypeErr.ReturningFromAFunction curr_pname}) None loc curr_pdesc @@ -330,7 +331,8 @@ let check_return_overrannotated tenv find_canonical_duplicate loc curr_pname cur *) let rhs_upper_bound = InferredNullability.get_nullability ret_inferred_nullability in if NullsafeRules.is_overannotated ~lhs ~rhs_upper_bound then - report_error tenv find_canonical_duplicate (TypeErr.Return_over_annotated curr_pname) None loc + report_error tenv find_canonical_duplicate + (TypeErr.Over_annotation (TypeErr.ReturnOverAnnotatedAsNullable curr_pname)) None loc curr_pdesc @@ -386,20 +388,22 @@ type resolved_param = let check_call_parameters tenv find_canonical_duplicate curr_pdesc node callee_attributes resolved_params loc instr_ref : unit = let callee_pname = callee_attributes.ProcAttributes.proc_name in - let check {num= param_num; formal; actual= orig_e2, nullability_actual} = + let check {num= param_position; formal; actual= orig_e2, nullability_actual} = let report () = - let description = + let param_description = match explain_expr tenv node orig_e2 with | Some descr -> descr | None -> "formal parameter " ^ Mangled.to_string formal.mangled in - let origin_descr = InferredNullability.descr_origin nullability_actual in - let callee_loc = callee_attributes.ProcAttributes.loc in + let rhs_origin_descr = InferredNullability.descr_origin nullability_actual in report_error tenv find_canonical_duplicate - (TypeErr.Parameter_annotation_inconsistent - (description, param_num, callee_pname, callee_loc, origin_descr)) + (TypeErr.Bad_assignment + { rhs_origin_descr + ; assignment_type= + TypeErr.PassingParamToAFunction + {param_description; param_position; function_procname= callee_pname} }) (Some instr_ref) loc curr_pdesc in if PatternMatch.type_is_class formal.param_annotated_type.typ then @@ -432,7 +436,10 @@ let check_inheritance_rule_for_return find_canonical_duplicate tenv loc ~base_pr ~overridden:overridden_nullability) then report_error tenv find_canonical_duplicate - (TypeErr.Inconsistent_subclass_return_annotation (overridden_proc_name, base_proc_name)) + (TypeErr.Inconsistent_subclass + { overridden_proc_name + ; base_proc_name + ; inconsistent_subclass_type= TypeErr.InconsistentReturn }) None loc overridden_proc_desc @@ -445,11 +452,12 @@ let check_inheritance_rule_for_param find_canonical_duplicate tenv loc ~overridd ~overridden:overridden_nullability) then report_error tenv find_canonical_duplicate - (TypeErr.Inconsistent_subclass_parameter_annotation - ( Mangled.to_string overridden_param_name - , param_position - , overridden_proc_name - , base_proc_name )) + (TypeErr.Inconsistent_subclass + { base_proc_name + ; overridden_proc_name + ; inconsistent_subclass_type= + TypeErr.InconsistentParam + {param_position; param_description= Mangled.to_string overridden_param_name} }) None loc overridden_proc_desc diff --git a/infer/src/nullsafe/typeErr.ml b/infer/src/nullsafe/typeErr.ml index 0dccefd0e..841c6fb7c 100644 --- a/infer/src/nullsafe/typeErr.ml +++ b/infer/src/nullsafe/typeErr.ml @@ -62,34 +62,39 @@ type origin_descr = string * Location.t option * AnnotatedSignature.t option (* ignore origin descr *) let compare_origin_descr _ _ = 0 -type parameter_not_nullable = - string - * (* description *) - int - * (* parameter number *) - Typ.Procname.t - * Location.t - * (* callee location *) - origin_descr -[@@deriving compare] - (** Instance of an error *) type err_instance = | Condition_redundant of (bool * string option) - | Inconsistent_subclass_return_annotation of Typ.Procname.t * Typ.Procname.t - | Inconsistent_subclass_parameter_annotation of string * int * Typ.Procname.t * Typ.Procname.t + | Inconsistent_subclass of + { base_proc_name: Typ.Procname.t + ; overridden_proc_name: Typ.Procname.t + ; inconsistent_subclass_type: inconsistent_subclass_type } | Field_not_initialized of Typ.Fieldname.t * Typ.Procname.t - | Field_annotation_inconsistent of Typ.Fieldname.t * origin_descr - | Field_over_annotated of Typ.Fieldname.t * Typ.Procname.t + | Over_annotation of over_annotation_type | Nullable_dereference of { nullable_object_descr: string option ; dereference_type: dereference_type ; origin_descr: origin_descr } - | Parameter_annotation_inconsistent of parameter_not_nullable - | Return_annotation_inconsistent of Typ.Procname.t * origin_descr - | Return_over_annotated of Typ.Procname.t + | Bad_assignment of {rhs_origin_descr: origin_descr; assignment_type: assignment_type} [@@deriving compare] +and inconsistent_subclass_type = + | InconsistentParam of {param_description: string; param_position: int} + | InconsistentReturn + +and over_annotation_type = + | FieldOverAnnotedAsNullable of Typ.Fieldname.t + | ReturnOverAnnotatedAsNullable of Typ.Procname.t + (** Return value of a method can be made non-nullable *) + +and assignment_type = + | PassingParamToAFunction of + { param_description: string + ; param_position: int + ; function_procname: Typ.Procname.t } + | AssigningToAField of Typ.Fieldname.t + | ReturningFromAFunction of Typ.Procname.t + and dereference_type = | MethodCall of Typ.Procname.t | AccessToField of Typ.Fieldname.t @@ -122,22 +127,14 @@ let get_forall = function true | Field_not_initialized _ -> false - | Field_annotation_inconsistent _ -> - false - | Field_over_annotated _ -> + | Over_annotation _ -> false - | Inconsistent_subclass_return_annotation _ -> + | Bad_assignment _ -> false - | Inconsistent_subclass_parameter_annotation _ -> + | Inconsistent_subclass _ -> false | Nullable_dereference _ -> false - | Parameter_annotation_inconsistent _ -> - false - | Return_annotation_inconsistent _ -> - false - | Return_over_annotated _ -> - false (** Reset the always field of the forall erros in the node, so if they are not set again @@ -221,153 +218,163 @@ type st_report_error = -> string -> unit -(** Report an error right now. *) -let report_error_now tenv (st_report_error : st_report_error) err_instance loc pdesc : unit = - let pname = Procdesc.get_proc_name pdesc in +let get_infer_issue_type = function + | Condition_redundant _ -> + IssueType.eradicate_condition_redundant + | Over_annotation (FieldOverAnnotedAsNullable _) -> + IssueType.eradicate_field_over_annotated + | Over_annotation (ReturnOverAnnotatedAsNullable _) -> + IssueType.eradicate_return_over_annotated + | Field_not_initialized _ -> + IssueType.eradicate_field_not_initialized + | Bad_assignment {assignment_type= PassingParamToAFunction _} -> + IssueType.eradicate_parameter_not_nullable + | Bad_assignment {assignment_type= AssigningToAField _} -> + IssueType.eradicate_field_not_nullable + | Bad_assignment {assignment_type= ReturningFromAFunction _} -> + IssueType.eradicate_return_not_nullable + | Nullable_dereference _ -> + IssueType.eradicate_nullable_dereference + | Inconsistent_subclass {inconsistent_subclass_type= InconsistentReturn} -> + IssueType.eradicate_inconsistent_subclass_return_annotation + | Inconsistent_subclass {inconsistent_subclass_type= InconsistentParam _} -> + IssueType.eradicate_inconsistent_subclass_parameter_annotation + + +(* If an error is related to a particular field, we support suppressing the + error via a supress annotation placed near the field declaration *) +let get_field_name_for_error_suppressing = function + | Over_annotation (FieldOverAnnotedAsNullable field_name) -> + Some field_name + | Field_not_initialized (field_name, _) -> + Some field_name + | Condition_redundant _ + | Over_annotation (ReturnOverAnnotatedAsNullable _) + (* In case of assignment to a field, it should be fixed case by case for each assignment *) + | Bad_assignment _ + | Nullable_dereference _ + | Inconsistent_subclass _ -> + None + + +let get_error_description err_instance = let nullable_annotation = "@Nullable" in - let kind, description, field_name = - match err_instance with - | Condition_redundant (is_always_true, s_opt) -> - ( IssueType.eradicate_condition_redundant - , P.sprintf "The condition %s is always %b according to the existing annotations." - (Option.value s_opt ~default:"") is_always_true - , None ) - | Field_not_initialized (fn, pn) -> - let constructor_name = - if Typ.Procname.is_constructor pn then "the constructor" - else - match pn with - | Typ.Procname.Java pn_java -> - MF.monospaced_to_string (Typ.Procname.Java.get_method pn_java) - | _ -> - MF.monospaced_to_string (Typ.Procname.to_simplified_string pn) - in - ( IssueType.eradicate_field_not_initialized - , Format.asprintf "Field %a is not initialized in %s and is not declared %a" - MF.pp_monospaced - (Typ.Fieldname.to_simplified_string fn) - constructor_name MF.pp_monospaced nullable_annotation - , Some fn ) - | Field_annotation_inconsistent (fn, (origin_description, _, _)) -> - let kind_s, description = - ( IssueType.eradicate_field_not_nullable - , Format.asprintf "Field %a can be null but is not declared %a. %s" MF.pp_monospaced - (Typ.Fieldname.to_simplified_string fn) - MF.pp_monospaced nullable_annotation origin_description ) - in - (kind_s, description, None) - | Field_over_annotated (fn, pn) -> - let constructor_name = - if Typ.Procname.is_constructor pn then "the constructor" - else - match pn with - | Typ.Procname.Java pn_java -> - Typ.Procname.Java.get_method pn_java - | _ -> - Typ.Procname.to_simplified_string pn - in - ( IssueType.eradicate_field_over_annotated - , Format.asprintf "Field %a is always initialized in %s but is declared %a" - MF.pp_monospaced - (Typ.Fieldname.to_simplified_string fn) - constructor_name MF.pp_monospaced nullable_annotation - , Some fn ) - | Nullable_dereference {nullable_object_descr; dereference_type; origin_descr= origin_str, _, _} - -> - let nullable_object_descr = - match dereference_type with - | MethodCall _ | AccessToField _ -> ( - match nullable_object_descr with - | None -> - "Object" - (* Just describe an object itself *) - | Some descr -> - MF.monospaced_to_string descr ) - | ArrayLengthAccess | AccessByIndex _ -> ( - (* In Java, those operations can be applied only to arrays *) - match nullable_object_descr with - | None -> - "Array" - | Some descr -> - Format.sprintf "Array %s" (MF.monospaced_to_string descr) ) - in - let action_descr = - match dereference_type with - | MethodCall method_name -> - Format.sprintf "calling %s" - (MF.monospaced_to_string (Typ.Procname.to_simplified_string method_name)) - | AccessToField field_name -> - Format.sprintf "accessing field %s" - (MF.monospaced_to_string (Typ.Fieldname.to_simplified_string field_name)) - | AccessByIndex {index_desc} -> - Format.sprintf "accessing at index %s" (MF.monospaced_to_string index_desc) - | ArrayLengthAccess -> - "accessing its length" - in - let description = - Format.sprintf "%s is nullable and is not locally checked for null when %s. %s" - nullable_object_descr action_descr origin_str - in - (IssueType.eradicate_nullable_dereference, description, None) - | Parameter_annotation_inconsistent (s, n, pn, _, (origin_desc, _, _)) -> - let kind_s, description = - ( IssueType.eradicate_parameter_not_nullable - , Format.asprintf - "%a needs a non-null value in parameter %d but argument %a can be null. %s" - MF.pp_monospaced - (Typ.Procname.to_simplified_string ~withclass:true pn) - n MF.pp_monospaced s origin_desc ) - in - (kind_s, description, None) - | Return_annotation_inconsistent (pn, (origin_description, _, _)) -> - let kind_s, description = - ( IssueType.eradicate_return_not_nullable - , Format.asprintf "Method %a may return null but it is not annotated with %a. %s" - MF.pp_monospaced - (Typ.Procname.to_simplified_string pn) - MF.pp_monospaced nullable_annotation origin_description ) - in - (kind_s, description, None) - | Return_over_annotated pn -> - ( IssueType.eradicate_return_over_annotated - , Format.asprintf "Method %a is annotated with %a but never returns null." MF.pp_monospaced - (Typ.Procname.to_simplified_string pn) - MF.pp_monospaced nullable_annotation - , None ) - | Inconsistent_subclass_return_annotation (pn, opn) -> - ( IssueType.eradicate_inconsistent_subclass_return_annotation - , Format.asprintf "Method %a is annotated with %a but overrides unannotated method %a." - MF.pp_monospaced - (Typ.Procname.to_simplified_string ~withclass:true pn) - MF.pp_monospaced nullable_annotation MF.pp_monospaced - (Typ.Procname.to_simplified_string ~withclass:true opn) - , None ) - | Inconsistent_subclass_parameter_annotation (param_name, pos, pn, opn) -> - let translate_position = function - | 1 -> - "First" - | 2 -> - "Second" - | 3 -> - "Third" - | n -> - string_of_int n ^ "th" - in - ( IssueType.eradicate_inconsistent_subclass_parameter_annotation - , Format.asprintf + match err_instance with + | Condition_redundant (is_always_true, s_opt) -> + P.sprintf "The condition %s is always %b according to the existing annotations." + (Option.value s_opt ~default:"") is_always_true + | Over_annotation (FieldOverAnnotedAsNullable field_name) -> + Format.asprintf "Field %a is always initialized in the constructor but is declared %a" + MF.pp_monospaced + (Typ.Fieldname.to_simplified_string field_name) + MF.pp_monospaced nullable_annotation + | Over_annotation (ReturnOverAnnotatedAsNullable proc_name) -> + Format.asprintf "Method %a is annotated with %a but never returns null." MF.pp_monospaced + (Typ.Procname.to_simplified_string proc_name) + MF.pp_monospaced nullable_annotation + | Field_not_initialized (field_name, proc_name) -> + let constructor_name = + if Typ.Procname.is_constructor proc_name then "the constructor" + else + match proc_name with + | Typ.Procname.Java pn_java -> + MF.monospaced_to_string (Typ.Procname.Java.get_method pn_java) + | _ -> + MF.monospaced_to_string (Typ.Procname.to_simplified_string proc_name) + in + Format.asprintf "Field %a is not initialized in %s and is not declared %a" MF.pp_monospaced + (Typ.Fieldname.to_simplified_string field_name) + constructor_name MF.pp_monospaced nullable_annotation + | Bad_assignment {rhs_origin_descr= origin_descr_string, _, _; assignment_type} -> ( + match assignment_type with + | PassingParamToAFunction {param_description; param_position; function_procname} -> + Format.asprintf "%a needs a non-null value in parameter %d but argument %a can be null. %s" + MF.pp_monospaced + (Typ.Procname.to_simplified_string ~withclass:true function_procname) + param_position MF.pp_monospaced param_description origin_descr_string + | AssigningToAField field_name -> + Format.asprintf "Field %a can be null but is not declared %a. %s" MF.pp_monospaced + (Typ.Fieldname.to_simplified_string field_name) + MF.pp_monospaced nullable_annotation origin_descr_string + | ReturningFromAFunction function_proc_name -> + Format.asprintf "Method %a may return null but it is not annotated with %a. %s" + MF.pp_monospaced + (Typ.Procname.to_simplified_string function_proc_name) + MF.pp_monospaced nullable_annotation origin_descr_string ) + | Nullable_dereference {nullable_object_descr; dereference_type; origin_descr= origin_str, _, _} + -> + let nullable_object_descr = + match dereference_type with + | MethodCall _ | AccessToField _ -> ( + match nullable_object_descr with + | None -> + "Object" + (* Just describe an object itself *) + | Some descr -> + MF.monospaced_to_string descr ) + | ArrayLengthAccess | AccessByIndex _ -> ( + (* In Java, those operations can be applied only to arrays *) + match nullable_object_descr with + | None -> + "Array" + | Some descr -> + Format.sprintf "Array %s" (MF.monospaced_to_string descr) ) + in + let action_descr = + match dereference_type with + | MethodCall method_name -> + Format.sprintf "calling %s" + (MF.monospaced_to_string (Typ.Procname.to_simplified_string method_name)) + | AccessToField field_name -> + Format.sprintf "accessing field %s" + (MF.monospaced_to_string (Typ.Fieldname.to_simplified_string field_name)) + | AccessByIndex {index_desc} -> + Format.sprintf "accessing at index %s" (MF.monospaced_to_string index_desc) + | ArrayLengthAccess -> + "accessing its length" + in + Format.sprintf "%s is nullable and is not locally checked for null when %s. %s" + nullable_object_descr action_descr origin_str + | Inconsistent_subclass {base_proc_name; overridden_proc_name; inconsistent_subclass_type} -> ( + let base_method_descr = Typ.Procname.to_simplified_string ~withclass:true base_proc_name in + let overridden_method_descr = + Typ.Procname.to_simplified_string ~withclass:true overridden_proc_name + in + match inconsistent_subclass_type with + | InconsistentReturn -> + Format.asprintf "Method %a is annotated with %a but overrides unannotated method %a." + MF.pp_monospaced overridden_method_descr MF.pp_monospaced nullable_annotation + MF.pp_monospaced base_method_descr + | InconsistentParam {param_description; param_position} -> + 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 not %a but is declared %ain the parent class method \ %a." - (translate_position pos) MF.pp_monospaced param_name MF.pp_monospaced - (Typ.Procname.to_simplified_string ~withclass:true pn) + (translate_position param_position) + MF.pp_monospaced param_description MF.pp_monospaced overridden_method_descr MF.pp_monospaced nullable_annotation MF.pp_monospaced nullable_annotation - MF.pp_monospaced - (Typ.Procname.to_simplified_string ~withclass:true opn) - , None ) - in + MF.pp_monospaced base_method_descr ) + + +(** Report an error right now. *) +let report_error_now tenv (st_report_error : st_report_error) err_instance loc pdesc : unit = + let pname = Procdesc.get_proc_name pdesc in + let infer_issue_type = get_infer_issue_type err_instance in + let field_name = get_field_name_for_error_suppressing err_instance in + let err_description = get_error_description err_instance in let severity = Severity.err_instance_get_severity tenv err_instance in - st_report_error pname pdesc kind loc ~field_name + st_report_error pname pdesc infer_issue_type loc ~field_name ~exception_kind:(fun k d -> Exceptions.Eradicate (k, d)) - ?severity description + ?severity err_description (** Report an error unless is has been reported already, or unless it's a forall error diff --git a/infer/src/nullsafe/typeErr.mli b/infer/src/nullsafe/typeErr.mli index f1a190a1e..eb3a4a93f 100644 --- a/infer/src/nullsafe/typeErr.mli +++ b/infer/src/nullsafe/typeErr.mli @@ -34,33 +34,39 @@ type origin_descr = string * Location.t option * AnnotatedSignature.t option (* callee signature *) -type parameter_not_nullable = - string - * (* description *) - int - * (* parameter number *) - Typ.Procname.t - * Location.t - * (* callee location *) - origin_descr - (** Instance of an error *) type err_instance = | Condition_redundant of (bool * string option) - | Inconsistent_subclass_return_annotation of Typ.Procname.t * Typ.Procname.t - | Inconsistent_subclass_parameter_annotation of string * int * Typ.Procname.t * Typ.Procname.t + | Inconsistent_subclass of + { base_proc_name: Typ.Procname.t + ; overridden_proc_name: Typ.Procname.t + ; inconsistent_subclass_type: inconsistent_subclass_type } | Field_not_initialized of Typ.Fieldname.t * Typ.Procname.t - | Field_annotation_inconsistent of Typ.Fieldname.t * origin_descr - | Field_over_annotated of Typ.Fieldname.t * Typ.Procname.t + | Over_annotation of over_annotation_type | Nullable_dereference of { nullable_object_descr: string option ; dereference_type: dereference_type ; origin_descr: origin_descr } - | Parameter_annotation_inconsistent of parameter_not_nullable - | Return_annotation_inconsistent of Typ.Procname.t * origin_descr - | Return_over_annotated of Typ.Procname.t + | Bad_assignment of {rhs_origin_descr: origin_descr; assignment_type: assignment_type} [@@deriving compare] +and inconsistent_subclass_type = + | InconsistentParam of {param_description: string; param_position: int} + | InconsistentReturn + +and over_annotation_type = + | FieldOverAnnotedAsNullable of Typ.Fieldname.t + | ReturnOverAnnotatedAsNullable of Typ.Procname.t + (** Return value of a method can be made non-nullable *) + +and assignment_type = + | PassingParamToAFunction of + { param_description: string + ; param_position: int + ; function_procname: Typ.Procname.t } + | AssigningToAField of Typ.Fieldname.t + | ReturningFromAFunction of Typ.Procname.t + and dereference_type = | MethodCall of Typ.Procname.t (** nullable_object.some_method() *) | AccessToField of Typ.Fieldname.t (** nullable_object.some_field *)