From 9697c6e294fca5c91487d5024b878122c7c3a023 Mon Sep 17 00:00:00 2001 From: Artem Pianykh Date: Thu, 20 Feb 2020 09:38:44 -0800 Subject: [PATCH] [nullsafe] Fix treatment of enum values wrt nullsafe mode Summary: Introduction of `ThirdPartyNonnull` nullability broke nullability refinement heuristic for enums. This diff fixes it and also adds tests so that we hopefully avoid such issues in future. Reviewed By: mityal Differential Revision: D19975810 fbshipit-source-id: f9245f305 --- infer/src/nullsafe/AnnotatedField.ml | 40 +++++++++---------- .../java/nullsafe-default/NullsafeMode.java | 36 +++++++++++++---- .../java/nullsafe-default/issues.exp | 20 +++++----- .../third-party-signatures/java.sig | 1 + 4 files changed, 59 insertions(+), 38 deletions(-) create mode 100644 infer/tests/codetoanalyze/java/nullsafe-default/third-party-signatures/java.sig diff --git a/infer/src/nullsafe/AnnotatedField.ml b/infer/src/nullsafe/AnnotatedField.ml index 4df5d32db..18c66446a 100644 --- a/infer/src/nullsafe/AnnotatedField.ml +++ b/infer/src/nullsafe/AnnotatedField.ml @@ -37,6 +37,7 @@ let is_enum_value tenv ~class_typ (field_info : Struct.field_info) = let get tenv field_name class_typ = + let open IOption.Let_syntax in let lookup = Tenv.lookup tenv in (* We currently don't support field-level strict mode annotation, so fetch it from class *) let nullsafe_mode = @@ -48,23 +49,22 @@ let get tenv field_name class_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 ~nullsafe_mode ~is_third_party field_typ - annotations - in - let corrected_nullability = - match nullability with - | AnnotatedNullability.UncheckedNonnull _ when is_enum_value -> - (* Enum values are the special case - they can not be null. So we can strengten nullability. - Note that if it is nullable, we do NOT change nullability: in this case this is probably - not an enum value, but just a static field annotated as nullable. - *) - AnnotatedNullability.StrictNonnull EnumValue - | _ -> - nullability - in - let annotated_type = AnnotatedType.{nullability= corrected_nullability; typ= field_typ} in - {annotation_deprecated= annotations; annotated_type} ) + let+ (Struct.{typ= field_typ; annotations} as field_info) = + Struct.get_field_info ~lookup field_name class_typ + in + let is_enum_value = is_enum_value tenv ~class_typ field_info in + let nullability = + AnnotatedNullability.of_type_and_annotation ~nullsafe_mode ~is_third_party field_typ annotations + in + let corrected_nullability = + if Nullability.is_nonnullish (AnnotatedNullability.get_nullability nullability) && is_enum_value + then + (* Enum values are the special case - they can not be null. So we can strengten nullability. + Note that if it is nullable, we do NOT change nullability: in this case this is probably + not an enum value, but just a static field annotated as nullable. + *) + AnnotatedNullability.StrictNonnull EnumValue + else nullability + in + let annotated_type = AnnotatedType.{nullability= corrected_nullability; typ= field_typ} in + {annotation_deprecated= annotations; annotated_type} diff --git a/infer/tests/codetoanalyze/java/nullsafe-default/NullsafeMode.java b/infer/tests/codetoanalyze/java/nullsafe-default/NullsafeMode.java index 9836401a5..4980062fa 100644 --- a/infer/tests/codetoanalyze/java/nullsafe-default/NullsafeMode.java +++ b/infer/tests/codetoanalyze/java/nullsafe-default/NullsafeMode.java @@ -9,11 +9,12 @@ package codetoanalyze.java.nullsafe_default; import com.facebook.infer.annotation.Nullsafe; import com.facebook.infer.annotation.NullsafeStrict; +import java.util.concurrent.TimeUnit; import javax.annotation.Nullable; import some.test.pckg.ThirdPartyTestClass; public class NullsafeMode { - abstract class VariousMethods { + abstract static class VariousMethods { public String returnVal() { return "OK"; } @@ -24,7 +25,7 @@ public class NullsafeMode { } } - class NonNullsafe extends VariousMethods { + static class NonNullsafe extends VariousMethods { String OK_passUncheckedToLocal(String arg) { return new TrustAllNullsafe().acceptVal(arg); } @@ -43,10 +44,10 @@ public class NullsafeMode { } } - class AnotherNonNullsafe extends VariousMethods {} + static class AnotherNonNullsafe extends VariousMethods {} @Nullsafe(Nullsafe.Mode.LOCAL) - class TrustAllNullsafe extends VariousMethods { + static class TrustAllNullsafe extends VariousMethods { public String acceptVal(String arg) { return arg; } @@ -83,7 +84,7 @@ public class NullsafeMode { } @Nullsafe(value = Nullsafe.Mode.LOCAL, trustOnly = @Nullsafe.TrustList({NonNullsafe.class})) - class TrustSomeNullsafe extends VariousMethods { + static class TrustSomeNullsafe extends VariousMethods { @Override public String returnVal() { return "OK"; @@ -108,7 +109,7 @@ public class NullsafeMode { } @Nullsafe(value = Nullsafe.Mode.LOCAL, trustOnly = @Nullsafe.TrustList({})) - class TrustNoneNullsafe extends VariousMethods { + static class TrustNoneNullsafe extends VariousMethods { String BAD_returnFromNonNullsafe() { return new NonNullsafe().returnVal(); } @@ -119,7 +120,7 @@ public class NullsafeMode { } @Nullsafe(Nullsafe.Mode.STRICT) - class NullsafeWithStrictMode extends VariousMethods { + static class NullsafeWithStrictMode extends VariousMethods { @Override public String returnVal() { return "OK"; @@ -138,8 +139,19 @@ public class NullsafeMode { } } + static class UncheckedParams { + public long mDelay; + + public UncheckedParams(long delay) { + mDelay = delay; + } + } + @NullsafeStrict - class StrictNullsafe extends VariousMethods { + static class StrictNullsafe extends VariousMethods { + private static final UncheckedParams PARAMS = + new UncheckedParams(TimeUnit.MINUTES.toMillis(42)); + @Override public String returnVal() { return "OK"; @@ -156,5 +168,13 @@ public class NullsafeMode { String OK_returnFromNullsafeWithStrictMode() { return new NullsafeWithStrictMode().returnVal(); } + + long OK_callMethodsOnThirdPartyEnumValues() { + return TimeUnit.MINUTES.toMillis(42); + } + + long OK_passResultOfCallingThirdPartyToStrict() { + return PARAMS.mDelay; + } } } diff --git a/infer/tests/codetoanalyze/java/nullsafe-default/issues.exp b/infer/tests/codetoanalyze/java/nullsafe-default/issues.exp index 63befc8b6..443e82791 100644 --- a/infer/tests/codetoanalyze/java/nullsafe-default/issues.exp +++ b/infer/tests/codetoanalyze/java/nullsafe-default/issues.exp @@ -147,16 +147,16 @@ 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 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$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 134. 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 165. 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 66. 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 74. 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 62.] +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 70.] +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 114. 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 98. 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 107.] +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 94. 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/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.] diff --git a/infer/tests/codetoanalyze/java/nullsafe-default/third-party-signatures/java.sig b/infer/tests/codetoanalyze/java/nullsafe-default/third-party-signatures/java.sig new file mode 100644 index 000000000..6ca056345 --- /dev/null +++ b/infer/tests/codetoanalyze/java/nullsafe-default/third-party-signatures/java.sig @@ -0,0 +1 @@ +java.lang.String#concat(java.lang.String) \ No newline at end of file