From e24cf9f15549c83f5837d1da9d3a20c2e4d4e144 Mon Sep 17 00:00:00 2001 From: Mitya Lyubarskiy Date: Tue, 24 Sep 2019 03:37:48 -0700 Subject: [PATCH] [nullsafe] Introduce NullsafeRules and start consolidating checks Summary: In nutshell, Nullsafe is driven by relatively simple set of rules. It is currently not well reflected in code: we are duplicating the same logic in different places, which is: - error prone (we need to adjust ensure all places are addressed if a new feature is introduced) - complicates understanding of nullsafe Consolidating checks will simplify introducing Unknown Nullability and strict/partial check modes. ## this diff This diff does it for one particular check. See follow up diffs re that proceed consolidation. ## future diffs Future diffs will: - consolidate other checks that use 'assignment rule' - introduce other rules, most notably 'dereference rule' and 'inheritance rule' Reviewed By: artempyanykh Differential Revision: D17498630 fbshipit-source-id: 079d36518 --- infer/src/nullsafe/NullsafeRules.ml | 19 +++++++++++++++++++ infer/src/nullsafe/NullsafeRules.mli | 24 ++++++++++++++++++++++++ infer/src/nullsafe/eradicateChecks.ml | 17 +++++++++-------- infer/src/nullsafe/typeCheck.ml | 12 +++--------- 4 files changed, 55 insertions(+), 17 deletions(-) create mode 100644 infer/src/nullsafe/NullsafeRules.ml create mode 100644 infer/src/nullsafe/NullsafeRules.mli diff --git a/infer/src/nullsafe/NullsafeRules.ml b/infer/src/nullsafe/NullsafeRules.ml new file mode 100644 index 000000000..5404fa8bc --- /dev/null +++ b/infer/src/nullsafe/NullsafeRules.ml @@ -0,0 +1,19 @@ +(* + * 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 + +let is_nonnull nullsafe_nullability = + match nullsafe_nullability with + | NullsafeType.Nullable _ -> + false + | NullsafeType.Nonnull _ -> + true + + +let passes_assignment_rule ~lhs ~rhs = + (not (is_nonnull lhs)) || not (InferredNullability.is_nullable rhs) diff --git a/infer/src/nullsafe/NullsafeRules.mli b/infer/src/nullsafe/NullsafeRules.mli new file mode 100644 index 000000000..af8cc64ba --- /dev/null +++ b/infer/src/nullsafe/NullsafeRules.mli @@ -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 + +(** This is a single place consolidating core rules driving Nullsafe type checking. + Nullsafe enforces similar rules in different places (e.g. places dealing with fields, + function calls, assignments, local variables etc.). + Those places might have additional specifics, but core checks should be done through this class. + If you are writing a new or modifying an existing check, ask yourself if you can directly + use already existng rules from this module. + If you feel you need a rule of a completely new nature, add it to this module. + As a rule of thumb, every different "check" that is responsible for detecting issues, should query + this module instead of doing things on their own. + *) + +val passes_assignment_rule : lhs:NullsafeType.nullability -> rhs:InferredNullability.t -> bool +(** Assignment rule: No expression of nullable type is ever assigned to a location + of non-nullable type. + *) diff --git a/infer/src/nullsafe/eradicateChecks.ml b/infer/src/nullsafe/eradicateChecks.ml index fe792f0e6..10fe65996 100644 --- a/infer/src/nullsafe/eradicateChecks.ml +++ b/infer/src/nullsafe/eradicateChecks.ml @@ -326,7 +326,7 @@ let check_call_receiver tenv find_canonical_duplicate curr_pdesc node typestate type resolved_param = { num: int - ; formal: Mangled.t * InferredNullability.t * Typ.t + ; formal: AnnotatedSignature.param_signature ; actual: Exp.t * InferredNullability.t ; is_formal_propagates_nullable: bool } @@ -334,15 +334,14 @@ 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= s1, nullability_formal, t1; actual= orig_e2, nullability_actual} = + let check {num= param_num; formal; actual= orig_e2, nullability_actual} = let report () = let description = match explain_expr tenv node orig_e2 with | Some descr -> descr | None -> - "formal parameter " ^ Mangled.to_string s1 + "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 @@ -351,10 +350,12 @@ let check_call_parameters tenv find_canonical_duplicate curr_pdesc node callee_a (description, param_num, callee_pname, callee_loc, origin_descr)) (Some instr_ref) loc curr_pdesc in - if PatternMatch.type_is_class t1 then - let is_nullable_formal = InferredNullability.is_nullable nullability_formal in - let is_nullable_actual = InferredNullability.is_nullable nullability_actual in - if (not is_nullable_formal) && is_nullable_actual then report () + if PatternMatch.type_is_class formal.param_nullsafe_type.typ then + if + not + (NullsafeRules.passes_assignment_rule ~lhs:formal.param_nullsafe_type.nullability + ~rhs:nullability_actual) + then report () in let should_check_parameters = match callee_pname with diff --git a/infer/src/nullsafe/typeCheck.ml b/infer/src/nullsafe/typeCheck.ml index 5e669e2c9..d4d4f76d8 100644 --- a/infer/src/nullsafe/typeCheck.ml +++ b/infer/src/nullsafe/typeCheck.ml @@ -688,26 +688,20 @@ let typecheck_instr tenv calls_this checks (node : Procdesc.Node.t) idenv curr_p in let typestate_after_call, finally_resolved_ret = let resolve_param i (formal_param, actual_param) = - let AnnotatedSignature.{mangled; param_annotation_deprecated; param_nullsafe_type} = - formal_param - in let (orig_e2, e2), t2 = actual_param in - let inferred_nullability_formal = - InferredNullability.from_nullsafe_type param_nullsafe_type (TypeOrigin.Formal mangled) - in let _, inferred_nullability_actual, _ = typecheck_expr find_canonical_duplicate calls_this checks tenv node instr_ref curr_pdesc typestate e2 (t2, InferredNullability.create ~is_nullable:false TypeOrigin.ONone, []) loc in - let formal = (mangled, inferred_nullability_formal, param_nullsafe_type.typ) in let actual = (orig_e2, inferred_nullability_actual) in let num = i + 1 in let is_formal_propagates_nullable = - Annotations.ia_is_propagates_nullable param_annotation_deprecated + Annotations.ia_is_propagates_nullable + formal_param.AnnotatedSignature.param_annotation_deprecated in - EradicateChecks.{num; formal; actual; is_formal_propagates_nullable} + EradicateChecks.{num; formal= formal_param; actual; is_formal_propagates_nullable} in (* If the function has @PropagatesNullable params, and inferred type for each of corresponding actual param is non-nullable, inferred type for the whole result