From 34cfa20faf4c40ba96d475750296b8cfc8fd39d2 Mon Sep 17 00:00:00 2001 From: Mitya Lyubarskiy Date: Fri, 27 Sep 2019 08:07:36 -0700 Subject: [PATCH] [nullsafe] Migrate checks for inheritance to NullsafeRules Summary: 1. This diff finishes the work of getting rid of using `Annot.Item.t` for making judgements about nullability. Instead, `AnnotatedNullability` is now used consistently in the codebase. Corresponding TODO items are deleted. 2. This diff proceeds consolidating checks to `NullsafeRules` (which will simplify introducing non-binary nullability in follow up diffs). 3. Code is simplified: we get rid of `fold2/ignore` + inlined calculation of the param position in favor of more straightward `zip` + `iteri` combo. Reviewed By: artempyanykh Differential Revision: D17570439 fbshipit-source-id: 52acf2c66 --- infer/src/nullsafe/AnnotatedSignature.ml | 9 +- infer/src/nullsafe/NullsafeRules.ml | 17 +++ infer/src/nullsafe/NullsafeRules.mli | 18 +++ infer/src/nullsafe/eradicateChecks.ml | 139 +++++++++++++++-------- 4 files changed, 131 insertions(+), 52 deletions(-) diff --git a/infer/src/nullsafe/AnnotatedSignature.ml b/infer/src/nullsafe/AnnotatedSignature.ml index 3f8765ab8..ee0cdce2f 100644 --- a/infer/src/nullsafe/AnnotatedSignature.ml +++ b/infer/src/nullsafe/AnnotatedSignature.ml @@ -9,11 +9,10 @@ module F = Format module L = Logging (* TODO(T54088319) remove Annot.Item.t from t: - 1. For everything dealing with nullability, use info from AnnotatedNullability instead. - 2. For other annotations guiding Nullsafe behavior, introduce corresponding datatypes: - a. Known ret value annotations (if any) - b. Known param annotations - c. Known method-level annotations. + For all helper annotations guiding Nullsafe behavior, introduce corresponding datatypes: + a. Known ret value annotations (if any) + b. Known param annotations + c. Known method-level annotations. *) type t = {ret: ret_signature; params: param_signature list} [@@deriving compare] diff --git a/infer/src/nullsafe/NullsafeRules.ml b/infer/src/nullsafe/NullsafeRules.ml index a3344dcdb..0b9d60e9c 100644 --- a/infer/src/nullsafe/NullsafeRules.ml +++ b/infer/src/nullsafe/NullsafeRules.ml @@ -50,3 +50,20 @@ let passes_assignment_rule_for_inferred_nullability ~lhs ~rhs = let lhs_nullability = nullability_of_inferred_nullability lhs in let rhs_nullability = nullability_of_inferred_nullability rhs in is_subtype ~subtype:rhs_nullability ~supertype:lhs_nullability + + +type type_role = Param | Ret + +let passes_inheritance_rule type_role ~base ~overridden = + let base_nullability = nullability_of_annotated_nullability base in + let overridden_nullability = nullability_of_annotated_nullability overridden in + let subtype, supertype = + match type_role with + | Ret -> + (* covariance for ret *) + (overridden_nullability, base_nullability) + | Param -> + (* contravariance for param *) + (base_nullability, overridden_nullability) + in + is_subtype ~subtype ~supertype diff --git a/infer/src/nullsafe/NullsafeRules.mli b/infer/src/nullsafe/NullsafeRules.mli index 033b4785a..0ff74feae 100644 --- a/infer/src/nullsafe/NullsafeRules.mli +++ b/infer/src/nullsafe/NullsafeRules.mli @@ -32,3 +32,21 @@ val passes_assignment_rule_for_annotated_nullability : val passes_assignment_rule_for_inferred_nullability : lhs:InferredNullability.t -> rhs:InferredNullability.t -> bool (** Variant of assignment rule where lhs nullability is inferred (e.g. might differ from formal nullability of a corresponding type) *) + +(******************************************************************************************* + *** Inheritance rule ********************************************************************** + *) + +type type_role = Param | Ret + +val passes_inheritance_rule : + type_role -> base:AnnotatedNullability.t -> overridden:AnnotatedNullability.t -> bool +(** Inheritance rule: + a) Return type for an overridden method is covariant: + overridden method is allowed to narrow down the return value to a subtype of the one from the + base method; this means it is OK to make the return value non-null when it was nullable in the base) + b) Parameter type for an overridden method is contravariant. + It is OK for a derived method to accept nullable in the params even if the base does not accept nullable. + NOTE: Rule a) is based on Java covariance rule for the return type. + In contrast, rule b) is nullsafe specific as Java does not support type contravariance for method params. + *) diff --git a/infer/src/nullsafe/eradicateChecks.ml b/infer/src/nullsafe/eradicateChecks.ml index 4e8b3ecc8..3535ebb63 100644 --- a/infer/src/nullsafe/eradicateChecks.ml +++ b/infer/src/nullsafe/eradicateChecks.ml @@ -10,7 +10,6 @@ open! IStd (** Module for the checks called by Eradicate. *) (* TODO(T54088319) get rid of annotation_deprecated: - - move all usages related to nullability to AnnotatedNullability. - introduce "field flags" and move all other usages to this dedicated datatype *) type field_type = {annotation_deprecated: Annot.Item.t; annotated_type: AnnotatedType.t} @@ -420,57 +419,103 @@ let check_call_parameters tenv find_canonical_duplicate curr_pdesc node callee_a List.iter ~f:check resolved_params -(** Checks if the annotations are consistent with the inherited class or with the +let check_inheritance_rule_for_return find_canonical_duplicate tenv loc ~base_proc_name + ~overridden_proc_name ~overridden_proc_desc ~base_nullability ~overridden_nullability = + if + not + (NullsafeRules.passes_inheritance_rule NullsafeRules.Ret ~base:base_nullability + ~overridden:overridden_nullability) + then + report_error tenv find_canonical_duplicate + (TypeErr.Inconsistent_subclass_return_annotation (overridden_proc_name, base_proc_name)) + None loc overridden_proc_desc + + +let check_inheritance_rule_for_param find_canonical_duplicate tenv loc ~overridden_param_name + ~base_proc_name ~overridden_proc_name ~overridden_proc_desc ~param_position ~base_nullability + ~overridden_nullability = + if + not + (NullsafeRules.passes_inheritance_rule NullsafeRules.Param ~base:base_nullability + ~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 )) + None loc overridden_proc_desc + + +let check_inheritance_rule_for_params find_canonical_duplicate tenv loc ~base_proc_name + ~overridden_proc_name ~overridden_proc_desc ~base_signature ~overridden_signature = + let base_params = base_signature.AnnotatedSignature.params in + let overridden_params = overridden_signature.AnnotatedSignature.params in + let zipped_params = List.zip base_params overridden_params in + match zipped_params with + | Ok base_and_overridden_params -> + let should_index_from_zero = is_virtual base_params in + (* Check the rule for each pair of base and overridden param *) + List.iteri base_and_overridden_params + ~f:(fun index + ( AnnotatedSignature.{param_annotated_type= {nullability= base_nullability}} + , AnnotatedSignature. + { mangled= overridden_param_name + ; param_annotated_type= {nullability= overridden_nullability} } ) + -> + check_inheritance_rule_for_param find_canonical_duplicate tenv loc ~overridden_param_name + ~base_proc_name ~overridden_proc_name ~overridden_proc_desc + ~param_position:(if should_index_from_zero then index else index + 1) + ~base_nullability ~overridden_nullability ) + | Unequal_lengths -> + (* Skip checking. + TODO (T5280249): investigate why argument lists can be of different length. *) + () + + +(* Check both params and return values for complying for co- and contravariance *) +let check_inheritance_rule_for_signature find_canonical_duplicate tenv loc ~base_proc_name + ~overridden_proc_name ~overridden_proc_desc ~base_signature ~overridden_signature = + (* Check params *) + check_inheritance_rule_for_params find_canonical_duplicate tenv loc ~base_proc_name + ~overridden_proc_name ~overridden_proc_desc ~base_signature ~overridden_signature ; + (* Check return value *) + match base_proc_name with + (* TODO model this as unknown nullability and get rid of that check *) + | Typ.Procname.Java java_pname when not (Typ.Procname.Java.is_external java_pname) -> + (* Check if return value is consistent with the base *) + let base_nullability = + base_signature.AnnotatedSignature.ret.ret_annotated_type.nullability + in + let overridden_nullability = + overridden_signature.AnnotatedSignature.ret.ret_annotated_type.nullability + in + check_inheritance_rule_for_return find_canonical_duplicate tenv loc ~base_proc_name + ~overridden_proc_name ~overridden_proc_desc ~base_nullability ~overridden_nullability + | _ -> + (* the analysis should not report return type inconsistencies with external code *) + () + + +(** Checks if the annotations are consistent with the derived classes and with the implemented interfaces *) let check_overridden_annotations find_canonical_duplicate tenv proc_name proc_desc annotated_signature = let start_node = Procdesc.get_start_node proc_desc in let loc = Procdesc.Node.get_loc start_node in - let check_return overriden_proc_name overriden_signature = - let ret_is_nullable = - Annotations.ia_is_nullable - annotated_signature.AnnotatedSignature.ret.ret_annotation_deprecated - and ret_overridden_nullable = - 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 - 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 - report_error tenv find_canonical_duplicate - (TypeErr.Inconsistent_subclass_parameter_annotation - (Mangled.to_string current_name, pos, proc_name, overriden_proc_name)) - None loc proc_desc - in - pos + 1 - in - (* TODO (#5280249): investigate why argument lists can be of different length *) - let current_params = annotated_signature.AnnotatedSignature.params - and overridden_params = overriden_signature.AnnotatedSignature.params in - let initial_pos = if is_virtual current_params then 0 else 1 in - if Int.equal (List.length current_params) (List.length overridden_params) then - ignore (List.fold2_exn ~f:compare ~init:initial_pos current_params overridden_params) - in - let check overriden_proc_name = - match PatternMatch.lookup_attributes tenv overriden_proc_name with - | Some attributes -> ( - let overridden_signature = Models.get_modelled_annotated_signature attributes in - check_params overriden_proc_name overridden_signature ; - (* the analysis should not report return type inconsistencies with external code *) - match overriden_proc_name with - | Typ.Procname.Java java_pname when not (Typ.Procname.Java.is_external java_pname) -> - check_return overriden_proc_name overridden_signature - | _ -> - () ) + let check_if_base_signature_matches_current base_proc_name = + match PatternMatch.lookup_attributes tenv base_proc_name with + | Some base_attributes -> + let base_signature = Models.get_modelled_annotated_signature base_attributes in + check_inheritance_rule_for_signature find_canonical_duplicate tenv loc ~base_proc_name + ~overridden_proc_name:proc_name ~overridden_proc_desc:proc_desc ~base_signature + ~overridden_signature:annotated_signature | None -> + (* Could not find the attributes - optimistically skipping the check *) + (* TODO(T54687014) ensure this is not an issue in practice *) () in - PatternMatch.override_iter check tenv proc_name + (* Iterate over all methods the current method overrides and see the current + method is compatible with all of them *) + PatternMatch.override_iter check_if_base_signature_matches_current tenv proc_name