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 2f955b7b51..768eb0a4ea 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 @@ -2,6 +2,7 @@ import com.google.common.base.Preconditions; import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.LiteralTree; import com.sun.source.tree.MethodInvocationTree; import com.sun.source.tree.ThrowTree; import com.sun.source.tree.Tree; @@ -20,6 +21,7 @@ import org.checkerframework.nullaway.dataflow.cfg.builder.ExtendedNode; import org.checkerframework.nullaway.dataflow.cfg.builder.Label; import org.checkerframework.nullaway.dataflow.cfg.builder.PhaseOneResult; +import org.checkerframework.nullaway.dataflow.cfg.node.IntegerLiteralNode; import org.checkerframework.nullaway.dataflow.cfg.node.MethodInvocationNode; import org.checkerframework.nullaway.dataflow.cfg.node.Node; import org.checkerframework.nullaway.dataflow.cfg.node.ThrowNode; @@ -149,6 +151,19 @@ public void insertThrowOnTrue(Node booleanExpressionNode, TypeMirror errorType) insertThrowOn(true, booleanExpressionNode, errorType); } + /** + * Extend the CFG to throw an exception unconditionally. + * + * @param errorType the type of the exception to throw. + */ + public void insertUnconditionalThrow(TypeMirror errorType) { + // the value that gets thrown does not matter; we just throw 0 + LiteralTree dummyValueToThrowTree = treeBuilder.buildLiteral(0); + handleArtificialTree(dummyValueToThrowTree); + Node dummyValueToThrowNode = new IntegerLiteralNode(dummyValueToThrowTree); + insertThrow(dummyValueToThrowNode, dummyValueToThrowTree, errorType); + } + private void insertThrowOn(boolean throwOn, Node booleanExpressionNode, TypeMirror errorType) { Tree tree = booleanExpressionNode.getTree(); Preconditions.checkArgument( @@ -165,6 +180,20 @@ private void insertThrowOn(boolean throwOn, Node booleanExpressionNode, TypeMirr throwOn ? endPrecondition : preconditionEntry); this.extendWithExtendedNode(cjump); this.addLabelForNextNode(preconditionEntry); + insertThrow(booleanExpressionNode, booleanExpressionTree, errorType); + this.addLabelForNextNode(endPrecondition); + } + + /** + * Extend the CFG to throw an exception with the given type, using the given expression tree as + * the thrown expression. + * + * @param thrownExpressionNode node representing the expression to throw + * @param thrownExpressionTree tree representing the expression to throw + * @param errorType the type of the exception to throw + */ + private void insertThrow( + Node thrownExpressionNode, ExpressionTree thrownExpressionTree, TypeMirror errorType) { ExtendedNode exNode = this.extendWithNodeWithException( new ThrowNode( @@ -173,7 +202,7 @@ private void insertThrowOn(boolean throwOn, Node booleanExpressionNode, TypeMirr // typing. @Override public ExpressionTree getExpression() { - return booleanExpressionTree; + return thrownExpressionTree; } @Override @@ -186,11 +215,10 @@ public R accept(TreeVisitor visitor, D data) { return visitor.visitThrow(this, data); } }, - booleanExpressionNode, + thrownExpressionNode, this.env.getTypeUtils()), errorType); exNode.setTerminatesExecution(true); - this.addLabelForNextNode(endPrecondition); } @Override 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 939c1e086d..0b61cb0cd4 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 @@ -155,18 +155,25 @@ public MethodInvocationNode onCFGBuildPhase1AfterVisitMethodInvocation( break; } } - if (arg != null && supported) { + if (supported) { + // In practice the failure may not be RuntimeException, however the + // throw is inserted after the method invocation where we must assume that + // any invocation is capable of throwing an unchecked throwable. if (runtimeExceptionType == null) { runtimeExceptionType = phase.classToErrorType(RuntimeException.class); } - // In practice the failure may not be RuntimeException, however the conditional - // throw is inserted after the method invocation where we must assume that - // any invocation is capable of throwing an unchecked throwable. Preconditions.checkNotNull(runtimeExceptionType); - if (booleanConstraint) { - phase.insertThrowOnTrue(arg, runtimeExceptionType); + // If arg != null, we found a single boolean antecedent that determines whether the call + // fails, as reflected by booleanConstraint. Otherwise, all antecedents are '_' (or there + // are no antecedents), meaning the method fails unconditionally + if (arg != null) { + if (booleanConstraint) { + phase.insertThrowOnTrue(arg, runtimeExceptionType); + } else { + phase.insertThrowOnFalse(arg, runtimeExceptionType); + } } else { - phase.insertThrowOnFalse(arg, runtimeExceptionType); + phase.insertUnconditionalThrow(runtimeExceptionType); } } } diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayContractsBooleanTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayContractsBooleanTests.java index 9eabae8504..201b4fccde 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayContractsBooleanTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayContractsBooleanTests.java @@ -248,6 +248,50 @@ public void compositeNullCheckFalseMultipleNonNull() { .doTest(); } + @Test + public void unconditionalFail() { + helper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " String test1(@Nullable Object o1) {", + " if (o1 == null) {", + " Validation.fail(\"o1 should not be null\");", + " }", + " return o1.toString();", + " }", + " String test2(@Nullable Object o1) {", + " if (o1 != null) {", + " Validation.fail(\"o1 should be null\");", + " }", + " // BUG: Diagnostic contains: dereferenced expression", + " return o1.toString();", + " }", + " String test3(@Nullable Object o1) {", + " Validation.fail(\"always fail\");", + " // this is unreachable code, so we do not report an error", + " return o1.toString();", + " }", + " String test4(@Nullable Object o1) {", + " if (o1 != null) {", + " System.out.println(o1.toString());", + " } else {", + " Validation.fail(\"o1 should not be null\");", + " }", + " return o1.toString();", + " }", + " String test5(@Nullable Object o1) {", + " if (o1 == null) {", + " Validation.fail();", + " }", + " return o1.toString();", + " }", + "}") + .doTest(); + } + @Test public void identityNotNull() { helper() @@ -565,6 +609,14 @@ private CompilationTestHelper helper() { " static void checkFalse(boolean value) {", " if (value) throw new RuntimeException();", " }", + " @Contract(\"_ -> fail\")", + " static void fail(String msg) {", + " throw new RuntimeException(msg);", + " }", + " @Contract(\" -> fail\")", + " static void fail() {", + " throw new RuntimeException(\"something failed\");", + " }", " @Contract(\"true -> true; false -> false\")", " static boolean identity(boolean value) {", " return value;",