From 2572819a5b49d464fe58617d310e6a450f8f756f Mon Sep 17 00:00:00 2001 From: Artem Pianykh Date: Thu, 20 Feb 2020 09:38:40 -0800 Subject: [PATCH] [nullsafe] Directly model nullability of values from third-party code Summary: We need to be able to differentiate `UncheckedNonnull`s in internal vs third-party code. Previously, those were under one `UncheckedNonnull` nullability which led to hacks for optmistic third-party parameter checks in `eradicateChecks.ml` and lack of third-party enforcement in `Nullsafe(LOCAL, trust=all)` mode (i.e. we want to trust internal unchecked code, but don't want to trust unvetted third-party). Now such values are properly modelled and can be accounted for regularly within rules. Also, various whitelists are refactored using `Nullability.is_considered_nonnull ~nullsafe_mode nullability`. `ErrorRenderingUtils` became a tad more convoluted, but oh well, one step at a time. Reviewed By: mityal Differential Revision: D19977086 fbshipit-source-id: 8337a47b9 --- infer/src/IR/JavaClassName.ml | 4 + infer/src/IR/JavaClassName.mli | 3 + infer/src/nullsafe/AnnotatedField.ml | 8 +- infer/src/nullsafe/AnnotatedNullability.ml | 13 +- infer/src/nullsafe/AnnotatedNullability.mli | 7 +- infer/src/nullsafe/AnnotatedSignature.ml | 24 ++- infer/src/nullsafe/AssignmentRule.ml | 172 +++++++++--------- infer/src/nullsafe/DereferenceRule.ml | 38 ++-- infer/src/nullsafe/ErrorRenderingUtils.ml | 112 +++++++----- infer/src/nullsafe/ErrorRenderingUtils.mli | 2 +- infer/src/nullsafe/InferredNullability.ml | 6 +- infer/src/nullsafe/InferredNullability.mli | 5 +- infer/src/nullsafe/InheritanceRule.ml | 16 +- infer/src/nullsafe/Nullability.ml | 29 +++ infer/src/nullsafe/Nullability.mli | 12 +- .../src/nullsafe/ThirdPartyAnnotationInfo.ml | 40 +++- .../src/nullsafe/ThirdPartyAnnotationInfo.mli | 13 +- infer/src/nullsafe/eradicateChecks.ml | 61 +------ infer/src/nullsafe/typeCheck.ml | 2 +- infer/src/nullsafe/typeOrigin.ml | 1 + infer/src/nullsafe/typeState.ml | 5 +- .../java/nullsafe-default/NullsafeMode.java | 18 ++ .../StrictModeForThirdParty.java | 4 + .../java/nullsafe-default/issues.exp | 19 +- .../some/test/pckg/ThirdPartyTestClass.java | 12 ++ 25 files changed, 365 insertions(+), 261 deletions(-) diff --git a/infer/src/IR/JavaClassName.ml b/infer/src/IR/JavaClassName.ml index 830a534f1..8735750c4 100644 --- a/infer/src/IR/JavaClassName.ml +++ b/infer/src/IR/JavaClassName.ml @@ -39,3 +39,7 @@ let pp fmt = function let package {package} = package let classname {classname} = classname + +let is_external_via_config t = + let package = package t in + Option.exists ~f:Config.java_package_is_external package diff --git a/infer/src/IR/JavaClassName.mli b/infer/src/IR/JavaClassName.mli index 46c75deac..f768ce8af 100644 --- a/infer/src/IR/JavaClassName.mli +++ b/infer/src/IR/JavaClassName.mli @@ -18,3 +18,6 @@ val pp : Format.formatter -> t -> unit val package : t -> string option val classname : t -> string + +val is_external_via_config : t -> bool +(** Considered external based on config flags. *) diff --git a/infer/src/nullsafe/AnnotatedField.ml b/infer/src/nullsafe/AnnotatedField.ml index 7dd9db7a6..4df5d32db 100644 --- a/infer/src/nullsafe/AnnotatedField.ml +++ b/infer/src/nullsafe/AnnotatedField.ml @@ -43,11 +43,17 @@ let get tenv field_name class_typ = Typ.name class_typ |> Option.value_map ~f:(NullsafeMode.of_class tenv) ~default:NullsafeMode.Default in + let is_third_party = + ThirdPartyAnnotationInfo.is_third_party_typ + (ThirdPartyAnnotationGlobalRepo.get_repo ()) + class_typ + in Struct.get_field_info ~lookup field_name class_typ |> Option.map ~f:(fun (Struct.{typ= field_typ; annotations} as field_info) -> let is_enum_value = is_enum_value tenv ~class_typ field_info in let nullability = - AnnotatedNullability.of_type_and_annotation field_typ annotations ~nullsafe_mode + AnnotatedNullability.of_type_and_annotation ~nullsafe_mode ~is_third_party field_typ + annotations in let corrected_nullability = match nullability with diff --git a/infer/src/nullsafe/AnnotatedNullability.ml b/infer/src/nullsafe/AnnotatedNullability.ml index 00805d73a..412a02a29 100644 --- a/infer/src/nullsafe/AnnotatedNullability.ml +++ b/infer/src/nullsafe/AnnotatedNullability.ml @@ -14,9 +14,11 @@ module F = Format nullsafe omits Nullability information in types used for local variable declarations: this information is inferred according to flow-sensitive inferrence rule. *) +(** See {!Nullability.t} for explanation *) type t = | Nullable of nullable_origin - | UncheckedNonnull of unchecked_nonnull_origin (** See {!Nullability.t} for explanation *) + | ThirdPartyNonnull + | UncheckedNonnull of unchecked_nonnull_origin | LocallyCheckedNonnull | StrictNonnull of strict_nonnull_origin [@@deriving compare] @@ -51,6 +53,8 @@ and strict_nonnull_origin = let get_nullability = function | Nullable _ -> Nullability.Nullable + | ThirdPartyNonnull -> + Nullability.ThirdPartyNonnull | UncheckedNonnull _ -> Nullability.UncheckedNonnull | LocallyCheckedNonnull -> @@ -88,6 +92,8 @@ let pp fmt t = match t with | Nullable origin -> F.fprintf fmt "Nullable[%s]" (string_of_nullable_origin origin) + | ThirdPartyNonnull -> + F.fprintf fmt "ThirdPartyNonnull" | UncheckedNonnull origin -> F.fprintf fmt "UncheckedNonnull[%s]" (string_of_declared_nonnull_origin origin) | LocallyCheckedNonnull -> @@ -96,7 +102,7 @@ let pp fmt t = F.fprintf fmt "StrictNonnull[%s]" (string_of_nonnull_origin origin) -let of_type_and_annotation ~(nullsafe_mode : NullsafeMode.t) typ annotations = +let of_type_and_annotation ~nullsafe_mode ~is_third_party typ annotations = if not (PatternMatch.type_is_class typ) then StrictNonnull PrimitiveType else if Annotations.ia_is_nullable annotations then let nullable_origin = @@ -111,6 +117,7 @@ let of_type_and_annotation ~(nullsafe_mode : NullsafeMode.t) typ annotations = | NullsafeMode.Local _ -> LocallyCheckedNonnull | NullsafeMode.Default -> - if Annotations.ia_is_nonnull annotations then UncheckedNonnull AnnotatedNonnull + if is_third_party then ThirdPartyNonnull + else if Annotations.ia_is_nonnull annotations then UncheckedNonnull AnnotatedNonnull (* Currently, we treat not annotated types as nonnull *) else UncheckedNonnull ImplicitlyNonnull diff --git a/infer/src/nullsafe/AnnotatedNullability.mli b/infer/src/nullsafe/AnnotatedNullability.mli index 6af09af58..19072069d 100644 --- a/infer/src/nullsafe/AnnotatedNullability.mli +++ b/infer/src/nullsafe/AnnotatedNullability.mli @@ -16,9 +16,11 @@ open! IStd user-provided annotations for local types, so annotated nullability applies only for types declared at methods and field level. *) +(** See {!Nullability.t} for explanation *) type t = | Nullable of nullable_origin - | UncheckedNonnull of unchecked_nonnull_origin (** See {!Nullability.t} for explanation *) + | ThirdPartyNonnull + | UncheckedNonnull of unchecked_nonnull_origin | LocallyCheckedNonnull | StrictNonnull of strict_nonnull_origin [@@deriving compare] @@ -52,7 +54,8 @@ and strict_nonnull_origin = val get_nullability : t -> Nullability.t -val of_type_and_annotation : nullsafe_mode:NullsafeMode.t -> Typ.t -> Annot.Item.t -> t +val of_type_and_annotation : + nullsafe_mode:NullsafeMode.t -> is_third_party:bool -> Typ.t -> Annot.Item.t -> t (** Given the type and its annotations, returns its nullability. NOTE: it does not take into account models etc., so this is intended to be used as a helper function for more high-level annotation processing. *) diff --git a/infer/src/nullsafe/AnnotatedSignature.ml b/infer/src/nullsafe/AnnotatedSignature.ml index 3f5171cea..7da116ce0 100644 --- a/infer/src/nullsafe/AnnotatedSignature.ml +++ b/infer/src/nullsafe/AnnotatedSignature.ml @@ -35,10 +35,11 @@ and model_source = InternalModel | ThirdPartyRepo of {filename: string; line_num [@@deriving compare] (* get nullability of method's return type given its annotations and information about its params *) -let nullability_for_return ret_type ret_annotations ~nullsafe_mode ~has_propagates_nullable_in_param - = +let nullability_for_return ret_type ret_annotations ~nullsafe_mode ~is_third_party + ~has_propagates_nullable_in_param = let nullability = - AnnotatedNullability.of_type_and_annotation ~nullsafe_mode ret_type ret_annotations + AnnotatedNullability.of_type_and_annotation ~nullsafe_mode ~is_third_party ret_type + ret_annotations in (* if any param is annotated with propagates nullable, then the result is nullable *) match nullability with @@ -53,10 +54,12 @@ let nullability_for_return ret_type ret_annotations ~nullsafe_mode ~has_propagat (* Given annotations for method signature, extract nullability information for return type and params *) -let extract_nullability ~nullsafe_mode ret_type ret_annotations param_annotated_types = +let extract_nullability ~nullsafe_mode ~is_third_party ret_type ret_annotations + param_annotated_types = let params_nullability = List.map param_annotated_types ~f:(fun (typ, annotations) -> - AnnotatedNullability.of_type_and_annotation typ annotations ~nullsafe_mode ) + AnnotatedNullability.of_type_and_annotation typ annotations ~nullsafe_mode ~is_third_party + ) in let has_propagates_nullable_in_param = List.exists params_nullability ~f:(function @@ -66,7 +69,8 @@ let extract_nullability ~nullsafe_mode ret_type ret_annotations param_annotated_ false ) in let return_nullability = - nullability_for_return ret_type ret_annotations ~nullsafe_mode ~has_propagates_nullable_in_param + nullability_for_return ret_type ret_annotations ~nullsafe_mode ~is_third_party + ~has_propagates_nullable_in_param in (return_nullability, params_nullability) @@ -77,6 +81,12 @@ let get ~nullsafe_mode proc_attributes : t = in let formals = proc_attributes.ProcAttributes.formals in let ret_type = proc_attributes.ProcAttributes.ret_type in + let procname = proc_attributes.ProcAttributes.proc_name in + let is_third_party = + ThirdPartyAnnotationInfo.is_third_party_proc + (ThirdPartyAnnotationGlobalRepo.get_repo ()) + procname + in (* zip formal params with annotation *) let params_with_annotations = let rec zip_params ial parl = @@ -100,7 +110,7 @@ let get ~nullsafe_mode proc_attributes : t = List.map params_with_annotations ~f:(fun ((_, typ), annotations) -> (typ, annotations)) in let return_nullability, params_nullability = - extract_nullability ~nullsafe_mode ret_type ret_annotation param_annotated_types + extract_nullability ~nullsafe_mode ~is_third_party ret_type ret_annotation param_annotated_types in let ret = { ret_annotation_deprecated= ret_annotation diff --git a/infer/src/nullsafe/AssignmentRule.ml b/infer/src/nullsafe/AssignmentRule.ml index ddf9fc89b..a40f66726 100644 --- a/infer/src/nullsafe/AssignmentRule.ml +++ b/infer/src/nullsafe/AssignmentRule.ml @@ -22,38 +22,22 @@ and function_info = ; param_position: int ; function_procname: Procname.t } -(** For better adoption we allow certain conversions. Otherwise using code checked under different - nullsafe modes becomes a pain because of extra warnings. *) -module AssignmentWhitelist = struct - let all_whitelisted = - [ (Nullability.StrictNonnull, Nullability.UncheckedNonnull) - ; (Nullability.LocallyCheckedNonnull, Nullability.UncheckedNonnull) - ; (Nullability.StrictNonnull, Nullability.LocallyCheckedNonnull) ] - - - let all_whitelisted_in_mode = function - | NullsafeMode.Default | NullsafeMode.Local NullsafeMode.Trust.All -> - all_whitelisted - | NullsafeMode.Local (NullsafeMode.Trust.Only ([] as _classes)) - | NullsafeMode.Local (NullsafeMode.Trust.Only _classes) -> - (* TODO(T61473665): case with specified non-empty classes not supported now, defaulting to trust=none *) - [(Nullability.StrictNonnull, Nullability.LocallyCheckedNonnull)] - | NullsafeMode.Strict -> - [] - - - let is_allowed_in_mode ~nullsafe_mode ~lhs ~rhs = - List.exists (all_whitelisted_in_mode nullsafe_mode) ~f:(Nullability.equal_pair (lhs, rhs)) - - - let is_potentially_allowed ~lhs ~rhs = - List.exists all_whitelisted ~f:(Nullability.equal_pair (lhs, rhs)) -end - let check ~(nullsafe_mode : NullsafeMode.t) ~lhs ~rhs = + let falls_under_optimistic_third_party = + match nullsafe_mode with + | NullsafeMode.Default when Config.nullsafe_optimistic_third_party_params_in_non_strict -> ( + match lhs with Nullability.ThirdPartyNonnull -> true | _ -> false ) + | _ -> + false + in let is_allowed_assignment = - Nullability.is_subtype ~subtype:rhs ~supertype:lhs - || AssignmentWhitelist.is_allowed_in_mode ~nullsafe_mode ~lhs ~rhs + Nullability.is_subtype ~supertype:lhs ~subtype:rhs + || falls_under_optimistic_third_party + (* For better adoption we allow certain conversions. Otherwise using code checked under + different nullsafe modes becomes a pain because of extra warnings. *) + (* TODO(T62521386): consider using caller context when determining nullability to get rid of + white-lists. *) + || Nullability.is_considered_nonnull ~nullsafe_mode rhs in Result.ok_if_true is_allowed_assignment ~error:{nullsafe_mode; lhs; rhs} @@ -95,17 +79,17 @@ let mk_description_for_bad_param_passed "`null`" | Nullability.Nullable -> "nullable" - | Nullability.StrictNonnull - | Nullability.UncheckedNonnull - | Nullability.LocallyCheckedNonnull -> - Logging.die InternalError "Invariant violation: unexpected nullability" + | other -> + Logging.die InternalError + "mk_description_for_bad_param:: invariant violation: unexpected nullability %a" + Nullability.pp other in Format.asprintf "%a is %s" MF.pp_monospaced actual_param_expression nullability_descr in let suggested_file_to_add_third_party = match model_source with | None -> - ThirdPartyAnnotationInfo.lookup_related_sig_file_by_package + ThirdPartyAnnotationInfo.lookup_related_sig_file_for_proc (ThirdPartyAnnotationGlobalRepo.get_repo ()) function_procname | Some _ -> @@ -158,61 +142,69 @@ let get_issue_type = function IssueType.eradicate_return_not_nullable -let violation_description {nullsafe_mode; lhs; rhs} ~assignment_location assignment_type ~rhs_origin - = - if AssignmentWhitelist.is_potentially_allowed ~lhs ~rhs then - (* This type of violation is more subtle than the normal case, so it should - be rendered in a special way. An 'impossible case' is checked in the - following call and will cause infer to die. *) - ErrorRenderingUtils.mk_special_nullsafe_issue ~nullsafe_mode ~bad_nullability:rhs - ~bad_usage_location:assignment_location rhs_origin - else - let nullability_evidence = - get_origin_opt assignment_type rhs_origin - |> Option.bind ~f:(fun origin -> TypeOrigin.get_description origin) - in - let nullability_evidence_as_suffix = - Option.value_map nullability_evidence ~f:(fun evidence -> ": " ^ evidence) ~default:"" - in - let module MF = MarkupFormatter in - let error_message = - match assignment_type with - | PassingParamToFunction function_info -> - mk_description_for_bad_param_passed function_info nullability_evidence - ~param_nullability:rhs - | AssigningToField field_name -> - let rhs_description = - Nullability.( - match rhs with - | Null -> - "`null`" - | Nullable -> - "a nullable" - | StrictNonnull | UncheckedNonnull | LocallyCheckedNonnull -> - Logging.die InternalError "Invariant violation: unexpected nullability") - in - Format.asprintf "%a is declared non-nullable but is assigned %s%s." MF.pp_monospaced - (Fieldname.get_field_name field_name) - rhs_description nullability_evidence_as_suffix - | ReturningFromFunction function_proc_name -> - let return_description = - Nullability.( - match rhs with - | Null -> - (* Return `null` in all_whitelisted branches *) - "`null`" - | Nullable -> - "a nullable value" - | StrictNonnull | UncheckedNonnull | LocallyCheckedNonnull -> - Logging.die InternalError "Invariant violation: unexpected nullability") - in - Format.asprintf "%a: return type is declared non-nullable but the method returns %s%s." - MF.pp_monospaced - (Procname.to_simplified_string ~withclass:false function_proc_name) - return_description nullability_evidence_as_suffix - in - let issue_type = get_issue_type assignment_type in - (error_message, issue_type, assignment_location) +let violation_description {nullsafe_mode; rhs} ~assignment_location assignment_type ~rhs_origin = + let special_message = + if not (NullsafeMode.equal NullsafeMode.Default nullsafe_mode) then + ErrorRenderingUtils.mk_special_nullsafe_issue ~nullsafe_mode ~bad_nullability:rhs + ~bad_usage_location:assignment_location rhs_origin + else None + in + match special_message with + | Some desc -> + desc + | _ -> + let nullability_evidence = + get_origin_opt assignment_type rhs_origin + |> Option.bind ~f:(fun origin -> TypeOrigin.get_description origin) + in + let nullability_evidence_as_suffix = + Option.value_map nullability_evidence ~f:(fun evidence -> ": " ^ evidence) ~default:"" + in + let module MF = MarkupFormatter in + let error_message = + match assignment_type with + | PassingParamToFunction function_info -> + mk_description_for_bad_param_passed function_info nullability_evidence + ~param_nullability:rhs + | AssigningToField field_name -> + let rhs_description = + Nullability.( + match rhs with + | Null -> + "`null`" + | Nullable -> + "a nullable" + | other -> + Logging.die InternalError + "violation_description(assign_field):: invariant violation: unexpected \ + nullability %a" + Nullability.pp other) + in + Format.asprintf "%a is declared non-nullable but is assigned %s%s." MF.pp_monospaced + (Fieldname.get_field_name field_name) + rhs_description nullability_evidence_as_suffix + | ReturningFromFunction function_proc_name -> + let return_description = + Nullability.( + match rhs with + | Null -> + (* Return `null` in all_whitelisted branches *) + "`null`" + | Nullable -> + "a nullable value" + | other -> + Logging.die InternalError + "violation_description(ret_fun):: invariant violation: unexpected \ + nullability %a" + Nullability.pp other) + in + Format.asprintf "%a: return type is declared non-nullable but the method returns %s%s." + MF.pp_monospaced + (Procname.to_simplified_string ~withclass:false function_proc_name) + return_description nullability_evidence_as_suffix + in + let issue_type = get_issue_type assignment_type in + (error_message, issue_type, assignment_location) let violation_severity {nullsafe_mode} = NullsafeMode.severity nullsafe_mode diff --git a/infer/src/nullsafe/DereferenceRule.ml b/infer/src/nullsafe/DereferenceRule.ml index 81e91e19d..2e40e0c2f 100644 --- a/infer/src/nullsafe/DereferenceRule.ml +++ b/infer/src/nullsafe/DereferenceRule.ml @@ -19,22 +19,9 @@ let check ~nullsafe_mode nullability = match nullability with | Nullability.Nullable | Nullability.Null -> Error {nullsafe_mode; nullability} - | Nullability.UncheckedNonnull -> ( - match nullsafe_mode with - | NullsafeMode.Strict | NullsafeMode.Local (NullsafeMode.Trust.Only []) -> - Error {nullsafe_mode; nullability} - | NullsafeMode.Local (NullsafeMode.Trust.Only _typ_names) -> - (* TODO(T61473665). For now treat as trust=none. *) - Error {nullsafe_mode; nullability} - | NullsafeMode.Local NullsafeMode.Trust.All | NullsafeMode.Default -> - Ok () ) - | Nullability.LocallyCheckedNonnull -> ( - match nullsafe_mode with - | NullsafeMode.Default | NullsafeMode.Local _ -> - Ok () - | NullsafeMode.Strict -> - Error {nullsafe_mode; nullability} ) - | Nullability.StrictNonnull -> + | other when not (Nullability.is_considered_nonnull ~nullsafe_mode other) -> + Error {nullsafe_mode; nullability} + | _ -> Ok () @@ -52,12 +39,15 @@ let get_origin_opt ~nullable_object_descr origin = let violation_description {nullsafe_mode; nullability} ~dereference_location dereference_type ~nullable_object_descr ~nullable_object_origin = let module MF = MarkupFormatter in - match nullability with - | Nullability.UncheckedNonnull | Nullability.LocallyCheckedNonnull -> - (* These types of violations are possible in non-default nullsafe mode. For them we - provide tailored error messages. *) + let special_message = + if not (NullsafeMode.equal NullsafeMode.Default nullsafe_mode) then ErrorRenderingUtils.mk_special_nullsafe_issue ~nullsafe_mode ~bad_nullability:nullability ~bad_usage_location:dereference_location nullable_object_origin + else None + in + match special_message with + | Some desc -> + desc | _ -> let what_is_dereferred_str = match dereference_type with @@ -104,10 +94,10 @@ let violation_description {nullsafe_mode; nullability} ~dereference_location der | Nullability.Nullable -> Format.sprintf "%s is nullable and is not locally checked for null when %s%s." what_is_dereferred_str action_descr suffix - | Nullability.UncheckedNonnull - | Nullability.LocallyCheckedNonnull - | Nullability.StrictNonnull -> - Logging.die InternalError "Invariant violation: unexpected nullability" + | other -> + Logging.die InternalError + "violation_description:: invariant violation: unexpected nullability %a" + Nullability.pp other in (description, IssueType.eradicate_nullable_dereference, dereference_location) diff --git a/infer/src/nullsafe/ErrorRenderingUtils.ml b/infer/src/nullsafe/ErrorRenderingUtils.ml index 81cd31816..7ca49d2a9 100644 --- a/infer/src/nullsafe/ErrorRenderingUtils.ml +++ b/infer/src/nullsafe/ErrorRenderingUtils.ml @@ -6,6 +6,7 @@ *) open! IStd +module F = Format let is_object_nullability_self_explanatory ~object_expression (object_origin : TypeOrigin.t) = (* Fundamentally, object can be of two kinds: @@ -85,41 +86,52 @@ let get_field_class_name field_name = let mk_coming_from nullsafe_mode nullability = match (nullsafe_mode, nullability) with + | NullsafeMode.Strict, Nullability.ThirdPartyNonnull | NullsafeMode.Strict, Nullability.UncheckedNonnull -> - "non-strict classes" + Some "non-strict classes" | NullsafeMode.Strict, Nullability.LocallyCheckedNonnull -> - "nullsafe-local classes" + Some "nullsafe-local classes" + | NullsafeMode.Local _, Nullability.ThirdPartyNonnull -> + Some "third-party classes" | NullsafeMode.Local _, Nullability.UncheckedNonnull -> - "non-nullsafe classes" - | (_ as mode), nullability -> - Logging.die InternalError "Unexpected: using %s in %a should not be a violation" - (Nullability.to_string nullability) - NullsafeMode.pp mode + Some "non-nullsafe classes" + | _ -> + None let mk_recommendation nullsafe_mode nullability what = match (nullsafe_mode, nullability) with + | NullsafeMode.Strict, Nullability.ThirdPartyNonnull | NullsafeMode.Strict, Nullability.UncheckedNonnull | NullsafeMode.Strict, Nullability.LocallyCheckedNonnull -> - Format.sprintf "make %s nullsafe strict" what + Some (F.sprintf "make %s nullsafe strict" what) | NullsafeMode.Local _, Nullability.UncheckedNonnull -> - Format.sprintf "make %s nullsafe" what - | (_ as mode), nullability -> - Logging.die InternalError "Unexpected: using %s in %a should not be a violation" - (Nullability.to_string nullability) - NullsafeMode.pp mode + Some (F.sprintf "make %s nullsafe" what) + | _ -> + None + + +let mk_recommendation_for_third_party_field nullsafe_mode nullability field = + match (nullsafe_mode, nullability) with + | NullsafeMode.Strict, Nullability.ThirdPartyNonnull -> + Some (F.sprintf "access %s via a nullsafe strict getter" field) + | NullsafeMode.Local _, Nullability.ThirdPartyNonnull -> + Some (F.sprintf "access %s via a nullsafe getter" field) + | _ -> + None let get_info object_origin nullsafe_mode bad_nullability = + let open IOption.Let_syntax in match object_origin with | TypeOrigin.MethodCall {pname; call_loc} -> let offending_object = - Format.asprintf "%a" MarkupFormatter.pp_monospaced + F.asprintf "%a" MarkupFormatter.pp_monospaced (Procname.to_simplified_string ~withclass:true pname) in let object_loc = call_loc in let suggested_third_party_sig_file = - ThirdPartyAnnotationInfo.lookup_related_sig_file_by_package + ThirdPartyAnnotationInfo.lookup_related_sig_file_for_proc (ThirdPartyAnnotationGlobalRepo.get_repo ()) pname in @@ -127,21 +139,29 @@ let get_info object_origin nullsafe_mode bad_nullability = (* Two main cases: it is either FB owned code or third party. We determine the difference based on presense of suggested_third_party_sig_file. *) - let coming_from_explanation, recommendation, issue_type = - let what_to_strictify = - Option.value (get_method_class_name pname) ~default:offending_object - in + let+ coming_from_explanation, recommendation, issue_type = match suggested_third_party_sig_file with - | None -> - ( mk_coming_from nullsafe_mode bad_nullability - , mk_recommendation nullsafe_mode bad_nullability what_to_strictify - , IssueType.eradicate_unchecked_usage_in_nullsafe ) | Some sig_file_name -> - ( "not vetted third party methods" - , Format.sprintf "add the correct signature to %s" - (ThirdPartyAnnotationGlobalRepo.get_user_friendly_third_party_sig_file_name - ~filename:sig_file_name) - , IssueType.eradicate_unvetted_third_party_in_nullsafe ) + (* Dereferences or assignment violations with regular Nullables + should not be special cased *) + if Nullability.equal Nullable bad_nullability then None + else + return + ( "not vetted third party methods" + , F.sprintf "add the correct signature to %s" + (ThirdPartyAnnotationGlobalRepo.get_user_friendly_third_party_sig_file_name + ~filename:sig_file_name) + , IssueType.eradicate_unvetted_third_party_in_nullsafe ) + | None -> + let* from = mk_coming_from nullsafe_mode bad_nullability in + let+ recommendation = + let what_to_strictify = + Option.value (get_method_class_name pname) ~default:offending_object + in + mk_recommendation nullsafe_mode bad_nullability what_to_strictify + in + let issue_type = IssueType.eradicate_unchecked_usage_in_nullsafe in + (from, recommendation, issue_type) in { offending_object ; object_loc @@ -150,41 +170,45 @@ let get_info object_origin nullsafe_mode bad_nullability = ; recommendation ; issue_type } | TypeOrigin.Field {field_name; access_loc} -> - let offending_object = - Format.asprintf "%a" MarkupFormatter.pp_monospaced - (Fieldname.to_simplified_string field_name) + let qualified_name = + F.asprintf "%a" MarkupFormatter.pp_monospaced (Fieldname.to_simplified_string field_name) + in + let unqualified_name = + F.asprintf "%a" MarkupFormatter.pp_monospaced (Fieldname.get_field_name field_name) in let object_loc = access_loc in (* TODO: currently we do not support third-party annotations for fields. Because of this, render error like it is a non-stict class. *) let what_is_used = "This field" in - let coming_from_explanation = mk_coming_from nullsafe_mode bad_nullability in - let recommendation = - mk_recommendation nullsafe_mode bad_nullability (get_field_class_name field_name) + let* coming_from_explanation = mk_coming_from nullsafe_mode bad_nullability in + let+ recommendation = + Option.first_some + (mk_recommendation_for_third_party_field nullsafe_mode bad_nullability unqualified_name) + (mk_recommendation nullsafe_mode bad_nullability (get_field_class_name field_name)) in let issue_type = IssueType.eradicate_unchecked_usage_in_nullsafe in - { offending_object + { offending_object= qualified_name ; object_loc ; coming_from_explanation ; what_is_used ; recommendation ; issue_type } | _ -> - Logging.die Logging.InternalError - "Invariant violation: unexpected origin of declared non-nullable value" + None let mk_special_nullsafe_issue ~nullsafe_mode ~bad_nullability ~bad_usage_location object_origin = - let { offending_object - ; object_loc - ; coming_from_explanation - ; what_is_used - ; recommendation - ; issue_type } = + let open IOption.Let_syntax in + let+ { offending_object + ; object_loc + ; coming_from_explanation + ; what_is_used + ; recommendation + ; issue_type } = get_info object_origin nullsafe_mode bad_nullability in let description = - Format.asprintf + F.asprintf "%s: `@Nullsafe%a` mode prohibits using values coming from %s without a check. %s is used at \ line %d. Either add a local check for null or assertion, or %s." offending_object NullsafeMode.pp nullsafe_mode coming_from_explanation what_is_used diff --git a/infer/src/nullsafe/ErrorRenderingUtils.mli b/infer/src/nullsafe/ErrorRenderingUtils.mli index 549fc6cb0..57f45c167 100644 --- a/infer/src/nullsafe/ErrorRenderingUtils.mli +++ b/infer/src/nullsafe/ErrorRenderingUtils.mli @@ -18,7 +18,7 @@ val mk_special_nullsafe_issue : -> bad_nullability:Nullability.t -> bad_usage_location:Location.t -> TypeOrigin.t - -> string * IssueType.t * Location.t + -> (string * IssueType.t * Location.t) option (** Situation when we tried to use nonnull values of incompatible modes. This is disallowed in strict and local mode. Returns a tuple (error message, issue type, error location). NOTE: Location of the error will be NOT in the place when the value is used (that is diff --git a/infer/src/nullsafe/InferredNullability.ml b/infer/src/nullsafe/InferredNullability.ml index 136b54b44..8fd04b339 100644 --- a/infer/src/nullsafe/InferredNullability.ml +++ b/infer/src/nullsafe/InferredNullability.ml @@ -13,11 +13,9 @@ let create origin = {nullability= TypeOrigin.get_nullability origin; origin} let get_nullability {nullability} = nullability -let is_nonnull_or_declared_nonnull {nullability} = - match nullability with StrictNonnull | UncheckedNonnull -> true | _ -> false +let is_nonnullish {nullability} = Nullability.is_nonnullish nullability - -let to_string {nullability} = Printf.sprintf "[%s]" (Nullability.to_string nullability) +let pp fmt {nullability} = Nullability.pp fmt nullability let join t1 t2 = let joined_nullability = Nullability.join t1.nullability t2.nullability in diff --git a/infer/src/nullsafe/InferredNullability.mli b/infer/src/nullsafe/InferredNullability.mli index 5fb834f89..e09c0afb4 100644 --- a/infer/src/nullsafe/InferredNullability.mli +++ b/infer/src/nullsafe/InferredNullability.mli @@ -20,7 +20,8 @@ val get_nullability : t -> Nullability.t val create : TypeOrigin.t -> t -val is_nonnull_or_declared_nonnull : t -> bool +val is_nonnullish : t -> bool +(** Check whether corresponding [Nullability] is [Nullability.is_nonnullish] *) val get_origin : t -> TypeOrigin.t (** The simple explanation of how was nullability inferred. *) @@ -39,4 +40,4 @@ val join : t -> t -> t val origin_is_fun_library : t -> bool -val to_string : t -> string +val pp : Format.formatter -> t -> unit diff --git a/infer/src/nullsafe/InheritanceRule.ml b/infer/src/nullsafe/InheritanceRule.ml index 8b3f495ee..2dd0c056c 100644 --- a/infer/src/nullsafe/InheritanceRule.ml +++ b/infer/src/nullsafe/InheritanceRule.ml @@ -12,15 +12,13 @@ type violation = {nullsafe_mode: NullsafeMode.t; base: Nullability.t; overridden type type_role = Param | Ret let is_whitelisted_violation ~subtype ~supertype = - match (subtype, supertype) with - | Nullability.UncheckedNonnull, Nullability.StrictNonnull -> - (* It is a violation that we are currently willing to ignore because - it is hard to obey in practice. - It might lead to unsoundness issues, so this might be reconsidered. - *) - true - | _ -> - false + (* When both nullabilities are kind-of non-nullable we don't want to raise the + issue. Without this suppression there will be a lot of non-actionable issues + raised for classes in one [NullsafeMode] inheriting from classes in the other + [NullsafeMode]. *) + (* TODO(T62521386): consider using caller context when determining nullability to get + rid of white-lists. *) + Nullability.is_nonnullish subtype && Nullability.is_nonnullish supertype let check ~nullsafe_mode type_role ~base ~overridden = diff --git a/infer/src/nullsafe/Nullability.ml b/infer/src/nullsafe/Nullability.ml index b1f265dfa..7c5d367f7 100644 --- a/infer/src/nullsafe/Nullability.ml +++ b/infer/src/nullsafe/Nullability.ml @@ -6,10 +6,14 @@ *) open! IStd +module F = Format type t = | Null (** The only possible value for that type is null *) | Nullable (** No guarantees on the nullability *) + | ThirdPartyNonnull + (** Values coming from third-party methods and fields not explictly annotated as [@Nullable]. + We still consider those as non-nullable but with the least level of confidence. *) | UncheckedNonnull (** The type comes from a signature that is annotated (explicitly or implicitly according to conventions) as non-nullable. However, it might still contain null since the truthfulness @@ -43,6 +47,8 @@ let join x y = Nullable | Nullable, _ | _, Nullable -> Nullable + | ThirdPartyNonnull, _ | _, ThirdPartyNonnull -> + ThirdPartyNonnull | UncheckedNonnull, _ | _, UncheckedNonnull -> UncheckedNonnull | LocallyCheckedNonnull, _ | _, LocallyCheckedNonnull -> @@ -53,14 +59,37 @@ let join x y = let is_subtype ~subtype ~supertype = equal (join subtype supertype) supertype +let is_considered_nonnull ~nullsafe_mode nullability = + let least_required = + match nullsafe_mode with + | NullsafeMode.Strict -> + StrictNonnull + | NullsafeMode.Local (NullsafeMode.Trust.Only _classes) -> + (* TODO(T61473665). For now treat trust with specified classes as trust=none. *) + LocallyCheckedNonnull + | NullsafeMode.Local NullsafeMode.Trust.All -> + UncheckedNonnull + | NullsafeMode.Default -> + ThirdPartyNonnull + in + is_subtype ~subtype:nullability ~supertype:least_required + + +let is_nonnullish t = is_considered_nonnull ~nullsafe_mode:NullsafeMode.Default t + let to_string = function | Null -> "Null" | Nullable -> "Nullable" + | ThirdPartyNonnull -> + "ThirdPartyNonnull" | UncheckedNonnull -> "UncheckedNonnull" | LocallyCheckedNonnull -> "LocallyCheckedNonnull" | StrictNonnull -> "StrictNonnull" + + +let pp fmt t = F.fprintf fmt "%s" (to_string t) diff --git a/infer/src/nullsafe/Nullability.mli b/infer/src/nullsafe/Nullability.mli index 584295a86..6929dbbc2 100644 --- a/infer/src/nullsafe/Nullability.mli +++ b/infer/src/nullsafe/Nullability.mli @@ -6,6 +6,7 @@ *) open! IStd +module F = Format (** Nullability is a central concept for Nullsafe type-checker. Informally, nullability is a "type" \- set of values together with some additional context. All nullsafe is interested about if @@ -18,6 +19,9 @@ open! IStd type t = | Null (** The only possible value for that type is null *) | Nullable (** No guarantees on the nullability *) + | ThirdPartyNonnull + (** Values coming from third-party methods and fields not explictly annotated as [@Nullable]. + We still consider those as non-nullable but with the least level of confidence. *) | UncheckedNonnull (** The type comes from a signature that is annotated (explicitly or implicitly according to conventions) as non-nullable. Hovewer, it might still contain null since the truthfullness @@ -48,4 +52,10 @@ val join : t -> t -> t (** Unique upper bound over two types: the most precise type that is a supertype of both. Practically, joins occur e.g. when two branches of execution flow are getting merged. *) -val to_string : t -> string +val is_considered_nonnull : nullsafe_mode:NullsafeMode.t -> t -> bool +(** Check whether a given nullability is considered non-nullable within a given [nullsafe_mode]. *) + +val is_nonnullish : t -> bool +(** Check whether a given nullability is one of the non-nullable types with no regards to the mode. *) + +val pp : F.formatter -> t -> unit diff --git a/infer/src/nullsafe/ThirdPartyAnnotationInfo.ml b/infer/src/nullsafe/ThirdPartyAnnotationInfo.ml index b906c7562..f0e644756 100644 --- a/infer/src/nullsafe/ThirdPartyAnnotationInfo.ml +++ b/infer/src/nullsafe/ThirdPartyAnnotationInfo.ml @@ -71,14 +71,12 @@ let does_package_match_file ~package sig_filename = let lookup_related_sig_file {filenames} ~package = List.filter filenames ~f:(does_package_match_file ~package) - |> List.max_elt - ~compare:(fun (* If two different files match the package, we choose the most specific; it will have the biggest length *) - filename1 - filename2 - -> String.length filename1 - String.length filename2 ) + (* In case two different files match the package, we choose the most specific; + it will have the longest length *) + |> List.max_elt ~compare:(fun name1 name2 -> String.length name1 - String.length name2) -let lookup_related_sig_file_by_package storage procname = +let lookup_related_sig_file_for_proc storage procname = let package = match procname with | Procname.Java java_pname -> @@ -87,3 +85,33 @@ let lookup_related_sig_file_by_package storage procname = None in Option.bind package ~f:(fun package -> lookup_related_sig_file storage ~package) + + +let is_third_party_proc storage procname = + let is_from_config = + match procname with + | Procname.Java java_pname -> + Procname.Java.is_external java_pname + | _ -> + false + in + let lookup_sig_file _ = lookup_related_sig_file_for_proc storage procname in + is_from_config || Option.is_some (lookup_sig_file ()) + + +(* There is a bit of duplication relative to [is_third_party_proc] due to mismatch between + [Typ.Name.Java] and [JavaClassName]. When those types are consolidated would be a good + idea to refactor this function. *) +let is_third_party_typ storage typ = + IOption.Let_syntax.( + let* typ_name = Typ.name typ in + let+ class_name = + match typ_name with Typ.JavaClass class_name -> return class_name | _ -> None + in + let is_from_config = JavaClassName.is_external_via_config class_name in + let lookup_sig_file _ = + let* package = JavaClassName.package class_name in + lookup_related_sig_file storage ~package + in + is_from_config || Option.is_some (lookup_sig_file ())) + |> Option.value ~default:false diff --git a/infer/src/nullsafe/ThirdPartyAnnotationInfo.mli b/infer/src/nullsafe/ThirdPartyAnnotationInfo.mli index 84ce84956..cc2ab0527 100644 --- a/infer/src/nullsafe/ThirdPartyAnnotationInfo.mli +++ b/infer/src/nullsafe/ThirdPartyAnnotationInfo.mli @@ -33,6 +33,13 @@ val find_nullability_info : storage -> ThirdPartyMethod.unique_repr -> signature val lookup_related_sig_file : storage -> package:string -> string option (** If the package is third-party, return the relevant .sig file to add signatures for this package. *) -val lookup_related_sig_file_by_package : storage -> Procname.t -> string option -(** If the function is third-party (based on its package), return relevant .sig file to add the - signature NOTE: this DOES NOT look if the function is is already *) +val lookup_related_sig_file_for_proc : storage -> Procname.t -> string option +(** If the function is third-party (based on its package), return relevant .sig file *) + +val is_third_party_proc : storage -> Procname.t -> bool +(** Checks whether a required procname comes from third-party code based on available .sig files and + config flags. NOTE: considering config flags is done for compatibility with the legacy behaviour + and will be removed in the future *) + +val is_third_party_typ : storage -> Typ.t -> bool +(** See [is_third_party_proc]. *) diff --git a/infer/src/nullsafe/eradicateChecks.ml b/infer/src/nullsafe/eradicateChecks.ml index 9df8b9000..76d3a936b 100644 --- a/infer/src/nullsafe/eradicateChecks.ml +++ b/infer/src/nullsafe/eradicateChecks.ml @@ -97,7 +97,7 @@ let check_condition_for_redundancy tenv ~is_always_true find_canonical_duplicate *) (* NOTE: We don't report the opposite case, namely when the expression is known to be always `null`. Consider changing this. *) - InferredNullability.is_nonnull_or_declared_nonnull inferred_nullability + InferredNullability.is_nonnullish inferred_nullability && Config.eradicate_condition_redundant && (not is_temp) && PatternMatch.type_is_class typ && (not (from_try_with_resources ())) && not (InferredNullability.origin_is_fun_library inferred_nullability) @@ -168,22 +168,15 @@ let check_field_assignment ~nullsafe_mode tenv find_canonical_duplicate curr_pde (Some instr_ref) loc curr_pdesc ) -let is_declared_nonnull AnnotatedField.{annotated_type} = - match annotated_type.nullability with - | AnnotatedNullability.Nullable _ -> - false - | AnnotatedNullability.UncheckedNonnull _ - | AnnotatedNullability.LocallyCheckedNonnull - | AnnotatedNullability.StrictNonnull _ -> - true +(* Check if the field declared as not nullable (implicitly or explicitly). If the field is + absent, we optimistically assume it is not nullable. - -(* Is field declared as non-nullable (implicitly or explicitly)? *) + TODO(T54687014) investigate if this leads to unsoundness issues in practice *) let is_field_declared_as_nonnull annotated_field_opt = - (* If the field is not present, we optimistically assume it is not nullable. - TODO(T54687014) investigate if this leads to unsoundness issues in practice - *) - Option.exists annotated_field_opt ~f:is_declared_nonnull + let is_nonnullish AnnotatedField.{annotated_type= {nullability}} = + Nullability.is_nonnullish (AnnotatedNullability.get_nullability nullability) + in + Option.exists annotated_field_opt ~f:is_nonnullish let lookup_field_in_typestate pname field_name typestate = @@ -429,32 +422,6 @@ type resolved_param = ; actual: Exp.t * InferredNullability.t ; is_formal_propagates_nullable: bool } -let is_third_party_via_sig_files proc_name = - Option.is_some - (ThirdPartyAnnotationInfo.lookup_related_sig_file_by_package - (ThirdPartyAnnotationGlobalRepo.get_repo ()) - proc_name) - - -let is_marked_third_party_in_config proc_name = - match proc_name with - | Procname.Java java_pname -> - (* TODO: migrate to the new way of checking for third party: use - signatures repository instead of looking it up in config params. - *) - Procname.Java.is_external java_pname - | _ -> - false - - -(* if this method belongs to a third party code, but is not modelled neigher internally nor externally *) -let is_third_party_without_model proc_name model_source = - let is_third_party = - is_third_party_via_sig_files proc_name || is_marked_third_party_in_config proc_name - in - is_third_party && Option.is_none model_source - - (** Check the parameters of a call. *) let check_call_parameters ~nullsafe_mode ~callee_annotated_signature tenv find_canonical_duplicate curr_pdesc node callee_attributes resolved_params loc instr_ref : unit = @@ -490,17 +457,7 @@ let check_call_parameters ~nullsafe_mode ~callee_annotated_signature tenv find_c let rhs = InferredNullability.get_nullability nullability_actual in Result.iter_error (AssignmentRule.check ~nullsafe_mode ~lhs ~rhs) ~f:report in - let should_ignore_parameters_check = - (* TODO(T52947663) model params in third-party non modelled method as a dedicated nullability type, - so this logic can be moved to [AssignmentRule.check] *) - NullsafeMode.equal nullsafe_mode NullsafeMode.Default - && Config.nullsafe_optimistic_third_party_params_in_non_strict - && is_third_party_without_model callee_pname - callee_annotated_signature.AnnotatedSignature.model_source - in - if not should_ignore_parameters_check then - (* left to right to avoid guessing the different lengths *) - List.iter ~f:check resolved_params + List.iter ~f:check resolved_params let check_inheritance_rule_for_return find_canonical_duplicate tenv loc ~nullsafe_mode diff --git a/infer/src/nullsafe/typeCheck.ml b/infer/src/nullsafe/typeCheck.ml index 1d6c852f9..b929fa561 100644 --- a/infer/src/nullsafe/typeCheck.ml +++ b/infer/src/nullsafe/typeCheck.ml @@ -450,7 +450,7 @@ let do_preconditions_check_not_null instr_ref tenv find_canonical_duplicate node let should_report = Config.eradicate_condition_redundant (* TODO: This condition should be extracted into a dedicated rule *) - && InferredNullability.is_nonnull_or_declared_nonnull nullability + && InferredNullability.is_nonnullish nullability && not (InferredNullability.origin_is_fun_library nullability) in ( if checks.eradicate && should_report then diff --git a/infer/src/nullsafe/typeOrigin.ml b/infer/src/nullsafe/typeOrigin.ml index 6937f0280..7aab6f47c 100644 --- a/infer/src/nullsafe/typeOrigin.ml +++ b/infer/src/nullsafe/typeOrigin.ml @@ -129,6 +129,7 @@ let get_method_ret_description pname call_loc match nullability with | AnnotatedNullability.Nullable _ -> "nullable" + | AnnotatedNullability.ThirdPartyNonnull | AnnotatedNullability.UncheckedNonnull _ | AnnotatedNullability.LocallyCheckedNonnull | AnnotatedNullability.StrictNonnull _ -> diff --git a/infer/src/nullsafe/typeState.ml b/infer/src/nullsafe/typeState.ml index 631d71031..e2aea4b50 100644 --- a/infer/src/nullsafe/typeState.ml +++ b/infer/src/nullsafe/typeState.ml @@ -27,10 +27,9 @@ let empty = M.empty let pp fmt typestate = let pp_one exp (typ, ta) = - F.fprintf fmt " %a -> [%s] %s %a@\n" Exp.pp exp + F.fprintf fmt " %a -> [%s] %a %a@\n" Exp.pp exp (TypeOrigin.to_string (InferredNullability.get_origin ta)) - (InferredNullability.to_string ta) - (Typ.pp_full Pp.text) typ + InferredNullability.pp ta (Typ.pp_full Pp.text) typ in let pp_map map = M.iter pp_one map in pp_map typestate diff --git a/infer/tests/codetoanalyze/java/nullsafe-default/NullsafeMode.java b/infer/tests/codetoanalyze/java/nullsafe-default/NullsafeMode.java index 896018a1e..9836401a5 100644 --- a/infer/tests/codetoanalyze/java/nullsafe-default/NullsafeMode.java +++ b/infer/tests/codetoanalyze/java/nullsafe-default/NullsafeMode.java @@ -10,6 +10,7 @@ package codetoanalyze.java.nullsafe_default; import com.facebook.infer.annotation.Nullsafe; import com.facebook.infer.annotation.NullsafeStrict; import javax.annotation.Nullable; +import some.test.pckg.ThirdPartyTestClass; public class NullsafeMode { abstract class VariousMethods { @@ -35,6 +36,11 @@ public class NullsafeMode { String OK_passUncheckedToStrict(String arg) { return new StrictNullsafe().acceptVal(arg); } + + void OK_passNullableToThirdPartyParam() { + new ThirdPartyTestClass().paramUnspecified(returnNull()); + return; + } } class AnotherNonNullsafe extends VariousMethods {} @@ -55,6 +61,18 @@ public class NullsafeMode { return (new NonNullsafe()).returnNull(); } + String BAD_returnFromUnvettedThirdParty() { + return new ThirdPartyTestClass().returnUnspecified(); + } + + String BAD_returnNullableFieldFromThirdParty() { + return new ThirdPartyTestClass().nullableField; + } + + String BAD_returnNonNullableFieldFromThirdParty() { + return new ThirdPartyTestClass().nonNullableField; + } + String OK_passLocalToStrictMode(String arg) { return new NullsafeWithStrictMode().acceptVal(arg); } diff --git a/infer/tests/codetoanalyze/java/nullsafe-default/StrictModeForThirdParty.java b/infer/tests/codetoanalyze/java/nullsafe-default/StrictModeForThirdParty.java index f4702ea93..1806d7d2c 100644 --- a/infer/tests/codetoanalyze/java/nullsafe-default/StrictModeForThirdParty.java +++ b/infer/tests/codetoanalyze/java/nullsafe-default/StrictModeForThirdParty.java @@ -45,6 +45,10 @@ public class StrictModeForThirdParty { obj.returnSpecifiedAsNullable().toString(); } + public void dereferenceFieldIsBAD() { + obj.nonNullableField.toString(); + } + public void dereferenceSpecifiedAsNonnullIsOK() { obj.returnSpecifiedAsNonnull().toString(); } diff --git a/infer/tests/codetoanalyze/java/nullsafe-default/issues.exp b/infer/tests/codetoanalyze/java/nullsafe-default/issues.exp index 06031e8ee..63befc8b6 100644 --- a/infer/tests/codetoanalyze/java/nullsafe-default/issues.exp +++ b/infer/tests/codetoanalyze/java/nullsafe-default/issues.exp @@ -147,15 +147,17 @@ codetoanalyze/java/nullsafe-default/NullMethodCall.java, codetoanalyze.java.null codetoanalyze/java/nullsafe-default/NullMethodCall.java, codetoanalyze.java.nullsafe_default.NullMethodCall.testSystemGetenvBad():int, 1, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [`envValue` is nullable and is not locally checked for null when calling `length()`: call to System.getenv(...) at line 240 (nullable according to nullsafe internal models).] codetoanalyze/java/nullsafe-default/NullMethodCall.java, codetoanalyze.java.nullsafe_default.NullMethodCall.withConditionalAssignemnt(codetoanalyze.java.nullsafe_default.NullMethodCall$AnotherI,boolean,java.lang.Object,java.lang.Object):void, 0, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [`i` is nullable and is not locally checked for null when calling `withObjectParameter(...)`.] codetoanalyze/java/nullsafe-default/NullMethodCall.java, codetoanalyze.java.nullsafe_default.NullMethodCall.withConjuction(codetoanalyze.java.nullsafe_default.NullMethodCall$AnotherI,boolean,boolean):void, 0, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [`i` is nullable and is not locally checked for null when calling `withBooleanParameter(...)`.] -codetoanalyze/java/nullsafe-default/NullsafeMode.java, codetoanalyze.java.nullsafe_default.NullsafeMode$NullsafeWithStrictMode.BAD_returnFromNonStrict():java.lang.String, 0, ERADICATE_UNCHECKED_USAGE_IN_NULLSAFE, no_bucket, ERROR, [`NullsafeMode$VariousMethods.returnVal()`: `@NullsafeStrict` mode prohibits using values coming from non-strict classes without a check. Result of this call is used at line 115. Either add a local check for null or assertion, or make NullsafeMode$VariousMethods nullsafe strict.] -codetoanalyze/java/nullsafe-default/NullsafeMode.java, codetoanalyze.java.nullsafe_default.NullsafeMode$StrictNullsafe.BAD_returnFromNonNullsafe():java.lang.String, 0, ERADICATE_UNCHECKED_USAGE_IN_NULLSAFE, no_bucket, ERROR, [`NullsafeMode$VariousMethods.returnVal()`: `@NullsafeStrict` mode prohibits using values coming from non-strict classes without a check. Result of this call is used at line 135. Either add a local check for null or assertion, or make NullsafeMode$VariousMethods nullsafe strict.] -codetoanalyze/java/nullsafe-default/NullsafeMode.java, codetoanalyze.java.nullsafe_default.NullsafeMode$TrustAllNullsafe.BAD_returnNullFromNonNulsafe():java.lang.String, 0, ERADICATE_RETURN_NOT_NULLABLE, no_bucket, ERROR, [`BAD_returnNullFromNonNulsafe()`: return type is declared non-nullable but the method returns a nullable value: call to returnNull() at line 55.] -codetoanalyze/java/nullsafe-default/NullsafeMode.java, codetoanalyze.java.nullsafe_default.NullsafeMode$TrustNoneNullsafe.BAD_returnFromNonNullsafe():java.lang.String, 0, ERADICATE_UNCHECKED_USAGE_IN_NULLSAFE, no_bucket, ERROR, [`NullsafeMode$VariousMethods.returnVal()`: `@NullsafeLocal(trust=none)` mode prohibits using values coming from non-nullsafe classes without a check. Result of this call is used at line 95. Either add a local check for null or assertion, or make NullsafeMode$VariousMethods nullsafe.] -codetoanalyze/java/nullsafe-default/NullsafeMode.java, codetoanalyze.java.nullsafe_default.NullsafeMode$TrustSomeNullsafe.BAD_returnFromAnotherNonNullsafe():java.lang.String, 0, ERADICATE_UNCHECKED_USAGE_IN_NULLSAFE, no_bucket, ERROR, [`NullsafeMode$VariousMethods.returnVal()`: `@NullsafeLocal(trust=none)` mode prohibits using values coming from non-nullsafe classes without a check. Result of this call is used at line 79. Either add a local check for null or assertion, or make NullsafeMode$VariousMethods nullsafe.] -codetoanalyze/java/nullsafe-default/NullsafeMode.java, codetoanalyze.java.nullsafe_default.NullsafeMode$TrustSomeNullsafe.BAD_returnNullFromNonNulsafe():java.lang.String, 0, ERADICATE_RETURN_NOT_NULLABLE, no_bucket, ERROR, [`BAD_returnNullFromNonNulsafe()`: return type is declared non-nullable but the method returns a nullable value: call to returnNull() at line 88.] -codetoanalyze/java/nullsafe-default/NullsafeMode.java, codetoanalyze.java.nullsafe_default.NullsafeMode$TrustSomeNullsafe.FP_OK_returnFromNonNullsafe():java.lang.String, 0, ERADICATE_UNCHECKED_USAGE_IN_NULLSAFE, no_bucket, ERROR, [`NullsafeMode$VariousMethods.returnVal()`: `@NullsafeLocal(trust=none)` mode prohibits using values coming from non-nullsafe classes without a check. Result of this call is used at line 75. Either add a local check for null or assertion, or make NullsafeMode$VariousMethods nullsafe.] +codetoanalyze/java/nullsafe-default/NullsafeMode.java, codetoanalyze.java.nullsafe_default.NullsafeMode$NullsafeWithStrictMode.BAD_returnFromNonStrict():java.lang.String, 0, ERADICATE_UNCHECKED_USAGE_IN_NULLSAFE, no_bucket, ERROR, [`NullsafeMode$VariousMethods.returnVal()`: `@NullsafeStrict` mode prohibits using values coming from non-strict classes without a check. Result of this call is used at line 133. Either add a local check for null or assertion, or make NullsafeMode$VariousMethods nullsafe strict.] +codetoanalyze/java/nullsafe-default/NullsafeMode.java, codetoanalyze.java.nullsafe_default.NullsafeMode$StrictNullsafe.BAD_returnFromNonNullsafe():java.lang.String, 0, ERADICATE_UNCHECKED_USAGE_IN_NULLSAFE, no_bucket, ERROR, [`NullsafeMode$VariousMethods.returnVal()`: `@NullsafeStrict` mode prohibits using values coming from non-strict classes without a check. Result of this call is used at line 153. Either add a local check for null or assertion, or make NullsafeMode$VariousMethods nullsafe strict.] +codetoanalyze/java/nullsafe-default/NullsafeMode.java, codetoanalyze.java.nullsafe_default.NullsafeMode$TrustAllNullsafe.BAD_returnFromUnvettedThirdParty():java.lang.String, 0, ERADICATE_UNVETTED_THIRD_PARTY_IN_NULLSAFE, no_bucket, ERROR, [`ThirdPartyTestClass.returnUnspecified()`: `@NullsafeLocal(trust=all)` mode prohibits using values coming from not vetted third party methods without a check. Result of this call is used at line 65. Either add a local check for null or assertion, or add the correct signature to nullsafe-default/third-party-signatures/some.test.pckg.sig.] +codetoanalyze/java/nullsafe-default/NullsafeMode.java, codetoanalyze.java.nullsafe_default.NullsafeMode$TrustAllNullsafe.BAD_returnNonNullableFieldFromThirdParty():java.lang.String, 0, ERADICATE_UNCHECKED_USAGE_IN_NULLSAFE, no_bucket, ERROR, [`ThirdPartyTestClass.nonNullableField`: `@NullsafeLocal(trust=all)` mode prohibits using values coming from third-party classes without a check. This field is used at line 73. Either add a local check for null or assertion, or access `nonNullableField` via a nullsafe getter.] +codetoanalyze/java/nullsafe-default/NullsafeMode.java, codetoanalyze.java.nullsafe_default.NullsafeMode$TrustAllNullsafe.BAD_returnNullFromNonNulsafe():java.lang.String, 0, ERADICATE_RETURN_NOT_NULLABLE, no_bucket, ERROR, [`BAD_returnNullFromNonNulsafe()`: return type is declared non-nullable but the method returns a nullable value: call to returnNull() at line 61.] +codetoanalyze/java/nullsafe-default/NullsafeMode.java, codetoanalyze.java.nullsafe_default.NullsafeMode$TrustAllNullsafe.BAD_returnNullableFieldFromThirdParty():java.lang.String, 0, ERADICATE_RETURN_NOT_NULLABLE, no_bucket, ERROR, [`BAD_returnNullableFieldFromThirdParty()`: return type is declared non-nullable but the method returns a nullable value: field nullableField at line 69.] +codetoanalyze/java/nullsafe-default/NullsafeMode.java, codetoanalyze.java.nullsafe_default.NullsafeMode$TrustNoneNullsafe.BAD_returnFromNonNullsafe():java.lang.String, 0, ERADICATE_UNCHECKED_USAGE_IN_NULLSAFE, no_bucket, ERROR, [`NullsafeMode$VariousMethods.returnVal()`: `@NullsafeLocal(trust=none)` mode prohibits using values coming from non-nullsafe classes without a check. Result of this call is used at line 113. Either add a local check for null or assertion, or make NullsafeMode$VariousMethods nullsafe.] +codetoanalyze/java/nullsafe-default/NullsafeMode.java, codetoanalyze.java.nullsafe_default.NullsafeMode$TrustSomeNullsafe.BAD_returnFromAnotherNonNullsafe():java.lang.String, 0, ERADICATE_UNCHECKED_USAGE_IN_NULLSAFE, no_bucket, ERROR, [`NullsafeMode$VariousMethods.returnVal()`: `@NullsafeLocal(trust=none)` mode prohibits using values coming from non-nullsafe classes without a check. Result of this call is used at line 97. Either add a local check for null or assertion, or make NullsafeMode$VariousMethods nullsafe.] +codetoanalyze/java/nullsafe-default/NullsafeMode.java, codetoanalyze.java.nullsafe_default.NullsafeMode$TrustSomeNullsafe.BAD_returnNullFromNonNulsafe():java.lang.String, 0, ERADICATE_RETURN_NOT_NULLABLE, no_bucket, ERROR, [`BAD_returnNullFromNonNulsafe()`: return type is declared non-nullable but the method returns a nullable value: call to returnNull() at line 106.] +codetoanalyze/java/nullsafe-default/NullsafeMode.java, codetoanalyze.java.nullsafe_default.NullsafeMode$TrustSomeNullsafe.FP_OK_returnFromNonNullsafe():java.lang.String, 0, ERADICATE_UNCHECKED_USAGE_IN_NULLSAFE, no_bucket, ERROR, [`NullsafeMode$VariousMethods.returnVal()`: `@NullsafeLocal(trust=none)` mode prohibits using values coming from non-nullsafe classes without a check. Result of this call is used at line 93. Either add a local check for null or assertion, or make NullsafeMode$VariousMethods nullsafe.] codetoanalyze/java/nullsafe-default/NullsafeMode.java, codetoanalyze.java.nullsafe_default.NullsafeMode$TrustSomeNullsafe.OK_returnFromAnotherNonNullsafeAsNullable():java.lang.String, 0, ERADICATE_RETURN_OVER_ANNOTATED, no_bucket, WARNING, [Method `OK_returnFromAnotherNonNullsafeAsNullable()` is annotated with `@Nullable` but never returns null.] -codetoanalyze/java/nullsafe-default/NullsafeMode.java, codetoanalyze.java.nullsafe_default.NullsafeMode$TrustSomeNullsafe.returnVal():java.lang.String, 0, ERADICATE_INCONSISTENT_SUBCLASS_PARAMETER_ANNOTATION, no_bucket, ERROR, [0th parameter `this` of method `NullsafeMode$TrustSomeNullsafe.returnVal()` is missing `@Nullable` declaration when overriding `NullsafeMode$VariousMethods.returnVal()`. The parent method declared it can handle `null` for this param, so the child should also declare that.] codetoanalyze/java/nullsafe-default/ParameterNotNullable.java, codetoanalyze.java.nullsafe_default.ParameterNotNullable$ConstructorCall.(codetoanalyze.java.nullsafe_default.ParameterNotNullable,int), 0, ERADICATE_PARAMETER_NOT_NULLABLE, no_bucket, WARNING, [`ParameterNotNullable$ConstructorCall(...)`: parameter #2(`s`) is declared non-nullable but the argument is `null`.] codetoanalyze/java/nullsafe-default/ParameterNotNullable.java, codetoanalyze.java.nullsafe_default.ParameterNotNullable.callNull():void, 1, ERADICATE_PARAMETER_NOT_NULLABLE, no_bucket, WARNING, [`ParameterNotNullable.test(...)`: parameter #1(`s`) is declared non-nullable but the argument `s` is `null`: null constant at line 39.] codetoanalyze/java/nullsafe-default/ParameterNotNullable.java, codetoanalyze.java.nullsafe_default.ParameterNotNullable.callNullable(java.lang.String):void, 0, ERADICATE_PARAMETER_NOT_NULLABLE, no_bucket, WARNING, [`ParameterNotNullable.test(...)`: parameter #1(`s`) is declared non-nullable but the argument `s` is nullable.] @@ -245,6 +247,7 @@ codetoanalyze/java/nullsafe-default/StrictMode.java, codetoanalyze.java.nullsafe codetoanalyze/java/nullsafe-default/StrictMode.java, codetoanalyze.java.nullsafe_default.Strict.strictClass_dereferenceNullableFieldIsBad():void, 0, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, ERROR, [`__new(...).nullable` is nullable and is not locally checked for null when calling `toString()`.] codetoanalyze/java/nullsafe-default/StrictMode.java, codetoanalyze.java.nullsafe_default.Strict.strictClass_dereferenceNullableMethodIsBad():void, 0, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, ERROR, [`__new(...).getNullable()` is nullable and is not locally checked for null when calling `toString()`.] codetoanalyze/java/nullsafe-default/StrictMode.java, codetoanalyze.java.nullsafe_default.Strict.strictClass_dereferenceNullableStaticMethodIsBad():void, 0, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, ERROR, [`staticNullable()` is nullable and is not locally checked for null when calling `toString()`.] +codetoanalyze/java/nullsafe-default/StrictModeForThirdParty.java, codetoanalyze.java.nullsafe_default.StrictModeForThirdParty.dereferenceFieldIsBAD():void, 0, ERADICATE_UNCHECKED_USAGE_IN_NULLSAFE, no_bucket, ERROR, [`ThirdPartyTestClass.nonNullableField`: `@NullsafeStrict` mode prohibits using values coming from non-strict classes without a check. This field is used at line 49. Either add a local check for null or assertion, or access `nonNullableField` via a nullsafe strict getter.] codetoanalyze/java/nullsafe-default/StrictModeForThirdParty.java, codetoanalyze.java.nullsafe_default.StrictModeForThirdParty.dereferenceSpecifiedAsNullableIsBAD():void, 0, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, ERROR, [`StrictModeForThirdParty.obj.returnSpecifiedAsNullable()` is nullable and is not locally checked for null when calling `toString()`: call to ThirdPartyTestClass.returnSpecifiedAsNullable() at line 45 (declared nullable in nullsafe-default/third-party-signatures/some.test.pckg.sig at line 2).] codetoanalyze/java/nullsafe-default/StrictModeForThirdParty.java, codetoanalyze.java.nullsafe_default.StrictModeForThirdParty.dereferenceUnspecifiedIsBAD():void, 0, ERADICATE_UNVETTED_THIRD_PARTY_IN_NULLSAFE, no_bucket, ERROR, [`ThirdPartyTestClass.returnUnspecified()`: `@NullsafeStrict` mode prohibits using values coming from not vetted third party methods without a check. Result of this call is used at line 41. Either add a local check for null or assertion, or add the correct signature to nullsafe-default/third-party-signatures/some.test.pckg.sig.] codetoanalyze/java/nullsafe-default/StrictModeForThirdParty.java, codetoanalyze.java.nullsafe_default.StrictModeForThirdParty.passingNullableParamToUnspecifiedIsBAD():void, 0, ERADICATE_PARAMETER_NOT_NULLABLE, no_bucket, ERROR, [Third-party `ThirdPartyTestClass.paramUnspecified(...)` is missing a signature that would allow passing a nullable to param #1(`param`). Actual argument `getNullable()` is nullable. Consider adding the correct signature of `ThirdPartyTestClass.paramUnspecified(...)` to nullsafe-default/third-party-signatures/some.test.pckg.sig.] diff --git a/infer/tests/codetoanalyze/java/nullsafe-default/third-party-test-code/some/test/pckg/ThirdPartyTestClass.java b/infer/tests/codetoanalyze/java/nullsafe-default/third-party-test-code/some/test/pckg/ThirdPartyTestClass.java index 5c0981058..7315b75d8 100644 --- a/infer/tests/codetoanalyze/java/nullsafe-default/third-party-test-code/some/test/pckg/ThirdPartyTestClass.java +++ b/infer/tests/codetoanalyze/java/nullsafe-default/third-party-test-code/some/test/pckg/ThirdPartyTestClass.java @@ -6,12 +6,24 @@ */ package some.test.pckg; +import javax.annotation.Nullable; + /** * A test third party class. We specify its annotations outside of this class, in a third-party * repository. */ public class ThirdPartyTestClass { + // Fields. + + public String nonNullableField; + + @Nullable public String nullableField; + + public ThirdPartyTestClass() { + nonNullableField = "OK"; + } + // Return values. // No information in 3rd party repo