From 1b19ebec14960e2c327fb360770f6208023b8372 Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Fri, 30 Sep 2022 19:36:14 -0400 Subject: [PATCH] fix #667 Contract annotations support boolean constraints For example, the following method will behave equivalently to guava `Preconditions.checkState` with regards to null checks: ```java @Contract("false -> fail") static void check(boolean pass) { if (!pass) throw new RuntimeException(); } ``` This implementation works in a couple of layers: Firstly, contracts with a single boolean resultin in failure are handled by inserting a conditional throw node inline following the annotated method. This provides support for complex boolean logic input validateion. Secondly, simple null equal-to and not-equal-to checks are handled by the existing ContractHandler. These don't support such complex inputs, however they are able to work correctly with boolean outputs rather than exclusively `fail` cases. Note that this PR also fixes an assumption in the `ContractHandler` which assumed that `null -> true` implied `!null -> false` which isn't guaranteed unless otherwise specified -- fixing this allows for several new test cases to work as expected. I've also updated the ContractHandler to use more precise language around antecedent nullness, previously null antecedents were described as nullable. Please let me know if I've misunderstood the logic, but reframing the values helped me to understand the logic. --- .../dataflow/cfg/NullAwayCFGBuilder.java | 29 +- .../handlers/contract/ContractHandler.java | 334 ++++++++++++----- .../handlers/contract/ContractUtils.java | 14 + .../NullAwayContractsBooleanTests.java | 351 ++++++++++++++++++ .../uber/nullaway/NullAwayContractsTests.java | 45 ++- 5 files changed, 673 insertions(+), 100 deletions(-) create mode 100644 nullaway/src/test/java/com/uber/nullaway/NullAwayContractsBooleanTests.java diff --git a/nullaway/src/main/java/com/uber/nullaway/dataflow/cfg/NullAwayCFGBuilder.java b/nullaway/src/main/java/com/uber/nullaway/dataflow/cfg/NullAwayCFGBuilder.java index 0e20e0e429..bf5070866d 100644 --- a/nullaway/src/main/java/com/uber/nullaway/dataflow/cfg/NullAwayCFGBuilder.java +++ b/nullaway/src/main/java/com/uber/nullaway/dataflow/cfg/NullAwayCFGBuilder.java @@ -127,25 +127,44 @@ public TypeMirror classToErrorType(Class klass) { } /** - * Extend the CFG to throw an exception if the passed expression node evaluates to false. + * Extend the CFG to throw an exception if the passed expression node evaluates to {@code + * false}. * * @param booleanExpressionNode a CFG Node representing a boolean expression. * @param errorType the type of the exception to throw if booleanExpressionNode evaluates to - * false. + * {@code false}. */ public void insertThrowOnFalse(Node booleanExpressionNode, TypeMirror errorType) { + insertThrowOn(false, booleanExpressionNode, errorType); + } + + /** + * Extend the CFG to throw an exception if the passed expression node evaluates to {@code true}. + * + * @param booleanExpressionNode a CFG Node representing a boolean expression. + * @param errorType the type of the exception to throw if booleanExpressionNode evaluates to + * {@code true}. + */ + public void insertThrowOnTrue(Node booleanExpressionNode, TypeMirror errorType) { + insertThrowOn(true, booleanExpressionNode, errorType); + } + + private void insertThrowOn(boolean throwOn, Node booleanExpressionNode, TypeMirror errorType) { Tree tree = booleanExpressionNode.getTree(); Preconditions.checkArgument( tree instanceof ExpressionTree, "Argument booleanExpressionNode must represent a boolean expression"); ExpressionTree booleanExpressionTree = (ExpressionTree) booleanExpressionNode.getTree(); Preconditions.checkNotNull(booleanExpressionTree); - Label falsePreconditionEntry = new Label(); + Label preconditionEntry = new Label(); Label endPrecondition = new Label(); this.scan(booleanExpressionTree, (Void) null); - ConditionalJump cjump = new ConditionalJump(endPrecondition, falsePreconditionEntry); + ConditionalJump cjump = + new ConditionalJump( + throwOn ? preconditionEntry : endPrecondition, + throwOn ? endPrecondition : preconditionEntry); this.extendWithExtendedNode(cjump); - this.addLabelForNextNode(falsePreconditionEntry); + this.addLabelForNextNode(preconditionEntry); ExtendedNode exNode = this.extendWithNodeWithException( new ThrowNode( diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/contract/ContractHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/contract/ContractHandler.java index 5a9548a97c..354dd0547c 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/contract/ContractHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/contract/ContractHandler.java @@ -40,9 +40,18 @@ import com.uber.nullaway.Nullness; import com.uber.nullaway.dataflow.AccessPath; import com.uber.nullaway.dataflow.AccessPathNullnessPropagation; +import com.uber.nullaway.dataflow.cfg.NullAwayCFGBuilder; import com.uber.nullaway.handlers.BaseNoOpHandler; +import java.util.Optional; import javax.annotation.Nullable; +import javax.lang.model.type.TypeMirror; +import org.checkerframework.nullaway.dataflow.cfg.node.AbstractNodeVisitor; +import org.checkerframework.nullaway.dataflow.cfg.node.BinaryOperationNode; +import org.checkerframework.nullaway.dataflow.cfg.node.EqualToNode; import org.checkerframework.nullaway.dataflow.cfg.node.MethodInvocationNode; +import org.checkerframework.nullaway.dataflow.cfg.node.Node; +import org.checkerframework.nullaway.dataflow.cfg.node.NotEqualNode; +import org.checkerframework.nullaway.dataflow.cfg.node.NullLiteralNode; /** * This Handler parses the jetbrains @Contract annotation and honors the nullness spec defined there @@ -82,6 +91,8 @@ public class ContractHandler extends BaseNoOpHandler { private @Nullable NullAway analysis; private @Nullable VisitorState state; + private @Nullable TypeMirror runtimeExceptionType; + public ContractHandler(Config config) { this.config = config; } @@ -93,6 +104,59 @@ public void onMatchTopLevelClass( this.state = state; } + @Override + public MethodInvocationNode onCFGBuildPhase1AfterVisitMethodInvocation( + NullAwayCFGBuilder.NullAwayCFGTranslationPhaseOne phase, + MethodInvocationTree tree, + MethodInvocationNode originalNode) { + Preconditions.checkNotNull(state); + Preconditions.checkNotNull(analysis); + Symbol.MethodSymbol callee = ASTHelpers.getSymbol(tree); + Preconditions.checkNotNull(callee); + for (String clause : ContractUtils.getContractClauses(callee, config)) { + if (!"fail".equals(getConsequent(clause, tree, analysis, state, callee))) { + continue; + } + String[] antecedent = + getAntecedent(clause, tree, analysis, state, callee, originalNode.getArguments().size()); + // Find a single value constraint that is not already known. If more than one argument with + // unknown nullness affect the method's result, then ignore this clause. + Node arg = null; + // Set to false if the rule is detected to be one we don't yet support + boolean supported = true; + boolean booleanConstraint = false; + + for (int i = 0; i < antecedent.length; ++i) { + String valueConstraint = antecedent[i].trim(); + if ("false".equals(valueConstraint) || "true".equals(valueConstraint)) { + if (arg != null) { + supported = false; + break; + } + booleanConstraint = Boolean.parseBoolean(valueConstraint); + arg = originalNode.getArgument(i); + } else if (!valueConstraint.equals("_")) { + // No need to implement complex handling here, 'onDataflowVisitMethodInvocation' will + // validate the contract. + supported = false; + break; + } + } + if (arg != null && supported) { + if (runtimeExceptionType == null) { + runtimeExceptionType = phase.classToErrorType(RuntimeException.class); + } + Preconditions.checkNotNull(runtimeExceptionType); + if (booleanConstraint) { + phase.insertThrowOnTrue(arg, runtimeExceptionType); + } else { + phase.insertThrowOnFalse(arg, runtimeExceptionType); + } + } + } + return originalNode; + } + @Override public NullnessHint onDataflowVisitMethodInvocation( MethodInvocationNode node, @@ -107,110 +171,194 @@ public NullnessHint onDataflowVisitMethodInvocation( Preconditions.checkNotNull(analysis); Symbol.MethodSymbol callee = ASTHelpers.getSymbol(node.getTree()); Preconditions.checkNotNull(callee); - // Check to see if this method has an @Contract annotation - String contractString = ContractUtils.getContractString(callee, config); - if (contractString != null && contractString.trim().length() > 0) { - // Found a contract, lets parse it. - String[] clauses = contractString.split(";"); - for (String clause : clauses) { - - MethodInvocationTree tree = castToNonNull(node.getTree()); - String[] antecedent = - getAntecedent(clause, tree, analysis, state, callee, node.getArguments().size()); - String consequent = getConsequent(clause, tree, analysis, state, callee); - - // Find a single value constraint that is not already known. If more than one arguments with - // unknown - // nullness affect the method's result, then ignore this clause. - int argIdx = -1; - Nullness argAntecedentNullness = null; - boolean supported = - true; // Set to false if the rule is detected to be one we don't yet support - - for (int i = 0; i < antecedent.length; ++i) { - String valueConstraint = antecedent[i].trim(); - if (valueConstraint.equals("_")) { - continue; - } else if (valueConstraint.equals("false") || valueConstraint.equals("true")) { + MethodInvocationTree tree = castToNonNull(node.getTree()); + for (String clause : ContractUtils.getContractClauses(callee, config)) { + + String[] antecedent = + getAntecedent(clause, tree, analysis, state, callee, node.getArguments().size()); + String consequent = getConsequent(clause, tree, analysis, state, callee); + + // Find a single value constraint that is not already known. If more than one arguments with + // unknown + // nullness affect the method's result, then ignore this clause. + Node arg = null; + Nullness argAntecedentNullness = null; + boolean supported = + true; // Set to false if the rule is detected to be one we don't yet support + + for (int i = 0; i < antecedent.length; ++i) { + String valueConstraint = antecedent[i].trim(); + if (valueConstraint.equals("_")) { + continue; + } else if (valueConstraint.equals("false") || valueConstraint.equals("true")) { + if (arg != null) { + // More than one argument involved in the antecedent, ignore this rule supported = false; break; - } else if (valueConstraint.equals("!null") - && inputs.valueOfSubNode(node.getArgument(i)).equals(Nullness.NONNULL)) { - // We already know this argument can't be null, so we can treat it as not part of the - // clause - // for the purpose of deciding the non-nullness of the other arguments. - continue; - } else if (valueConstraint.equals("null") || valueConstraint.equals("!null")) { - if (argIdx != -1) { - // More than one argument involved in the antecedent, ignore this rule - supported = false; - break; - } - argIdx = i; - argAntecedentNullness = - valueConstraint.equals("null") ? Nullness.NULLABLE : Nullness.NONNULL; - } else { - String errorMessage = - "Invalid @Contract annotation detected for method " - + callee - + ". It contains the following uparseable clause: " - + clause - + " (unknown value constraint: " - + valueConstraint - + ", see https://www.jetbrains.com/help/idea/contract-annotations.html)."; - state.reportMatch( - analysis - .getErrorBuilder() - .createErrorDescription( - new ErrorMessage( - ErrorMessage.MessageTypes.ANNOTATION_VALUE_INVALID, errorMessage), - tree, - analysis.buildDescription(tree), - state, - null)); + } + Node argument = node.getArgument(i); + Optional isNullTarget = argument.accept(NullEqualityVisitor.IS_NULL, None.INSTANCE); + Optional notNullTarget = + argument.accept(NullEqualityVisitor.NOT_NULL, None.INSTANCE); + Node nullTestTarget = isNullTarget.orElse(notNullTarget.orElse(null)); + if (nullTestTarget == null) { supported = false; break; } - } - if (!supported) { - // Too many arguments involved, or unsupported @Contract features. On to next clause in - // the - // contract expression + // isNullTarget is equivalent to 'null ->' while notNullTarget is equivalent + // to '!null ->'. However, the valueConstraint may reverse the check. + boolean inverted = isNullTarget.isEmpty() == valueConstraint.equals("true"); + arg = nullTestTarget; + argAntecedentNullness = inverted ? Nullness.NONNULL : Nullness.NULL; + } else if (valueConstraint.equals("!null") + && inputs.valueOfSubNode(node.getArgument(i)).equals(Nullness.NONNULL)) { + // We already know this argument can't be null, so we can treat it as not part of the + // clause + // for the purpose of deciding the non-nullness of the other arguments. continue; - } - if (argIdx == -1) { - // The antecedent is unconditionally true. Check for the ... -> !null case and set the - // return nullness accordingly - if (consequent.equals("!null")) { - return NullnessHint.FORCE_NONNULL; + } else if (valueConstraint.equals("null") || valueConstraint.equals("!null")) { + if (arg != null) { + // More than one argument involved in the antecedent, ignore this rule + supported = false; + break; } - continue; - } - if (argAntecedentNullness == null) { - throw new IllegalStateException("argAntecedentNullness should have been set"); - } - // The nullness of one argument is all that matters for the antecedent, let's negate the - // consequent to fix the nullness of this argument. - AccessPath accessPath = - AccessPath.getAccessPathForNodeNoMapGet(node.getArgument(argIdx), apContext); - if (accessPath == null) { - continue; + arg = node.getArgument(i); + argAntecedentNullness = valueConstraint.equals("null") ? Nullness.NULL : Nullness.NONNULL; + } else { + String errorMessage = + "Invalid @Contract annotation detected for method " + + callee + + ". It contains the following uparseable clause: " + + clause + + " (unknown value constraint: " + + valueConstraint + + ", see https://www.jetbrains.com/help/idea/contract-annotations.html)."; + state.reportMatch( + analysis + .getErrorBuilder() + .createErrorDescription( + new ErrorMessage( + ErrorMessage.MessageTypes.ANNOTATION_VALUE_INVALID, errorMessage), + tree, + analysis.buildDescription(tree), + state, + null)); + supported = false; + break; } - if (consequent.equals("false") && argAntecedentNullness.equals(Nullness.NULLABLE)) { - // If argIdx being null implies the return of the method being false, then the return - // being true implies argIdx is not null and we must mark it as such in the then update. - thenUpdates.set(accessPath, Nullness.NONNULL); - } else if (consequent.equals("true") && argAntecedentNullness.equals(Nullness.NULLABLE)) { - // If argIdx being null implies the return of the method being true, then the return being - // false implies argIdx is not null and we must mark it as such in the else update. - elseUpdates.set(accessPath, Nullness.NONNULL); - } else if (consequent.equals("fail") && argAntecedentNullness.equals(Nullness.NULLABLE)) { - // If argIdx being null implies the method throws an exception, then we can mark it as - // non-null on both non-exceptional exits from the method - bothUpdates.set(accessPath, Nullness.NONNULL); + } + if (!supported) { + // Too many arguments involved, or unsupported @Contract features. On to next clause in + // the + // contract expression + continue; + } + if (arg == null) { + // The antecedent is unconditionally true. Check for the ... -> !null case and set the + // return nullness accordingly + if (consequent.equals("!null")) { + return NullnessHint.FORCE_NONNULL; } + continue; + } + if (argAntecedentNullness == null) { + throw new IllegalStateException("argAntecedentNullness should have been set"); + } + // The nullness of one argument is all that matters for the antecedent, let's negate the + // consequent to fix the nullness of this argument. + AccessPath accessPath = AccessPath.getAccessPathForNodeNoMapGet(arg, apContext); + if (accessPath == null) { + continue; + } + if (consequent.equals("false") && argAntecedentNullness.equals(Nullness.NULL)) { + // Arg being null implies the return of the method being false. + elseUpdates.set(accessPath, Nullness.NULL); + } else if (consequent.equals("false") && argAntecedentNullness.equals(Nullness.NONNULL)) { + // Arg being non-null implies the return of the method being false. + elseUpdates.set(accessPath, Nullness.NONNULL); + } else if (consequent.equals("true") && argAntecedentNullness.equals(Nullness.NULL)) { + // Arg being null implies the return of the method being true. + thenUpdates.set(accessPath, Nullness.NULL); + } else if (consequent.equals("true") && argAntecedentNullness.equals(Nullness.NONNULL)) { + // Arg being non-null implies the return of the method being true. + thenUpdates.set(accessPath, Nullness.NONNULL); + } else if (consequent.equals("fail") && argAntecedentNullness.equals(Nullness.NONNULL)) { + // Arg being non-null implies the method throws an exception, then we can mark it as + // null on both non-exceptional exits from the method. + bothUpdates.set(accessPath, Nullness.NULL); + } else if (consequent.equals("fail") && argAntecedentNullness.equals(Nullness.NULL)) { + // Arg being null implies the method throws an exception, then we can mark it as + // non-null on both non-exceptional exits from the method. + bothUpdates.set(accessPath, Nullness.NONNULL); } } return NullnessHint.UNKNOWN; } + + private static final class IsNullLiteralNodeVisitor extends AbstractNodeVisitor { + private static final IsNullLiteralNodeVisitor INSTANCE = new IsNullLiteralNodeVisitor(); + + @Override + public Boolean visitNode(Node node, None unused) { + return false; + } + + @Override + public Boolean visitNullLiteral(NullLiteralNode node, None unused) { + return true; + } + } + + private static final class NullEqualityVisitor extends AbstractNodeVisitor, None> { + + private static final NullEqualityVisitor IS_NULL = new NullEqualityVisitor(true); + private static final NullEqualityVisitor NOT_NULL = new NullEqualityVisitor(false); + + private final boolean equals; + + NullEqualityVisitor(boolean equals) { + this.equals = equals; + } + + @Override + public Optional visitNode(Node node, None unused) { + return Optional.empty(); + } + + @Override + public Optional visitNotEqual(NotEqualNode notEqualNode, None unused) { + if (equals) { + return Optional.empty(); + } else { + return visit(notEqualNode); + } + } + + @Override + public Optional visitEqualTo(EqualToNode equalToNode, None unused) { + if (equals) { + return visit(equalToNode); + } else { + return Optional.empty(); + } + } + + private Optional visit(BinaryOperationNode comparison) { + Node lhs = comparison.getLeftOperand(); + Node rhs = comparison.getRightOperand(); + boolean lhsIsNullLiteral = lhs.accept(IsNullLiteralNodeVisitor.INSTANCE, None.INSTANCE); + boolean rhsIsNullLiteral = rhs.accept(IsNullLiteralNodeVisitor.INSTANCE, None.INSTANCE); + if (lhsIsNullLiteral && !rhsIsNullLiteral) { + return Optional.of(rhs); + } + if (!lhsIsNullLiteral && rhsIsNullLiteral) { + return Optional.of(lhs); + } + return Optional.empty(); + } + } + + private enum None { + INSTANCE + } } diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/contract/ContractUtils.java b/nullaway/src/main/java/com/uber/nullaway/handlers/contract/ContractUtils.java index 10c62c7579..ac54e11326 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/contract/ContractUtils.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/contract/ContractUtils.java @@ -17,6 +17,8 @@ /** An utility class for {@link ContractHandler} and {@link ContractCheckHandler}. */ public class ContractUtils { + private static final String[] EMPTY_STRING_ARRAY = new String[0]; + /** * Returns a set of field names excluding their receivers (e.g. "this.a" will be "a") * @@ -128,4 +130,16 @@ static String getContractString(Symbol.MethodSymbol methodSymbol, Config config) } return null; } + + static String[] getContractClauses(Symbol.MethodSymbol callee, Config config) { + // Check to see if this method has an @Contract annotation + String contractString = getContractString(callee, config); + if (contractString != null) { + String trimmedContractString = contractString.trim(); + if (!trimmedContractString.isEmpty()) { + return trimmedContractString.split(";"); + } + } + return EMPTY_STRING_ARRAY; + } } diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayContractsBooleanTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayContractsBooleanTests.java new file mode 100644 index 0000000000..1feb7d9f2f --- /dev/null +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayContractsBooleanTests.java @@ -0,0 +1,351 @@ +package com.uber.nullaway; + +import com.google.errorprone.CompilationTestHelper; +import java.util.Arrays; +import org.junit.Test; + +public class NullAwayContractsBooleanTests extends NullAwayTestsBase { + + @Test + public void nonNullCheckIsTrueIsNotNullable() { + helper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " String test(@Nullable Object o1) {", + " Validation.checkTrue(o1 != null);", + " return o1.toString();", + " }", + "}") + .doTest(); + } + + @Test + public void nonNullCheckIsTrueIsNotNullableReversed() { + helper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " String test(@Nullable Object o1) {", + " Validation.checkTrue(null != o1);", + " return o1.toString();", + " }", + "}") + .doTest(); + } + + @Test + public void nullCheckIsFalseIsNotNullable() { + helper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " String test(@Nullable Object o1) {", + " Validation.checkFalse(o1 == null);", + " return o1.toString();", + " }", + "}") + .doTest(); + } + + @Test + public void nullCheckIsFalseIsNotNullableReversed() { + helper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " String test(@Nullable Object o1) {", + " Validation.checkFalse(null == o1);", + " return o1.toString();", + " }", + "}") + .doTest(); + } + + @Test + public void nullCheckIsTrueIsNull() { + helper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " String test(@Nullable Object o1) {", + " Validation.checkTrue(o1 == null);", + " // BUG: Diagnostic contains: dereferenced expression", + " return o1.toString();", + " }", + "}") + .doTest(); + } + + @Test + public void nullCheckIsTrueIsNullReversed() { + helper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " String test(@Nullable Object o1) {", + " Validation.checkTrue(null == o1);", + " // BUG: Diagnostic contains: dereferenced expression", + " return o1.toString();", + " }", + "}") + .doTest(); + } + + @Test + public void nonNullCheckIsFalseIsNull() { + helper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " String test(@Nullable Object o1) {", + " Validation.checkFalse(o1 != null);", + " // BUG: Diagnostic contains: dereferenced expression", + " return o1.toString();", + " }", + "}") + .doTest(); + } + + @Test + public void nonNullCheckIsFalseIsNullReversed() { + helper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " String test(@Nullable Object o1) {", + " Validation.checkFalse(null != o1);", + " // BUG: Diagnostic contains: dereferenced expression", + " return o1.toString();", + " }", + "}") + .doTest(); + } + + @Test + public void compositeNullCheckAndStringEquality() { + helper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " String test(@Nullable Object o1) {", + " Validation.checkTrue(o1 != null && o1.toString().equals(\"\"));", + " return o1.toString();", + " }", + "}") + .doTest(); + } + + @Test + public void compositeNullCheckMultipleNonNull() { + helper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " String test(@Nullable Object o1, @Nullable Object o2) {", + " Validation.checkTrue(o1 != null && o2 != null);", + " return o1.toString() + o2.toString();", + " }", + "}") + .doTest(); + } + + @Test + public void compositeNullCheckFalseAndStringEquality() { + helper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " String test(@Nullable Object o1) {", + " Validation.checkFalse(o1 == null || o1.toString().equals(\"\"));", + " return o1.toString();", + " }", + "}") + .doTest(); + } + + @Test + public void compositeNullCheckFalseMultipleNonNull() { + helper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " String test(@Nullable Object o1, @Nullable Object o2) {", + " Validation.checkFalse(o1 == null || o2 == null);", + " return o1.toString() + o2.toString();", + " }", + "}") + .doTest(); + } + + @Test + public void identityNotNull() { + helper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " String test(@Nullable Object o1) {", + " if (Validation.identity(null != o1)) {", + " return o1.toString();", + " } else {", + " // BUG: Diagnostic contains: dereferenced expression", + " return o1.toString();", + " }", + " }", + "}") + .doTest(); + } + + @Test + public void invertIsNull() { + helper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " String test(@Nullable Object o1) {", + " if (Validation.invert(null == o1)) {", + " return o1.toString();", + " } else {", + " // BUG: Diagnostic contains: dereferenced expression", + " return o1.toString();", + " }", + " }", + "}") + .doTest(); + } + + @Test + public void identityIsNull() { + helper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " String test(@Nullable Object o1) {", + " if (Validation.identity(null == o1)) {", + " // BUG: Diagnostic contains: dereferenced expression", + " return o1.toString();", + " } else {", + " return o1.toString();", + " }", + " }", + "}") + .doTest(); + } + + @Test + public void invertNotNull() { + helper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " String test(@Nullable Object o1) {", + " if (Validation.invert(null != o1)) {", + " // BUG: Diagnostic contains: dereferenced expression", + " return o1.toString();", + " } else {", + " return o1.toString();", + " }", + " }", + "}") + .doTest(); + } + + @Test + public void checkAndReturn() { + helper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "import org.jetbrains.annotations.Contract;", + "class Test {", + " @Contract(\"false -> fail\")", + " static boolean checkAndReturn(boolean value) {", + " if (!value) {", + " throw new RuntimeException();", + " }", + " return true;", + " }", + " String test1(@Nullable Object o1, @Nullable Object o2) {", + " if (checkAndReturn(o1 != null) && o2 != null) {", + " return o1.toString() + o2.toString();", + " } else {", + " return o1.toString() + ", + " // BUG: Diagnostic contains: dereferenced expression", + " o2.toString();", + " }", + " }", + " boolean test2(@Nullable Object o1, @Nullable Object o2) {", + " return checkAndReturn(o1 != null) && o1.toString().isEmpty();", + " }", + " boolean test3(@Nullable Object o1, @Nullable Object o2) {", + " return checkAndReturn(o1 == null) ", + " // BUG: Diagnostic contains: dereferenced expression", + " && o1.toString().isEmpty();", + " }", + "}") + .doTest(); + } + + private CompilationTestHelper helper() { + return makeTestHelperWithArgs( + Arrays.asList( + "-d", + temporaryFolder.getRoot().getAbsolutePath(), + "-XepOpt:NullAway:AnnotatedPackages=com.uber")) + .addSourceLines( + "Validation.java", + "package com.uber;", + "import org.jetbrains.annotations.Contract;", + "public final class Validation {", + " @Contract(\"false -> fail\")", + " static void checkTrue(boolean value) {", + " if (!value) throw new RuntimeException();", + " }", + " @Contract(\"true -> fail\")", + " static void checkFalse(boolean value) {", + " if (!value) throw new RuntimeException();", + " }", + " @Contract(\"true -> true; false -> false\")", + " static boolean identity(boolean value) {", + " return value;", + " }", + " @Contract(\"true -> false; false -> true\")", + " static boolean invert(boolean value) {", + " return !value;", + " }", + "}"); + } +} diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayContractsTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayContractsTests.java index 452676a6b4..e7b9853cbb 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayContractsTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayContractsTests.java @@ -42,9 +42,9 @@ public void basicContractAnnotation() { "import javax.annotation.Nullable;", "import org.jetbrains.annotations.Contract;", "public class NullnessChecker {", - " @Contract(\"_, null -> true\")", + " @Contract(\"_, null -> true; _, !null -> false\")", " static boolean isNull(boolean flag, @Nullable Object o) { return o == null; }", - " @Contract(\"null -> false\")", + " @Contract(\"null -> false; !null -> true\")", " static boolean isNonNull(@Nullable Object o) { return o != null; }", " @Contract(\"null -> fail\")", " static void assertNonNull(@Nullable Object o) { if (o != null) throw new Error(); }", @@ -267,4 +267,45 @@ public void customContractAnnotation() { "}") .doTest(); } + + @Test + public void pedanticEdgeCaseContract() { + makeTestHelperWithArgs( + Arrays.asList( + "-d", + temporaryFolder.getRoot().getAbsolutePath(), + "-XepOpt:NullAway:AnnotatedPackages=com.uber")) + .addSourceLines( + "NullnessChecker.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "import org.jetbrains.annotations.Contract;", + "public class NullnessChecker {", + " @Contract(\"null, _ -> false; _, null -> false\")", + " static boolean check(@Nullable Object o1, @Nullable Object o2) {", + " // null, _ -> false", + " if (o1 == null) {", + " return false;", + " }", + " // _, null -> false", + " if (o2 == null) {", + " return false;", + " }", + " return true;", + " }", + "}") + .addSourceLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " String test1(@Nullable Object o) {", + " return NullnessChecker.check(\"\", o)", + " // BUG: Diagnostic contains: dereferenced expression", + " ? o.toString()", + " : \"null\";", + " }", + "}") + .doTest(); + } }