From b1810ef3ff053b55da0498f3d87ae78d803ea545 Mon Sep 17 00:00:00 2001 From: Mitya Lyubarskiy Date: Tue, 10 Sep 2019 09:52:37 -0700 Subject: [PATCH] [nullsafe] @Nonnull should not suppress Field Not Initialized warning Summary: There are currently plenty of ways to suppress the warning, including Inject, Initializer, and SuppressFieldNotInitialized annotations. This one (annotating field with Nonnull) is counter-intuitive and does not align with gradual nullsafe semantics we are working on. Reviewed By: artempyanykh Differential Revision: D17281702 fbshipit-source-id: 132e1b687 --- infer/src/nullsafe/eradicateChecks.ml | 6 +----- .../java/nullsafe-default/FieldNotInitialized.java | 11 +++++------ .../codetoanalyze/java/nullsafe-default/issues.exp | 9 ++++++--- .../java/nullsafe-gradual/FieldNotInitialized.java | 6 ------ 4 files changed, 12 insertions(+), 20 deletions(-) diff --git a/infer/src/nullsafe/eradicateChecks.ml b/infer/src/nullsafe/eradicateChecks.ml index 6eb83c03c..133c157c6 100644 --- a/infer/src/nullsafe/eradicateChecks.ml +++ b/infer/src/nullsafe/eradicateChecks.ml @@ -211,7 +211,6 @@ let check_constructor_initialization tenv find_canonical_duplicate curr_pname cu match get_field_annotation tenv fn ts with None -> false | Some (_, ia) -> f ia in let nullable_annotated = annotated_with Annotations.ia_is_nullable in - let nonnull_annotated = annotated_with Annotations.ia_is_nonnull in let injector_readonly_annotated = annotated_with Annotations.ia_is_field_injector_readonly in @@ -254,10 +253,7 @@ let check_constructor_initialization tenv find_canonical_duplicate curr_pname cu in if should_check_field_initialization then ( (* Check if field is missing annotation. *) - if - (not (nullable_annotated || nonnull_annotated)) - && not may_be_assigned_in_final_typestate - then + if (not nullable_annotated) && not may_be_assigned_in_final_typestate then report_error tenv find_canonical_duplicate (TypeErr.Field_not_initialized (fn, curr_pname)) None loc curr_pdesc ; diff --git a/infer/tests/codetoanalyze/java/nullsafe-default/FieldNotInitialized.java b/infer/tests/codetoanalyze/java/nullsafe-default/FieldNotInitialized.java index bdd2bcedb..640022cfb 100644 --- a/infer/tests/codetoanalyze/java/nullsafe-default/FieldNotInitialized.java +++ b/infer/tests/codetoanalyze/java/nullsafe-default/FieldNotInitialized.java @@ -24,13 +24,11 @@ public class FieldNotInitialized { String notNullIsBAD; // BAD: need to initialize it - @Nullable String nullableIsOK; // OK: will be init with null + @Nonnull String nonnullIsBAD; // BAD: explicit annotation does not make it better - @Nonnull - String nonnullIsOK; // Means: assume it will be initialized to a nonnull value somewhere else. + @NonNull String nonNullIsBAD; // BAD: explicit annotation does not make it better - @NonNull - String nonNullIsOK; // Means: assume it will be initialized to a nonnull value somewhere else. + @Nullable String nullableIsOK; // OK: will be init with null @Inject String injectIsOK; // Means: assume it will be initialized via dependency injection @@ -44,7 +42,8 @@ public class FieldNotInitialized { void testNullifyFields() { bindIsOK = null; // OK: the framework could write null into the field suppressViewNullabilityIsOK = null; // OK: the framework could write null into the field - nonnullIsOK = null; // BAD: can not nullify this + nonnullIsBAD = null; // BAD: explicit annotation does not allow nullifying + nonNullIsBAD = null; // BAD: explicit annotation does not allow nullifying injectIsOK = null; // BAD: can not nullify this } diff --git a/infer/tests/codetoanalyze/java/nullsafe-default/issues.exp b/infer/tests/codetoanalyze/java/nullsafe-default/issues.exp index b7acd5753..2d6939229 100644 --- a/infer/tests/codetoanalyze/java/nullsafe-default/issues.exp +++ b/infer/tests/codetoanalyze/java/nullsafe-default/issues.exp @@ -17,7 +17,7 @@ codetoanalyze/java/nullsafe-default/CustomAnnotations.java, codetoanalyze.java.n 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`] -codetoanalyze/java/nullsafe-default/FieldNotInitialized.java, codetoanalyze.java.nullsafe_default.FieldNotInitialized$InitWithOtherClass.(codetoanalyze.java.nullsafe_default.FieldNotInitialized,codetoanalyze.java.nullsafe_default.FieldNotInitialized$InitWithOtherClass$OtherClass), 1, ERADICATE_FIELD_NOT_NULLABLE, no_bucket, WARNING, [Field `FieldNotInitialized$InitWithOtherClass.bad` can be null but is not declared `@Nullable`. (Origin: field FieldNotInitialized$InitWithOtherClass$OtherClass.nullable at line 166)] +codetoanalyze/java/nullsafe-default/FieldNotInitialized.java, codetoanalyze.java.nullsafe_default.FieldNotInitialized$InitWithOtherClass.(codetoanalyze.java.nullsafe_default.FieldNotInitialized,codetoanalyze.java.nullsafe_default.FieldNotInitialized$InitWithOtherClass$OtherClass), 1, ERADICATE_FIELD_NOT_NULLABLE, no_bucket, WARNING, [Field `FieldNotInitialized$InitWithOtherClass.bad` can be null but is not declared `@Nullable`. (Origin: field FieldNotInitialized$InitWithOtherClass$OtherClass.nullable at line 165)] codetoanalyze/java/nullsafe-default/FieldNotInitialized.java, codetoanalyze.java.nullsafe_default.FieldNotInitialized$InitWithTheSameClass.(codetoanalyze.java.nullsafe_default.FieldNotInitialized,codetoanalyze.java.nullsafe_default.FieldNotInitialized$InitWithTheSameClass), 0, ERADICATE_FIELD_NOT_INITIALIZED, no_bucket, WARNING, [Field `FieldNotInitialized$InitWithTheSameClass.bad` is not initialized in the constructor and is not declared `@Nullable`] codetoanalyze/java/nullsafe-default/FieldNotInitialized.java, codetoanalyze.java.nullsafe_default.FieldNotInitialized$InitializationOrder.(codetoanalyze.java.nullsafe_default.FieldNotInitialized,int), 0, ERADICATE_FIELD_NOT_INITIALIZED, no_bucket, WARNING, [Field `FieldNotInitialized$InitializationOrder.o1` is not initialized in the constructor and is not declared `@Nullable`] codetoanalyze/java/nullsafe-default/FieldNotInitialized.java, codetoanalyze.java.nullsafe_default.FieldNotInitialized$OnlyRead.(codetoanalyze.java.nullsafe_default.FieldNotInitialized), 0, ERADICATE_FIELD_NOT_INITIALIZED, no_bucket, WARNING, [Field `FieldNotInitialized$OnlyRead.o` is not initialized in the constructor and is not declared `@Nullable`] @@ -27,8 +27,11 @@ codetoanalyze/java/nullsafe-default/FieldNotInitialized.java, codetoanalyze.java codetoanalyze/java/nullsafe-default/FieldNotInitialized.java, codetoanalyze.java.nullsafe_default.FieldNotInitialized$ShouldInitializeInAllBranches.(codetoanalyze.java.nullsafe_default.FieldNotInitialized,int), 0, ERADICATE_FIELD_NOT_INITIALIZED, no_bucket, WARNING, [Field `FieldNotInitialized$ShouldInitializeInAllBranches.f2` is not initialized in the constructor and is not declared `@Nullable`] codetoanalyze/java/nullsafe-default/FieldNotInitialized.java, codetoanalyze.java.nullsafe_default.FieldNotInitialized$WriteItselfIsBAD.(codetoanalyze.java.nullsafe_default.FieldNotInitialized), 0, ERADICATE_FIELD_NOT_INITIALIZED, no_bucket, WARNING, [Field `FieldNotInitialized$WriteItselfIsBAD.bad` 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.notNullIsBAD` is not initialized in the constructor and is not declared `@Nullable`] -codetoanalyze/java/nullsafe-default/FieldNotInitialized.java, codetoanalyze.java.nullsafe_default.FieldNotInitialized.testNullifyFields():void, 3, ERADICATE_FIELD_NOT_NULLABLE, no_bucket, WARNING, [Field `FieldNotInitialized.nonnullIsOK` can be null but is not declared `@Nullable`. (Origin: null constant at line 47)] -codetoanalyze/java/nullsafe-default/FieldNotInitialized.java, codetoanalyze.java.nullsafe_default.FieldNotInitialized.testNullifyFields():void, 4, ERADICATE_FIELD_NOT_NULLABLE, no_bucket, WARNING, [Field `FieldNotInitialized.injectIsOK` can be null but is not declared `@Nullable`. (Origin: null constant at line 48)] +codetoanalyze/java/nullsafe-default/FieldNotInitialized.java, codetoanalyze.java.nullsafe_default.FieldNotInitialized.(), 0, ERADICATE_FIELD_NOT_INITIALIZED, no_bucket, WARNING, [Field `FieldNotInitialized.nonNullIsBAD` 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.nonnullIsBAD` is not initialized in the constructor and is not declared `@Nullable`] +codetoanalyze/java/nullsafe-default/FieldNotInitialized.java, codetoanalyze.java.nullsafe_default.FieldNotInitialized.testNullifyFields():void, 3, ERADICATE_FIELD_NOT_NULLABLE, no_bucket, WARNING, [Field `FieldNotInitialized.nonnullIsBAD` can be null but is not declared `@Nullable`. (Origin: null constant at line 45)] +codetoanalyze/java/nullsafe-default/FieldNotInitialized.java, codetoanalyze.java.nullsafe_default.FieldNotInitialized.testNullifyFields():void, 4, ERADICATE_FIELD_NOT_NULLABLE, no_bucket, WARNING, [Field `FieldNotInitialized.nonNullIsBAD` can be null but is not declared `@Nullable`. (Origin: null constant at line 46)] +codetoanalyze/java/nullsafe-default/FieldNotInitialized.java, codetoanalyze.java.nullsafe_default.FieldNotInitialized.testNullifyFields():void, 5, ERADICATE_FIELD_NOT_NULLABLE, no_bucket, WARNING, [Field `FieldNotInitialized.injectIsOK` can be null but is not declared `@Nullable`. (Origin: null constant at line 47)] 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.] diff --git a/infer/tests/codetoanalyze/java/nullsafe-gradual/FieldNotInitialized.java b/infer/tests/codetoanalyze/java/nullsafe-gradual/FieldNotInitialized.java index a10cccb49..63a2e8782 100644 --- a/infer/tests/codetoanalyze/java/nullsafe-gradual/FieldNotInitialized.java +++ b/infer/tests/codetoanalyze/java/nullsafe-gradual/FieldNotInitialized.java @@ -7,10 +7,8 @@ package codetoanalyze.java.nullsafe_gradual; -import android.support.annotation.NonNull; import android.widget.EditText; import com.facebook.infer.annotation.SuppressViewNullability; -import javax.annotation.Nonnull; import javax.annotation.Nullable; import javax.inject.Inject; @@ -23,12 +21,8 @@ public class FieldNotInitialized { @Nullable String b; - @Nonnull String c; // Means: assume it will be initialized to a nonnull value somewhere else. - @Inject String d; // Means: assume it will be initialized via dependency injection - @NonNull String e; - @Bind EditText f; // Means: assume it will be initialized, and ignore null assignment @SuppressViewNullability EditText g;