From 7132a84b0d14eeb6949d634a8c20763d801e8de5 Mon Sep 17 00:00:00 2001 From: Mitya Lyubarskiy Date: Thu, 12 Sep 2019 05:55:11 -0700 Subject: [PATCH] [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 --- .../nullsafe-default/CustomAnnotations.java | 117 ++++++++++++------ .../java/nullsafe-default/issues.exp | 21 +++- 2 files changed, 97 insertions(+), 41 deletions(-) diff --git a/infer/tests/codetoanalyze/java/nullsafe-default/CustomAnnotations.java b/infer/tests/codetoanalyze/java/nullsafe-default/CustomAnnotations.java index 77213f0af..e51485f56 100644 --- a/infer/tests/codetoanalyze/java/nullsafe-default/CustomAnnotations.java +++ b/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 - String m(@PropagatesNullable String s) { - return s; - } + class TestOneParameter { - void mBad() { - m(null).length(); // bad: m returns null - } + // means return null iff s is null + String propagatesNullable(@PropagatesNullable String s) { + return s; + } - void mGood() { - m("").length(); - } + @Nullable + String nullable(@Nullable String s) { + return s; + } - // limitation: we currently cannot check the body, and just trust the annotation - String cannotCheckBody(@PropagatesNullable String s) { - return null; // nothing is reported here - } + void test(@Nullable String sNullable, String sNonnull) { + // null constant + 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() { - cannotCheckBody("").length(); // this is an NPE but is not found - } + // limitation: we currently cannot check the body, and just trust the annotation + String cannotCheckBody(@PropagatesNullable String s) { + return null; // nothing is reported here + } - // second parameter: means return null iff s2 is null - String m2(@Nullable String s1, @PropagatesNullable String s2) { - return s2; + void illustrateFalseNegativeAsCannotCheckBody() { + cannotCheckBody("").length(); // this is an NPE but is not found + } } - void m2Bad() { - m2(null, null).length(); // bad: m2 returns null - } + class TestSecondParameter { + // means return null iff s2 is null + String propagatesNullable(@Nullable String s1, @PropagatesNullable String s2) { + return s2; + } - void m2Good() { - m2(null, "").length(); - } + @Nullable + String nullable(@Nullable String s1, @Nullable String s2) { + return s2; + } - // two parameters: means return null iff either s1 or s2 is null - String m12(@PropagatesNullable String s1, @PropagatesNullable String s2) { - return s1 == null ? s1 : s2; - } + // Let's ensure that @PropagatesNullable applies only to the parameter + // that was annotated with it, and not to other params. + 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() { - m12(null, "").length(); // bad: m12 returns null - } + // First is nonnull, second is nullable + propagatesNullable(sNonnull, sNullable).length(); // BAD: result can be null + nullable(sNonnull, sNullable).length(); // BAD: result can be null - void m12Bad2() { - m12("", null).length(); // bad: m12 returns null + // First is nullable, second is nonnull + 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() { - m12("", "").length(); + class TestBothParams { + // 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 + } } } diff --git a/infer/tests/codetoanalyze/java/nullsafe-default/issues.exp b/infer/tests/codetoanalyze/java/nullsafe-default/issues.exp index 5c9358a91..da2ae80df 100644 --- a/infer/tests/codetoanalyze/java/nullsafe-default/issues.exp +++ b/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.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.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.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$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$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$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$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.(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.(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.(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`]