From 6cd33ba6eb94807a5cc984aa173a92fb252e77f6 Mon Sep 17 00:00:00 2001
From: Carter Kozak <ckozak@ckozak.net>
Date: Mon, 3 Oct 2022 14:49:19 -0400
Subject: [PATCH] PreconditionsHandler reflects Guava Preconditions exception
 types (#668)

This change does not impact CFG construction, since it already assumed that method calls (like the `Preconditions` calls) could throw any unchecked exception and built the CFG accordingly.  Still, it's good to more accurately model which exception type may be thrown by each `Preconditions` method.
---
 .../handlers/PreconditionsHandler.java        | 23 ++++++---
 .../nullaway/NullAwayPreconditionTests.java   | 48 +++++++++++++++++++
 2 files changed, 65 insertions(+), 6 deletions(-)

diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/PreconditionsHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/PreconditionsHandler.java
index ff42995dea..2cb276de32 100644
--- a/nullaway/src/main/java/com/uber/nullaway/handlers/PreconditionsHandler.java
+++ b/nullaway/src/main/java/com/uber/nullaway/handlers/PreconditionsHandler.java
@@ -19,7 +19,8 @@ public class PreconditionsHandler extends BaseNoOpHandler {
   @Nullable private Name preconditionsClass;
   @Nullable private Name checkArgumentMethod;
   @Nullable private Name checkStateMethod;
-  @Nullable TypeMirror preconditionErrorType;
+  @Nullable TypeMirror preconditionCheckArgumentErrorType;
+  @Nullable TypeMirror preconditionCheckStateErrorType;
 
   @Override
   public MethodInvocationNode onCFGBuildPhase1AfterVisitMethodInvocation(
@@ -31,13 +32,23 @@ public MethodInvocationNode onCFGBuildPhase1AfterVisitMethodInvocation(
       preconditionsClass = callee.name.table.fromString(PRECONDITIONS_CLASS_NAME);
       checkArgumentMethod = callee.name.table.fromString(CHECK_ARGUMENT_METHOD_NAME);
       checkStateMethod = callee.name.table.fromString(CHECK_STATE_METHOD_NAME);
-      preconditionErrorType = phase.classToErrorType(IllegalArgumentException.class);
+      preconditionCheckArgumentErrorType = phase.classToErrorType(IllegalArgumentException.class);
+      preconditionCheckStateErrorType = phase.classToErrorType(IllegalStateException.class);
     }
-    Preconditions.checkNotNull(preconditionErrorType);
+    Preconditions.checkNotNull(preconditionCheckArgumentErrorType);
+    Preconditions.checkNotNull(preconditionCheckStateErrorType);
     if (callee.enclClass().getQualifiedName().equals(preconditionsClass)
-        && (callee.name.equals(checkArgumentMethod) || callee.name.equals(checkStateMethod))
-        && callee.getParameters().size() > 0) {
-      phase.insertThrowOnFalse(originalNode.getArgument(0), preconditionErrorType);
+        && !callee.getParameters().isEmpty()) {
+      // Attempt to match Precondition check methods to the expected exception type, providing as
+      // much context as possible for static analysis.
+      // In practice this may not be strictly necessary because the conditional throw is inserted
+      // after the method invocation, thus analysis must assume that the preconditions call is
+      // capable of throwing any unchecked throwable.
+      if (callee.name.equals(checkArgumentMethod)) {
+        phase.insertThrowOnFalse(originalNode.getArgument(0), preconditionCheckArgumentErrorType);
+      } else if (callee.name.equals(checkStateMethod)) {
+        phase.insertThrowOnFalse(originalNode.getArgument(0), preconditionCheckStateErrorType);
+      }
     }
     return originalNode;
   }
diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayPreconditionTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayPreconditionTests.java
index fb663c5d42..927aeac863 100644
--- a/nullaway/src/test/java/com/uber/nullaway/NullAwayPreconditionTests.java
+++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayPreconditionTests.java
@@ -222,4 +222,52 @@ public void inconclusiveCheckArgumentTest() {
             "}")
         .doTest();
   }
+
+  @Test
+  public void checkArgumentCatchException() {
+    makeTestHelperWithArgs(
+            Arrays.asList(
+                "-d",
+                temporaryFolder.getRoot().getAbsolutePath(),
+                "-XepOpt:NullAway:AnnotatedPackages=com.uber"))
+        .addSourceLines(
+            "Test.java",
+            "package com.uber;",
+            "import javax.annotation.Nullable;",
+            "import com.google.common.base.Preconditions;",
+            "class Test {",
+            "  private void foo(@Nullable Object a) {",
+            "    try {",
+            "      Preconditions.checkArgument(a != null);",
+            "    } catch (IllegalArgumentException e) {}",
+            "    // BUG: Diagnostic contains: dereferenced expression a is @Nullable",
+            "    a.toString();",
+            "  }",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  public void checkStateCatchException() {
+    makeTestHelperWithArgs(
+            Arrays.asList(
+                "-d",
+                temporaryFolder.getRoot().getAbsolutePath(),
+                "-XepOpt:NullAway:AnnotatedPackages=com.uber"))
+        .addSourceLines(
+            "Test.java",
+            "package com.uber;",
+            "import javax.annotation.Nullable;",
+            "import com.google.common.base.Preconditions;",
+            "class Test {",
+            "  private void foo(@Nullable Object a) {",
+            "    try {",
+            "      Preconditions.checkState(a != null);",
+            "    } catch (IllegalStateException e) {}",
+            "    // BUG: Diagnostic contains: dereferenced expression a is @Nullable",
+            "    a.toString();",
+            "  }",
+            "}")
+        .doTest();
+  }
 }