Introduce FluentFutureHandler as a workaround for Guava Futures/FluentFuture (#771)

lazaroclapp · web-flow · commit 37f4bb8a9775 · 2023-06-21T15:33:32.000-07:00
We need this due to our incomplete support for generics, and the fact that #765 fixes a previous false negative which would cause our `@NonNull` model for Guava's `Function` to be ignored when determining the correct overriding of the functional interface by a lambda. After the fix, however, code such as: ``` FluentFuture .from(...) .transform(s -> { if(...) {...} else { return null; } }, executor); ``` will fail with an error about `Function::apply` having a `@NonNull` result. Where nullability should actually depend on the parameter `T` of the `ListenableFuture<T>` in question. Unfortunately, usage of this futures API is common internally for us, and our generics support is far from fully supporting this in a sound manner, so this diff introduces a (hopefully temporary) workaround, in the form of handler for the Futures/FluentFuture Guava APIs: This works by special casing the return nullability of `com.google.common.base.Function` and `com.google.common.util.concurrent.AsyncFunction` to be e.g. `Function<@nullable T>` whenever these functional interfaces are implemented as a lambda expression passed to a list of specific methods of `com.google.common.util.concurrent.FluentFuture` or `com.google.common.util.concurrent.Futures`. This is unsound, but permits the code example above to pass NullAway checking, while strictly reducing the safety hole of NullAway versions prior to PR #765.
diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/Handlers.java b/nullaway/src/main/java/com/uber/nullaway/handlers/Handlers.java
@@ -28,6 +28,7 @@
 import com.uber.nullaway.handlers.contract.ContractHandler;
 import com.uber.nullaway.handlers.contract.fieldcontract.EnsuresNonNullHandler;
 import com.uber.nullaway.handlers.contract.fieldcontract.RequiresNonNullHandler;
+import com.uber.nullaway.handlers.temporary.FluentFutureHandler;
 
 /** Utility static methods for the handlers package. */
 public class Handlers {
@@ -75,6 +76,7 @@ public static Handler buildDefault(Config config) {
       handlerListBuilder.add(new ContractCheckHandler(config));
     }
     handlerListBuilder.add(new LombokHandler(config));
+    handlerListBuilder.add(new FluentFutureHandler());
 
     return new CompositeHandler(handlerListBuilder.build());
   }
diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/temporary/FluentFutureHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/temporary/FluentFutureHandler.java
@@ -0,0 +1,91 @@
+package com.uber.nullaway.handlers.temporary;
+
+import com.google.errorprone.VisitorState;
+import com.google.errorprone.util.ASTHelpers;
+import com.sun.source.tree.LambdaExpressionTree;
+import com.sun.source.tree.MethodInvocationTree;
+import com.sun.tools.javac.code.Symbol;
+import com.uber.nullaway.NullabilityUtil;
+import com.uber.nullaway.Nullness;
+import com.uber.nullaway.handlers.BaseNoOpHandler;
+import java.util.Arrays;
+import javax.lang.model.element.Name;
+
+/**
+ * This handler provides a temporary workaround due to our lack of support for generics, which
+ * allows natural usage of Futures/FluentFuture Guava APIs. It can potentially introduce false
+ * negatives, however, and should be deprecated as soon as full generic support is available.
+ *
+ * <p>This works by special casing the return nullability of {@link com.google.common.base.Function}
+ * and {@link com.google.common.util.concurrent.AsyncFunction} to be e.g. {@code Function<@Nullable
+ * T>} whenever these functional interfaces are implemented as a lambda expression passed to a list
+ * of specific methods of {@link com.google.common.util.concurrent.FluentFuture} or {@link
+ * com.google.common.util.concurrent.Futures}. We cannot currently check that {@code T} for {@code
+ * FluentFuture<T>} is a {@code @Nullable} type, so this is unsound. However, we have found many
+ * cases in practice where these lambdas include {@code null} returns, which were already being
+ * ignored (due to a bug) before PR #765. This handler offers the best possible support for these
+ * cases, at least until our generics support is mature enough to handle them.
+ *
+ * <p>Note: Package {@code com.uber.nullaway.handlers.temporary} is meant for this sort of temporary
+ * workaround handler, to be removed as future NullAway features make them unnecessary. This is a
+ * hack, but the best of a bunch of bad options.
+ */
+public class FluentFutureHandler extends BaseNoOpHandler {
+
+  private static final String GUAVA_FUNCTION_CLASS_NAME = "com.google.common.base.Function";
+  private static final String GUAVA_ASYNC_FUNCTION_CLASS_NAME =
+      "com.google.common.util.concurrent.AsyncFunction";
+  private static final String FLUENT_FUTURE_CLASS_NAME =
+      "com.google.common.util.concurrent.FluentFuture";
+  private static final String FUTURES_CLASS_NAME = "com.google.common.util.concurrent.Futures";
+  private static final String FUNCTION_APPLY_METHOD_NAME = "apply";
+  private static final String[] FLUENT_FUTURE_INCLUDE_LIST_METHODS = {
+    "catching", "catchingAsync", "transform", "transformAsync"
+  };
+
+  private static boolean isGuavaFunctionDotApply(Symbol.MethodSymbol methodSymbol) {
+    Name className = methodSymbol.enclClass().flatName();
+    return (className.contentEquals(GUAVA_FUNCTION_CLASS_NAME)
+            || className.contentEquals(GUAVA_ASYNC_FUNCTION_CLASS_NAME))
+        && methodSymbol.name.contentEquals(FUNCTION_APPLY_METHOD_NAME);
+  }
+
+  private static boolean isFluentFutureIncludeListMethod(Symbol.MethodSymbol methodSymbol) {
+    Name className = methodSymbol.enclClass().flatName();
+    return (className.contentEquals(FLUENT_FUTURE_CLASS_NAME)
+            || className.contentEquals(FUTURES_CLASS_NAME))
+        && Arrays.stream(FLUENT_FUTURE_INCLUDE_LIST_METHODS)
+            .anyMatch(s -> methodSymbol.name.contentEquals(s));
+  }
+
+  @Override
+  public Nullness onOverrideMethodReturnNullability(
+      Symbol.MethodSymbol methodSymbol,
+      VisitorState state,
+      boolean isAnnotated,
+      Nullness returnNullness) {
+    // We only care about lambda's implementing Guava's Function
+    if (!isGuavaFunctionDotApply(methodSymbol)) {
+      return returnNullness;
+    }
+    // Check if we are inside a lambda passed as an argument to a method call:
+    LambdaExpressionTree enclosingLambda =
+        ASTHelpers.findEnclosingNode(state.getPath(), LambdaExpressionTree.class);
+    if (enclosingLambda == null
+        || !NullabilityUtil.getFunctionalInterfaceMethod(enclosingLambda, state.getTypes())
+            .equals(methodSymbol)) {
+      return returnNullness;
+    }
+    MethodInvocationTree methodInvocation =
+        ASTHelpers.findEnclosingNode(state.getPath(), MethodInvocationTree.class);
+    if (methodInvocation == null || !methodInvocation.getArguments().contains(enclosingLambda)) {
+      return returnNullness;
+    }
+    // Check if that method call is one of the FluentFuture APIs we care about
+    Symbol.MethodSymbol lambdaConsumerMethodSymbol = ASTHelpers.getSymbol(methodInvocation);
+    if (!isFluentFutureIncludeListMethod(lambdaConsumerMethodSymbol)) {
+      return returnNullness;
+    }
+    return Nullness.NULLABLE;
+  }
+}
diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayFunctionalInterfaceNullabilityTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayFunctionalInterfaceNullabilityTests.java
@@ -0,0 +1,110 @@
+package com.uber.nullaway;
+
+import org.junit.Test;
+
+public class NullAwayFunctionalInterfaceNullabilityTests extends NullAwayTestsBase {
+
+  @Test
+  public void multipleTypeParametersInstantiation() {
+    defaultCompilationHelper
+        .addSourceLines(
+            "NullableFunction.java",
+            "package com.uber.unannotated;", // As if a third-party lib, since override is invalid
+            "import javax.annotation.Nullable;",
+            "import java.util.function.Function;",
+            "@FunctionalInterface",
+            "public interface NullableFunction<F, T> extends Function<F, T> {",
+            "  @Override",
+            "  @Nullable",
+            "  T apply(@Nullable F input);",
+            "}")
+        .addSourceLines(
+            "Test.java",
+            "package com.uber;",
+            "import javax.annotation.Nullable;",
+            "import java.util.function.Function;",
+            "import com.uber.unannotated.NullableFunction;",
+            "class Test {",
+            "  private static void takesNullableFunction(NullableFunction<String, String> nf) { }",
+            "  private static void takesNonNullableFunction(Function<String, String> f) { }",
+            "  private static void passesNullableFunction() {",
+            "    takesNullableFunction(s -> { return null; });",
+            "  }",
+            "  private static void passesNullableFunctionToNonNull() {",
+            "    takesNonNullableFunction(s -> { return null; });",
+            "  }",
+            "}")
+        .addSourceLines(
+            "TestGuava.java",
+            "package com.uber;",
+            "import javax.annotation.Nullable;",
+            "import com.google.common.base.Function;",
+            "import com.uber.unannotated.NullableFunction;",
+            "class TestGuava {",
+            "  private static void takesNullableFunction(NullableFunction<String, String> nf) { }",
+            "  private static void takesNonNullableFunction(Function<String, String> f) { }",
+            "  private static void passesNullableFunction() {",
+            "    takesNullableFunction(s -> { return null; });",
+            "  }",
+            "  private static void passesNullableFunctionToNonNull() {",
+            "    // BUG: Diagnostic contains: returning @Nullable expression",
+            "    takesNonNullableFunction(s -> { return null; });",
+            "  }",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  public void futuresFunctionLambdas() {
+    // See FluentFutureHandler
+    defaultCompilationHelper
+        .addSourceLines(
+            "TestGuava.java",
+            "package com.uber;",
+            "import org.jspecify.annotations.Nullable;",
+            "import com.google.common.base.Function;",
+            "import com.google.common.util.concurrent.FluentFuture;",
+            "import com.google.common.util.concurrent.Futures;",
+            "import com.google.common.util.concurrent.ListenableFuture;",
+            "import java.util.concurrent.Executor;",
+            "class TestGuava {",
+            "  private static ListenableFuture<@Nullable String> fluentFutureCatching(Executor executor) {",
+            "    return FluentFuture",
+            "        .from(Futures.immediateFuture(\"hi\"))",
+            "        .catching(Throwable.class, e -> { return null; }, executor);",
+            "  }",
+            "  private static ListenableFuture<@Nullable String> fluentFutureCatchingAsync(Executor executor) {",
+            "    return FluentFuture",
+            "        .from(Futures.immediateFuture(\"hi\"))",
+            "        .catchingAsync(Throwable.class, e -> { return null; }, executor);",
+            "  }",
+            "  private static ListenableFuture<@Nullable String> fluentFutureTransform(Executor executor) {",
+            "    return FluentFuture",
+            "        .from(Futures.immediateFuture(\"hi\"))",
+            "        .transform(s -> { return null; }, executor);",
+            "  }",
+            "  private static ListenableFuture<@Nullable String> fluentFutureTransformAsync(Executor executor) {",
+            "    return FluentFuture",
+            "        .from(Futures.immediateFuture(\"hi\"))",
+            "        .transformAsync(s -> { return null; }, executor);",
+            "  }",
+            "  private static ListenableFuture<String> fluentFutureTransformNoNull(Executor executor) {",
+            "    return FluentFuture",
+            "        .from(Futures.immediateFuture(\"hi\"))",
+            "        // Should be an error when we have full generics support, false-negative for now",
+            "        .transform(s -> { return s; }, executor);",
+            "  }",
+            "  private static ListenableFuture<String> fluentFutureUnsafe(Executor executor) {",
+            "    return FluentFuture",
+            "        .from(Futures.immediateFuture(\"hi\"))",
+            "        // Should be an error when we have full generics support, false-negative for now",
+            "        .transform(s -> { return null; }, executor);",
+            "  }",
+            "  private static ListenableFuture<@Nullable String> futuresTransform(Executor executor) {",
+            "    return Futures",
+            "        .transform(Futures.immediateFuture(\"hi\"), s -> { return null; }, executor);",
+            "  }",
+            "}")
+        .doTest();
+  }
+}

Original file line number	Diff line number	Diff line change
`@@ -28,6 +28,7 @@`
`28`	`28`	`import com.uber.nullaway.handlers.contract.ContractHandler;`
`29`	`29`	`import com.uber.nullaway.handlers.contract.fieldcontract.EnsuresNonNullHandler;`
`30`	`30`	`import com.uber.nullaway.handlers.contract.fieldcontract.RequiresNonNullHandler;`
	`31`	`+import com.uber.nullaway.handlers.temporary.FluentFutureHandler;`
`31`	`32`
`32`	`33`	`/** Utility static methods for the handlers package. */`
`33`	`34`	`public class Handlers {`
`@@ -75,6 +76,7 @@ public static Handler buildDefault(Config config) {`
`75`	`76`	`handlerListBuilder.add(new ContractCheckHandler(config));`
`76`	`77`	`}`
`77`	`78`	`handlerListBuilder.add(new LombokHandler(config));`
	`79`	`+ handlerListBuilder.add(new FluentFutureHandler());`
`78`	`80`
`79`	`81`	`return new CompositeHandler(handlerListBuilder.build());`
`80`	`82`	`}`