From 91d68098e3bc5123b737bc376ad1d226fca05188 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Wed, 14 Feb 2024 17:33:57 -0800 Subject: [PATCH] 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. --- .../com/uber/nullaway/AbstractConfig.java | 7 +++ .../main/java/com/uber/nullaway/Config.java | 7 +++ .../com/uber/nullaway/DummyOptionsConfig.java | 5 +++ .../nullaway/ErrorProneCLIFlagsConfig.java | 4 ++ .../com/uber/nullaway/handlers/Handlers.java | 2 +- .../temporary/FluentFutureHandler.java | 24 +++++++--- ...ayFunctionalInterfaceNullabilityTests.java | 45 +++++++++++++++++++ 7 files changed, 87 insertions(+), 7 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/AbstractConfig.java b/nullaway/src/main/java/com/uber/nullaway/AbstractConfig.java index 9303b9380c..3f236c7ce5 100644 --- 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 skippedLibraryModels; + protected ImmutableSet extraFuturesClasses; + /** --- JarInfer configs --- */ protected boolean jarInferEnabled; @@ -299,6 +301,11 @@ public boolean isSkippedLibraryModel(String classDotMethod) { return skippedLibraryModels.contains(classDotMethod); } + @Override + public ImmutableSet 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 index 8b0b084203..65374dcd95 100644 --- 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 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 index de423e3fab..a9694fc413 100644 --- 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 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 index 0240d77bec..6f2b578445 100644 --- 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 index 22792ddd5f..f82f343ffb 100644 --- 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 index 04ccd6b419..537e87968f 100644 --- 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,14 +37,25 @@ 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 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 futuresClassNames; + + public FluentFutureHandler(Config config) { + ImmutableSet.Builder 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) @@ -50,10 +63,9 @@ private static boolean isGuavaFunctionDotApply(Symbol.MethodSymbol methodSymbol) && 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 index 6406b2b733..25e8846537 100644 --- 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 ListenableFuture transform(ListenableFuture future, Function 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 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 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(); + } }