From 9397b6865022a049f2bcd37de027365c3d4cdb5a Mon Sep 17 00:00:00 2001 From: Mitya Lyubarskiy Date: Wed, 15 Apr 2020 06:23:51 -0700 Subject: [PATCH] [nullsafe] Introduce LocallyTrustedNonnull nullability Summary: This diff finishes the stack fixing wrong promotion recommendations! Reviewed By: artempyanykh Differential Revision: D20949029 fbshipit-source-id: 59b6ad93f --- infer/src/nullsafe/AnnotatedNullability.ml | 7 ++++++- infer/src/nullsafe/AnnotatedNullability.mli | 1 + infer/src/nullsafe/AssignmentRule.ml | 6 ++++-- infer/src/nullsafe/ErrorRenderingUtils.ml | 3 +++ infer/src/nullsafe/Nullability.ml | 14 ++++++++++++-- infer/src/nullsafe/Nullability.mli | 4 ++++ infer/src/nullsafe/typeOrigin.ml | 2 ++ .../java/nullsafe-default/ModePromotions.java | 3 +-- .../codetoanalyze/java/nullsafe-default/issues.exp | 2 +- 9 files changed, 34 insertions(+), 8 deletions(-) diff --git a/infer/src/nullsafe/AnnotatedNullability.ml b/infer/src/nullsafe/AnnotatedNullability.ml index 60c28adad..dbb88622f 100644 --- a/infer/src/nullsafe/AnnotatedNullability.ml +++ b/infer/src/nullsafe/AnnotatedNullability.ml @@ -19,6 +19,7 @@ type t = | Nullable of nullable_origin | ThirdPartyNonnull | UncheckedNonnull of unchecked_nonnull_origin + | LocallyTrustedNonnull | LocallyCheckedNonnull | StrictNonnull of strict_nonnull_origin [@@deriving compare] @@ -47,6 +48,8 @@ let get_nullability = function Nullability.ThirdPartyNonnull | UncheckedNonnull _ -> Nullability.UncheckedNonnull + | LocallyTrustedNonnull -> + Nullability.LocallyTrustedNonnull | LocallyCheckedNonnull -> Nullability.LocallyCheckedNonnull | StrictNonnull _ -> @@ -88,6 +91,8 @@ let pp fmt t = F.fprintf fmt "ThirdPartyNonnull" | UncheckedNonnull origin -> F.fprintf fmt "UncheckedNonnull[%s]" (string_of_declared_nonnull_origin origin) + | LocallyTrustedNonnull -> + F.fprintf fmt "LocallyTrustedNonnull" | LocallyCheckedNonnull -> F.fprintf fmt "LocallyCheckedNonnull" | StrictNonnull origin -> @@ -131,4 +136,4 @@ let of_type_and_annotation ~is_callee_in_trust_list ~nullsafe_mode ~is_third_par if Annotations.ia_is_nonnull annotations then UncheckedNonnull AnnotatedNonnull else UncheckedNonnull ImplicitlyNonnull in - if is_callee_in_trust_list then LocallyCheckedNonnull else preliminary_nullability + if is_callee_in_trust_list then LocallyTrustedNonnull else preliminary_nullability diff --git a/infer/src/nullsafe/AnnotatedNullability.mli b/infer/src/nullsafe/AnnotatedNullability.mli index 66660893e..b5f42681a 100644 --- a/infer/src/nullsafe/AnnotatedNullability.mli +++ b/infer/src/nullsafe/AnnotatedNullability.mli @@ -21,6 +21,7 @@ type t = | Nullable of nullable_origin | ThirdPartyNonnull | UncheckedNonnull of unchecked_nonnull_origin + | LocallyTrustedNonnull | LocallyCheckedNonnull | StrictNonnull of strict_nonnull_origin [@@deriving compare] diff --git a/infer/src/nullsafe/AssignmentRule.ml b/infer/src/nullsafe/AssignmentRule.ml index 58833608f..a22fa0f27 100644 --- a/infer/src/nullsafe/AssignmentRule.ml +++ b/infer/src/nullsafe/AssignmentRule.ml @@ -113,8 +113,10 @@ module ReportableViolation = struct MF.pp_monospaced procname_str param_position pp_param_name param_signature.mangled argument_description nullability_evidence_as_suffix MF.pp_monospaced procname_str where_to_add_signature - | Nullability.LocallyCheckedNonnull | Nullability.UncheckedNonnull | Nullability.StrictNonnull - -> + | Nullability.LocallyCheckedNonnull + | Nullability.LocallyTrustedNonnull + | Nullability.UncheckedNonnull + | Nullability.StrictNonnull -> let nonnull_evidence = match model_source with | None -> diff --git a/infer/src/nullsafe/ErrorRenderingUtils.ml b/infer/src/nullsafe/ErrorRenderingUtils.ml index aa37acf92..5d968383a 100644 --- a/infer/src/nullsafe/ErrorRenderingUtils.ml +++ b/infer/src/nullsafe/ErrorRenderingUtils.ml @@ -26,6 +26,9 @@ module UserFriendlyNullable = struct Some (UntrustedNonnull LocallyCheckedNonnull) | Nullability.ThirdPartyNonnull -> Some (UntrustedNonnull ThirdPartyNonnull) + | Nullability.LocallyTrustedNonnull -> + (* The value is trusted in the current mode by definition, hence is not treated as nullable. *) + None | Nullability.StrictNonnull -> None end diff --git a/infer/src/nullsafe/Nullability.ml b/infer/src/nullsafe/Nullability.ml index 9709cb74d..1e3df12b1 100644 --- a/infer/src/nullsafe/Nullability.ml +++ b/infer/src/nullsafe/Nullability.ml @@ -13,6 +13,7 @@ type t = | Nullable | ThirdPartyNonnull | UncheckedNonnull + | LocallyTrustedNonnull | LocallyCheckedNonnull | StrictNonnull [@@deriving compare, equal] @@ -33,6 +34,8 @@ let join x y = ThirdPartyNonnull | UncheckedNonnull, _ | _, UncheckedNonnull -> UncheckedNonnull + | LocallyTrustedNonnull, _ | _, LocallyTrustedNonnull -> + LocallyTrustedNonnull | LocallyCheckedNonnull, _ | _, LocallyCheckedNonnull -> LocallyCheckedNonnull | StrictNonnull, StrictNonnull -> @@ -46,12 +49,17 @@ let is_considered_nonnull ~nullsafe_mode nullability = 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. *) + | NullsafeMode.Local (NullsafeMode.Trust.Only []) -> + (* Though "trust none" is technically a subcase of trust some, + we need this pattern to be different from the one below so we can detect possible + promotions from "trust some" to "trust none" *) LocallyCheckedNonnull + | NullsafeMode.Local (NullsafeMode.Trust.Only _classes) -> + LocallyTrustedNonnull | NullsafeMode.Local NullsafeMode.Trust.All -> UncheckedNonnull | NullsafeMode.Default -> + (* In default mode, we trust everything, even not annotated third party. *) ThirdPartyNonnull in is_subtype ~subtype:nullability ~supertype:least_required @@ -68,6 +76,8 @@ let to_string = function "ThirdPartyNonnull" | UncheckedNonnull -> "UncheckedNonnull" + | LocallyTrustedNonnull -> + "LocallyTrustedNonnull" | LocallyCheckedNonnull -> "LocallyCheckedNonnull" | StrictNonnull -> diff --git a/infer/src/nullsafe/Nullability.mli b/infer/src/nullsafe/Nullability.mli index 1bd02e33f..d6373ddac 100644 --- a/infer/src/nullsafe/Nullability.mli +++ b/infer/src/nullsafe/Nullability.mli @@ -28,6 +28,10 @@ type t = conventions) as non-nullable. Hovewer, the class is not checked under [@Nullsafe], so its actual nullability is not truthworhy, as the class might contain arbitrary number of nullability issues *) + | LocallyTrustedNonnull + (** The same as [UncheckedNonnull], but the value comes from a class explicitly listed as + trusted in the class under analysis. It is less truthworthy than [LocallyCheckedNonnull] + as no actual verification were performed. *) | LocallyCheckedNonnull (** Non-nullable value that comes from a class checked under [@NullsafeLocal] mode. Local mode type-checks files against its dependencies but does not require the dependencies to be diff --git a/infer/src/nullsafe/typeOrigin.ml b/infer/src/nullsafe/typeOrigin.ml index 07222e0c8..c3083537a 100644 --- a/infer/src/nullsafe/typeOrigin.ml +++ b/infer/src/nullsafe/typeOrigin.ml @@ -66,6 +66,7 @@ let get_nullability = function Nullability.Nullable | AnnotatedNullability.UncheckedNonnull _ | AnnotatedNullability.ThirdPartyNonnull + | AnnotatedNullability.LocallyTrustedNonnull | AnnotatedNullability.LocallyCheckedNonnull | AnnotatedNullability.StrictNonnull _ -> (* Nonnull param should be treated as trusted inside this function context: @@ -131,6 +132,7 @@ let get_method_ret_description pname call_loc "nullable" | AnnotatedNullability.ThirdPartyNonnull | AnnotatedNullability.UncheckedNonnull _ + | AnnotatedNullability.LocallyTrustedNonnull | AnnotatedNullability.LocallyCheckedNonnull | AnnotatedNullability.StrictNonnull _ -> "non-nullable" diff --git a/infer/tests/codetoanalyze/java/nullsafe-default/ModePromotions.java b/infer/tests/codetoanalyze/java/nullsafe-default/ModePromotions.java index 090cb4879..e158ca769 100644 --- a/infer/tests/codetoanalyze/java/nullsafe-default/ModePromotions.java +++ b/infer/tests/codetoanalyze/java/nullsafe-default/ModePromotions.java @@ -81,11 +81,10 @@ class TrustSome_DoesNotUseTrusted_CanBePromotedToTrustNone { } } -// FIXME: promo is incorrectly calculated as trust none @Nullsafe( value = Nullsafe.Mode.LOCAL, trustOnly = @Nullsafe.TrustList({Default_NoDeps_CanBePromotedToStrict.class})) -class TrustSome_UsesTrusted_NoPromo_FIXME { +class TrustSome_UsesTrusted_NoPromo { static String f() { return Default_NoDeps_CanBePromotedToStrict.f(); } diff --git a/infer/tests/codetoanalyze/java/nullsafe-default/issues.exp b/infer/tests/codetoanalyze/java/nullsafe-default/issues.exp index 3b014aa80..6e0193a71 100644 --- a/infer/tests/codetoanalyze/java/nullsafe-default/issues.exp +++ b/infer/tests/codetoanalyze/java/nullsafe-default/issues.exp @@ -172,7 +172,7 @@ codetoanalyze/java/nullsafe-default/ModePromotions.java, Linters_dummy_method, 1 codetoanalyze/java/nullsafe-default/ModePromotions.java, Linters_dummy_method, 1, ERADICATE_META_CLASS_IS_NULLSAFE, no_bucket, INFO, [], Strict_NoDeps_NoPromos, codetoanalyze.java.nullsafe_default, issues: 0, curr_mode: "Strict" codetoanalyze/java/nullsafe-default/ModePromotions.java, Linters_dummy_method, 1, ERADICATE_META_CLASS_IS_NULLSAFE, no_bucket, INFO, [], TrustSome_DoesNotUseTrusted_CanBePromotedToTrustNone, codetoanalyze.java.nullsafe_default, issues: 0, curr_mode: "LocalTrustSome", promote_mode: "LocalTrustNone" codetoanalyze/java/nullsafe-default/ModePromotions.java, Linters_dummy_method, 1, ERADICATE_META_CLASS_IS_NULLSAFE, no_bucket, INFO, [], TrustSome_TrustToLocalIsNotNeeded_CanBePromotedToTrustNone, codetoanalyze.java.nullsafe_default, issues: 0, curr_mode: "LocalTrustSome", promote_mode: "LocalTrustNone" -codetoanalyze/java/nullsafe-default/ModePromotions.java, Linters_dummy_method, 1, ERADICATE_META_CLASS_IS_NULLSAFE, no_bucket, INFO, [], TrustSome_UsesTrusted_NoPromo_FIXME, codetoanalyze.java.nullsafe_default, issues: 0, curr_mode: "LocalTrustSome", promote_mode: "LocalTrustNone" +codetoanalyze/java/nullsafe-default/ModePromotions.java, Linters_dummy_method, 1, ERADICATE_META_CLASS_IS_NULLSAFE, no_bucket, INFO, [], TrustSome_UsesTrusted_NoPromo, codetoanalyze.java.nullsafe_default, issues: 0, curr_mode: "LocalTrustSome" codetoanalyze/java/nullsafe-default/MyPreconditions.java, Linters_dummy_method, 1, ERADICATE_META_CLASS_CAN_BE_NULLSAFE, no_bucket, ADVICE, [Congrats! Class codetoanalyze.java.nullsafe_default.MyPreconditions is free of nullability issues. Mark it `@Nullsafe(Nullsafe.Mode.Local)` to prevent regressions.], MyPreconditions, codetoanalyze.java.nullsafe_default, issues: 0, curr_mode: "Default", promote_mode: "Strict" codetoanalyze/java/nullsafe-default/NestedFieldAccess.java, Linters_dummy_method, 1, ERADICATE_META_CLASS_CAN_BE_NULLSAFE, no_bucket, ADVICE, [Congrats! Class codetoanalyze.java.nullsafe_default.NestedFieldAccess is free of nullability issues. Mark it `@Nullsafe(Nullsafe.Mode.Local)` to prevent regressions.], NestedFieldAccess, codetoanalyze.java.nullsafe_default, issues: 0, curr_mode: "Default", promote_mode: "Strict" codetoanalyze/java/nullsafe-default/NestedFieldAccess.java, Linters_dummy_method, 1, ERADICATE_META_CLASS_CAN_BE_NULLSAFE, no_bucket, ADVICE, [Congrats! Class codetoanalyze.java.nullsafe_default.NestedFieldAccess$C is free of nullability issues. Mark it `@Nullsafe(Nullsafe.Mode.Local)` to prevent regressions.], NestedFieldAccess$C, codetoanalyze.java.nullsafe_default, issues: 0, curr_mode: "Default", promote_mode: "Strict"