Handle methods that fail unconditionally in ContractHandler

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, D> R accept(TreeVisitor<R, D> visitor, D data) {
                       return visitor.visitThrow(this, data);
                     }
                   },
-                  booleanExpressionNode,
+                  thrownExpressionNode,
                   this.env.getTypeUtils()),
               errorType);
       exNode.setTerminatesExecution(true);
-      this.addLabelForNextNode(endPrecondition);
     }
 
     @Override

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

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;",