From 46cf10741137e557e5db74f5e37bfc1c9d241d4e Mon Sep 17 00:00:00 2001 From: Mitya Lyubarskiy Date: Tue, 3 Sep 2019 03:31:17 -0700 Subject: [PATCH] [nullsafe] Remove functionality for @Present annotations Summary: `Present` annotation was an experiment made many years ago that never got into real usage. The idea was to annotate Optional<> types with Present, which means that it is safe to call get(). We don't plan to support `Present` annotation for optional types in the near future. Support of `Present` annotation requires extra levels of abstraction that make the changing the behavior and introducing new features harder. A lot of checks for nullability are written in generic way so they also check for presense. Getting rid of that will allow us to simplify our work for introducing new semantics for nullsafe. Reviewed By: ngorogiannis Differential Revision: D17153432 fbshipit-source-id: c5ea9bdf1 --- infer/man/man1/infer-full.txt | 8 -- infer/man/man1/infer-report.txt | 4 - infer/man/man1/infer.txt | 4 - infer/src/base/Config.ml | 6 - infer/src/base/Config.mli | 2 - infer/src/base/IssueType.ml | 16 --- infer/src/base/IssueType.mli | 10 +- infer/src/checkers/annotations.ml | 4 - infer/src/checkers/annotations.mli | 4 - infer/src/nullsafe/AnnotatedSignature.ml | 17 +-- infer/src/nullsafe/AnnotatedSignature.mli | 2 +- infer/src/nullsafe/eradicateChecks.ml | 94 +++++--------- infer/src/nullsafe/modelTables.ml | 23 ---- infer/src/nullsafe/modelTables.mli | 6 - infer/src/nullsafe/models.ml | 14 +-- infer/src/nullsafe/typeAnnotation.ml | 40 ++---- infer/src/nullsafe/typeAnnotation.mli | 6 +- infer/src/nullsafe/typeCheck.ml | 107 +++++----------- infer/src/nullsafe/typeErr.ml | 116 ++++++------------ infer/src/nullsafe/typeErr.mli | 10 +- .../codetoanalyze/java/eradicate/Makefile | 1 - .../java/eradicate/PresentTest.java | 92 -------------- .../codetoanalyze/java/eradicate/issues.exp | 4 - 23 files changed, 123 insertions(+), 467 deletions(-) delete mode 100644 infer/tests/codetoanalyze/java/eradicate/PresentTest.java diff --git a/infer/man/man1/infer-full.txt b/infer/man/man1/infer-full.txt index 6f538e551..dad63e257 100644 --- a/infer/man/man1/infer-full.txt +++ b/infer/man/man1/infer-full.txt @@ -379,18 +379,14 @@ OPTIONS ERADICATE_FIELD_NOT_MUTABLE (enabled by default), ERADICATE_FIELD_NOT_NULLABLE (enabled by default), ERADICATE_FIELD_OVER_ANNOTATED (enabled by default), - ERADICATE_FIELD_VALUE_ABSENT (enabled by default), ERADICATE_INCONSISTENT_SUBCLASS_PARAMETER_ANNOTATION (enabled by default), ERADICATE_INCONSISTENT_SUBCLASS_RETURN_ANNOTATION (enabled by default), ERADICATE_NULLABLE_DEREFERENCE (enabled by default), ERADICATE_PARAMETER_NOT_NULLABLE (enabled by default), - ERADICATE_PARAMETER_VALUE_ABSENT (enabled by default), ERADICATE_RETURN_NOT_NULLABLE (enabled by default), ERADICATE_RETURN_OVER_ANNOTATED (enabled by default), - ERADICATE_RETURN_VALUE_NOT_PRESENT (enabled by default), - ERADICATE_VALUE_NOT_PRESENT (enabled by default), EXECUTION_TIME_COMPLEXITY_INCREASE (enabled by default), EXECUTION_TIME_COMPLEXITY_INCREASE_COLD_START (enabled by default), @@ -1309,10 +1305,6 @@ INTERNAL OPTIONS Activates: Field over-annotated warnings (Conversely: --no-eradicate-field-over-annotated) - --eradicate-optional-present - Activates: Check for @Present annotations (Conversely: - --no-eradicate-optional-present) - --eradicate-return-over-annotated Activates: Return over-annotated warning (Conversely: --no-eradicate-return-over-annotated) diff --git a/infer/man/man1/infer-report.txt b/infer/man/man1/infer-report.txt index db2b9dc38..048eb30a7 100644 --- a/infer/man/man1/infer-report.txt +++ b/infer/man/man1/infer-report.txt @@ -128,18 +128,14 @@ OPTIONS ERADICATE_FIELD_NOT_MUTABLE (enabled by default), ERADICATE_FIELD_NOT_NULLABLE (enabled by default), ERADICATE_FIELD_OVER_ANNOTATED (enabled by default), - ERADICATE_FIELD_VALUE_ABSENT (enabled by default), ERADICATE_INCONSISTENT_SUBCLASS_PARAMETER_ANNOTATION (enabled by default), ERADICATE_INCONSISTENT_SUBCLASS_RETURN_ANNOTATION (enabled by default), ERADICATE_NULLABLE_DEREFERENCE (enabled by default), ERADICATE_PARAMETER_NOT_NULLABLE (enabled by default), - ERADICATE_PARAMETER_VALUE_ABSENT (enabled by default), ERADICATE_RETURN_NOT_NULLABLE (enabled by default), ERADICATE_RETURN_OVER_ANNOTATED (enabled by default), - ERADICATE_RETURN_VALUE_NOT_PRESENT (enabled by default), - ERADICATE_VALUE_NOT_PRESENT (enabled by default), EXECUTION_TIME_COMPLEXITY_INCREASE (enabled by default), EXECUTION_TIME_COMPLEXITY_INCREASE_COLD_START (enabled by default), diff --git a/infer/man/man1/infer.txt b/infer/man/man1/infer.txt index d46fea83f..ba1154b0c 100644 --- a/infer/man/man1/infer.txt +++ b/infer/man/man1/infer.txt @@ -379,18 +379,14 @@ OPTIONS ERADICATE_FIELD_NOT_MUTABLE (enabled by default), ERADICATE_FIELD_NOT_NULLABLE (enabled by default), ERADICATE_FIELD_OVER_ANNOTATED (enabled by default), - ERADICATE_FIELD_VALUE_ABSENT (enabled by default), ERADICATE_INCONSISTENT_SUBCLASS_PARAMETER_ANNOTATION (enabled by default), ERADICATE_INCONSISTENT_SUBCLASS_RETURN_ANNOTATION (enabled by default), ERADICATE_NULLABLE_DEREFERENCE (enabled by default), ERADICATE_PARAMETER_NOT_NULLABLE (enabled by default), - ERADICATE_PARAMETER_VALUE_ABSENT (enabled by default), ERADICATE_RETURN_NOT_NULLABLE (enabled by default), ERADICATE_RETURN_OVER_ANNOTATED (enabled by default), - ERADICATE_RETURN_VALUE_NOT_PRESENT (enabled by default), - ERADICATE_VALUE_NOT_PRESENT (enabled by default), EXECUTION_TIME_COMPLEXITY_INCREASE (enabled by default), EXECUTION_TIME_COMPLEXITY_INCREASE_COLD_START (enabled by default), diff --git a/infer/src/base/Config.ml b/infer/src/base/Config.ml index bf289adf9..6c3581340 100644 --- a/infer/src/base/Config.ml +++ b/infer/src/base/Config.ml @@ -1407,10 +1407,6 @@ and eradicate_field_over_annotated = CLOpt.mk_bool ~long:"eradicate-field-over-annotated" "Field over-annotated warnings" -and eradicate_optional_present = - CLOpt.mk_bool ~long:"eradicate-optional-present" "Check for @Present annotations" - - and eradicate_return_over_annotated = CLOpt.mk_bool ~long:"eradicate-return-over-annotated" "Return over-annotated warning" @@ -2864,8 +2860,6 @@ and eradicate_field_not_mutable = !eradicate_field_not_mutable and eradicate_field_over_annotated = !eradicate_field_over_annotated -and eradicate_optional_present = !eradicate_optional_present - and eradicate_return_over_annotated = !eradicate_return_over_annotated and eradicate_verbose = !eradicate_verbose diff --git a/infer/src/base/Config.mli b/infer/src/base/Config.mli index 63f44123e..4c9b9dee9 100644 --- a/infer/src/base/Config.mli +++ b/infer/src/base/Config.mli @@ -352,8 +352,6 @@ val eradicate_field_not_mutable : bool val eradicate_field_over_annotated : bool -val eradicate_optional_present : bool - val eradicate_return_over_annotated : bool val eradicate_verbose : bool diff --git a/infer/src/base/IssueType.ml b/infer/src/base/IssueType.ml index c88c1210e..cce8f2113 100644 --- a/infer/src/base/IssueType.ml +++ b/infer/src/base/IssueType.ml @@ -243,10 +243,6 @@ let eradicate_field_over_annotated = register_from_string "ERADICATE_FIELD_OVER_ANNOTATED" ~hum:"Field Over Annotated" -let eradicate_field_value_absent = - register_from_string "ERADICATE_FIELD_VALUE_ABSENT" ~hum:"Field Value Absent" - - let eradicate_inconsistent_subclass_parameter_annotation = register_from_string "ERADICATE_INCONSISTENT_SUBCLASS_PARAMETER_ANNOTATION" ~hum:"Inconsistent Subclass Parameter Annotation" @@ -265,10 +261,6 @@ let eradicate_parameter_not_nullable = register_from_string "ERADICATE_PARAMETER_NOT_NULLABLE" ~hum:"Parameter Not Nullable" -let eradicate_parameter_value_absent = - register_from_string "ERADICATE_PARAMETER_VALUE_ABSENT" ~hum:"Parameter Value Absent" - - let eradicate_return_not_nullable = register_from_string "ERADICATE_RETURN_NOT_NULLABLE" ~hum:"Return Not Nullable" @@ -277,14 +269,6 @@ let eradicate_return_over_annotated = register_from_string "ERADICATE_RETURN_OVER_ANNOTATED" ~hum:"Return Over Annotated" -let eradicate_return_value_not_present = - register_from_string "ERADICATE_RETURN_VALUE_NOT_PRESENT" ~hum:"Return Value Not Present" - - -let eradicate_value_not_present = - register_from_string "ERADICATE_VALUE_NOT_PRESENT" ~hum:"Value Not Present" - - let expensive_cost_call ~kind ~is_on_cold_start = register_from_cost_string ~enabled:false ~kind ~is_on_cold_start "EXPENSIVE_%s" diff --git a/infer/src/base/IssueType.mli b/infer/src/base/IssueType.mli index 9b5617618..047ad8566 100644 --- a/infer/src/base/IssueType.mli +++ b/infer/src/base/IssueType.mli @@ -32,7 +32,7 @@ val register_from_string : `enabled`. This trick allows to deal with disabling/enabling dynamic AL issues from the config, when we don't know all params yet. Thus, the human-readable description can be updated when we encounter the - definition of the issue type, eg in AL. + definition of the issue type, eg in AL. *) val set_enabled : t -> bool -> unit @@ -152,8 +152,6 @@ val eradicate_field_not_nullable : t val eradicate_field_over_annotated : t -val eradicate_field_value_absent : t - val eradicate_inconsistent_subclass_parameter_annotation : t val eradicate_inconsistent_subclass_return_annotation : t @@ -162,16 +160,10 @@ val eradicate_nullable_dereference : t val eradicate_parameter_not_nullable : t -val eradicate_parameter_value_absent : t - val eradicate_return_not_nullable : t val eradicate_return_over_annotated : t -val eradicate_return_value_not_present : t - -val eradicate_value_not_present : t - val expensive_cost_call : kind:CostKind.t -> is_on_cold_start:bool -> t val exposed_insecure_intent_handling : t diff --git a/infer/src/checkers/annotations.ml b/infer/src/checkers/annotations.ml index 23b77f26b..6aa9d5760 100644 --- a/infer/src/checkers/annotations.ml +++ b/infer/src/checkers/annotations.ml @@ -69,8 +69,6 @@ let not_thread_safe = "NotThreadSafe" let performance_critical = "PerformanceCritical" -let present = "Present" - let prop = "Prop" let propagates_nullable = "PropagatesNullable" @@ -163,8 +161,6 @@ let ia_is_propagates_nullable ia = ia_ends_with ia propagates_nullable let ia_is_nullable ia = ia_ends_with ia nullable || ia_is_propagates_nullable ia -let ia_is_present ia = ia_ends_with ia present - let ia_is_nonnull ia = List.exists ~f:(ia_ends_with ia) [nonnull; notnull; camel_nonnull] let ia_is_false_on_null ia = ia_ends_with ia false_on_null diff --git a/infer/src/checkers/annotations.mli b/infer/src/checkers/annotations.mli index 1b23bf558..677dfc1ef 100644 --- a/infer/src/checkers/annotations.mli +++ b/infer/src/checkers/annotations.mli @@ -25,8 +25,6 @@ val nonnull : string val performance_critical : string -val present : string - val prop : string val for_non_ui_thread : string @@ -83,8 +81,6 @@ val ia_is_nonnull : Annot.Item.t -> bool val ia_is_nullable : Annot.Item.t -> bool -val ia_is_present : Annot.Item.t -> bool - val ia_is_true_on_null : Annot.Item.t -> bool val ia_is_verify : Annot.Item.t -> bool diff --git a/infer/src/nullsafe/AnnotatedSignature.ml b/infer/src/nullsafe/AnnotatedSignature.ml index f83e4e642..34f07bad8 100644 --- a/infer/src/nullsafe/AnnotatedSignature.ml +++ b/infer/src/nullsafe/AnnotatedSignature.ml @@ -11,15 +11,9 @@ module L = Logging type t = {ret: Annot.Item.t * Typ.t; params: (Mangled.t * Annot.Item.t * Typ.t) list} [@@deriving compare] -type annotation = Nullable | Present [@@deriving compare] - -let ia_is ann ia = - match ann with - | Nullable -> - Annotations.ia_is_nullable ia - | Present -> - Annotations.ia_is_present ia +type annotation = Nullable [@@deriving compare] +let ia_is ann ia = match ann with Nullable -> Annotations.ia_is_nullable ia let get proc_attributes : t = let method_annotation = proc_attributes.ProcAttributes.method_annotation in @@ -64,12 +58,7 @@ let pp proc_name fmt annotated_signature = let mk_ann_str s = {Annot.class_name= s; parameters= []} -let mk_ann = function - | Nullable -> - mk_ann_str Annotations.nullable - | Present -> - mk_ann_str Annotations.present - +let mk_ann = function Nullable -> mk_ann_str Annotations.nullable let mk_ia ann ia = if ia_is ann ia then ia else (mk_ann ann, true) :: ia diff --git a/infer/src/nullsafe/AnnotatedSignature.mli b/infer/src/nullsafe/AnnotatedSignature.mli index e438bcf12..7c71aa6dc 100644 --- a/infer/src/nullsafe/AnnotatedSignature.mli +++ b/infer/src/nullsafe/AnnotatedSignature.mli @@ -14,7 +14,7 @@ type t = ; params: (Mangled.t * Annot.Item.t * Typ.t) list (** Annotated parameters. *) } [@@deriving compare] -type annotation = Nullable | Present [@@deriving compare] +type annotation = Nullable [@@deriving compare] val param_has_annot : (Annot.Item.t -> bool) -> Pvar.t -> t -> bool (** Check if the given parameter has an annotation in the given signature *) diff --git a/infer/src/nullsafe/eradicateChecks.ml b/infer/src/nullsafe/eradicateChecks.ml index 6fa2e434d..419757500 100644 --- a/infer/src/nullsafe/eradicateChecks.ml +++ b/infer/src/nullsafe/eradicateChecks.ml @@ -39,7 +39,7 @@ let is_virtual = function (p, _, _) :: _ when Mangled.is_this p -> true | _ -> f (** 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 : unit = - if TypeAnnotation.get_value AnnotatedSignature.Nullable ta then + if TypeAnnotation.is_nullable ta then let origin_descr = TypeAnnotation.descr_origin ta in report_error tenv find_canonical_duplicate (TypeErr.Null_field_access (explain_expr tenv node exp, fname, origin_descr, false)) @@ -49,7 +49,7 @@ let check_field_access tenv find_canonical_duplicate curr_pname node instr_ref e (** Check an access to an array *) let check_array_access tenv find_canonical_duplicate curr_pname node instr_ref array_exp fname ta loc indexed = - if TypeAnnotation.get_value AnnotatedSignature.Nullable ta then + if TypeAnnotation.is_nullable ta then let origin_descr = TypeAnnotation.descr_origin ta in report_error tenv find_canonical_duplicate (TypeErr.Null_field_access (explain_expr tenv node array_exp, fname, origin_descr, indexed)) @@ -62,7 +62,6 @@ type from_call = | From_instanceof (** x instanceof C *) | From_is_false_on_null (** returns false on null *) | From_is_true_on_null (** returns true on null *) - | From_optional_isPresent (** x.isPresent *) | From_containsKey (** x.containsKey *) [@@deriving compare] @@ -117,7 +116,7 @@ let check_condition tenv case_zero find_canonical_duplicate curr_pdesc node e ty let is_temp = Idenv.exp_is_temp idenv e in let nonnull = is_fun_nonnull ta in let should_report = - (not (TypeAnnotation.get_value AnnotatedSignature.Nullable ta)) + (not (TypeAnnotation.is_nullable ta)) && (Config.eradicate_condition_redundant || nonnull) && true_branch && ((not is_temp) || nonnull) && PatternMatch.type_is_class typ && (not (from_try_with_resources ())) @@ -147,12 +146,12 @@ let check_field_assignment tenv find_canonical_duplicate curr_pdesc node instr_r let curr_pattrs = Procdesc.get_attributes curr_pdesc in let t_lhs, ta_lhs, _ = typecheck_expr node instr_ref curr_pdesc typestate exp_lhs - (typ, TypeAnnotation.const AnnotatedSignature.Nullable false TypeOrigin.ONone, [loc]) + (typ, TypeAnnotation.const_nullable false TypeOrigin.ONone, [loc]) loc in let _, ta_rhs, _ = typecheck_expr node instr_ref curr_pdesc typestate exp_rhs - (typ, TypeAnnotation.const AnnotatedSignature.Nullable false TypeOrigin.ONone, [loc]) + (typ, TypeAnnotation.const_nullable false TypeOrigin.ONone, [loc]) loc in let field_is_injector_readwrite () = @@ -168,19 +167,12 @@ let check_field_assignment tenv find_canonical_duplicate curr_pdesc node instr_r in let should_report_nullable = (not (AndroidFramework.is_destroy_method curr_pname)) - && (not (TypeAnnotation.get_value AnnotatedSignature.Nullable ta_lhs)) - && TypeAnnotation.get_value AnnotatedSignature.Nullable ta_rhs - && PatternMatch.type_is_class t_lhs + && (not (TypeAnnotation.is_nullable ta_lhs)) + && TypeAnnotation.is_nullable ta_rhs && PatternMatch.type_is_class t_lhs && (not (Typ.Fieldname.Java.is_outer_instance fname)) && (not (field_is_injector_readwrite ())) && not (field_is_in_cleanup_context ()) in - let should_report_absent = - Config.eradicate_optional_present - && TypeAnnotation.get_value AnnotatedSignature.Present ta_lhs - && (not (TypeAnnotation.get_value AnnotatedSignature.Present ta_rhs)) - && not (Typ.Fieldname.Java.is_outer_instance fname) - in let should_report_mutable = let field_is_mutable () = match t_ia_opt with Some (_, ia) -> Annotations.ia_is_mutable ia | _ -> false @@ -194,13 +186,10 @@ let check_field_assignment tenv find_canonical_duplicate curr_pdesc node instr_r true ) && not (field_is_mutable ()) in - ( if should_report_nullable || should_report_absent then - let ann = - if should_report_nullable then AnnotatedSignature.Nullable else AnnotatedSignature.Present - in + ( if should_report_nullable then let origin_descr = TypeAnnotation.descr_origin ta_rhs in report_error tenv find_canonical_duplicate - (TypeErr.Field_annotation_inconsistent (ann, fname, origin_descr)) + (TypeErr.Field_annotation_inconsistent (fname, origin_descr)) (Some instr_ref) loc curr_pdesc ) ; if should_report_mutable then let origin_descr = TypeAnnotation.descr_origin ta_rhs in @@ -253,7 +242,7 @@ let check_constructor_initialization tenv find_canonical_duplicate curr_pname cu in let may_be_nullable_in_final_typestate () = final_type_annotation_with true (Lazy.force final_constructor_typestates) (fun ta -> - TypeAnnotation.get_value AnnotatedSignature.Nullable ta ) + TypeAnnotation.is_nullable ta ) in let should_check_field_initialization = let in_current_class = @@ -300,7 +289,6 @@ let check_return_annotation tenv find_canonical_duplicate curr_pdesc ret_range ~f:(fun (_, ia, _) -> Annotations.ia_is_propagates_nullable ia) annotated_signature.params in - let ret_annotated_present = Annotations.ia_is_present ret_ia in match ret_range with (* Disables the warnings since it is not clear how to annotate the return value of lambdas *) | Some _ @@ -311,25 +299,18 @@ let check_return_annotation tenv find_canonical_duplicate curr_pdesc ret_range false -> () | Some (_, final_ta, _) -> - let final_nullable = TypeAnnotation.get_value AnnotatedSignature.Nullable final_ta in - let final_present = TypeAnnotation.get_value AnnotatedSignature.Present final_ta in + let is_final_nullable = TypeAnnotation.is_nullable final_ta in let origin_descr = TypeAnnotation.descr_origin final_ta in let return_not_nullable = - final_nullable && (not ret_annotated_nullable) && not ret_implicitly_nullable - in - let return_value_not_present = - Config.eradicate_optional_present && (not final_present) && ret_annotated_present + is_final_nullable && (not ret_annotated_nullable) && not ret_implicitly_nullable in let return_over_annotated = - (not final_nullable) && ret_annotated_nullable && Config.eradicate_return_over_annotated + (not is_final_nullable) && ret_annotated_nullable && Config.eradicate_return_over_annotated in - ( if return_not_nullable || return_value_not_present then - let ann = - if return_not_nullable then AnnotatedSignature.Nullable else AnnotatedSignature.Present - in + if return_not_nullable then report_error tenv find_canonical_duplicate - (TypeErr.Return_annotation_inconsistent (ann, curr_pname, origin_descr)) - None loc curr_pdesc ) ; + (TypeErr.Return_annotation_inconsistent (curr_pname, origin_descr)) + None loc curr_pdesc ; if return_over_annotated then report_error tenv find_canonical_duplicate (TypeErr.Return_over_annotated curr_pname) None loc curr_pdesc @@ -344,23 +325,15 @@ let check_call_receiver tenv find_canonical_duplicate curr_pdesc node typestate | ((original_this_e, this_e), typ) :: _ -> let _, this_ta, _ = typecheck_expr tenv node instr_ref curr_pdesc typestate this_e - (typ, TypeAnnotation.const AnnotatedSignature.Nullable false TypeOrigin.ONone, []) + (typ, TypeAnnotation.const_nullable false TypeOrigin.ONone, []) loc in - let null_method_call = TypeAnnotation.get_value AnnotatedSignature.Nullable this_ta in - let optional_get_on_absent = - Config.eradicate_optional_present - && Models.is_optional_get callee_pname - && not (TypeAnnotation.get_value AnnotatedSignature.Present this_ta) - in - if null_method_call || optional_get_on_absent then - let ann = - if null_method_call then AnnotatedSignature.Nullable else AnnotatedSignature.Present - in + let null_method_call = TypeAnnotation.is_nullable this_ta in + if null_method_call then let descr = explain_expr tenv node original_this_e in let origin_descr = TypeAnnotation.descr_origin this_ta in report_error tenv find_canonical_duplicate - (TypeErr.Call_receiver_annotation_inconsistent (ann, descr, callee_pname, origin_descr)) + (TypeErr.Call_receiver_annotation_inconsistent (descr, callee_pname, origin_descr)) (Some instr_ref) loc curr_pdesc | [] -> () @@ -376,8 +349,9 @@ 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, ta1, t1; actual= orig_e2, ta2} = - let report ann = + let check {num= param_num; formal= s1, annotation_formal, t1; actual= orig_e2, annotation_actual} + = + let report () = let description = match explain_expr tenv node orig_e2 with | Some descr -> @@ -385,27 +359,17 @@ let check_call_parameters tenv find_canonical_duplicate curr_pdesc node callee_a | None -> "formal parameter " ^ Mangled.to_string s1 in - let origin_descr = TypeAnnotation.descr_origin ta2 in + let origin_descr = TypeAnnotation.descr_origin annotation_actual in let callee_loc = callee_attributes.ProcAttributes.loc in report_error tenv find_canonical_duplicate (TypeErr.Parameter_annotation_inconsistent - (ann, description, param_num, callee_pname, callee_loc, origin_descr)) + (description, param_num, callee_pname, callee_loc, origin_descr)) (Some instr_ref) loc curr_pdesc in - let check_ann ann = - let b1 = TypeAnnotation.get_value ann ta1 in - let b2 = TypeAnnotation.get_value ann ta2 in - match (ann, b1, b2) with - | AnnotatedSignature.Nullable, false, true -> - report ann - | AnnotatedSignature.Present, true, false -> - report ann - | _ -> - () - in - if PatternMatch.type_is_class t1 then ( - check_ann AnnotatedSignature.Nullable ; - if Config.eradicate_optional_present then check_ann AnnotatedSignature.Present ) + if PatternMatch.type_is_class t1 then + let is_nullable_formal = TypeAnnotation.is_nullable annotation_formal in + let is_nullable_actual = TypeAnnotation.is_nullable annotation_actual in + if (not is_nullable_formal) && is_nullable_actual then report () in let should_check_parameters = match callee_pname with diff --git a/infer/src/nullsafe/modelTables.ml b/infer/src/nullsafe/modelTables.ml index becaebbb3..df7091079 100644 --- a/infer/src/nullsafe/modelTables.ml +++ b/infer/src/nullsafe/modelTables.ml @@ -182,16 +182,6 @@ let check_argument_list = ) ] -let optional_get_list : ((_ * bool list) * _) list = - [ ((o, []), "Optional.get():java.lang.Object") - ; ((o, []), "com.google.common.base.Optional.get():java.lang.Object") ] - - -let optional_isPresent_list : ((_ * bool list) * _) list = - [ ((o, []), "Optional.isPresent():boolean") - ; ((o, []), "com.google.common.base.Optional.isPresent():boolean") ] - - (** Models for boolean functions that return true on null. *) let true_on_null_list : ((_ * bool list) * _) list = [ (n1, "android.text.TextUtils.isEmpty(java.lang.CharSequence):boolean") @@ -581,13 +571,6 @@ let annotated_list_nullable = ; (ng, "java.util.concurrent.atomic.AtomicReference.get():java.lang.Object") ] -(** Models for @Present annotations *) -let annotated_list_present = - [ ((n, [o]), "Optional.of(java.lang.Object):Optional") - ; ( (n, [o]) - , "com.google.common.base.Optional.of(java.lang.Object):com.google.common.base.Optional" ) ] - - (** Models for methods that do not return *) let noreturn_list = [((o, [o]), "java.lang.System.exit(int):void")] @@ -603,8 +586,6 @@ let this_file = Filename.basename __FILE__ let annotated_table_nullable = mk_table annotated_list_nullable -let annotated_table_present = mk_table annotated_list_present - let check_not_null_table, check_not_null_parameter_table = (mk_table check_not_null_list, mk_table check_not_null_parameter_list) @@ -617,10 +598,6 @@ let containsKey_table = mk_table containsKey_list let mapPut_table = mk_table mapPut_list -let optional_get_table = mk_table optional_get_list - -let optional_isPresent_table = mk_table optional_isPresent_list - let noreturn_table = mk_table noreturn_list let true_on_null_table = mk_table true_on_null_list diff --git a/infer/src/nullsafe/modelTables.mli b/infer/src/nullsafe/modelTables.mli index 7b7ca65ea..268a3eb93 100644 --- a/infer/src/nullsafe/modelTables.mli +++ b/infer/src/nullsafe/modelTables.mli @@ -14,8 +14,6 @@ val this_file : string val annotated_table_nullable : model_table_t -val annotated_table_present : model_table_t - val check_not_null_table : model_table_t val check_not_null_parameter_table : (string, int) Caml.Hashtbl.t @@ -28,10 +26,6 @@ val containsKey_table : model_table_t val mapPut_table : model_table_t -val optional_get_table : model_table_t - -val optional_isPresent_table : model_table_t - val noreturn_table : model_table_t val true_on_null_table : model_table_t diff --git a/infer/src/nullsafe/models.ml b/infer/src/nullsafe/models.ml index 4c07862c2..13535fa87 100644 --- a/infer/src/nullsafe/models.ml +++ b/infer/src/nullsafe/models.ml @@ -38,13 +38,7 @@ let get_modelled_annotated_signature proc_attributes = AnnotatedSignature.mark proc_name AnnotatedSignature.Nullable ann_sig mark with Caml.Not_found -> ann_sig in - let lookup_models_present ann_sig = - try - let mark = Hashtbl.find annotated_table_present proc_id in - AnnotatedSignature.mark proc_name AnnotatedSignature.Present ann_sig mark - with Caml.Not_found -> ann_sig - in - annotated_signature |> lookup_models_nullable |> lookup_models_present + annotated_signature |> lookup_models_nullable (** Return true when the procedure has been modelled for nullable. *) @@ -80,12 +74,6 @@ let is_check_argument proc_name = (** Check if the procedure does not return. *) let is_noreturn proc_name = table_has_procedure noreturn_table proc_name -(** Check if the procedure is Optional.get(). *) -let is_optional_get proc_name = table_has_procedure optional_get_table proc_name - -(** Check if the procedure is Optional.isPresent(). *) -let is_optional_isPresent proc_name = table_has_procedure optional_isPresent_table proc_name - (** Check if the procedure returns true on null. *) let is_true_on_null proc_name = table_has_procedure true_on_null_table proc_name diff --git a/infer/src/nullsafe/typeAnnotation.ml b/infer/src/nullsafe/typeAnnotation.ml index e68a7d9b1..d9c04e075 100644 --- a/infer/src/nullsafe/typeAnnotation.ml +++ b/infer/src/nullsafe/typeAnnotation.ml @@ -17,20 +17,12 @@ type t = {map: bool AnnotationsMap.t; origin: TypeOrigin.t} [@@deriving compare] let equal = [%compare.equal: t] -let get_value ann ta = try AnnotationsMap.find ann ta.map with Caml.Not_found -> false +let is_nullable ta = try AnnotationsMap.find Nullable ta.map with Caml.Not_found -> false -let set_value ann b ta = - if Bool.equal (get_value ann ta) b then ta else {ta with map= AnnotationsMap.add ann b ta.map} +let set_nullable b ta = + if Bool.equal (is_nullable ta) b then ta else {ta with map= AnnotationsMap.add Nullable b ta.map} -let get_nullable = get_value AnnotatedSignature.Nullable - -let get_present = get_value Present - -let set_nullable b = set_value Nullable b - -let set_present b = set_value Present b - let descr_origin ta = let descr_opt = TypeOrigin.get_description ta.origin in match descr_opt with @@ -40,22 +32,17 @@ let descr_origin ta = ("(Origin: " ^ str ^ ")", loc_opt, sig_opt) -let to_string ta = - let nullable_s = if get_nullable ta then " @Nullable" else "" in - let present_s = if get_present ta then " @Present" else "" in - nullable_s ^ present_s - +let to_string ta = if is_nullable ta then " @Nullable" else "" let join ta1 ta2 = - let nul1, nul2 = (get_nullable ta1, get_nullable ta2) in + let nul1, nul2 = (is_nullable ta1, is_nullable ta2) in let choose_left = match (nul1, nul2) with false, true -> false | _ -> true in let ta_chosen, ta_other = if choose_left then (ta1, ta2) else (ta2, ta1) in - let present = get_present ta1 && get_present ta2 in let origin = if Bool.equal nul1 nul2 then TypeOrigin.join ta_chosen.origin ta_other.origin else ta_chosen.origin in - let ta' = set_present present {ta_chosen with origin} in + let ta' = {ta_chosen with origin} in if equal ta' ta1 then None else Some ta' @@ -69,20 +56,11 @@ let origin_is_fun_library ta = false -let const annotation b origin = - let nullable, present = - match annotation with - | AnnotatedSignature.Nullable -> - (b, false) - | AnnotatedSignature.Present -> - (false, b) - in +let const_nullable b origin = let ta = {origin; map= AnnotationsMap.empty} in - set_present present (set_nullable nullable ta) + set_nullable b ta let with_origin ta o = {ta with origin= o} -let from_item_annotation ia origin = - let ta = const Nullable (Annotations.ia_is_nullable ia) origin in - set_value Present (Annotations.ia_is_present ia) ta +let from_item_annotation ia origin = const_nullable (Annotations.ia_is_nullable ia) origin diff --git a/infer/src/nullsafe/typeAnnotation.mli b/infer/src/nullsafe/typeAnnotation.mli index b26659489..8aba0b72f 100644 --- a/infer/src/nullsafe/typeAnnotation.mli +++ b/infer/src/nullsafe/typeAnnotation.mli @@ -11,7 +11,7 @@ open! IStd type t [@@deriving compare] -val const : AnnotatedSignature.annotation -> bool -> TypeOrigin.t -> t +val const_nullable : bool -> TypeOrigin.t -> t val descr_origin : t -> TypeErr.origin_descr (** Human-readable description of the origin of a nullable value. *) @@ -20,13 +20,13 @@ val from_item_annotation : Annot.Item.t -> TypeOrigin.t -> t val get_origin : t -> TypeOrigin.t -val get_value : AnnotatedSignature.annotation -> t -> bool +val is_nullable : t -> bool val join : t -> t -> t option val origin_is_fun_library : t -> bool -val set_value : AnnotatedSignature.annotation -> bool -> t -> t +val set_nullable : bool -> t -> t val to_string : t -> string diff --git a/infer/src/nullsafe/typeCheck.ml b/infer/src/nullsafe/typeCheck.ml index fde696115..80c0ac684 100644 --- a/infer/src/nullsafe/typeCheck.ml +++ b/infer/src/nullsafe/typeCheck.ml @@ -13,8 +13,6 @@ module DExp = DecompiledExp (** Module to treat selected complex expressions as constants. *) module ComplexExpressions = struct - let procname_optional_isPresent = Models.is_optional_isPresent - let procname_instanceof = Typ.Procname.equal BuiltinDecl.__instanceof let procname_is_false_on_null tenv pn = @@ -44,8 +42,7 @@ module ComplexExpressions = struct (** Recognize *all* the procedures treated specially in conditionals *) let procname_used_in_condition pn = - procname_optional_isPresent pn || procname_instanceof pn || procname_containsKey pn - || BuiltinDecl.is_declared pn + procname_instanceof pn || procname_containsKey pn || BuiltinDecl.is_declared pn exception Not_handled @@ -118,7 +115,7 @@ let rec typecheck_expr find_canonical_duplicate visited checks tenv node instr_r | _ when Exp.is_null_literal e -> let typ, ta, locs = tr_default in if PatternMatch.type_is_class typ then - (typ, TypeAnnotation.const AnnotatedSignature.Nullable true (TypeOrigin.Const loc), locs) + (typ, TypeAnnotation.const_nullable true (TypeOrigin.Const loc), locs) else (typ, TypeAnnotation.with_origin ta (TypeOrigin.Const loc), locs) | Exp.Lvar pvar -> ( match TypeState.lookup_pvar pvar typestate with @@ -137,13 +134,13 @@ let rec typecheck_expr find_canonical_duplicate visited checks tenv node instr_r typestate e1 tr_default loc | Exp.Const _ -> let typ, _, locs = tr_default in - (typ, TypeAnnotation.const AnnotatedSignature.Nullable false (TypeOrigin.Const loc), locs) + (typ, TypeAnnotation.const_nullable false (TypeOrigin.Const loc), locs) | Exp.Lfield (exp, fn, typ) -> let _, _, locs = tr_default in let _, ta, locs' = typecheck_expr find_canonical_duplicate visited checks tenv node instr_ref curr_pdesc typestate exp - (typ, TypeAnnotation.const AnnotatedSignature.Nullable false TypeOrigin.ONone, locs) + (typ, TypeAnnotation.const_nullable false TypeOrigin.ONone, locs) loc in let exp_origin = TypeAnnotation.get_origin ta in @@ -407,7 +404,7 @@ let typecheck_instr tenv calls_this checks (node : Procdesc.Node.t) idenv curr_p let typecheck_expr_simple typestate1 exp1 typ1 origin1 loc1 = typecheck_expr find_canonical_duplicate calls_this checks tenv node instr_ref curr_pdesc typestate1 exp1 - (typ1, TypeAnnotation.const AnnotatedSignature.Nullable false origin1, [loc1]) + (typ1, TypeAnnotation.const_nullable false origin1, [loc1]) loc1 in (* check if there are errors in exp1 *) @@ -461,7 +458,7 @@ let typecheck_instr tenv calls_this checks (node : Procdesc.Node.t) idenv curr_p when Typ.Procname.equal pn BuiltinDecl.__new || Typ.Procname.equal pn BuiltinDecl.__new_array -> TypeState.add_id id - (typ, TypeAnnotation.const AnnotatedSignature.Nullable false TypeOrigin.New, [loc]) + (typ, TypeAnnotation.const_nullable false TypeOrigin.New, [loc]) typestate (* new never returns null *) | Sil.Call ((id, _), Exp.Const (Const.Cfun pn), (e, typ) :: _, loc, _) @@ -475,7 +472,7 @@ let typecheck_instr tenv calls_this checks (node : Procdesc.Node.t) idenv curr_p let _, ta, _ = typecheck_expr find_canonical_duplicate calls_this checks tenv node instr_ref curr_pdesc typestate array_exp - (t, TypeAnnotation.const AnnotatedSignature.Nullable false TypeOrigin.ONone, [loc]) + (t, TypeAnnotation.const_nullable false TypeOrigin.ONone, [loc]) loc in if checks.eradicate then @@ -484,9 +481,7 @@ let typecheck_instr tenv calls_this checks (node : Procdesc.Node.t) idenv curr_p (Typ.Fieldname.Java.from_string "length") ta loc false ; TypeState.add_id id - ( Typ.mk (Tint Typ.IInt) - , TypeAnnotation.const AnnotatedSignature.Nullable false TypeOrigin.New - , [loc] ) + (Typ.mk (Tint Typ.IInt), TypeAnnotation.const_nullable false TypeOrigin.New, [loc]) typestate | Sil.Call (_, Exp.Const (Const.Cfun pn), _, _, _) when BuiltinDecl.is_declared pn -> typestate (* skip othe builtins *) @@ -550,7 +545,7 @@ let typecheck_instr tenv calls_this checks (node : Procdesc.Node.t) idenv curr_p | Some (t, ta, _) -> let should_report = Config.eradicate_condition_redundant - && (not (TypeAnnotation.get_value AnnotatedSignature.Nullable ta)) + && (not (TypeAnnotation.is_nullable ta)) && not (TypeAnnotation.origin_is_fun_library ta) in ( if checks.eradicate && should_report then @@ -560,7 +555,7 @@ let typecheck_instr tenv calls_this checks (node : Procdesc.Node.t) idenv curr_p (true, EradicateChecks.explain_expr tenv node cond, false)) (Some instr_ref) loc curr_pdesc ) ; TypeState.add pvar - (t, TypeAnnotation.const AnnotatedSignature.Nullable false TypeOrigin.ONone, [loc]) + (t, TypeAnnotation.const_nullable false TypeOrigin.ONone, [loc]) typestate'' | None -> typestate' @@ -592,18 +587,20 @@ let typecheck_instr tenv calls_this checks (node : Procdesc.Node.t) idenv curr_p in (* Handle Preconditions.checkState for &&-separated conditions x!=null. *) let do_preconditions_check_state typestate' = - let handle_pvar ann b typestate1 pvar = + let handle_pvar b typestate1 pvar = (* handle the annotation flag for pvar *) match TypeState.lookup_pvar pvar typestate1 with | Some (t, _, _) -> - TypeState.add pvar (t, TypeAnnotation.const ann b TypeOrigin.ONone, [loc]) typestate1 + TypeState.add pvar + (t, TypeAnnotation.const_nullable b TypeOrigin.ONone, [loc]) + typestate1 | None -> typestate1 in let res_typestate = ref typestate' in - let set_flag pvar ann b = + let set_nullable_flag pvar b = (* set the annotation flag for pvar *) - res_typestate := pvar_apply loc (handle_pvar ann b) !res_typestate pvar + res_typestate := pvar_apply loc (handle_pvar b) !res_typestate pvar in let handle_negated_condition cond_node = let do_instr instr = @@ -611,7 +608,7 @@ let typecheck_instr tenv calls_this checks (node : Procdesc.Node.t) idenv curr_p let cond_e = Idenv.expand_expr_temps idenv cond_node expression in match convert_complex_exp_to_pvar cond_node false cond_e typestate' loc with | Exp.Lvar pvar', _ -> - set_flag pvar' AnnotatedSignature.Nullable false + set_nullable_flag pvar' false | _ -> () in @@ -627,13 +624,6 @@ let typecheck_instr tenv calls_this checks (node : Procdesc.Node.t) idenv curr_p in Instrs.iter ~f:do_instr (Procdesc.Node.get_instrs cond_node) in - let handle_optional_isPresent node' e = - match convert_complex_exp_to_pvar node' false e typestate' loc with - | Exp.Lvar pvar', _ -> - set_flag pvar' AnnotatedSignature.Present true - | _ -> - () - in match call_params with | ((_, Exp.Lvar pvar), _) :: _ -> ( (* temporary variable for the value of the boolean condition *) @@ -647,19 +637,6 @@ let typecheck_instr tenv calls_this checks (node : Procdesc.Node.t) idenv curr_p (Procdesc.Node.get_preds boolean_assignment_node) ; !res_typestate | None -> - ( match Errdesc.find_program_variable_assignment curr_node pvar with - | None -> - () - | Some (node', id) -> - let () = - match Errdesc.find_normal_variable_funcall node' id with - | Some (Exp.Const (Const.Cfun pn), [e], _, _) - when ComplexExpressions.procname_optional_isPresent pn -> - handle_optional_isPresent node' e - | _ -> - () - in - () ) ; !res_typestate ) | _ -> typestate' @@ -712,14 +689,14 @@ let typecheck_instr tenv calls_this checks (node : Procdesc.Node.t) idenv curr_p let _, ta2, _ = typecheck_expr find_canonical_duplicate calls_this checks tenv node instr_ref curr_pdesc typestate e2 - (t2, TypeAnnotation.const AnnotatedSignature.Nullable false TypeOrigin.ONone, []) + (t2, TypeAnnotation.const_nullable false TypeOrigin.ONone, []) loc in let formal = (s1, ta1, t1) 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 actual_is_nullable = TypeAnnotation.get_value AnnotatedSignature.Nullable ta2 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} in @@ -733,14 +710,10 @@ let typecheck_instr tenv calls_this checks (node : Procdesc.Node.t) idenv curr_p let resolved_ret' = let ret_ta, ret_typ = resolved_ret in let ret_ta' = - let actual_nullable = - TypeAnnotation.get_value AnnotatedSignature.Nullable actual_ta - in - let old_nullable = - TypeAnnotation.get_value AnnotatedSignature.Nullable ret_ta - in - let new_nullable = old_nullable || actual_nullable in - TypeAnnotation.set_value AnnotatedSignature.Nullable new_nullable ret_ta + let is_actual_nullable = TypeAnnotation.is_nullable actual_ta in + let is_old_nullable = TypeAnnotation.is_nullable ret_ta in + let is_new_nullable = is_old_nullable || is_actual_nullable in + TypeAnnotation.set_nullable is_new_nullable ret_ta in (ret_ta', ret_typ) in @@ -834,10 +807,6 @@ let typecheck_instr tenv calls_this checks (node : Procdesc.Node.t) idenv curr_p let from_instanceof e : Exp.t option = from_call ComplexExpressions.procname_instanceof e in - (* check if the expression is coming from Optional.isPresent *) - let from_optional_isPresent e : Exp.t option = - from_call ComplexExpressions.procname_optional_isPresent e - in (* check if the expression is coming from a procedure returning false on null *) let from_is_false_on_null e : Exp.t option = from_call (ComplexExpressions.procname_is_false_on_null tenv) e @@ -882,13 +851,13 @@ let typecheck_instr tenv calls_this checks (node : Procdesc.Node.t) idenv curr_p | None -> (typestate, e, EradicateChecks.From_condition) in - let set_flag e' ann b typestate2 = + let set_nullable_flag e' b typestate2 = (* add constraint on e' for annotation ann *) let handle_pvar typestate' pvar = match TypeState.lookup_pvar pvar typestate' with | Some (t, ta1, locs) -> - if TypeAnnotation.get_value ann ta1 <> b then - let ta2 = TypeAnnotation.set_value ann b ta1 in + if TypeAnnotation.is_nullable ta1 <> b then + let ta2 = TypeAnnotation.set_nullable b ta1 in TypeState.add pvar (t, ta2, locs) typestate' else typestate' | None -> @@ -922,8 +891,7 @@ let typecheck_instr tenv calls_this checks (node : Procdesc.Node.t) idenv curr_p match from_call with | EradicateChecks.From_is_true_on_null -> (* if f returns true on null, then false branch implies != null *) - if TypeAnnotation.get_value AnnotatedSignature.Nullable ta then - set_flag e' AnnotatedSignature.Nullable false typestate2 + if TypeAnnotation.is_nullable ta then set_nullable_flag e' false typestate2 else typestate2 | _ -> typestate2 ) @@ -937,16 +905,12 @@ let typecheck_instr tenv calls_this checks (node : Procdesc.Node.t) idenv curr_p (* (e1 instanceof C) implies (e1 != null) *) (typestate, e1, EradicateChecks.From_instanceof) | None -> ( - match from_optional_isPresent e with + match from_is_false_on_null e with | Some e1 -> - (typestate, e1, EradicateChecks.From_optional_isPresent) - | None -> ( - match from_is_false_on_null e with - | Some e1 -> - (typestate, e1, EradicateChecks.From_is_false_on_null) - | None -> - if Option.is_some (from_containsKey e) then handle_containsKey e - else (typestate, e, EradicateChecks.From_condition) ) ) + (typestate, e1, EradicateChecks.From_is_false_on_null) + | None -> + if Option.is_some (from_containsKey e) then handle_containsKey e + else (typestate, e, EradicateChecks.From_condition) ) in let e', typestate2 = convert_complex_exp_to_pvar node' false e1 typestate1 loc in let typ, ta, _ = @@ -956,18 +920,13 @@ let typecheck_instr tenv calls_this checks (node : Procdesc.Node.t) idenv curr_p EradicateChecks.check_nonzero tenv find_canonical_duplicate curr_pdesc node e' typ ta true_branch from_call idenv linereader loc instr_ref ; match from_call with - | EradicateChecks.From_optional_isPresent -> - if not (TypeAnnotation.get_value AnnotatedSignature.Present ta) then - set_flag e' AnnotatedSignature.Present true typestate2 - else typestate2 | EradicateChecks.From_is_true_on_null -> typestate2 | EradicateChecks.From_condition | EradicateChecks.From_containsKey | EradicateChecks.From_instanceof | EradicateChecks.From_is_false_on_null -> - if TypeAnnotation.get_value AnnotatedSignature.Nullable ta then - set_flag e' AnnotatedSignature.Nullable false typestate2 + if TypeAnnotation.is_nullable ta then set_nullable_flag e' false typestate2 else typestate2 ) | Exp.UnOp (Unop.LNot, Exp.BinOp (Binop.Eq, e1, e2), _) -> check_condition node' (Exp.BinOp (Binop.Ne, e1, e2)) diff --git a/infer/src/nullsafe/typeErr.ml b/infer/src/nullsafe/typeErr.ml index b0aa10869..1a2446e45 100644 --- a/infer/src/nullsafe/typeErr.ml +++ b/infer/src/nullsafe/typeErr.ml @@ -63,8 +63,7 @@ type origin_descr = string * Location.t option * AnnotatedSignature.t option let compare_origin_descr _ _ = 0 type parameter_not_nullable = - AnnotatedSignature.annotation - * string + string * (* description *) int * (* parameter number *) @@ -81,13 +80,12 @@ type err_instance = | Inconsistent_subclass_parameter_annotation of string * int * Typ.Procname.t * Typ.Procname.t | Field_not_initialized of Typ.Fieldname.t * Typ.Procname.t | Field_not_mutable of Typ.Fieldname.t * origin_descr - | Field_annotation_inconsistent of AnnotatedSignature.annotation * Typ.Fieldname.t * origin_descr + | Field_annotation_inconsistent of Typ.Fieldname.t * origin_descr | Field_over_annotated of Typ.Fieldname.t * Typ.Procname.t | Null_field_access of string option * Typ.Fieldname.t * origin_descr * bool - | Call_receiver_annotation_inconsistent of - AnnotatedSignature.annotation * string option * Typ.Procname.t * origin_descr + | Call_receiver_annotation_inconsistent of string option * Typ.Procname.t * origin_descr | Parameter_annotation_inconsistent of parameter_not_nullable - | Return_annotation_inconsistent of AnnotatedSignature.annotation * Typ.Procname.t * origin_descr + | Return_annotation_inconsistent of Typ.Procname.t * origin_descr | Return_over_annotated of Typ.Procname.t [@@deriving compare] @@ -106,18 +104,18 @@ module H = Hashtbl.Make (struct Hashtbl.hash (2, string_hash (Typ.Fieldname.to_string fn ^ Typ.Procname.to_string pn)) | Field_not_mutable (fn, _) -> Hashtbl.hash (3, string_hash (Typ.Fieldname.to_string fn)) - | Field_annotation_inconsistent (ann, fn, _) -> - Hashtbl.hash (4, ann, string_hash (Typ.Fieldname.to_string fn)) + | Field_annotation_inconsistent (fn, _) -> + Hashtbl.hash (4, string_hash (Typ.Fieldname.to_string fn)) | Field_over_annotated (fn, pn) -> Hashtbl.hash (5, string_hash (Typ.Fieldname.to_string fn ^ Typ.Procname.to_string pn)) | Null_field_access (so, fn, _, _) -> Hashtbl.hash (6, string_opt_hash so, string_hash (Typ.Fieldname.to_string fn)) - | Call_receiver_annotation_inconsistent (ann, so, pn, _) -> - Hashtbl.hash (7, ann, string_opt_hash so, Typ.Procname.hash pn) - | Parameter_annotation_inconsistent (ann, s, n, pn, _, _) -> - Hashtbl.hash (8, ann, string_hash s, n, Typ.Procname.hash pn) - | Return_annotation_inconsistent (ann, pn, _) -> - Hashtbl.hash (9, ann, Typ.Procname.hash pn) + | Call_receiver_annotation_inconsistent (so, pn, _) -> + Hashtbl.hash (7, string_opt_hash so, Typ.Procname.hash pn) + | Parameter_annotation_inconsistent (s, n, pn, _, _) -> + Hashtbl.hash (8, string_hash s, n, Typ.Procname.hash pn) + | Return_annotation_inconsistent (pn, _) -> + Hashtbl.hash (9, Typ.Procname.hash pn) | Return_over_annotated pn -> Hashtbl.hash (10, Typ.Procname.hash pn) | Inconsistent_subclass_return_annotation (pn, opn) -> @@ -235,7 +233,7 @@ module Severity = struct let err_instance_get_severity tenv err_instance : Exceptions.severity option = match err_instance with - | Call_receiver_annotation_inconsistent (AnnotatedSignature.Nullable, _, _, origin_descr) + | Call_receiver_annotation_inconsistent (_, _, origin_descr) | Null_field_access (_, _, origin_descr, _) -> origin_descr_get_severity tenv origin_descr | _ -> @@ -260,7 +258,6 @@ let report_error_now tenv (st_report_error : st_report_error) err_instance loc p let pname = Procdesc.get_proc_name pdesc in let nullable_annotation = "@Nullable" in let mutable_annotation = "@Mutable" in - let present_annotation = "@Present" in let kind, description, field_name = match err_instance with | Condition_redundant (b, s_opt, nonnull) -> @@ -294,21 +291,12 @@ let report_error_now tenv (st_report_error : st_report_error) err_instance loc p (Typ.Fieldname.to_simplified_string fn) MF.pp_monospaced mutable_annotation origin_description , None ) - | Field_annotation_inconsistent (ann, fn, (origin_description, _, _)) -> + | Field_annotation_inconsistent (fn, (origin_description, _, _)) -> let kind_s, description = - match ann with - | AnnotatedSignature.Nullable -> - ( 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 ) - | AnnotatedSignature.Present -> - ( IssueType.eradicate_field_value_absent - , Format.asprintf - "Field %a is assigned a possibly absent value but is declared %a. %s" - MF.pp_monospaced - (Typ.Fieldname.to_simplified_string fn) - MF.pp_monospaced present_annotation origin_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) -> @@ -336,60 +324,34 @@ let report_error_now tenv (st_report_error : st_report_error) err_instance loc p (Typ.Fieldname.to_simplified_string fn) origin_description , None ) - | Call_receiver_annotation_inconsistent (ann, s_opt, pn, (origin_description, _, _)) -> + | Call_receiver_annotation_inconsistent (s_opt, pn, (origin_description, _, _)) -> let kind_s, description = - match ann with - | AnnotatedSignature.Nullable -> - ( IssueType.eradicate_nullable_dereference - , Format.asprintf - "The value of %a in the call to %a is nullable and is not locally checked for \ - null. %s" - MF.pp_monospaced (Option.value s_opt ~default:"") MF.pp_monospaced - (Typ.Procname.to_simplified_string pn) - origin_description ) - | AnnotatedSignature.Present -> - ( IssueType.eradicate_value_not_present - , Format.asprintf "The value of %a in the call to %a is not %a. %s" MF.pp_monospaced - (Option.value s_opt ~default:"") MF.pp_monospaced - (Typ.Procname.to_simplified_string pn) - MF.pp_monospaced present_annotation origin_description ) + ( IssueType.eradicate_nullable_dereference + , Format.asprintf + "The value of %a in the call to %a is nullable and is not locally checked for null. \ + %s" + MF.pp_monospaced (Option.value s_opt ~default:"") MF.pp_monospaced + (Typ.Procname.to_simplified_string pn) + origin_description ) in (kind_s, description, None) - | Parameter_annotation_inconsistent (ann, s, n, pn, _, (origin_desc, _, _)) -> + | Parameter_annotation_inconsistent (s, n, pn, _, (origin_desc, _, _)) -> let kind_s, description = - match ann with - | AnnotatedSignature.Nullable -> - ( 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 ) - | AnnotatedSignature.Present -> - ( IssueType.eradicate_parameter_value_absent - , Format.asprintf - "%a needs a present value in parameter %d but argument %a can be absent. %s" - MF.pp_monospaced - (Typ.Procname.to_simplified_string ~withclass:true pn) - n MF.pp_monospaced s origin_desc ) + ( 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 (ann, pn, (origin_description, _, _)) -> + | Return_annotation_inconsistent (pn, (origin_description, _, _)) -> let kind_s, description = - match ann with - | AnnotatedSignature.Nullable -> - ( 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 ) - | AnnotatedSignature.Present -> - ( IssueType.eradicate_return_value_not_present - , Format.asprintf - "Method %a may return an absent value but it is annotated with %a. %s" - MF.pp_monospaced - (Typ.Procname.to_simplified_string pn) - MF.pp_monospaced present_annotation origin_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 -> diff --git a/infer/src/nullsafe/typeErr.mli b/infer/src/nullsafe/typeErr.mli index 2aae36a28..782c68da1 100644 --- a/infer/src/nullsafe/typeErr.mli +++ b/infer/src/nullsafe/typeErr.mli @@ -35,8 +35,7 @@ type origin_descr = string * Location.t option * AnnotatedSignature.t option (* callee signature *) type parameter_not_nullable = - AnnotatedSignature.annotation - * string + string * (* description *) int * (* parameter number *) @@ -52,13 +51,12 @@ type err_instance = | Inconsistent_subclass_parameter_annotation of string * int * Typ.Procname.t * Typ.Procname.t | Field_not_initialized of Typ.Fieldname.t * Typ.Procname.t | Field_not_mutable of Typ.Fieldname.t * origin_descr - | Field_annotation_inconsistent of AnnotatedSignature.annotation * Typ.Fieldname.t * origin_descr + | Field_annotation_inconsistent of Typ.Fieldname.t * origin_descr | Field_over_annotated of Typ.Fieldname.t * Typ.Procname.t | Null_field_access of string option * Typ.Fieldname.t * origin_descr * bool - | Call_receiver_annotation_inconsistent of - AnnotatedSignature.annotation * string option * Typ.Procname.t * origin_descr + | Call_receiver_annotation_inconsistent of string option * Typ.Procname.t * origin_descr | Parameter_annotation_inconsistent of parameter_not_nullable - | Return_annotation_inconsistent of AnnotatedSignature.annotation * Typ.Procname.t * origin_descr + | Return_annotation_inconsistent of Typ.Procname.t * origin_descr | Return_over_annotated of Typ.Procname.t val node_reset_forall : Procdesc.Node.t -> unit diff --git a/infer/tests/codetoanalyze/java/eradicate/Makefile b/infer/tests/codetoanalyze/java/eradicate/Makefile index dc96fe528..aaba3f768 100644 --- a/infer/tests/codetoanalyze/java/eradicate/Makefile +++ b/infer/tests/codetoanalyze/java/eradicate/Makefile @@ -8,7 +8,6 @@ TESTS_DIR = ../../.. INFER_OPTIONS = \ --eradicate-only \ --eradicate-return-over-annotated \ - --eradicate-optional-present \ --eradicate-condition-redundant \ --debug-exceptions INFERPRINT_OPTIONS = --issues-tests diff --git a/infer/tests/codetoanalyze/java/eradicate/PresentTest.java b/infer/tests/codetoanalyze/java/eradicate/PresentTest.java deleted file mode 100644 index ab365cc8f..000000000 --- a/infer/tests/codetoanalyze/java/eradicate/PresentTest.java +++ /dev/null @@ -1,92 +0,0 @@ -/* - * 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. - */ - -package codetoanalyze.java.eradicate; - -import com.facebook.infer.annotation.Assertions; -import com.facebook.infer.annotation.Present; -import com.google.common.base.Optional; - -public class PresentTest { - - void argPresent(@Present Optional present, Optional absent) {} - - void testPresent(@Present Optional present, Optional absent) { - argPresent(present, absent); // OK - argPresent(present, present); // OK - argPresent(present, absent); // OK - argPresent(absent, absent); // Bad - } - - class TestPresentAnnotationBasic { - void testBasicConditional(Optional o) { - if (o.isPresent()) { - o.get(); - } - } - - Optional absent = Optional.absent(); - @Present Optional present = Optional.of("abc"); - - @Present - Optional returnPresent() { - if (absent.isPresent()) { - return absent; - } else return Optional.of("abc"); - } - - @Present - Optional returnPresentBad() { - absent.get(); // Bad: get is unsafe - return absent; // Bad: should return present - } - - void expectPresent(@Present Optional x) {} - - void bar() { - expectPresent(present); - String s; - s = returnPresent().get(); - s = present.get(); - - Assertions.assertCondition(absent.isPresent()); - expectPresent(absent); - } - - void testOptionalAbsent() { - expectPresent(Optional.absent()); // Bad - } - } - - class TestPresentFieldOfInnerClass { - class D { - @SuppressFieldNotInitialized Optional s; - } - - class D1 { - // Different bytecode generated when the field is private - @SuppressFieldNotInitialized private Optional s; - } - - void testD(D d) { - if (d.s.isPresent()) { - d.s.get(); - } - } - - void testD1(D1 d1) { - if (d1.s.isPresent()) { - d1.s.get(); - } - } - - void testD1Condition(D1 d1) { - Assertions.assertCondition(d1.s.isPresent()); - d1.s.get(); - } - } -} diff --git a/infer/tests/codetoanalyze/java/eradicate/issues.exp b/infer/tests/codetoanalyze/java/eradicate/issues.exp index 240894051..133e76f61 100644 --- a/infer/tests/codetoanalyze/java/eradicate/issues.exp +++ b/infer/tests/codetoanalyze/java/eradicate/issues.exp @@ -108,10 +108,6 @@ codetoanalyze/java/eradicate/ParameterNotNullable.java, codetoanalyze.java.eradi codetoanalyze/java/eradicate/ParameterNotNullable.java, codetoanalyze.java.eradicate.ParameterNotNullable.testThreeParameters():void, 2, ERADICATE_PARAMETER_NOT_NULLABLE, no_bucket, WARNING, [`ParameterNotNullable.threeParameters(...)` needs a non-null value in parameter 1 but argument `null` can be null. (Origin: null constant at line 88)] codetoanalyze/java/eradicate/ParameterNotNullable.java, codetoanalyze.java.eradicate.ParameterNotNullable.testThreeParameters():void, 3, ERADICATE_PARAMETER_NOT_NULLABLE, no_bucket, WARNING, [`ParameterNotNullable.threeParameters(...)` needs a non-null value in parameter 2 but argument `null` can be null. (Origin: null constant at line 89)] codetoanalyze/java/eradicate/ParameterNotNullable.java, codetoanalyze.java.eradicate.ParameterNotNullable.testThreeParameters():void, 4, ERADICATE_PARAMETER_NOT_NULLABLE, no_bucket, WARNING, [`ParameterNotNullable.threeParameters(...)` needs a non-null value in parameter 3 but argument `null` can be null. (Origin: null constant at line 90)] -codetoanalyze/java/eradicate/PresentTest.java, codetoanalyze.java.eradicate.PresentTest$TestPresentAnnotationBasic.returnPresentBad():com.google.common.base.Optional, 0, ERADICATE_RETURN_VALUE_NOT_PRESENT, no_bucket, WARNING, [Method `returnPresentBad()` may return an absent value but it is annotated with `@Present`. (Origin: field PresentTest$TestPresentAnnotationBasic.absent at line 44)] -codetoanalyze/java/eradicate/PresentTest.java, codetoanalyze.java.eradicate.PresentTest$TestPresentAnnotationBasic.returnPresentBad():com.google.common.base.Optional, 1, ERADICATE_VALUE_NOT_PRESENT, no_bucket, WARNING, [The value of `PresentTest$TestPresentAnnotationBasic.absent` in the call to `get()` is not `@Present`. (Origin: field PresentTest$TestPresentAnnotationBasic.absent at line 44)] -codetoanalyze/java/eradicate/PresentTest.java, codetoanalyze.java.eradicate.PresentTest$TestPresentAnnotationBasic.testOptionalAbsent():void, 1, ERADICATE_PARAMETER_VALUE_ABSENT, no_bucket, WARNING, [`PresentTest$TestPresentAnnotationBasic.expectPresent(...)` needs a present value in parameter 1 but argument `absent()` can be absent. (Origin: call to absent() at line 61)] -codetoanalyze/java/eradicate/PresentTest.java, codetoanalyze.java.eradicate.PresentTest.testPresent(com.google.common.base.Optional,com.google.common.base.Optional):void, 4, ERADICATE_PARAMETER_VALUE_ABSENT, no_bucket, WARNING, [`PresentTest.argPresent(...)` needs a present value in parameter 1 but argument `absent` can be absent. (Origin: method parameter absent)] codetoanalyze/java/eradicate/Redundant.java, Redundant.bad():void, 2, ERADICATE_CONDITION_REDUNDANT, no_bucket, WARNING, [The condition s is always true according to the existing annotations.] codetoanalyze/java/eradicate/ReturnNotNullable.java, codetoanalyze.java.eradicate.ReturnNotNullable$ConditionalAssignment.test(boolean):java.lang.Object, 0, ERADICATE_RETURN_NOT_NULLABLE, no_bucket, WARNING, [Method `test(...)` may return null but it is not annotated with `@Nullable`. (Origin: field ReturnNotNullable$ConditionalAssignment.f1 at line 213)] codetoanalyze/java/eradicate/ReturnNotNullable.java, codetoanalyze.java.eradicate.ReturnNotNullable.constantToNullableIsOverannotated():java.lang.String, 0, ERADICATE_RETURN_OVER_ANNOTATED, no_bucket, WARNING, [Method `constantToNullableIsOverannotated()` is annotated with `@Nullable` but never returns null.]