From bd1b55ef51eecc057171e07978392f558ebbb656 Mon Sep 17 00:00:00 2001 From: Mitya Lyubarskiy Date: Mon, 18 Nov 2019 01:51:25 -0800 Subject: [PATCH] [nullsafe] Methods rendering the error accept TypeOrigin.t instead of string Summary: This diff also removes clumsy typeErr.origin_desc and improves interface of TypeOrigin. This will allow to render smarter error message, depending on the context (see follow up diff). Reviewed By: ngorogiannis Differential Revision: D18527692 fbshipit-source-id: b7eb11db8 --- infer/src/nullsafe/AssignmentRule.ml | 3 +- infer/src/nullsafe/AssignmentRule.mli | 2 +- infer/src/nullsafe/DereferenceRule.ml | 5 ++- infer/src/nullsafe/DereferenceRule.mli | 2 +- infer/src/nullsafe/InferredNullability.ml | 9 ------ infer/src/nullsafe/InferredNullability.mli | 5 --- infer/src/nullsafe/eradicateChecks.ml | 16 +++++----- infer/src/nullsafe/typeErr.ml | 36 ++++++++-------------- infer/src/nullsafe/typeErr.mli | 6 ++-- infer/src/nullsafe/typeOrigin.ml | 20 ++++++------ infer/src/nullsafe/typeOrigin.mli | 2 +- 11 files changed, 43 insertions(+), 63 deletions(-) diff --git a/infer/src/nullsafe/AssignmentRule.ml b/infer/src/nullsafe/AssignmentRule.ml index d043d2574..12d519cd8 100644 --- a/infer/src/nullsafe/AssignmentRule.ml +++ b/infer/src/nullsafe/AssignmentRule.ml @@ -35,7 +35,8 @@ let check ~is_strict_mode ~lhs ~rhs = Result.ok_if_true is_allowed_assignment ~error:{is_strict_mode; lhs; rhs} -let violation_description _ assignment_type ~rhs_origin_descr = +let violation_description _ assignment_type ~rhs_origin = + let rhs_origin_descr = TypeOrigin.get_description rhs_origin |> Option.value ~default:"" in let module MF = MarkupFormatter in match assignment_type with | PassingParamToFunction {param_description; param_position; function_procname} -> diff --git a/infer/src/nullsafe/AssignmentRule.mli b/infer/src/nullsafe/AssignmentRule.mli index ee2df89ff..1a6897929 100644 --- a/infer/src/nullsafe/AssignmentRule.mli +++ b/infer/src/nullsafe/AssignmentRule.mli @@ -23,4 +23,4 @@ type assignment_type = | ReturningFromFunction of Typ.Procname.t [@@deriving compare] -val violation_description : violation -> assignment_type -> rhs_origin_descr:string -> string +val violation_description : violation -> assignment_type -> rhs_origin:TypeOrigin.t -> string diff --git a/infer/src/nullsafe/DereferenceRule.ml b/infer/src/nullsafe/DereferenceRule.ml index 15f062820..c191e183f 100644 --- a/infer/src/nullsafe/DereferenceRule.ml +++ b/infer/src/nullsafe/DereferenceRule.ml @@ -25,8 +25,11 @@ let check ~is_strict_mode nullability = Ok () -let violation_description _ dereference_type ~nullable_object_descr ~origin_descr = +let violation_description _ dereference_type ~nullable_object_descr ~nullable_object_origin = let module MF = MarkupFormatter in + let origin_descr = + TypeOrigin.get_description nullable_object_origin |> Option.value ~default:"" + in let nullable_object_descr = match dereference_type with | MethodCall _ | AccessToField _ -> ( diff --git a/infer/src/nullsafe/DereferenceRule.mli b/infer/src/nullsafe/DereferenceRule.mli index 2f6f69ca6..f8b005602 100644 --- a/infer/src/nullsafe/DereferenceRule.mli +++ b/infer/src/nullsafe/DereferenceRule.mli @@ -25,5 +25,5 @@ val violation_description : violation -> dereference_type -> nullable_object_descr:string option - -> origin_descr:string + -> nullable_object_origin:TypeOrigin.t -> string diff --git a/infer/src/nullsafe/InferredNullability.ml b/infer/src/nullsafe/InferredNullability.ml index a14724206..761e5bd6f 100644 --- a/infer/src/nullsafe/InferredNullability.ml +++ b/infer/src/nullsafe/InferredNullability.ml @@ -21,15 +21,6 @@ let is_nonnull {nullability} = match nullability with Nullable -> false | DeclaredNonnull -> false | Nonnull -> true -let descr_origin t = - let descr_opt = TypeOrigin.get_description t.origin in - match descr_opt with - | None -> - ("", None, None) - | Some (str, loc_opt, sig_opt) -> - ("(Origin: " ^ str ^ ")", loc_opt, sig_opt) - - let to_string {nullability} = Printf.sprintf "[%s]" (Nullability.to_string nullability) let join t1 t2 = diff --git a/infer/src/nullsafe/InferredNullability.mli b/infer/src/nullsafe/InferredNullability.mli index 08ba64ef5..5d904f9fc 100644 --- a/infer/src/nullsafe/InferredNullability.mli +++ b/infer/src/nullsafe/InferredNullability.mli @@ -27,11 +27,6 @@ val is_nonnull_or_declared_nonnull : t -> bool val is_nonnull : t -> bool -val descr_origin : t -> TypeErr.origin_descr -(** Human-readable description of the origin of a value. - (How did nullsafe infer the nullability ) - *) - val get_origin : t -> TypeOrigin.t (** The simple explanation of how was nullability inferred. *) diff --git a/infer/src/nullsafe/eradicateChecks.ml b/infer/src/nullsafe/eradicateChecks.ml index 3ba855141..fcdbeedb5 100644 --- a/infer/src/nullsafe/eradicateChecks.ml +++ b/infer/src/nullsafe/eradicateChecks.ml @@ -32,11 +32,11 @@ let check_object_dereference ~is_strict_mode tenv find_canonical_duplicate curr_ (DereferenceRule.check ~is_strict_mode (InferredNullability.get_nullability inferred_nullability)) ~f:(fun dereference_violation -> - let origin_descr = InferredNullability.descr_origin inferred_nullability in + let nullable_object_origin = InferredNullability.get_origin inferred_nullability in let nullable_object_descr = explain_expr tenv node object_exp in let type_error = TypeErr.Nullable_dereference - {dereference_violation; nullable_object_descr; dereference_type; origin_descr} + {dereference_violation; nullable_object_descr; dereference_type; nullable_object_origin} in report_error tenv find_canonical_duplicate type_error (Some instr_ref) loc curr_pname ) @@ -159,11 +159,11 @@ let check_field_assignment ~is_strict_mode tenv find_canonical_duplicate curr_pd && not (field_is_in_cleanup_context ()) in if should_report then - let rhs_origin_descr = InferredNullability.descr_origin inferred_nullability_rhs in + let rhs_origin = InferredNullability.get_origin inferred_nullability_rhs in report_error tenv find_canonical_duplicate (TypeErr.Bad_assignment { assignment_violation - ; rhs_origin_descr + ; rhs_origin ; assignment_type= AssignmentRule.AssigningToField fname }) (Some instr_ref) loc curr_pdesc ) @@ -326,11 +326,11 @@ let check_return_not_nullable ~is_strict_mode tenv find_canonical_duplicate loc let lhs = AnnotatedNullability.get_nullability ret_signature.ret_annotated_type.nullability in let rhs = InferredNullability.get_nullability ret_inferred_nullability in Result.iter_error (AssignmentRule.check ~is_strict_mode ~lhs ~rhs) ~f:(fun assignment_violation -> - let rhs_origin_descr = InferredNullability.descr_origin ret_inferred_nullability in + let rhs_origin = InferredNullability.get_origin ret_inferred_nullability in report_error tenv find_canonical_duplicate (TypeErr.Bad_assignment { assignment_violation - ; rhs_origin_descr + ; rhs_origin ; assignment_type= AssignmentRule.ReturningFromFunction curr_pname }) None loc curr_pdesc ) @@ -436,11 +436,11 @@ let check_call_parameters ~is_strict_mode tenv find_canonical_duplicate curr_pde | None -> "formal parameter " ^ Mangled.to_string formal.mangled in - let rhs_origin_descr = InferredNullability.descr_origin nullability_actual in + let rhs_origin = InferredNullability.get_origin nullability_actual in report_error tenv find_canonical_duplicate (TypeErr.Bad_assignment { assignment_violation - ; rhs_origin_descr + ; rhs_origin ; assignment_type= AssignmentRule.PassingParamToFunction {param_description; param_position; function_procname= callee_pname} }) diff --git a/infer/src/nullsafe/typeErr.ml b/infer/src/nullsafe/typeErr.ml index 3fe6589f6..c86222234 100644 --- a/infer/src/nullsafe/typeErr.ml +++ b/infer/src/nullsafe/typeErr.ml @@ -56,12 +56,6 @@ end (* InstrRef *) -type origin_descr = string * Location.t option * AnnotatedSignature.t option - -(* callee signature *) -(* ignore origin descr *) -let compare_origin_descr _ _ = 0 - (** Instance of an error *) type err_instance = | Condition_redundant of (bool * string option) @@ -78,11 +72,11 @@ type err_instance = { dereference_violation: DereferenceRule.violation ; dereference_type: DereferenceRule.dereference_type ; nullable_object_descr: string option - ; origin_descr: origin_descr } + ; nullable_object_origin: TypeOrigin.t } | Bad_assignment of { assignment_violation: AssignmentRule.violation ; assignment_type: AssignmentRule.assignment_type - ; rhs_origin_descr: origin_descr } + ; rhs_origin: TypeOrigin.t } [@@deriving compare] module H = Hashtbl.Make (struct @@ -172,18 +166,18 @@ module Severity = struct None - let origin_descr_get_severity tenv origin_descr = - match origin_descr with - | _, _, Some signature -> - this_type_get_severity tenv signature - | _, _, None -> + let origin_get_severity tenv origin = + match origin with + | TypeOrigin.MethodCall {annotated_signature} -> + this_type_get_severity tenv annotated_signature + | _ -> None let err_instance_get_severity tenv err_instance : Exceptions.severity option = match err_instance with - | Nullable_dereference {origin_descr} -> - origin_descr_get_severity tenv origin_descr + | Nullable_dereference {nullable_object_origin} -> + origin_get_severity tenv nullable_object_origin | _ -> None end @@ -261,16 +255,12 @@ let get_error_description err_instance = 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= rhs_origin_descr, _, _; assignment_type; assignment_violation} - -> - AssignmentRule.violation_description assignment_violation assignment_type ~rhs_origin_descr + | Bad_assignment {rhs_origin; assignment_type; assignment_violation} -> + AssignmentRule.violation_description assignment_violation assignment_type ~rhs_origin | Nullable_dereference - { dereference_violation - ; nullable_object_descr - ; dereference_type - ; origin_descr= origin_descr, _, _ } -> + {dereference_violation; nullable_object_descr; dereference_type; nullable_object_origin} -> DereferenceRule.violation_description dereference_violation dereference_type - ~nullable_object_descr ~origin_descr + ~nullable_object_descr ~nullable_object_origin | Inconsistent_subclass {inheritance_violation; violation_type; base_proc_name; overridden_proc_name} -> InheritanceRule.violation_description inheritance_violation violation_type ~base_proc_name diff --git a/infer/src/nullsafe/typeErr.mli b/infer/src/nullsafe/typeErr.mli index 74f7dd70c..db8d217ea 100644 --- a/infer/src/nullsafe/typeErr.mli +++ b/infer/src/nullsafe/typeErr.mli @@ -30,8 +30,6 @@ end (* InstrRefT *) module InstrRef : InstrRefT -type origin_descr = string * Location.t option * AnnotatedSignature.t option - (* callee signature *) (** Instance of an error *) @@ -50,11 +48,11 @@ type err_instance = { dereference_violation: DereferenceRule.violation ; dereference_type: DereferenceRule.dereference_type ; nullable_object_descr: string option - ; origin_descr: origin_descr } + ; nullable_object_origin: TypeOrigin.t } | Bad_assignment of { assignment_violation: AssignmentRule.violation ; assignment_type: AssignmentRule.assignment_type - ; rhs_origin_descr: origin_descr } + ; rhs_origin: TypeOrigin.t } [@@deriving compare] val node_reset_forall : Procdesc.Node.t -> unit diff --git a/infer/src/nullsafe/typeOrigin.ml b/infer/src/nullsafe/typeOrigin.ml index 33dd0b8b6..b8e072557 100644 --- a/infer/src/nullsafe/typeOrigin.ml +++ b/infer/src/nullsafe/typeOrigin.ml @@ -104,19 +104,16 @@ let rec to_string = function "Undef" -let get_description origin = +let get_description_impl origin = let atline loc = " at line " ^ string_of_int loc.Location.line in match origin with | NullConst loc -> - Some ("null constant" ^ atline loc, Some loc, None) + Some ("null constant" ^ atline loc) | Field {field_name; access_loc} -> - Some - ( "field " ^ Typ.Fieldname.to_simplified_string field_name ^ atline access_loc - , Some access_loc - , None ) + Some ("field " ^ Typ.Fieldname.to_simplified_string field_name ^ atline access_loc) | MethodParameter {mangled} -> - Some ("method parameter " ^ Mangled.to_string mangled, None, None) - | MethodCall {pname; call_loc; annotated_signature} -> + Some ("method parameter " ^ Mangled.to_string mangled) + | MethodCall {pname; call_loc} -> let modelled_in = (* TODO(T54088319) don't calculate this info and propagate it from AnnotatedNullability instead *) if Models.is_modelled_for_nullability_as_internal pname then @@ -128,7 +125,7 @@ let get_description origin = (Typ.Procname.to_simplified_string pname) modelled_in (atline call_loc) in - Some (description, Some call_loc, Some annotated_signature) + Some description (* These are origins of non-nullable expressions that are result of evaluating of some rvalue. Because they are non-nullable and they are rvalues, we won't get normal type violations With them. All we could get is things like condition redundant or overannotated. @@ -142,6 +139,11 @@ let get_description origin = None +let get_description origin = + get_description_impl origin + |> Option.map ~f:(fun description -> Format.sprintf "(Origin: %s)" description) + + let join o1 o2 = match (o1, o2) with (* left priority *) diff --git a/infer/src/nullsafe/typeOrigin.mli b/infer/src/nullsafe/typeOrigin.mli index 8386be861..177e88241 100644 --- a/infer/src/nullsafe/typeOrigin.mli +++ b/infer/src/nullsafe/typeOrigin.mli @@ -46,7 +46,7 @@ val equal : t -> t -> bool val get_nullability : t -> Nullability.t -val get_description : t -> TypeErr.origin_descr option +val get_description : t -> string option (** Get a description to be used for error messages. *) val join : t -> t -> t