From 7a09618dc4b0816f7949d87da93ac762c19e720a Mon Sep 17 00:00:00 2001 From: Mitya Lyubarskiy Date: Thu, 12 Sep 2019 05:54:59 -0700 Subject: [PATCH] [nullsafe] Make test for @TrueOnNull and @FalseOnNull annotation specific about positive and negative cases Summary: Let's consolidate "positive" and "negative" cased together by adding an example of not annotated class as a source of "negative" cases. Also join the case with modelled methods to the same class. Reviewed By: jberdine Differential Revision: D17284101 fbshipit-source-id: e15e60691 --- .../nullsafe-default/CustomAnnotations.java | 95 +++++++++++-------- .../java/nullsafe-default/issues.exp | 21 ++-- 2 files changed, 68 insertions(+), 48 deletions(-) diff --git a/infer/tests/codetoanalyze/java/nullsafe-default/CustomAnnotations.java b/infer/tests/codetoanalyze/java/nullsafe-default/CustomAnnotations.java index 38f2c4664..77213f0af 100644 --- a/infer/tests/codetoanalyze/java/nullsafe-default/CustomAnnotations.java +++ b/infer/tests/codetoanalyze/java/nullsafe-default/CustomAnnotations.java @@ -16,54 +16,87 @@ import javax.annotation.Nullable; public class CustomAnnotations { - static class MyTextUtils { + // Example of API that benefits from annotating with @TrueOnNull and @FalseOnNull + static class AnnotatedTextUtils { @TrueOnNull - static boolean isEmpty(@Nullable java.lang.CharSequence s) { + static boolean isEmpty(@Nullable CharSequence s) { return s == null || s.equals(""); } @FalseOnNull - static boolean isNotEmpty(@Nullable java.lang.CharSequence s) { + static boolean isNotEmpty(@Nullable CharSequence s) { return s != null && s.length() > 0; } } - class TestTextUtilsIsEmpty { - void textUtilsNotIsEmpty(@Nullable CharSequence s) { - if (!TextUtils.isEmpty(s)) { - s.toString(); // OK - } + // The same API, but not annotated + static class NotAnnotatedTextUtils { + static boolean isEmpty(@Nullable CharSequence s) { + return s == null || s.equals(""); } - void textUtilsIsEmpty(@Nullable CharSequence s) { - if (TextUtils.isEmpty(s)) { - s.toString(); // BAD - } + static boolean isNotEmpty(@Nullable CharSequence s) { + return s != null && s.length() > 0; } + } + + class Test { + void testTrueOnNull(@Nullable CharSequence s) { + // Explicitly annotated + if (!AnnotatedTextUtils.isEmpty(s)) { + s.toString(); // OK: if we are here, we know that s is not null + } + + // Not annotated + if (!NotAnnotatedTextUtils.isEmpty(s)) { + s.toString(); // BAD: the typecker does not know s can not be null + } - void myTextUtilsNotIsEmpty(@Nullable CharSequence s) { - if (!MyTextUtils.isEmpty(s)) { - s.toString(); // OK + if (AnnotatedTextUtils.isEmpty(s)) { + s.toString(); // BAD: s can be null or an empty string } } - void myTextUtilsIsEmpty(@Nullable CharSequence s) { - if (MyTextUtils.isEmpty(s)) { - s.toString(); // BAD + void testFalseOnNull(@Nullable CharSequence s) { + // Explicitly annotated + if (AnnotatedTextUtils.isNotEmpty(s)) { + s.toString(); // OK: if we are here, we know that `s` is not null + } + + // Not annotated + if (NotAnnotatedTextUtils.isNotEmpty(s)) { + s.toString(); // BAD: the typecker does not know `s` can not be null + } + + if (!AnnotatedTextUtils.isNotEmpty(s)) { + s.toString(); // BAD: `s` can be null or an empty string } } - void myTextUtilsIsNotEmpty(@Nullable CharSequence s) { - if (MyTextUtils.isNotEmpty(s)) { - s.toString(); // OK + void testModelledTrueOnNull(String s) { + // TextUtils.isEmpty is modelled as TrueOnNull + if (!TextUtils.isEmpty(s)) { + s.toString(); // OK: if we are here, we know that `s` is not null + } + + // Strings.isNullOrEmpty is modelled as TrueOnNull + if (!Strings.isNullOrEmpty(s)) { + s.toString(); // OK: if we are here, we know that `s` is not null + } + + if (!NotAnnotatedTextUtils.isEmpty(s)) { + s.toString(); // BAD: the typechecker can not figure this out for not modelled class } } - void myTextUtilsNotIsNotEmpty(@Nullable CharSequence s) { - if (!MyTextUtils.isNotEmpty(s)) { - s.toString(); // BAD + void testEarlyReturn(@Nullable CharSequence s1, @Nullable CharSequence s2) { + if (AnnotatedTextUtils.isEmpty(s1) || NotAnnotatedTextUtils.isEmpty(s2)) { + return; } + + s1.toString(); // OK: if `s1` was null, we would no rech this point + s2.toString(); // BAD: typechecker can not figure this for not annotated class } } @@ -123,18 +156,4 @@ public class CustomAnnotations { } } - class TestModeledTrueOnNull { - - void testIsEmptyOrNullOk(@Nullable String string) { - if (!Strings.isNullOrEmpty(string)) { - string.contains("Infer"); - } - } - - void testIsEmptyOrNullBad(@Nullable String string) { - if (Strings.isNullOrEmpty(string)) { - string.contains("Infer"); - } - } - } } diff --git a/infer/tests/codetoanalyze/java/nullsafe-default/issues.exp b/infer/tests/codetoanalyze/java/nullsafe-default/issues.exp index 45a155909..5c9358a91 100644 --- a/infer/tests/codetoanalyze/java/nullsafe-default/issues.exp +++ b/infer/tests/codetoanalyze/java/nullsafe-default/issues.exp @@ -4,16 +4,17 @@ codetoanalyze/java/nullsafe-default/ButterKnife.java, codetoanalyze.java.nullsaf codetoanalyze/java/nullsafe-default/ButterKnife.java, codetoanalyze.java.nullsafe_default.ButterKnife.dereferencingNullableIsBAD():void, 1, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [The value of `ButterKnife.nullable` in the call to `length()` is nullable and is not locally checked for null. (Origin: field ButterKnife.nullable at line 33)] codetoanalyze/java/nullsafe-default/ButterKnife.java, codetoanalyze.java.nullsafe_default.ButterKnife.passingToNullableForNullableIsBAD():void, 1, ERADICATE_PARAMETER_NOT_NULLABLE, no_bucket, WARNING, [`ButterKnife.f(...)` needs a non-null value in parameter 1 but argument `ButterKnife.nullable` can be null. (Origin: field ButterKnife.nullable at line 61)] codetoanalyze/java/nullsafe-default/CapturedParam.java, codetoanalyze.java.nullsafe_default.CapturedParam.dereferencingNullableIsBAD(java.lang.Object):void, 1, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [The value of `parameter` in the call to `toString()` is nullable and is not locally checked for null. (Origin: method parameter parameter)] -codetoanalyze/java/nullsafe-default/CustomAnnotations.java, codetoanalyze.java.nullsafe_default.CustomAnnotations$TestModeledTrueOnNull.testIsEmptyOrNullBad(java.lang.String):void, 2, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [The value of `string` in the call to `contains(...)` is nullable and is not locally checked for null. (Origin: method parameter string)] -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 114)] -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 118)] -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 101)] -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 79)] -codetoanalyze/java/nullsafe-default/CustomAnnotations.java, codetoanalyze.java.nullsafe_default.CustomAnnotations$TestTextUtilsIsEmpty.myTextUtilsIsEmpty(java.lang.CharSequence):void, 2, 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$TestTextUtilsIsEmpty.myTextUtilsNotIsNotEmpty(java.lang.CharSequence):void, 2, 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$TestTextUtilsIsEmpty.textUtilsIsEmpty(java.lang.CharSequence):void, 1, ERADICATE_PARAMETER_NOT_NULLABLE, no_bucket, WARNING, [`TextUtils.isEmpty(...)` needs a non-null value in parameter 1 but argument `s` can be null. (Origin: method parameter s)] -codetoanalyze/java/nullsafe-default/CustomAnnotations.java, codetoanalyze.java.nullsafe_default.CustomAnnotations$TestTextUtilsIsEmpty.textUtilsIsEmpty(java.lang.CharSequence):void, 2, 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$TestTextUtilsIsEmpty.textUtilsNotIsEmpty(java.lang.CharSequence):void, 1, ERADICATE_PARAMETER_NOT_NULLABLE, no_bucket, WARNING, [`TextUtils.isEmpty(...)` needs a non-null value in parameter 1 but argument `s` can be null. (Origin: method parameter s)] +codetoanalyze/java/nullsafe-default/CustomAnnotations.java, codetoanalyze.java.nullsafe_default.CustomAnnotations$Test.testEarlyReturn(java.lang.CharSequence,java.lang.CharSequence):void, 6, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [The value of `s2` in the call to `toString()` is nullable and is not locally checked for null. (Origin: method parameter s2)] +codetoanalyze/java/nullsafe-default/CustomAnnotations.java, codetoanalyze.java.nullsafe_default.CustomAnnotations$Test.testFalseOnNull(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.testFalseOnNull(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$Test.testModelledTrueOnNull(java.lang.String):void, 2, 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.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/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`]