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 85e10e644a..606a7bb847 100644 --- 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 new file mode 100644 index 0000000000..04ccd6b419 --- /dev/null +++ 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. + * + *

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} 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. + * + *

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 new file mode 100644 index 0000000000..6406b2b733 --- /dev/null +++ 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 extends Function {", + " @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 nf) { }", + " private static void takesNonNullableFunction(Function 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 nf) { }", + " private static void takesNonNullableFunction(Function 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 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 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(); + } +}