diff --git a/infer/tests/codetoanalyze/java/nullsafe-default/ConditionRedundant.java b/infer/tests/codetoanalyze/java/nullsafe-default/ConditionRedundant.java new file mode 100644 index 000000000..fce03cdd1 --- /dev/null +++ b/infer/tests/codetoanalyze/java/nullsafe-default/ConditionRedundant.java @@ -0,0 +1,248 @@ +/* + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ +package codetoanalyze.java.nullsafe_default; + +import com.facebook.infer.annotation.Assertions; +import com.google.common.base.Preconditions; +import javax.annotation.Nullable; + +public class ConditionRedundant { + + String fieldNonnull = ""; + @Nullable String fieldNullable = ""; + + @Nullable + String getNullable() { + return null; + } + + String getNonnull() { + return ""; + } + + void compareNEQ_NonnullIsBAD(String s) { + if (s != null) { // BAD: condition redundant + // Do something + } + } + + void compareNEQ_NullableIsOK(@Nullable String s) { + if (s != null) { // OK: comparing with nullable + // Do something + } + } + + void compareEQ_NonnullIsBAD(String s) { + if (s == null) { // BAD: condition redundant + // Do something + } + } + + void compareEQ_NullableIsOK(@Nullable String s) { + if (s == null) { // OK: comparing with nullable + // Do something + } + } + + // `if` is not essential, we test all comparisons expressions + + void outsideOfIfCompareNonnullIsBAD(String s) { + boolean b = s != null; // BAD: condition redundant + } + + void outsideOfIfCompareNullableIsOK(@Nullable String s) { + boolean b = s != null; // OK: comparing with nullable + } + + // comparing with nonnull is redundant even if it is a part of expression + + void conjunctionBothNonnullIsBAD(String s1, String s2) { + if (s1 != null && s2 != null) { // BAD: both clauses are redudant + // Do something + } + } + + void conjunctionOneNonnullIsBAD(@Nullable String s1, String s2) { + if (s1 != null && s2 != null) { // BAD: one clause is redundant + // Do something + } + } + + void conjunctionBothNullableIsOK(@Nullable String s1, @Nullable String s2) { + if (s1 != null && s2 != null) { // OK: both clauses make sense + // Do something + } + } + + void disjunctionBothNonnullIsBAD(String s1, String s2) { + if (s1 != null || s2 != null) { // BAD: both clauses are redudant + // Do something + } + } + + void disjunctionOneNonnullIsBAD(@Nullable String s1, String s2) { + if (s1 != null || s2 != null) { // BAD: one clause is redundant + // Do something + } + } + + void disjunctionBothNullableIsOK(@Nullable String s1, @Nullable String s2) { + if (s1 != null || s2 != null) { // OK: both clauses make sense + // Do something + } + } + + // Adding some irrelevant conditions does not make the issue go away + + void irrelevantConditionWithNonnullIsBAD(String s1, @Nullable String s2, int someInt) { + if (someInt == 1 || s1 == null || s2 != null) { // BAD: check for s1 is redundant + // Do something + } + } + + void irrelevantConditionAllNullablesIsOK(@Nullable String s1, @Nullable String s2, int someInt) { + if (someInt == 1 || s1 == null || s2 != null) { // OK: all clauses maeke sense + // Do something + } + } + + // Comparing expressions (not local variables) with null + + void ternary_NonnullInBothBranchesIsBAD(String s1, String s2, int someInt) { + // BAD: comparing nonnull with null is redundant + if ((someInt == 1 ? s1 : s2) == null) { + // Do something + } + } + + void ternary_NullableInBothBranchesIsOK(@Nullable String s1, @Nullable String s2, int someInt) { + // OK: the result is nullable + if ((someInt == 1 ? s1 : s2) == null) { + // Do something + } + } + + void ternary_NonnullInOneBranch_FirstBranch_IsOK(String s1, @Nullable String s2, int someInt) { + // OK: the result is nullable + if ((someInt == 1 ? s1 : s2) == null) { + // Do something + } + } + + // But if we just swap the order, we have a FP. + // (CFG extracts this expression to a form when one of flows contain only nonnull, hence the + // report). + // TODO(T54065455) Don't report in this case + void FP_ternary_NonnullInOneBranch_SecondBranch_ShouldBeOK( + @Nullable String s1, String s2, int someInt) { + if ((someInt == 1 ? s1 : s2) == null) { // FP: expression can be null + // Do something + } + } + + void testFlowSensitivity(@Nullable String nullable1, @Nullable String nullable2) { + if (nullable1 != null) { // OK: comparing nullable with null + if (nullable1 != null) { // BAD: now nullable1 is nonnull + if (nullable2 != null) { // OK: nullable2 can still be null + if (nullable1 != null) { // BAD: nullable1 is still nonnull + // Do something + } + } + } + } + } + + // Test comparison with functions + + void comparingNonnullFunctionIsBAD() { + if (getNonnull() != null) { // BAD: comparing with nonnull + // do something + } + } + + void comparingNullableFunctionIsOK() { + if (getNullable() != null) { // OK: comparing with nullable + // do something + } + } + + // Test comparison with fields + + void comparingNonnullFieldIsBAD() { + if (fieldNonnull != null) { // BAD: condition redundant + // Do something + } + } + + void comparingNullableFieldIsOK() { + if (fieldNullable != null) { // OK: comparing with nullable + // Do something + } + } + + void comparingNullableFieldThatIsAlreadyCheckedIsBAD() { + if (fieldNullable != null) { // OK: comparing with nullable + if (fieldNullable != null) { + // BAD: at this point we already know field is not nullable + } + } + } + + // Test assertions that are modelled in Nullsafe + + void checkNotNull_NonnullIsBAD(String s) { + Preconditions.checkNotNull(s, "BAD: we already know it is not nullable"); + } + + void checkNotNull_NullableIsOK(@Nullable String s) { + Preconditions.checkNotNull(s, "totally legit check"); + } + + void checkArgument_NonnullIsBAd(String s) { + Preconditions.checkArgument(s != null, "BAD: we know it is not null"); + } + + void checkArgument_NullableIsOK(@Nullable String s) { + Preconditions.checkArgument(s != null, "totally legit check"); + } + + void assertNotNull_NonnullIsBAD(String s) { + Assertions.assertNotNull(s, "BAD: we know it is not null"); + } + + void assertNotNull_NullableIsOK(@Nullable String s) { + Assertions.assertNotNull(s, "totally legit check"); + } + + // Test nullability inference in try-catch + + static void maythrow() throws java.io.IOException {} + + void comparingWithNullIfAssignedBeforeThrowableIsBAD() throws java.io.IOException { + String s = null; + try { + s = "123"; + maythrow(); + } finally { + if (s != null) { // BAD: this is redundant + // Do something + } + } + } + + void comparingWithNullIfAssignedAfterThrowableIsOK() throws java.io.IOException { + String s = null; + try { + maythrow(); + s = "123"; + } finally { + if (s != null) { // OK: if `maythrow` throws, it will indeed be null + // Do something + } + } + } +} diff --git a/infer/tests/codetoanalyze/java/nullsafe-default/Redundant.java b/infer/tests/codetoanalyze/java/nullsafe-default/Redundant.java deleted file mode 100644 index 666eb5a3d..000000000 --- a/infer/tests/codetoanalyze/java/nullsafe-default/Redundant.java +++ /dev/null @@ -1,30 +0,0 @@ -/* - * Copyright (c) Facebook, Inc. and its affiliates. - * - * This source code is licensed under the MIT license found in the - * LICENSE file in the root directory of this source tree. - */ -public class Redundant { - - void bad() { - String s = "123"; - if (s != null) { - int n = s.length(); - } - } - - static void maythrow() throws java.io.IOException {} - - void good() throws java.io.IOException { - String s = null; - - try { - maythrow(); - s = "123"; - } finally { - if (s != null) { // this is not redundant - int n = s.length(); - } - } - } -} diff --git a/infer/tests/codetoanalyze/java/nullsafe-default/issues.exp b/infer/tests/codetoanalyze/java/nullsafe-default/issues.exp index 216ce4400..a6639f50c 100644 --- a/infer/tests/codetoanalyze/java/nullsafe-default/issues.exp +++ b/infer/tests/codetoanalyze/java/nullsafe-default/issues.exp @@ -4,6 +4,27 @@ 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/ConditionRedundant.java, codetoanalyze.java.nullsafe_default.ConditionRedundant.FP_ternary_NonnullInOneBranch_SecondBranch_ShouldBeOK(java.lang.String,java.lang.String,int):void, 2, ERADICATE_CONDITION_REDUNDANT, no_bucket, WARNING, [The condition s2 is always false according to the existing annotations.] +codetoanalyze/java/nullsafe-default/ConditionRedundant.java, codetoanalyze.java.nullsafe_default.ConditionRedundant.assertNotNull_NonnullIsBAD(java.lang.String):void, 1, ERADICATE_CONDITION_REDUNDANT, no_bucket, WARNING, [The condition (s!=null) is always true according to the existing annotations.] +codetoanalyze/java/nullsafe-default/ConditionRedundant.java, codetoanalyze.java.nullsafe_default.ConditionRedundant.checkArgument_NonnullIsBAd(java.lang.String):void, 1, ERADICATE_CONDITION_REDUNDANT, no_bucket, WARNING, [The condition s is always true according to the existing annotations.] +codetoanalyze/java/nullsafe-default/ConditionRedundant.java, codetoanalyze.java.nullsafe_default.ConditionRedundant.checkNotNull_NonnullIsBAD(java.lang.String):void, 1, ERADICATE_CONDITION_REDUNDANT, no_bucket, WARNING, [The condition (s!=null) is always true according to the existing annotations.] +codetoanalyze/java/nullsafe-default/ConditionRedundant.java, codetoanalyze.java.nullsafe_default.ConditionRedundant.compareEQ_NonnullIsBAD(java.lang.String):void, 1, ERADICATE_CONDITION_REDUNDANT, no_bucket, WARNING, [The condition s is always false according to the existing annotations.] +codetoanalyze/java/nullsafe-default/ConditionRedundant.java, codetoanalyze.java.nullsafe_default.ConditionRedundant.compareNEQ_NonnullIsBAD(java.lang.String):void, 1, ERADICATE_CONDITION_REDUNDANT, no_bucket, WARNING, [The condition s is always true according to the existing annotations.] +codetoanalyze/java/nullsafe-default/ConditionRedundant.java, codetoanalyze.java.nullsafe_default.ConditionRedundant.comparingNonnullFieldIsBAD():void, 1, ERADICATE_CONDITION_REDUNDANT, no_bucket, WARNING, [The condition ConditionRedundant.fieldNonnull is always true according to the existing annotations.] +codetoanalyze/java/nullsafe-default/ConditionRedundant.java, codetoanalyze.java.nullsafe_default.ConditionRedundant.comparingNonnullFunctionIsBAD():void, 1, ERADICATE_CONDITION_REDUNDANT, no_bucket, WARNING, [The condition lang.String(this)V is always true according to the existing annotations.] +codetoanalyze/java/nullsafe-default/ConditionRedundant.java, codetoanalyze.java.nullsafe_default.ConditionRedundant.comparingNullableFieldThatIsAlreadyCheckedIsBAD():void, 2, ERADICATE_CONDITION_REDUNDANT, no_bucket, WARNING, [The condition ConditionRedundant.fieldNullable is always true according to the existing annotations.] +codetoanalyze/java/nullsafe-default/ConditionRedundant.java, codetoanalyze.java.nullsafe_default.ConditionRedundant.comparingWithNullIfAssignedBeforeThrowableIsBAD():void, 6, ERADICATE_CONDITION_REDUNDANT, no_bucket, WARNING, [The condition s is always true according to the existing annotations.] +codetoanalyze/java/nullsafe-default/ConditionRedundant.java, codetoanalyze.java.nullsafe_default.ConditionRedundant.conjunctionBothNonnullIsBAD(java.lang.String,java.lang.String):void, 1, ERADICATE_CONDITION_REDUNDANT, no_bucket, WARNING, [The condition s1 is always true according to the existing annotations.] +codetoanalyze/java/nullsafe-default/ConditionRedundant.java, codetoanalyze.java.nullsafe_default.ConditionRedundant.conjunctionBothNonnullIsBAD(java.lang.String,java.lang.String):void, 1, ERADICATE_CONDITION_REDUNDANT, no_bucket, WARNING, [The condition s2 is always true according to the existing annotations.] +codetoanalyze/java/nullsafe-default/ConditionRedundant.java, codetoanalyze.java.nullsafe_default.ConditionRedundant.conjunctionOneNonnullIsBAD(java.lang.String,java.lang.String):void, 1, ERADICATE_CONDITION_REDUNDANT, no_bucket, WARNING, [The condition s2 is always true according to the existing annotations.] +codetoanalyze/java/nullsafe-default/ConditionRedundant.java, codetoanalyze.java.nullsafe_default.ConditionRedundant.disjunctionBothNonnullIsBAD(java.lang.String,java.lang.String):void, 1, ERADICATE_CONDITION_REDUNDANT, no_bucket, WARNING, [The condition s2 is always true according to the existing annotations.] +codetoanalyze/java/nullsafe-default/ConditionRedundant.java, codetoanalyze.java.nullsafe_default.ConditionRedundant.disjunctionBothNonnullIsBAD(java.lang.String,java.lang.String):void, 1, ERADICATE_CONDITION_REDUNDANT, no_bucket, WARNING, [The condition s1 is always false according to the existing annotations.] +codetoanalyze/java/nullsafe-default/ConditionRedundant.java, codetoanalyze.java.nullsafe_default.ConditionRedundant.disjunctionOneNonnullIsBAD(java.lang.String,java.lang.String):void, 1, ERADICATE_CONDITION_REDUNDANT, no_bucket, WARNING, [The condition s2 is always true according to the existing annotations.] +codetoanalyze/java/nullsafe-default/ConditionRedundant.java, codetoanalyze.java.nullsafe_default.ConditionRedundant.irrelevantConditionWithNonnullIsBAD(java.lang.String,java.lang.String,int):void, 1, ERADICATE_CONDITION_REDUNDANT, no_bucket, WARNING, [The condition s1 is always true according to the existing annotations.] +codetoanalyze/java/nullsafe-default/ConditionRedundant.java, codetoanalyze.java.nullsafe_default.ConditionRedundant.outsideOfIfCompareNonnullIsBAD(java.lang.String):void, 1, ERADICATE_CONDITION_REDUNDANT, no_bucket, WARNING, [The condition s is always true according to the existing annotations.] +codetoanalyze/java/nullsafe-default/ConditionRedundant.java, codetoanalyze.java.nullsafe_default.ConditionRedundant.ternary_NonnullInBothBranchesIsBAD(java.lang.String,java.lang.String,int):void, 2, ERADICATE_CONDITION_REDUNDANT, no_bucket, WARNING, [The condition s2 is always false according to the existing annotations.] +codetoanalyze/java/nullsafe-default/ConditionRedundant.java, codetoanalyze.java.nullsafe_default.ConditionRedundant.testFlowSensitivity(java.lang.String,java.lang.String):void, 2, ERADICATE_CONDITION_REDUNDANT, no_bucket, WARNING, [The condition nullable1 is always true according to the existing annotations.] +codetoanalyze/java/nullsafe-default/ConditionRedundant.java, codetoanalyze.java.nullsafe_default.ConditionRedundant.testFlowSensitivity(java.lang.String,java.lang.String):void, 4, ERADICATE_CONDITION_REDUNDANT, no_bucket, WARNING, [The condition nullable1 is always true according to the existing annotations.] 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)] @@ -168,7 +189,6 @@ codetoanalyze/java/nullsafe-default/ParameterNotNullable.java, codetoanalyze.jav codetoanalyze/java/nullsafe-default/ParameterNotNullable.java, codetoanalyze.java.nullsafe_default.ParameterNotNullable.testThreeParameters():void, 2, ERADICATE_PARAMETER_NOT_NULLABLE, no_bucket, WARNING, [`ParameterNotNullable.threeParameters(...)` needs a non-null value in parameter 1 but argument `null` can be null. (Origin: null constant at line 88)] codetoanalyze/java/nullsafe-default/ParameterNotNullable.java, codetoanalyze.java.nullsafe_default.ParameterNotNullable.testThreeParameters():void, 3, ERADICATE_PARAMETER_NOT_NULLABLE, no_bucket, WARNING, [`ParameterNotNullable.threeParameters(...)` needs a non-null value in parameter 2 but argument `null` can be null. (Origin: null constant at line 89)] codetoanalyze/java/nullsafe-default/ParameterNotNullable.java, codetoanalyze.java.nullsafe_default.ParameterNotNullable.testThreeParameters():void, 4, ERADICATE_PARAMETER_NOT_NULLABLE, no_bucket, WARNING, [`ParameterNotNullable.threeParameters(...)` needs a non-null value in parameter 3 but argument `null` can be null. (Origin: null constant at line 90)] -codetoanalyze/java/nullsafe-default/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/nullsafe-default/ReturnNotNullable.java, codetoanalyze.java.nullsafe_default.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 213)] codetoanalyze/java/nullsafe-default/ReturnNotNullable.java, codetoanalyze.java.nullsafe_default.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/nullsafe-default/ReturnNotNullable.java, codetoanalyze.java.nullsafe_default.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)]