[nullsafe] Reorder and add more cases for checking @PropagatesNullable annotation

Summary:
1. Split into 3 subclasses for 3 major set of features we test
2. Document a known FP
3. More clear names

Reviewed By: jberdine

Differential Revision: D17285902

fbshipit-source-id: 66e3b5668

infer/tests/codetoanalyze/java/nullsafe-default/CustomAnnotations.java

@@ -103,56 +103,99 @@ public class CustomAnnotations {
   // Tests for the annotation @PropagatesNullable
   class TestPropagatesNullable {
 
-    // one parameter: means return null iff s is null
+    class TestOneParameter {
-    String m(@PropagatesNullable String s) {
-      return s;
-    }
 
-    void mBad() {
+      // means return null iff s is null
-      m(null).length(); // bad: m returns null
+      String propagatesNullable(@PropagatesNullable String s) {
-    }
+        return s;
+      }
 
-    void mGood() {
+      @Nullable
-      m("").length();
+      String nullable(@Nullable String s) {
-    }
+        return s;
+      }
 
-    // limitation: we currently cannot check the body, and just trust the annotation
+      void test(@Nullable String sNullable, String sNonnull) {
-    String cannotCheckBody(@PropagatesNullable String s) {
+        // null constant
-      return null; // nothing is reported here
+        propagatesNullable(null).length(); // BAD: will be NPE
-    }
+        nullable(null).length(); // BAD: result might be null
+
+        // nullable
+        propagatesNullable(sNullable).length(); // BAD: result can be null
+        nullable(sNullable).length(); // BAD: result can be null
+
+        // string literal
+        propagatesNullable("").length(); // OK: result can not be null
+        nullable("").length(); // BAD: typechecker can not figure out that this is safe
+
+        // nonnull
+        propagatesNullable(sNonnull).length(); // OK: result can not be null
+        nullable(sNonnull).length(); // BAD: typechecker can not figure out that this is safe
+
+        // flow sensitive nonnull FALSE POSITIVE
+        if (sNullable != null) {
+          // here we know that sNullable is not null, so it should be safe, but it is not the case
+          // TODO(T53770056) fix it.
+          propagatesNullable(sNullable).length();
+          nullable(sNullable).length();
+        }
+      }
 
-    void illustrateFalseNegativeAsCannotCheckBody() {
+      // limitation: we currently cannot check the body, and just trust the annotation
-      cannotCheckBody("").length(); // this is an NPE but is not found
+      String cannotCheckBody(@PropagatesNullable String s) {
-    }
+        return null; // nothing is reported here
+      }
 
-    // second parameter: means return null iff s2 is null
+      void illustrateFalseNegativeAsCannotCheckBody() {
-    String m2(@Nullable String s1, @PropagatesNullable String s2) {
+        cannotCheckBody("").length(); // this is an NPE but is not found
-      return s2;
+      }
     }
 
-    void m2Bad() {
+    class TestSecondParameter {
-      m2(null, null).length(); // bad: m2 returns null
+      // means return null iff s2 is null
-    }
+      String propagatesNullable(@Nullable String s1, @PropagatesNullable String s2) {
+        return s2;
+      }
 
-    void m2Good() {
+      @Nullable
-      m2(null, "").length();
+      String nullable(@Nullable String s1, @Nullable String s2) {
-    }
+        return s2;
+      }
 
-    // two parameters: means return null iff either s1 or s2 is null
+      // Let's ensure that @PropagatesNullable applies only to the parameter
-    String m12(@PropagatesNullable String s1, @PropagatesNullable String s2) {
+      // that was annotated with it, and not to other params.
-      return s1 == null ? s1 : s2;
+      void test(@Nullable String sNullable, String sNonnull) {
-    }
+        // Both nullable
+        propagatesNullable(sNullable, sNullable).length(); // BAD: result can be null
+        nullable(sNullable, sNullable).length(); // BAD: result can be null
 
-    void m12Bad1() {
+        // First is nonnull, second is nullable
-      m12(null, "").length(); // bad: m12 returns null
+        propagatesNullable(sNonnull, sNullable).length(); // BAD: result can be null
-    }
+        nullable(sNonnull, sNullable).length(); // BAD: result can be null
 
-    void m12Bad2() {
+        // First is nullable, second is nonnull
-      m12("", null).length(); // bad: m12 returns null
+        propagatesNullable(sNullable, sNonnull).length(); // OK: result can not be null
+        nullable(sNullable, sNonnull).length(); // BAD: typechecker can not figure this out
+
+        // Both nonnullable
+        propagatesNullable(sNonnull, sNonnull).length(); // OK: result can not be null
+        nullable(sNonnull, sNonnull).length(); // BAD: typechecker can not figure this out
+      }
     }
 
-    void m12Good() {
+    class TestBothParams {
-      m12("", "").length();
+      // both parameters are annotated:
+      // means return null iff either s1 or s2 is null
+      String propagatesNullable(@PropagatesNullable String s1, @PropagatesNullable String s2) {
+        return s1 == null ? s1 : s2;
+      }
+
+      void testBothParamsShouldBeNonnull(@Nullable String sNullable, String sNonnull) {
+        propagatesNullable(sNullable, sNullable).length(); // BAD: result can be null
+        propagatesNullable(sNonnull, sNullable).length(); // BAD: result can be null
+        propagatesNullable(sNullable, sNonnull).length(); // BAD: result can be null
+        propagatesNullable(sNonnull, sNonnull).length(); // OK: result can be null
+      }
     }
   }
 

infer/tests/codetoanalyze/java/nullsafe-default/issues.exp

@@ -11,10 +11,23 @@ codetoanalyze/java/nullsafe-default/CustomAnnotations.java, codetoanalyze.java.n
 codetoanalyze/java/nullsafe-default/CustomAnnotations.java, codetoanalyze.java.nullsafe_default.CustomAnnotations$Test.testModelledTrueOnNull(java.lang.String):void, 7, ERADICATE_CONDITION_REDUNDANT, no_bucket, WARNING, [The condition s is always false according to the existing annotations.]
 codetoanalyze/java/nullsafe-default/CustomAnnotations.java, codetoanalyze.java.nullsafe_default.CustomAnnotations$Test.testTrueOnNull(java.lang.CharSequence):void, 8, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [The value of `s` in the call to `toString()` is nullable and is not locally checked for null. (Origin: method parameter s)]
 codetoanalyze/java/nullsafe-default/CustomAnnotations.java, codetoanalyze.java.nullsafe_default.CustomAnnotations$Test.testTrueOnNull(java.lang.CharSequence):void, 12, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [The value of `s` in the call to `toString()` is nullable and is not locally checked for null. (Origin: method parameter s)]
-codetoanalyze/java/nullsafe-default/CustomAnnotations.java, codetoanalyze.java.nullsafe_default.CustomAnnotations$TestPropagatesNullable.m12Bad1():void, 1, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [The value of `m12(...)` in the call to `length()` is nullable and is not locally checked for null. (Origin: call to m12(...) at line 147)]
+codetoanalyze/java/nullsafe-default/CustomAnnotations.java, codetoanalyze.java.nullsafe_default.CustomAnnotations$TestPropagatesNullable$TestBothParams.testBothParamsShouldBeNonnull(java.lang.String,java.lang.String):void, 1, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [The value of `propagatesNullable(...)` in the call to `length()` is nullable and is not locally checked for null. (Origin: call to propagatesNullable(...) at line 194)]
-codetoanalyze/java/nullsafe-default/CustomAnnotations.java, codetoanalyze.java.nullsafe_default.CustomAnnotations$TestPropagatesNullable.m12Bad2():void, 1, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [The value of `m12(...)` in the call to `length()` is nullable and is not locally checked for null. (Origin: call to m12(...) at line 151)]
+codetoanalyze/java/nullsafe-default/CustomAnnotations.java, codetoanalyze.java.nullsafe_default.CustomAnnotations$TestPropagatesNullable$TestBothParams.testBothParamsShouldBeNonnull(java.lang.String,java.lang.String):void, 2, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [The value of `propagatesNullable(...)` in the call to `length()` is nullable and is not locally checked for null. (Origin: call to propagatesNullable(...) at line 195)]
-codetoanalyze/java/nullsafe-default/CustomAnnotations.java, codetoanalyze.java.nullsafe_default.CustomAnnotations$TestPropagatesNullable.m2Bad():void, 1, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [The value of `m2(...)` in the call to `length()` is nullable and is not locally checked for null. (Origin: call to m2(...) at line 134)]
+codetoanalyze/java/nullsafe-default/CustomAnnotations.java, codetoanalyze.java.nullsafe_default.CustomAnnotations$TestPropagatesNullable$TestBothParams.testBothParamsShouldBeNonnull(java.lang.String,java.lang.String):void, 3, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [The value of `propagatesNullable(...)` in the call to `length()` is nullable and is not locally checked for null. (Origin: call to propagatesNullable(...) at line 196)]
-codetoanalyze/java/nullsafe-default/CustomAnnotations.java, codetoanalyze.java.nullsafe_default.CustomAnnotations$TestPropagatesNullable.mBad():void, 1, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [The value of `m(...)` in the call to `length()` is nullable and is not locally checked for null. (Origin: call to m(...) at line 112)]
+codetoanalyze/java/nullsafe-default/CustomAnnotations.java, codetoanalyze.java.nullsafe_default.CustomAnnotations$TestPropagatesNullable$TestOneParameter.test(java.lang.String,java.lang.String):void, 2, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [The value of `propagatesNullable(...)` in the call to `length()` is nullable and is not locally checked for null. (Origin: call to propagatesNullable(...) at line 120)]
+codetoanalyze/java/nullsafe-default/CustomAnnotations.java, codetoanalyze.java.nullsafe_default.CustomAnnotations$TestPropagatesNullable$TestOneParameter.test(java.lang.String,java.lang.String):void, 3, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [The value of `nullable(...)` in the call to `length()` is nullable and is not locally checked for null. (Origin: call to nullable(...) at line 121)]
+codetoanalyze/java/nullsafe-default/CustomAnnotations.java, codetoanalyze.java.nullsafe_default.CustomAnnotations$TestPropagatesNullable$TestOneParameter.test(java.lang.String,java.lang.String):void, 6, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [The value of `propagatesNullable(...)` in the call to `length()` is nullable and is not locally checked for null. (Origin: call to propagatesNullable(...) at line 124)]
+codetoanalyze/java/nullsafe-default/CustomAnnotations.java, codetoanalyze.java.nullsafe_default.CustomAnnotations$TestPropagatesNullable$TestOneParameter.test(java.lang.String,java.lang.String):void, 7, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [The value of `nullable(...)` in the call to `length()` is nullable and is not locally checked for null. (Origin: call to nullable(...) at line 125)]
+codetoanalyze/java/nullsafe-default/CustomAnnotations.java, codetoanalyze.java.nullsafe_default.CustomAnnotations$TestPropagatesNullable$TestOneParameter.test(java.lang.String,java.lang.String):void, 11, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [The value of `nullable(...)` in the call to `length()` is nullable and is not locally checked for null. (Origin: call to nullable(...) at line 129)]
+codetoanalyze/java/nullsafe-default/CustomAnnotations.java, codetoanalyze.java.nullsafe_default.CustomAnnotations$TestPropagatesNullable$TestOneParameter.test(java.lang.String,java.lang.String):void, 15, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [The value of `nullable(...)` in the call to `length()` is nullable and is not locally checked for null. (Origin: call to nullable(...) at line 133)]
+codetoanalyze/java/nullsafe-default/CustomAnnotations.java, codetoanalyze.java.nullsafe_default.CustomAnnotations$TestPropagatesNullable$TestOneParameter.test(java.lang.String,java.lang.String):void, 21, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [The value of `propagatesNullable(...)` in the call to `length()` is nullable and is not locally checked for null. (Origin: call to propagatesNullable(...) at line 124)]
+codetoanalyze/java/nullsafe-default/CustomAnnotations.java, codetoanalyze.java.nullsafe_default.CustomAnnotations$TestPropagatesNullable$TestOneParameter.test(java.lang.String,java.lang.String):void, 22, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [The value of `nullable(...)` in the call to `length()` is nullable and is not locally checked for null. (Origin: call to nullable(...) at line 125)]
+codetoanalyze/java/nullsafe-default/CustomAnnotations.java, codetoanalyze.java.nullsafe_default.CustomAnnotations$TestPropagatesNullable$TestSecondParameter.test(java.lang.String,java.lang.String):void, 2, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [The value of `propagatesNullable(...)` in the call to `length()` is nullable and is not locally checked for null. (Origin: call to propagatesNullable(...) at line 169)]
+codetoanalyze/java/nullsafe-default/CustomAnnotations.java, codetoanalyze.java.nullsafe_default.CustomAnnotations$TestPropagatesNullable$TestSecondParameter.test(java.lang.String,java.lang.String):void, 3, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [The value of `nullable(...)` in the call to `length()` is nullable and is not locally checked for null. (Origin: call to nullable(...) at line 170)]
+codetoanalyze/java/nullsafe-default/CustomAnnotations.java, codetoanalyze.java.nullsafe_default.CustomAnnotations$TestPropagatesNullable$TestSecondParameter.test(java.lang.String,java.lang.String):void, 6, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [The value of `propagatesNullable(...)` in the call to `length()` is nullable and is not locally checked for null. (Origin: call to propagatesNullable(...) at line 173)]
+codetoanalyze/java/nullsafe-default/CustomAnnotations.java, codetoanalyze.java.nullsafe_default.CustomAnnotations$TestPropagatesNullable$TestSecondParameter.test(java.lang.String,java.lang.String):void, 7, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [The value of `nullable(...)` in the call to `length()` is nullable and is not locally checked for null. (Origin: call to nullable(...) at line 174)]
+codetoanalyze/java/nullsafe-default/CustomAnnotations.java, codetoanalyze.java.nullsafe_default.CustomAnnotations$TestPropagatesNullable$TestSecondParameter.test(java.lang.String,java.lang.String):void, 11, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [The value of `nullable(...)` in the call to `length()` is nullable and is not locally checked for null. (Origin: call to nullable(...) at line 178)]
+codetoanalyze/java/nullsafe-default/CustomAnnotations.java, codetoanalyze.java.nullsafe_default.CustomAnnotations$TestPropagatesNullable$TestSecondParameter.test(java.lang.String,java.lang.String):void, 15, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [The value of `nullable(...)` in the call to `length()` is nullable and is not locally checked for null. (Origin: call to nullable(...) at line 182)]
 codetoanalyze/java/nullsafe-default/FieldNotInitialized.java, codetoanalyze.java.nullsafe_default.FieldNotInitialized$InitCircular.<init>(codetoanalyze.java.nullsafe_default.FieldNotInitialized), 0, ERADICATE_FIELD_NOT_INITIALIZED, no_bucket, WARNING, [Field `FieldNotInitialized$InitCircular.bad` is not initialized in the constructor and is not declared `@Nullable`]
 codetoanalyze/java/nullsafe-default/FieldNotInitialized.java, codetoanalyze.java.nullsafe_default.FieldNotInitialized$InitCircular.<init>(codetoanalyze.java.nullsafe_default.FieldNotInitialized), 0, ERADICATE_FIELD_NOT_INITIALIZED, no_bucket, WARNING, [Field `FieldNotInitialized$InitCircular.stillBad` is not initialized in the constructor and is not declared `@Nullable`]
 codetoanalyze/java/nullsafe-default/FieldNotInitialized.java, codetoanalyze.java.nullsafe_default.FieldNotInitialized$InitIfNull.<init>(codetoanalyze.java.nullsafe_default.FieldNotInitialized), 0, ERADICATE_FIELD_NOT_INITIALIZED, no_bucket, WARNING, [Field `FieldNotInitialized$InitIfNull.shouldBeGood_FIXME` is not initialized in the constructor and is not declared `@Nullable`]