Support for extra Futures classes (#909)

This allows for a list of classes to be passed in via a command-line argument `-XepOpt:NullAway:ExtraFuturesClasses`. Such classes will be treated equivalently to built-in Guava Futures classes via the `FluentFutureHandler`. This is a follow-up to #771.
uber · Feb 15, 2024 · 91d6809 · 91d6809
1 parent 084bd96
commit 91d6809
Show file tree

Hide file tree

Showing 7 changed files with 87 additions and 7 deletions.
diff --git a/nullaway/src/main/java/com/uber/nullaway/AbstractConfig.java b/nullaway/src/main/java/com/uber/nullaway/AbstractConfig.java
@@ -103,6 +103,8 @@ public abstract class AbstractConfig implements Config {
 
   protected ImmutableSet<String> skippedLibraryModels;
 
+  protected ImmutableSet<String> extraFuturesClasses;
+
   /** --- JarInfer configs --- */
   protected boolean jarInferEnabled;
 
@@ -299,6 +301,11 @@ public boolean isSkippedLibraryModel(String classDotMethod) {
     return skippedLibraryModels.contains(classDotMethod);
   }
 
+  @Override
+  public ImmutableSet<String> getExtraFuturesClasses() {
+    return extraFuturesClasses;
+  }
+
   @AutoValue
   abstract static class MethodClassAndName {
 

diff --git a/nullaway/src/main/java/com/uber/nullaway/Config.java b/nullaway/src/main/java/com/uber/nullaway/Config.java
@@ -263,6 +263,13 @@ public interface Config {
    */
   boolean isSkippedLibraryModel(String classDotMethod);
 
+  /**
+   * Gets the set of classes that should be treated as equivalent to a Guava fluent futures class.
+   *
+   * @see com.uber.nullaway.handlers.temporary.FluentFutureHandler
+   */
+  Set<String> getExtraFuturesClasses();
+
   // --- JarInfer configs ---
 
   /**

diff --git a/nullaway/src/main/java/com/uber/nullaway/DummyOptionsConfig.java b/nullaway/src/main/java/com/uber/nullaway/DummyOptionsConfig.java
@@ -190,6 +190,11 @@ public boolean isSkippedLibraryModel(String classDotMethod) {
     throw new IllegalStateException(ERROR_MESSAGE);
   }
 
+  @Override
+  public Set<String> getExtraFuturesClasses() {
+    throw new IllegalStateException(ERROR_MESSAGE);
+  }
+
   @Override
   public boolean isJarInferEnabled() {
     throw new IllegalStateException(ERROR_MESSAGE);

diff --git a/nullaway/src/main/java/com/uber/nullaway/ErrorProneCLIFlagsConfig.java b/nullaway/src/main/java/com/uber/nullaway/ErrorProneCLIFlagsConfig.java
@@ -75,6 +75,8 @@ final class ErrorProneCLIFlagsConfig extends AbstractConfig {
 
   static final String FL_SKIP_LIBRARY_MODELS = EP_FL_NAMESPACE + ":IgnoreLibraryModelsFor";
 
+  static final String FL_EXTRA_FUTURES = EP_FL_NAMESPACE + ":ExtraFuturesClasses";
+
   /** --- JarInfer configs --- */
   static final String FL_JI_ENABLED = EP_FL_NAMESPACE + ":JarInferEnabled";
 
@@ -220,6 +222,8 @@ final class ErrorProneCLIFlagsConfig extends AbstractConfig {
           "Invalid -XepOpt:" + FL_SUPPRESS_COMMENT + " value. Comment must be single line.");
     }
     skippedLibraryModels = getFlagStringSet(flags, FL_SKIP_LIBRARY_MODELS);
+    extraFuturesClasses = getFlagStringSet(flags, FL_EXTRA_FUTURES);
+
     /* --- JarInfer configs --- */
     jarInferEnabled = flags.getBoolean(FL_JI_ENABLED).orElse(false);
     jarInferUseReturnAnnotations = flags.getBoolean(FL_JI_USE_RETURN).orElse(false);

diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/Handlers.java b/nullaway/src/main/java/com/uber/nullaway/handlers/Handlers.java
@@ -80,7 +80,7 @@ public static Handler buildDefault(Config config) {
       handlerListBuilder.add(new ContractCheckHandler(config));
     }
     handlerListBuilder.add(new LombokHandler(config));
-    handlerListBuilder.add(new FluentFutureHandler());
+    handlerListBuilder.add(new FluentFutureHandler(config));
 
     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
@@ -1,10 +1,12 @@
 package com.uber.nullaway.handlers.temporary;
 
+import com.google.common.collect.ImmutableSet;
 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.Config;
 import com.uber.nullaway.NullabilityUtil;
 import com.uber.nullaway.Nullness;
 import com.uber.nullaway.handlers.BaseNoOpHandler;
@@ -35,25 +37,35 @@ 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 ImmutableSet<String> FUTURES_CLASS_NAMES =
+      ImmutableSet.of(
+          "com.google.common.util.concurrent.FluentFuture",
+          "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 final ImmutableSet<String> futuresClassNames;
+
+  public FluentFutureHandler(Config config) {
+    ImmutableSet.Builder<String> builder = ImmutableSet.builder();
+    builder.addAll(FUTURES_CLASS_NAMES);
+    builder.addAll(config.getExtraFuturesClasses());
+    futuresClassNames = builder.build();
+  }
+
   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) {
+  private boolean isFluentFutureIncludeListMethod(Symbol.MethodSymbol methodSymbol) {
     Name className = methodSymbol.enclClass().flatName();
-    return (className.contentEquals(FLUENT_FUTURE_CLASS_NAME)
-            || className.contentEquals(FUTURES_CLASS_NAME))
+    return futuresClassNames.contains(className.toString())
         && Arrays.stream(FLUENT_FUTURE_INCLUDE_LIST_METHODS)
             .anyMatch(s -> methodSymbol.name.contentEquals(s));
   }

diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayFunctionalInterfaceNullabilityTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayFunctionalInterfaceNullabilityTests.java
@@ -1,5 +1,6 @@
 package com.uber.nullaway;
 
+import java.util.Arrays;
 import org.junit.Test;
 
 public class NullAwayFunctionalInterfaceNullabilityTests extends NullAwayTestsBase {
@@ -107,4 +108,48 @@ public void futuresFunctionLambdas() {
             "}")
         .doTest();
   }
+
+  @Test
+  public void extraFuturesClassesLambda() {
+    makeTestHelperWithArgs(
+            Arrays.asList(
+                "-d",
+                temporaryFolder.getRoot().getAbsolutePath(),
+                "-XepOpt:NullAway:AnnotatedPackages=com.uber",
+                "-XepOpt:NullAway:ExtraFuturesClasses=com.uber.extra.MyFutures"))
+        .addSourceLines(
+            "MyFutures.java",
+            "package com.uber.extra;",
+            "import com.google.common.base.Function;",
+            "import com.google.common.util.concurrent.Futures;",
+            "import com.google.common.util.concurrent.ListenableFuture;",
+            "import java.util.concurrent.Executor;",
+            "public class MyFutures {",
+            "  public static <V> ListenableFuture<V> transform(ListenableFuture<V> future, Function<V, V> function, Executor executor) {",
+            "    return Futures.transform(future, function, executor);",
+            "  }",
+            "}")
+        .addSourceLines(
+            "TestMyFutures.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 void takeFn(Function<String, String> f) { }",
+            "  private static void passToAnnotatedFunction() {",
+            "    // Normally we get an error since Guava Functions are modeled to have a @NonNull return",
+            "    // BUG: Diagnostic contains: returning @Nullable expression from method",
+            "    takeFn(s -> { return null; });",
+            "  }",
+            "  private static void passToExtraFuturesClass(ListenableFuture<String> f, Executor e) {",
+            "    // here we do not expect an error since MyFutures is in the extra futures classes",
+            "    com.uber.extra.MyFutures.transform(f, u -> { return null; }, e);",
+            "  }",
+            "}")
+        .doTest();
+  }
 }