From 5b0bdfb297b537a6dd0cbb32a08c31487dfae386 Mon Sep 17 00:00:00 2001 From: Mitya Lyubarskiy Date: Thu, 29 Aug 2019 07:19:13 -0700 Subject: [PATCH] [nullsafe] Refine semantics of @Nonnull: obey minimum surprise principle Summary: `nullsafe` currently allows the following: ``` public void Nonnull Object willBeOK() { return null; } ``` But disallows the following: ``` public void Object willBeAnIssue() { return null; } ``` This was a deliberate choice made back in 2014. The motivation was to provide a way to tell the checker "I know it can not be null, trust me". A huge problem with that approach is that it is extremely non-intuitive and surprising, and contradicts with pretty much everything when Nonnull or similar annotations are used in external world. This is not the way how checkers should be supressed. We do provide 2 options to express this intention, namely `assertNotNull` and `assumeNotNull` would do the thing. This is a much better approach for additional reason: assertNotNull is granular and applies only to the exact expression that is under question. In contrast, suppressing the check on the whole function level make any modifications of a function dangerous. Reviewed By: artempyanykh Differential Revision: D16984213 fbshipit-source-id: 0ba0f623b --- infer/src/nullsafe/eradicateChecks.ml | 4 +- .../java/eradicate/ReturnNotNullable.java | 84 +++++++++++++++++-- .../codetoanalyze/java/eradicate/issues.exp | 19 +++-- 3 files changed, 89 insertions(+), 18 deletions(-) diff --git a/infer/src/nullsafe/eradicateChecks.ml b/infer/src/nullsafe/eradicateChecks.ml index a56d502cf..6fa2e434d 100644 --- a/infer/src/nullsafe/eradicateChecks.ml +++ b/infer/src/nullsafe/eradicateChecks.ml @@ -301,7 +301,6 @@ let check_return_annotation tenv find_canonical_duplicate curr_pdesc ret_range annotated_signature.params in let ret_annotated_present = Annotations.ia_is_present ret_ia in - let ret_annotated_nonnull = Annotations.ia_is_nonnull ret_ia in match ret_range with (* Disables the warnings since it is not clear how to annotate the return value of lambdas *) | Some _ @@ -316,8 +315,7 @@ let check_return_annotation tenv find_canonical_duplicate curr_pdesc ret_range let final_present = TypeAnnotation.get_value AnnotatedSignature.Present final_ta in let origin_descr = TypeAnnotation.descr_origin final_ta in let return_not_nullable = - final_nullable && (not ret_annotated_nullable) && (not ret_implicitly_nullable) - && not ret_annotated_nonnull + final_nullable && (not ret_annotated_nullable) && not ret_implicitly_nullable in let return_value_not_present = Config.eradicate_optional_present && (not final_present) && ret_annotated_present diff --git a/infer/tests/codetoanalyze/java/eradicate/ReturnNotNullable.java b/infer/tests/codetoanalyze/java/eradicate/ReturnNotNullable.java index d7ffc52fa..295195b37 100644 --- a/infer/tests/codetoanalyze/java/eradicate/ReturnNotNullable.java +++ b/infer/tests/codetoanalyze/java/eradicate/ReturnNotNullable.java @@ -27,29 +27,97 @@ public class ReturnNotNullable { return null; } - String returnNull() { + // ------------------------------------------------------------ + // Converting different things to not annotated types. + // By default, (not annotated type is treated as non nullable). + + String nullToNotAnnotatedIsBad() { return null; } - String returnNullable(@Nullable String s) { + String nullableToNotAnnotatedIsBad(@Nullable String s) { + return s; + } + + String notAnnotatedToNotAnnotatedIsOK(String s) { return s; } + String nonNullToNotAnnotatedIsOK(@Nonnull String s) { + return s; + } + + String constantToNotAnnotatedIsOK() { + return "abc"; + } + + // ------------------------------------------------------------ + // Converting different things to @Nonnull. + // (Should be the same as converting to not annotated). + @Nonnull - String returnNonnull() { + String nullToNonnullIsBad() { + return null; + } + + @Nonnull + String nullableToNonnullIsBad(@Nullable String s) { + return s; + } + + @Nonnull + String notAnnotatedToNonnullIsOK(String s) { + return s; + } + + @Nonnull + String nonNullToNonnullIsOK(@Nonnull String s) { + return s; + } + + @Nonnull + String constantToNonNullIsOK() { return "abc"; } + // ------------------------------------------------------------ + // Converting different things to @Nullable. + // This is either + // 1. OK when inferred and annotated return types are both nullable, or + // 2. Leads to ERADICATE_RETURN_OVER_ANNOTATED when inferred return type + // is not nullable, but function is still annotated with @Nullable. + // This often happens when the API author decides to return @Nullable + // (anticipating future change) even though the current implementation returns non-null. + // Because of this the warning is currently turned off by default and is recommended + // to use only in specific scenarious, like code migrations. + @Nullable - String returnNullOK() { + String nullToNullableIsOK() { return null; } @Nullable - String returnNullableOK(@Nullable String s) { + String nullableToNullableIsOK(@Nullable String s) { + return s; + } + + @Nullable + String notAnnotatedNullableIsOverannotated(String s) { + return s; + } + + @Nullable + String nonNullToNullableIsOverannotated(@Nonnull String s) { return s; } + @Nullable + String constantToNullableIsOverannotated() { + return "abc"; + } + + // ------------------------------------------------------- + String throwException(@Nullable Exception e, boolean bad) throws Exception { if (bad) { throw (e); // no ERADICATE_RETURN_NOT_NULLABLE should be reported @@ -59,14 +127,14 @@ public class ReturnNotNullable { @Nullable String redundantEq() { - String s = returnNonnull(); + String s = constantToNonNullIsOK(); int n = s == null ? 0 : s.length(); return s; } @Nullable String redundantNeq() { - String s = returnNonnull(); + String s = constantToNonNullIsOK(); int n = s != null ? 0 : s.length(); return s; } @@ -85,7 +153,7 @@ public class ReturnNotNullable { Object tryWithResourcesReturnNullable(String path) throws IOException { try (BufferedReader br = nn(new BufferedReader(new FileReader(path)))) { - return returnNullOK(); + return nullToNullableIsOK(); } } diff --git a/infer/tests/codetoanalyze/java/eradicate/issues.exp b/infer/tests/codetoanalyze/java/eradicate/issues.exp index 3c5f816c7..87d56d9cb 100644 --- a/infer/tests/codetoanalyze/java/eradicate/issues.exp +++ b/infer/tests/codetoanalyze/java/eradicate/issues.exp @@ -113,14 +113,19 @@ codetoanalyze/java/eradicate/PresentTest.java, codetoanalyze.java.eradicate.Pres codetoanalyze/java/eradicate/PresentTest.java, codetoanalyze.java.eradicate.PresentTest$TestPresentAnnotationBasic.testOptionalAbsent():void, 1, ERADICATE_PARAMETER_VALUE_ABSENT, no_bucket, WARNING, [`PresentTest$TestPresentAnnotationBasic.expectPresent(...)` needs a present value in parameter 1 but argument `absent()` can be absent. (Origin: call to absent() at line 61)] codetoanalyze/java/eradicate/PresentTest.java, codetoanalyze.java.eradicate.PresentTest.testPresent(com.google.common.base.Optional,com.google.common.base.Optional):void, 4, ERADICATE_PARAMETER_VALUE_ABSENT, no_bucket, WARNING, [`PresentTest.argPresent(...)` needs a present value in parameter 1 but argument `absent` can be absent. (Origin: method parameter absent)] codetoanalyze/java/eradicate/Redundant.java, Redundant.bad():void, 2, ERADICATE_CONDITION_REDUNDANT, no_bucket, WARNING, [The condition s is always true according to the existing annotations.] -codetoanalyze/java/eradicate/ReturnNotNullable.java, codetoanalyze.java.eradicate.ReturnNotNullable$ConditionalAssignment.test(boolean):java.lang.Object, 0, ERADICATE_RETURN_NOT_NULLABLE, no_bucket, WARNING, [Method `test(...)` may return null but it is not annotated with `@Nullable`. (Origin: field ReturnNotNullable$ConditionalAssignment.f1 at line 143)] -codetoanalyze/java/eradicate/ReturnNotNullable.java, codetoanalyze.java.eradicate.ReturnNotNullable.getResourceNullable(java.lang.Class,java.lang.String):java.net.URL, 0, ERADICATE_RETURN_NOT_NULLABLE, no_bucket, WARNING, [Method `getResourceNullable(...)` may return null but it is not annotated with `@Nullable`. (Origin: call to getResource(...) modelled in modelTables.ml at line 123)] +codetoanalyze/java/eradicate/ReturnNotNullable.java, codetoanalyze.java.eradicate.ReturnNotNullable$ConditionalAssignment.test(boolean):java.lang.Object, 0, ERADICATE_RETURN_NOT_NULLABLE, no_bucket, WARNING, [Method `test(...)` may return null but it is not annotated with `@Nullable`. (Origin: field ReturnNotNullable$ConditionalAssignment.f1 at line 211)] +codetoanalyze/java/eradicate/ReturnNotNullable.java, codetoanalyze.java.eradicate.ReturnNotNullable.constantToNullableIsOverannotated():java.lang.String, 0, ERADICATE_RETURN_OVER_ANNOTATED, no_bucket, WARNING, [Method `constantToNullableIsOverannotated()` is annotated with `@Nullable` but never returns null.] +codetoanalyze/java/eradicate/ReturnNotNullable.java, codetoanalyze.java.eradicate.ReturnNotNullable.getResourceNullable(java.lang.Class,java.lang.String):java.net.URL, 0, ERADICATE_RETURN_NOT_NULLABLE, no_bucket, WARNING, [Method `getResourceNullable(...)` may return null but it is not annotated with `@Nullable`. (Origin: call to getResource(...) modelled in modelTables.ml at line 191)] +codetoanalyze/java/eradicate/ReturnNotNullable.java, codetoanalyze.java.eradicate.ReturnNotNullable.nonNullToNullableIsOverannotated(java.lang.String):java.lang.String, 0, ERADICATE_RETURN_OVER_ANNOTATED, no_bucket, WARNING, [Method `nonNullToNullableIsOverannotated(...)` is annotated with `@Nullable` but never returns null.] +codetoanalyze/java/eradicate/ReturnNotNullable.java, codetoanalyze.java.eradicate.ReturnNotNullable.notAnnotatedNullableIsOverannotated(java.lang.String):java.lang.String, 0, ERADICATE_RETURN_OVER_ANNOTATED, no_bucket, WARNING, [Method `notAnnotatedNullableIsOverannotated(...)` is annotated with `@Nullable` but never returns null.] +codetoanalyze/java/eradicate/ReturnNotNullable.java, codetoanalyze.java.eradicate.ReturnNotNullable.nullToNonnullIsBad():java.lang.String, 0, ERADICATE_RETURN_NOT_NULLABLE, no_bucket, WARNING, [Method `nullToNonnullIsBad()` may return null but it is not annotated with `@Nullable`. (Origin: null constant at line 60)] +codetoanalyze/java/eradicate/ReturnNotNullable.java, codetoanalyze.java.eradicate.ReturnNotNullable.nullToNotAnnotatedIsBad():java.lang.String, 0, ERADICATE_RETURN_NOT_NULLABLE, no_bucket, WARNING, [Method `nullToNotAnnotatedIsBad()` may return null but it is not annotated with `@Nullable`. (Origin: null constant at line 35)] +codetoanalyze/java/eradicate/ReturnNotNullable.java, codetoanalyze.java.eradicate.ReturnNotNullable.nullableToNonnullIsBad(java.lang.String):java.lang.String, 0, ERADICATE_RETURN_NOT_NULLABLE, no_bucket, WARNING, [Method `nullableToNonnullIsBad(...)` may return null but it is not annotated with `@Nullable`. (Origin: method parameter s)] +codetoanalyze/java/eradicate/ReturnNotNullable.java, codetoanalyze.java.eradicate.ReturnNotNullable.nullableToNotAnnotatedIsBad(java.lang.String):java.lang.String, 0, ERADICATE_RETURN_NOT_NULLABLE, no_bucket, WARNING, [Method `nullableToNotAnnotatedIsBad(...)` may return null but it is not annotated with `@Nullable`. (Origin: method parameter s)] codetoanalyze/java/eradicate/ReturnNotNullable.java, codetoanalyze.java.eradicate.ReturnNotNullable.redundantEq():java.lang.String, 0, ERADICATE_RETURN_OVER_ANNOTATED, no_bucket, WARNING, [Method `redundantEq()` is annotated with `@Nullable` but never returns null.] codetoanalyze/java/eradicate/ReturnNotNullable.java, codetoanalyze.java.eradicate.ReturnNotNullable.redundantEq():java.lang.String, 2, ERADICATE_CONDITION_REDUNDANT_NONNULL, no_bucket, WARNING, [The condition s is always false according to the existing annotations.] codetoanalyze/java/eradicate/ReturnNotNullable.java, codetoanalyze.java.eradicate.ReturnNotNullable.redundantNeq():java.lang.String, 0, ERADICATE_RETURN_OVER_ANNOTATED, no_bucket, WARNING, [Method `redundantNeq()` is annotated with `@Nullable` but never returns null.] codetoanalyze/java/eradicate/ReturnNotNullable.java, codetoanalyze.java.eradicate.ReturnNotNullable.redundantNeq():java.lang.String, 2, ERADICATE_CONDITION_REDUNDANT_NONNULL, no_bucket, WARNING, [The condition s is always true according to the existing annotations.] -codetoanalyze/java/eradicate/ReturnNotNullable.java, codetoanalyze.java.eradicate.ReturnNotNullable.returnNull():java.lang.String, 0, ERADICATE_RETURN_NOT_NULLABLE, no_bucket, WARNING, [Method `returnNull()` may return null but it is not annotated with `@Nullable`. (Origin: null constant at line 31)] -codetoanalyze/java/eradicate/ReturnNotNullable.java, codetoanalyze.java.eradicate.ReturnNotNullable.returnNullable(java.lang.String):java.lang.String, 0, ERADICATE_RETURN_NOT_NULLABLE, no_bucket, WARNING, [Method `returnNullable(...)` may return null but it is not annotated with `@Nullable`. (Origin: method parameter s)] -codetoanalyze/java/eradicate/ReturnNotNullable.java, codetoanalyze.java.eradicate.ReturnNotNullable.return_null_in_catch():java.lang.String, 0, ERADICATE_RETURN_NOT_NULLABLE, no_bucket, WARNING, [Method `return_null_in_catch()` may return null but it is not annotated with `@Nullable`. (Origin: null constant at line 106)] -codetoanalyze/java/eradicate/ReturnNotNullable.java, codetoanalyze.java.eradicate.ReturnNotNullable.return_null_in_catch_after_throw():java.lang.String, 0, ERADICATE_RETURN_NOT_NULLABLE, no_bucket, WARNING, [Method `return_null_in_catch_after_throw()` may return null but it is not annotated with `@Nullable`. (Origin: null constant at line 118)] -codetoanalyze/java/eradicate/ReturnNotNullable.java, codetoanalyze.java.eradicate.ReturnNotNullable.tryWithResourcesReturnNullable(java.lang.String):java.lang.Object, 0, ERADICATE_RETURN_NOT_NULLABLE, no_bucket, WARNING, [Method `tryWithResourcesReturnNullable(...)` may return null but it is not annotated with `@Nullable`. (Origin: call to returnNullOK() at line 88)] +codetoanalyze/java/eradicate/ReturnNotNullable.java, codetoanalyze.java.eradicate.ReturnNotNullable.return_null_in_catch():java.lang.String, 0, ERADICATE_RETURN_NOT_NULLABLE, no_bucket, WARNING, [Method `return_null_in_catch()` may return null but it is not annotated with `@Nullable`. (Origin: null constant at line 174)] +codetoanalyze/java/eradicate/ReturnNotNullable.java, codetoanalyze.java.eradicate.ReturnNotNullable.return_null_in_catch_after_throw():java.lang.String, 0, ERADICATE_RETURN_NOT_NULLABLE, no_bucket, WARNING, [Method `return_null_in_catch_after_throw()` may return null but it is not annotated with `@Nullable`. (Origin: null constant at line 186)] +codetoanalyze/java/eradicate/ReturnNotNullable.java, codetoanalyze.java.eradicate.ReturnNotNullable.tryWithResourcesReturnNullable(java.lang.String):java.lang.Object, 0, ERADICATE_RETURN_NOT_NULLABLE, no_bucket, WARNING, [Method `tryWithResourcesReturnNullable(...)` may return null but it is not annotated with `@Nullable`. (Origin: call to nullToNullableIsOK() at line 156)]