From 693f2944c793d057ae7d2062a60cb792bdcc922d Mon Sep 17 00:00:00 2001 From: Mitya Lyubarskiy Date: Fri, 20 Sep 2019 05:50:32 -0700 Subject: [PATCH] [nullsafe] Convert AnnotatedSignature subtypes from tuples to records Summary: 1/ Nikos Gorogiannis pointed out that - for highly reused public types, records (especially when >= 3 params) are generally more readable than tuples. - Records simplify code modifications, especially adding new fields. And we are going to add some, namely param flags, in the future. 2/ Let's make the fact that annotated signature is deprecated more visible; it will also simplify searching for usages when we will be getting rid of them. Reviewed By: ngorogiannis Differential Revision: D17475033 fbshipit-source-id: 7740c979b --- infer/src/nullsafe/AnnotatedSignature.ml | 83 +++++++++++++---------- infer/src/nullsafe/AnnotatedSignature.mli | 12 +++- infer/src/nullsafe/eradicate.ml | 16 ++--- infer/src/nullsafe/eradicateChecks.ml | 34 ++++++---- infer/src/nullsafe/typeCheck.ml | 37 ++++++---- infer/src/nullsafe/typeErr.ml | 5 +- 6 files changed, 111 insertions(+), 76 deletions(-) diff --git a/infer/src/nullsafe/AnnotatedSignature.ml b/infer/src/nullsafe/AnnotatedSignature.ml index 4b7bfe076..62f54eaf1 100644 --- a/infer/src/nullsafe/AnnotatedSignature.ml +++ b/infer/src/nullsafe/AnnotatedSignature.ml @@ -15,8 +15,16 @@ module L = Logging b. Known param annotations c. Known method-level annotations. *) -type t = - {ret: Annot.Item.t * NullsafeType.t; params: (Mangled.t * Annot.Item.t * NullsafeType.t) list} + +type t = {ret: ret_signature; params: param_signature list} [@@deriving compare] + +and ret_signature = {ret_annotation_deprecated: Annot.Item.t; ret_nullsafe_type: NullsafeType.t} +[@@deriving compare] + +and param_signature = + { param_annotation_deprecated: Annot.Item.t + ; mangled: Mangled.t + ; param_nullsafe_type: NullsafeType.t } [@@deriving compare] (* get nullability of method's return type given its annotations and information about its params *) @@ -51,10 +59,11 @@ let extract_nullability return_annotation param_annotations = let get proc_attributes : t = - let method_annotation = proc_attributes.ProcAttributes.method_annotation in + let Annot.Method.{return= return_annotation; params= original_params_annotation} = + proc_attributes.ProcAttributes.method_annotation + in let formals = proc_attributes.ProcAttributes.formals in let ret_type = proc_attributes.ProcAttributes.ret_type in - let Annot.Method.{return; params} = method_annotation in (* zip formal params with annotation *) let params_with_annotations = let rec zip_params ial parl = @@ -72,33 +81,40 @@ let get proc_attributes : t = this should never happen *) assert false in - List.rev (zip_params (List.rev params) (List.rev formals)) + List.rev (zip_params (List.rev original_params_annotation) (List.rev formals)) + in + let _, final_params_annotation = List.unzip params_with_annotations in + let return_nullability, params_nullability = + extract_nullability return_annotation final_params_annotation + in + let ret = + { ret_annotation_deprecated= return_annotation + ; ret_nullsafe_type= NullsafeType.{nullability= return_nullability; typ= ret_type} } in - let _, param_annotations = List.unzip params_with_annotations in - let return_nullability, params_nullability = extract_nullability return param_annotations in - let ret = (return, NullsafeType.{nullability= return_nullability; typ= ret_type}) in let params = - List.zip_exn params_with_annotations params_nullability - |> List.map ~f:(function ((mangled, typ), annot), nullability -> - (mangled, annot, NullsafeType.{nullability; typ}) ) + List.map2_exn params_with_annotations params_nullability + ~f:(fun ((mangled, typ), param_annotation_deprecated) nullability -> + {param_annotation_deprecated; mangled; param_nullsafe_type= NullsafeType.{nullability; typ}} + ) in {ret; params} let param_has_annot predicate pvar ann_sig = List.exists - ~f:(fun (param, param_annot, _) -> - Mangled.equal param (Pvar.get_name pvar) && predicate param_annot ) + ~f:(fun {mangled; param_annotation_deprecated} -> + Mangled.equal mangled (Pvar.get_name pvar) && predicate param_annotation_deprecated ) ann_sig.params let pp proc_name fmt annotated_signature = let pp_ia fmt ia = if ia <> [] then F.fprintf fmt "%a " Annot.Item.pp ia in - let pp_annotated_param fmt (mangled, ia, nullsafe_type) = - F.fprintf fmt " %a%a %a" pp_ia ia NullsafeType.pp nullsafe_type Mangled.pp mangled + let pp_annotated_param fmt {mangled; param_annotation_deprecated; param_nullsafe_type} = + F.fprintf fmt " %a%a %a" pp_ia param_annotation_deprecated NullsafeType.pp param_nullsafe_type + Mangled.pp mangled in - let ia, nullsafe_type = annotated_signature.ret in - F.fprintf fmt "%a%a %s (%a )" pp_ia ia NullsafeType.pp nullsafe_type + let {ret_annotation_deprecated; ret_nullsafe_type} = annotated_signature.ret in + F.fprintf fmt "%a%a %s (%a )" pp_ia ret_annotation_deprecated NullsafeType.pp ret_nullsafe_type (Typ.Procname.to_simplified_string proc_name) (Pp.comma_seq pp_annotated_param) annotated_signature.params @@ -122,14 +138,18 @@ let set_modelled_nullability_for_nullsafe_type nullsafe_type should_set_nullable let set_modelled_nullability proc_name asig (nullability_for_ret, params_nullability) = - let set_modelled_nullability_for_param (mangled, original_annotation, original_nullsafe_type) - should_set_nullable = - let final_annotation = - if should_set_nullable then mk_ia_nullable original_annotation else original_annotation - in - ( mangled - , final_annotation - , set_modelled_nullability_for_nullsafe_type original_nullsafe_type should_set_nullable ) + let set_modelled_nullability_for_param param should_set_nullable = + { param with + param_annotation_deprecated= + mark_ia_nullability param.param_annotation_deprecated should_set_nullable + ; param_nullsafe_type= + set_modelled_nullability_for_nullsafe_type param.param_nullsafe_type should_set_nullable } + in + let set_modelled_nullability_for_ret ret should_set_nullable = + { ret_annotation_deprecated= + mark_ia_nullability ret.ret_annotation_deprecated should_set_nullable + ; ret_nullsafe_type= + set_modelled_nullability_for_nullsafe_type ret.ret_nullsafe_type should_set_nullable } in let final_params = let fail () = @@ -140,11 +160,9 @@ let set_modelled_nullability proc_name asig (nullability_for_ret, params_nullabi in let rec model_param_nullability original_params params_nullability = match (original_params, params_nullability) with - | (mangled, annotation, nullsafe_type) :: params_tail, nullability_tail - when Mangled.is_this mangled -> + | param :: params_tail, nullability_tail when Mangled.is_this param.mangled -> (* Skip "this" param - there is no notion of "nullable this" *) - (mangled, annotation, nullsafe_type) - :: model_param_nullability params_tail nullability_tail + param :: model_param_nullability params_tail nullability_tail | param :: params_tail, should_set_nullable :: nullability_tail -> set_modelled_nullability_for_param param should_set_nullable :: model_param_nullability params_tail nullability_tail @@ -156,9 +174,4 @@ let set_modelled_nullability proc_name asig (nullability_for_ret, params_nullabi in model_param_nullability asig.params params_nullability in - let original_ret_annotations, original_ret_nullsafe_type = asig.ret in - let final_ret_nullsafe_type = - set_modelled_nullability_for_nullsafe_type original_ret_nullsafe_type nullability_for_ret - in - let final_ret_annotation = mark_ia_nullability original_ret_annotations nullability_for_ret in - {ret= (final_ret_annotation, final_ret_nullsafe_type); params= final_params} + {ret= set_modelled_nullability_for_ret asig.ret nullability_for_ret; params= final_params} diff --git a/infer/src/nullsafe/AnnotatedSignature.mli b/infer/src/nullsafe/AnnotatedSignature.mli index 9c025ec76..3fa51ab02 100644 --- a/infer/src/nullsafe/AnnotatedSignature.mli +++ b/infer/src/nullsafe/AnnotatedSignature.mli @@ -9,9 +9,15 @@ open! IStd -type t = - { ret: Annot.Item.t * NullsafeType.t (** Annotated return type. *) - ; params: (Mangled.t * Annot.Item.t * NullsafeType.t) list (** Annotated parameters. *) } +type t = {ret: ret_signature; params: param_signature list} [@@deriving compare] + +and ret_signature = {ret_annotation_deprecated: Annot.Item.t; ret_nullsafe_type: NullsafeType.t} +[@@deriving compare] + +and param_signature = + { param_annotation_deprecated: Annot.Item.t + ; mangled: Mangled.t + ; param_nullsafe_type: NullsafeType.t } [@@deriving compare] val param_has_annot : (Annot.Item.t -> bool) -> Pvar.t -> t -> bool diff --git a/infer/src/nullsafe/eradicate.ml b/infer/src/nullsafe/eradicate.ml index 91a85e796..2a2de7e4e 100644 --- a/infer/src/nullsafe/eradicate.ml +++ b/infer/src/nullsafe/eradicate.ml @@ -32,14 +32,13 @@ end module MkCallback (Extension : ExtensionT) : CallBackT = struct let callback1 tenv find_canonical_duplicate calls_this checks idenv curr_pname curr_pdesc annotated_signature linereader proc_loc : bool * TypeState.t option = - let mk s = Pvar.mk s curr_pname in - let add_formal typestate (s, ia, NullsafeType.{typ}) = - let pvar = mk s in + let add_formal typestate (param_signature : AnnotatedSignature.param_signature) = + let pvar = Pvar.mk param_signature.mangled curr_pname in let ta = - let origin = TypeOrigin.Formal s in - TypeAnnotation.from_item_annotation ia origin + let origin = TypeOrigin.Formal param_signature.mangled in + TypeAnnotation.from_item_annotation param_signature.param_annotation_deprecated origin in - TypeState.add pvar (typ, ta, []) typestate + TypeState.add pvar (param_signature.param_nullsafe_type.typ, ta, []) typestate in let get_initial_typestate () = let typestate_empty = TypeState.empty in @@ -48,7 +47,7 @@ module MkCallback (Extension : ExtensionT) : CallBackT = struct (* Check the nullable flag computed for the return value and report inconsistencies. *) let check_return find_canonical_duplicate exit_node final_typestate annotated_signature loc : unit = - let _, ret_nullsafe_type = annotated_signature.AnnotatedSignature.ret in + let AnnotatedSignature.{ret_nullsafe_type} = annotated_signature.AnnotatedSignature.ret in let ret_pvar = Procdesc.get_ret_var curr_pdesc in let ret_range = TypeState.lookup_pvar ret_pvar final_typestate in let typ_found_opt = @@ -251,8 +250,9 @@ module MkCallback (Extension : ExtensionT) : CallBackT = struct (let is_initializer proc_attributes = PatternMatch.method_is_initializer tenv proc_attributes || - let ia, _ = + let ia = (Models.get_modelled_annotated_signature proc_attributes).AnnotatedSignature.ret + .ret_annotation_deprecated in Annotations.ia_is_initializer ia in diff --git a/infer/src/nullsafe/eradicateChecks.ml b/infer/src/nullsafe/eradicateChecks.ml index bbcb21ff5..854ba6a8f 100644 --- a/infer/src/nullsafe/eradicateChecks.ml +++ b/infer/src/nullsafe/eradicateChecks.ml @@ -37,7 +37,12 @@ let explain_expr tenv node e = None -let is_virtual = function (p, _, _) :: _ when Mangled.is_this p -> true | _ -> false +let is_virtual = function + | AnnotatedSignature.{mangled} :: _ when Mangled.is_this mangled -> + true + | _ -> + false + (** Check an access (read or write) to a field. *) let check_field_access tenv find_canonical_duplicate curr_pname node instr_ref exp fname ta loc : @@ -155,8 +160,10 @@ let check_field_assignment tenv find_canonical_duplicate curr_pdesc node instr_r false in let field_is_in_cleanup_context () = - let ia, _ = (Models.get_modelled_annotated_signature curr_pattrs).AnnotatedSignature.ret in - Annotations.ia_is_cleanup ia + let AnnotatedSignature.{ret_annotation_deprecated} = + (Models.get_modelled_annotated_signature curr_pattrs).ret + in + Annotations.ia_is_cleanup ret_annotation_deprecated in let should_report_nullable = (not (AndroidFramework.is_destroy_method curr_pname)) @@ -256,12 +263,13 @@ let check_constructor_initialization tenv find_canonical_duplicate curr_pname cu (** Check the annotations when returning from a method. *) let check_return_annotation tenv find_canonical_duplicate curr_pdesc ret_range (annotated_signature : AnnotatedSignature.t) ret_implicitly_nullable loc : unit = - let ret_ia, _ = annotated_signature.ret in + let AnnotatedSignature.{ret_annotation_deprecated} = annotated_signature.ret in let curr_pname = Procdesc.get_proc_name curr_pdesc in let ret_annotated_nullable = - Annotations.ia_is_nullable ret_ia + Annotations.ia_is_nullable ret_annotation_deprecated || List.exists - ~f:(fun (_, ia, _) -> Annotations.ia_is_propagates_nullable ia) + ~f:(fun AnnotatedSignature.{param_annotation_deprecated} -> + Annotations.ia_is_propagates_nullable param_annotation_deprecated ) annotated_signature.params in match ret_range with @@ -369,20 +377,20 @@ let check_overridden_annotations find_canonical_duplicate tenv proc_name proc_de let loc = Procdesc.Node.get_loc start_node in let check_return overriden_proc_name overriden_signature = let ret_is_nullable = - let ia, _ = annotated_signature.AnnotatedSignature.ret in - Annotations.ia_is_nullable ia + Annotations.ia_is_nullable + annotated_signature.AnnotatedSignature.ret.ret_annotation_deprecated and ret_overridden_nullable = - let overriden_ia, _ = overriden_signature.AnnotatedSignature.ret in - Annotations.ia_is_nullable overriden_ia + Annotations.ia_is_nullable + overriden_signature.AnnotatedSignature.ret.ret_annotation_deprecated in if ret_is_nullable && not ret_overridden_nullable then report_error tenv find_canonical_duplicate (TypeErr.Inconsistent_subclass_return_annotation (proc_name, overriden_proc_name)) None loc proc_desc and check_params overriden_proc_name overriden_signature = - let compare pos current_param overriden_param : int = - let current_name, current_ia, _ = current_param in - let _, overriden_ia, _ = overriden_param in + let compare pos + AnnotatedSignature.{mangled= current_name; param_annotation_deprecated= current_ia} + AnnotatedSignature.{param_annotation_deprecated= overriden_ia} = let () = if (not (Annotations.ia_is_nullable current_ia)) && Annotations.ia_is_nullable overriden_ia then diff --git a/infer/src/nullsafe/typeCheck.ml b/infer/src/nullsafe/typeCheck.ml index 7579d7d0f..2274af051 100644 --- a/infer/src/nullsafe/typeCheck.ml +++ b/infer/src/nullsafe/typeCheck.ml @@ -19,8 +19,8 @@ module ComplexExpressions = struct match PatternMatch.lookup_attributes tenv pn with | Some proc_attributes -> let annotated_signature = Models.get_modelled_annotated_signature proc_attributes in - let ret_ann, _ = annotated_signature.AnnotatedSignature.ret in - Annotations.ia_is_false_on_null ret_ann + let AnnotatedSignature.{ret_annotation_deprecated} = annotated_signature.ret in + Annotations.ia_is_false_on_null ret_annotation_deprecated | None -> false @@ -30,8 +30,8 @@ module ComplexExpressions = struct match PatternMatch.lookup_attributes tenv pn with | Some proc_attributes -> let annotated_signature = Models.get_modelled_annotated_signature proc_attributes in - let ret_ann, _ = annotated_signature.AnnotatedSignature.ret in - Annotations.ia_is_true_on_null ret_ann + let AnnotatedSignature.{ret_annotation_deprecated} = annotated_signature.ret in + Annotations.ia_is_true_on_null ret_annotation_deprecated | None -> false in @@ -289,7 +289,7 @@ let typecheck_instr tenv calls_this checks (node : Procdesc.Node.t) idenv curr_p let is_parameter_field pvar = (* parameter.field *) let name = Pvar.get_name pvar in - let filter (s, _, _) = Mangled.equal s name in + let filter AnnotatedSignature.{mangled} = Mangled.equal mangled name in List.exists ~f:filter curr_annotated_signature.AnnotatedSignature.params in let is_static_field pvar = @@ -684,19 +684,26 @@ let typecheck_instr tenv calls_this checks (node : Procdesc.Node.t) idenv curr_p in let typestate_after_call, resolved_ret = let resolve_param i (sparam, cparam) = - let s1, ia1, NullsafeType.{typ= t1} = sparam in + let AnnotatedSignature.{mangled; param_annotation_deprecated; param_nullsafe_type} = + sparam + in let (orig_e2, e2), t2 = cparam in - let ta1 = TypeAnnotation.from_item_annotation ia1 (TypeOrigin.Formal s1) in + let ta1 = + TypeAnnotation.from_item_annotation param_annotation_deprecated + (TypeOrigin.Formal mangled) + in let _, ta2, _ = typecheck_expr find_canonical_duplicate calls_this checks tenv node instr_ref curr_pdesc typestate e2 (t2, TypeAnnotation.const_nullable false TypeOrigin.ONone, []) loc in - let formal = (s1, ta1, t1) in + let formal = (mangled, ta1, param_nullsafe_type.typ) in let actual = (orig_e2, ta2) in let num = i + 1 in - let formal_is_propagates_nullable = Annotations.ia_is_propagates_nullable ia1 in + let formal_is_propagates_nullable = + Annotations.ia_is_propagates_nullable param_annotation_deprecated + in let actual_is_nullable = TypeAnnotation.is_nullable ta2 in let propagates_nullable = formal_is_propagates_nullable && actual_is_nullable in EradicateChecks.{num; formal; actual; propagates_nullable} @@ -727,7 +734,7 @@ let typecheck_instr tenv calls_this checks (node : Procdesc.Node.t) idenv curr_p handle_params resolved_ret resolved_params in let resolved_ret_ = - let ret_ia, ret_typ = callee_annotated_signature.AnnotatedSignature.ret in + let ret = callee_annotated_signature.AnnotatedSignature.ret in let is_library = Summary.OnDisk.proc_is_library callee_attributes in let origin = TypeOrigin.Proc @@ -736,8 +743,8 @@ let typecheck_instr tenv calls_this checks (node : Procdesc.Node.t) idenv curr_p ; annotated_signature= callee_annotated_signature ; is_library } in - let ret_ta = TypeAnnotation.from_item_annotation ret_ia origin in - (ret_ta, ret_typ) + let ret_ta = TypeAnnotation.from_item_annotation ret.ret_annotation_deprecated origin in + (ret_ta, ret.ret_nullsafe_type) in let sig_len = List.length signature_params in let call_len = List.length call_params in @@ -750,10 +757,10 @@ let typecheck_instr tenv calls_this checks (node : Procdesc.Node.t) idenv curr_p let call_slice = slice call_params in let sig_call_params = List.filter - ~f:(fun (sparam, _) -> - let s, _, _ = sparam in + ~f:(fun ((sparam : AnnotatedSignature.param_signature), _) -> let param_is_this = - Mangled.is_this s || String.is_prefix ~prefix:"this$" (Mangled.to_string s) + Mangled.is_this sparam.mangled + || String.is_prefix ~prefix:"this$" (Mangled.to_string sparam.mangled) in not param_is_this ) (List.zip_exn sig_slice call_slice) diff --git a/infer/src/nullsafe/typeErr.ml b/infer/src/nullsafe/typeErr.ml index b960661ac..89bc4eadd 100644 --- a/infer/src/nullsafe/typeErr.ml +++ b/infer/src/nullsafe/typeErr.ml @@ -178,8 +178,9 @@ module Severity = struct let this_type_get_severity tenv (signature : AnnotatedSignature.t) = match signature.params with - | (p, _, NullsafeType.{typ= this_type}) :: _ when Mangled.is_this p -> - Option.bind ~f:get_severity (PatternMatch.type_get_annotation tenv this_type) + | AnnotatedSignature.{mangled; param_nullsafe_type} :: _ when Mangled.is_this mangled -> + (* TODO(T54088319) get rid of direct access to annotation *) + Option.bind ~f:get_severity (PatternMatch.type_get_annotation tenv param_nullsafe_type.typ) | _ -> None