From 260176251c5e346f56ac4f4692e656fa5ff64f53 Mon Sep 17 00:00:00 2001 From: Mitya Lyubarskiy Date: Mon, 9 Sep 2019 01:54:48 -0700 Subject: [PATCH] [nullsafe] Make @Initializer annotation test specific and without "builder" boilerplate Summary: 1. Remove boilerplate with builder that uses builder initializer; it demostates a usecase but it is not really relevant for the test so it distracts attention. Instead, describe the usecase in the comment 2. Add good and bad cases so it is obvious what exactly do we test. Reviewed By: artempyanykh Differential Revision: D17204969 fbshipit-source-id: 005ea078b --- .../nullsafe-default/FieldNotInitialized.java | 62 ++++++++++++++++++ .../nullsafe-default/FieldNotNullable.java | 64 ------------------- .../java/nullsafe-default/issues.exp | 15 +++-- 3 files changed, 71 insertions(+), 70 deletions(-) diff --git a/infer/tests/codetoanalyze/java/nullsafe-default/FieldNotInitialized.java b/infer/tests/codetoanalyze/java/nullsafe-default/FieldNotInitialized.java index 218c64f1e..153ed0f83 100644 --- a/infer/tests/codetoanalyze/java/nullsafe-default/FieldNotInitialized.java +++ b/infer/tests/codetoanalyze/java/nullsafe-default/FieldNotInitialized.java @@ -9,6 +9,8 @@ package codetoanalyze.java.nullsafe_default; import android.support.annotation.NonNull; import android.widget.EditText; +import com.facebook.infer.annotation.Assertions; +import com.facebook.infer.annotation.Initializer; import com.facebook.infer.annotation.SuppressViewNullability; import javax.annotation.Nonnull; import javax.annotation.Nullable; @@ -160,3 +162,63 @@ public class FieldNotInitialized { } } } + +/** + * If a method is marked with @Initializer annotation, we essentially treat is as a constuctror: if + * a field is initialized in one of such methods, we assume this method will be called before using + * the field, so we don't consider it "not initialized" error. A popular usecase for that is a + * Builder pattern, when required fields are set not in the constuctor, but in corresponding + * setters, and then build() method checks in runtime that all fields are initialized. + */ +class TestInitializerAnnotation { + String initInConstructorIsOK; + String initInInitilizerMethod1IsOK; + String initInInitilizerMethod2IsOK; + String initInAnyOtherMethodIsBAD; + String initByNullableInInitializedMethodIsBAD; + String dontInitAtAllIsBAD; + @Nullable String dontInitOptionalIsOK; + + TestInitializerAnnotation() { + initInConstructorIsOK = ""; + } + + @Initializer + void set1(String value) { + // OK: we assume set1() will be called before the class is actually used + this.initInInitilizerMethod1IsOK = value; + } + + @Initializer + void set2(String value) { + // OK: we assume set2() will be called before the class is actually used + this.initInInitilizerMethod2IsOK = value; + } + + void set3(String value) { + // BAD: though the field is initialized here, set3 is not marked as @Initialized + this.initInAnyOtherMethodIsBAD = value; + } + + @Initializer + void set4(@Nullable String value) { + // BAD: method is marked as @Initializer, but the value can be null + this.initByNullableInInitializedMethodIsBAD = value; + } + + // Example of a typical usecase: + // a build() method that is supposed to be called before the class is used. + Object build() { + // Fail hard if the required fields are not initialzed. + // Unfortunately, this will lead to "condition redundant" warnings, despite the fact + // that checking for this makes total sense. + // TODO(T53531699) don't issue "condition redundant" warning in this case. + Assertions.assertCondition( + initInInitilizerMethod1IsOK != null && initInInitilizerMethod2IsOK != null); + + // ... do some stuff + + // return some meaninful object + return ""; + } +} diff --git a/infer/tests/codetoanalyze/java/nullsafe-default/FieldNotNullable.java b/infer/tests/codetoanalyze/java/nullsafe-default/FieldNotNullable.java index 63203280a..03c77fbd8 100644 --- a/infer/tests/codetoanalyze/java/nullsafe-default/FieldNotNullable.java +++ b/infer/tests/codetoanalyze/java/nullsafe-default/FieldNotNullable.java @@ -10,9 +10,7 @@ package codetoanalyze.java.nullsafe_default; import android.app.Activity; import android.os.Bundle; import android.support.v4.app.Fragment; -import com.facebook.infer.annotation.Assertions; import com.facebook.infer.annotation.Cleanup; -import com.facebook.infer.annotation.Initializer; import javax.annotation.Nullable; /** @@ -99,65 +97,3 @@ class MixedInitializers extends Activity { } } -class TestInitializerBuilder { - String required1; - String required2; - @Nullable String optional; - - // No FIELD_NOT_INITIALIZED error should be reported, because of the @Initializer annotations. - TestInitializerBuilder() {} - - // This is an initializer and must always be called before build(). - @Initializer - TestInitializerBuilder setRequired1(String required1) { - this.required1 = required1; - return this; - } - - // This is an initializer and must always be called before build(). - @Initializer - TestInitializerBuilder setRequired2(String required2) { - this.required2 = required2; - return this; - } - - TestInitializerBuilder setOptional(String optional) { - this.optional = optional; - return this; - } - - TestInitializer build() { - // Fail hard if the required fields are not initialzed - Assertions.assertCondition(required1 != null && required2 != null); - - return new TestInitializer(this); - } - -} - -class TestInitializer { - String required1; // should always be set - String required2; // should always be set - @Nullable String optional; // optionally set - - TestInitializer(TestInitializerBuilder b) { - required1 = b.required1; - required2 = b.required2; - optional = b.optional; - } - - static void testInitializerClientA() { - TestInitializerBuilder b = new TestInitializerBuilder(); - b.setRequired1("hello"); - b.setRequired2("world"); - TestInitializer x = b.build(); - } - - static void testInitializerClientB() { - TestInitializerBuilder b = new TestInitializerBuilder(); - b.setRequired1("a"); - b.setRequired2("b"); - b.setOptional("c"); - TestInitializer x = b.build(); - } -} diff --git a/infer/tests/codetoanalyze/java/nullsafe-default/issues.exp b/infer/tests/codetoanalyze/java/nullsafe-default/issues.exp index 056187e74..e56af927b 100644 --- a/infer/tests/codetoanalyze/java/nullsafe-default/issues.exp +++ b/infer/tests/codetoanalyze/java/nullsafe-default/issues.exp @@ -22,14 +22,17 @@ codetoanalyze/java/nullsafe-default/FieldNotInitialized.java, codetoanalyze.java codetoanalyze/java/nullsafe-default/FieldNotInitialized.java, codetoanalyze.java.nullsafe_default.FieldNotInitialized$Swap.(codetoanalyze.java.nullsafe_default.FieldNotInitialized), 0, ERADICATE_FIELD_NOT_INITIALIZED, no_bucket, WARNING, [Field `FieldNotInitialized$Swap.o1` is not initialized in the constructor and is not declared `@Nullable`] codetoanalyze/java/nullsafe-default/FieldNotInitialized.java, codetoanalyze.java.nullsafe_default.FieldNotInitialized$WriteItself.(codetoanalyze.java.nullsafe_default.FieldNotInitialized), 0, ERADICATE_FIELD_NOT_INITIALIZED, no_bucket, WARNING, [Field `FieldNotInitialized$WriteItself.o` is not initialized in the constructor and is not declared `@Nullable`] codetoanalyze/java/nullsafe-default/FieldNotInitialized.java, codetoanalyze.java.nullsafe_default.FieldNotInitialized.(), 0, ERADICATE_FIELD_NOT_INITIALIZED, no_bucket, WARNING, [Field `FieldNotInitialized.a` is not initialized in the constructor and is not declared `@Nullable`] -codetoanalyze/java/nullsafe-default/FieldNotNullable.java, codetoanalyze.java.nullsafe_default.CanAssignNullInCleanupMethods.assignNullInAnyOtherMethodIsBAD():void, 1, ERADICATE_FIELD_NOT_NULLABLE, no_bucket, WARNING, [Field `CanAssignNullInCleanupMethods.someObject` can be null but is not declared `@Nullable`. (Origin: null constant at line 48)] -codetoanalyze/java/nullsafe-default/FieldNotNullable.java, codetoanalyze.java.nullsafe_default.FieldNotNullable.(), 4, ERADICATE_FIELD_NOT_NULLABLE, no_bucket, WARNING, [Field `FieldNotNullable.initializeNonNullableWithNullIsBAD` can be null but is not declared `@Nullable`. (Origin: null constant at line 56)] +codetoanalyze/java/nullsafe-default/FieldNotInitialized.java, codetoanalyze.java.nullsafe_default.TestInitializerAnnotation.(), 0, ERADICATE_FIELD_NOT_INITIALIZED, no_bucket, WARNING, [Field `TestInitializerAnnotation.dontInitAtAllIsBAD` is not initialized in the constructor and is not declared `@Nullable`] +codetoanalyze/java/nullsafe-default/FieldNotInitialized.java, codetoanalyze.java.nullsafe_default.TestInitializerAnnotation.(), 0, ERADICATE_FIELD_NOT_INITIALIZED, no_bucket, WARNING, [Field `TestInitializerAnnotation.initInAnyOtherMethodIsBAD` is not initialized in the constructor and is not declared `@Nullable`] +codetoanalyze/java/nullsafe-default/FieldNotInitialized.java, codetoanalyze.java.nullsafe_default.TestInitializerAnnotation.build():java.lang.Object, 5, ERADICATE_CONDITION_REDUNDANT, no_bucket, WARNING, [The condition TestInitializerAnnotation.initInInitilizerMethod2IsOK is always true according to the existing annotations.] +codetoanalyze/java/nullsafe-default/FieldNotInitialized.java, codetoanalyze.java.nullsafe_default.TestInitializerAnnotation.build():java.lang.Object, 5, ERADICATE_CONDITION_REDUNDANT, no_bucket, WARNING, [The condition TestInitializerAnnotation.initInInitilizerMethod1IsOK is always true according to the existing annotations.] +codetoanalyze/java/nullsafe-default/FieldNotInitialized.java, codetoanalyze.java.nullsafe_default.TestInitializerAnnotation.set4(java.lang.String):void, 2, ERADICATE_FIELD_NOT_NULLABLE, no_bucket, WARNING, [Field `TestInitializerAnnotation.initByNullableInInitializedMethodIsBAD` can be null but is not declared `@Nullable`. (Origin: method parameter value)] +codetoanalyze/java/nullsafe-default/FieldNotNullable.java, codetoanalyze.java.nullsafe_default.CanAssignNullInCleanupMethods.assignNullInAnyOtherMethodIsBAD():void, 1, ERADICATE_FIELD_NOT_NULLABLE, no_bucket, WARNING, [Field `CanAssignNullInCleanupMethods.someObject` can be null but is not declared `@Nullable`. (Origin: null constant at line 46)] +codetoanalyze/java/nullsafe-default/FieldNotNullable.java, codetoanalyze.java.nullsafe_default.FieldNotNullable.(), 4, ERADICATE_FIELD_NOT_NULLABLE, no_bucket, WARNING, [Field `FieldNotNullable.initializeNonNullableWithNullIsBAD` can be null but is not declared `@Nullable`. (Origin: null constant at line 54)] codetoanalyze/java/nullsafe-default/FieldNotNullable.java, codetoanalyze.java.nullsafe_default.FieldNotNullable.getNullable():java.lang.String, 0, ERADICATE_RETURN_OVER_ANNOTATED, no_bucket, WARNING, [Method `getNullable()` is annotated with `@Nullable` but never returns null.] -codetoanalyze/java/nullsafe-default/FieldNotNullable.java, codetoanalyze.java.nullsafe_default.FieldNotNullable.setNullableToNotNullableIsBAD(java.lang.String):void, 1, ERADICATE_FIELD_NOT_NULLABLE, no_bucket, WARNING, [Field `FieldNotNullable.notNullable` can be null but is not declared `@Nullable`. (Origin: null constant at line 69)] +codetoanalyze/java/nullsafe-default/FieldNotNullable.java, codetoanalyze.java.nullsafe_default.FieldNotNullable.setNullableToNotNullableIsBAD(java.lang.String):void, 1, ERADICATE_FIELD_NOT_NULLABLE, no_bucket, WARNING, [Field `FieldNotNullable.notNullable` can be null but is not declared `@Nullable`. (Origin: null constant at line 67)] codetoanalyze/java/nullsafe-default/FieldNotNullable.java, codetoanalyze.java.nullsafe_default.FieldNotNullable.setNullableToNotNullableIsBAD(java.lang.String):void, 2, ERADICATE_FIELD_NOT_NULLABLE, no_bucket, WARNING, [Field `FieldNotNullable.notNullable` can be null but is not declared `@Nullable`. (Origin: method parameter s)] -codetoanalyze/java/nullsafe-default/FieldNotNullable.java, codetoanalyze.java.nullsafe_default.FieldNotNullable.setNullableToNotNullableIsBAD(java.lang.String):void, 3, ERADICATE_FIELD_NOT_NULLABLE, no_bucket, WARNING, [Field `FieldNotNullable.notNullable` can be null but is not declared `@Nullable`. (Origin: call to getNullable() at line 71)] -codetoanalyze/java/nullsafe-default/FieldNotNullable.java, codetoanalyze.java.nullsafe_default.TestInitializerBuilder.build():codetoanalyze.java.nullsafe_default.TestInitializer, 2, ERADICATE_CONDITION_REDUNDANT, no_bucket, WARNING, [The condition TestInitializerBuilder.required1 is always true according to the existing annotations.] -codetoanalyze/java/nullsafe-default/FieldNotNullable.java, codetoanalyze.java.nullsafe_default.TestInitializerBuilder.build():codetoanalyze.java.nullsafe_default.TestInitializer, 2, ERADICATE_CONDITION_REDUNDANT, no_bucket, WARNING, [The condition TestInitializerBuilder.required2 is always true according to the existing annotations.] +codetoanalyze/java/nullsafe-default/FieldNotNullable.java, codetoanalyze.java.nullsafe_default.FieldNotNullable.setNullableToNotNullableIsBAD(java.lang.String):void, 3, ERADICATE_FIELD_NOT_NULLABLE, no_bucket, WARNING, [Field `FieldNotNullable.notNullable` can be null but is not declared `@Nullable`. (Origin: call to getNullable() at line 69)] codetoanalyze/java/nullsafe-default/InconsistentSubclassAnnotation.java, codetoanalyze.java.nullsafe_default.ExtendsExternalLibrary.externalMethod2(java.lang.Object):void, 0, ERADICATE_INCONSISTENT_SUBCLASS_PARAMETER_ANNOTATION, no_bucket, WARNING, [First parameter `object` of method `ExtendsExternalLibrary.externalMethod2(...)` is not `@Nullable` but is declared `@Nullable`in the parent class method `SomeExternalClass.externalMethod2(...)`.] codetoanalyze/java/nullsafe-default/InconsistentSubclassAnnotation.java, codetoanalyze.java.nullsafe_default.InconsistentSubclassAnnotation.implementInAnotherFile(java.lang.String):java.lang.String, 0, ERADICATE_INCONSISTENT_SUBCLASS_PARAMETER_ANNOTATION, no_bucket, WARNING, [First parameter `s` of method `InconsistentSubclassAnnotation.implementInAnotherFile(...)` is not `@Nullable` but is declared `@Nullable`in the parent class method `InconsistentSubclassAnnotationInterface.implementInAnotherFile(...)`.] codetoanalyze/java/nullsafe-default/InconsistentSubclassAnnotation.java, codetoanalyze.java.nullsafe_default.InconsistentSubclassAnnotation.overloadedMethod():java.lang.Object, 0, ERADICATE_INCONSISTENT_SUBCLASS_RETURN_ANNOTATION, no_bucket, WARNING, [Method `InconsistentSubclassAnnotation.overloadedMethod()` is annotated with `@Nullable` but overrides unannotated method `InconsistentSubclassAnnotationInterface.overloadedMethod()`.]