pghs975uc
/
infer_clone
1
0
Fork 0
Code Issues Pull Requests Releases Wiki Activity

[nullsafe] Refine semantics of @Nonnull: obey minimum surprise principle

Browse Source 
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
master
Mitya Lyubarskiy 5 years ago committed by Facebook Github Bot
parent 32983e129b
commit 5b0bdfb297
3 changed files with 89 additions and 18 deletions
Show Stats Download Patch File Download Diff File

4
infer/src/nullsafe/eradicateChecks.ml
Unescape View File

@ -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

84
infer/tests/codetoanalyze/java/eradicate/ReturnNotNullable.java
Unescape View File

@ -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();
}
}

19
infer/tests/codetoanalyze/java/eradicate/issues.exp
Unescape View File

@ -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)]

Write Preview
Loading…
Cancel
Save