[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

@@ -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}

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;
+    }
   }
 }

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.<init>(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.]

infer/tests/codetoanalyze/java/nullsafe-default/third-party-signatures/java.sig

@@ -0,0 +1 @@
+java.lang.String#concat(java.lang.String)