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