Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 9 commits into from
Jun 21, 2023

Conversation

lazaroclapp
Copy link
Collaborator

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.

@lazaroclapp lazaroclapp requested a review from msridhar June 21, 2023 22:06
@coveralls
Copy link

coveralls commented Jun 21, 2023

Pull Request Test Coverage Report for Build #1105

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 2 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.03%) to 92.79%

Files with Coverage Reduction New Missed Lines %
../nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPath.java 1 96.32%
../nullaway/src/main/java/com/uber/nullaway/handlers/Handlers.java 1 96.43%
Totals Coverage Status
Change from base Build #1104: 0.03%
Covered Lines: 5611
Relevant Lines: 6047

💛 - Coveralls

Copy link
Collaborator

@msridhar msridhar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One typo fix and this should be good to land. Approving assuming the typo will be fixed :-)

This is really a case where we need much deeper generics support, and we are just not there yet. Hopefully we can get closer soon.

I would ideally like to do perf benchmarking here, and I can't wait until #770 lands to make it easy! For now, based on reading the code I don't think it should have much perf impact, so we can go ahead and land.

"catching", "catchingAsync", "transform", "transformAsync"
};

private static boolean isGuavaFunctionDotAppy(Symbol.MethodSymbol methodSymbol) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private static boolean isGuavaFunctionDotAppy(Symbol.MethodSymbol methodSymbol) {
private static boolean isGuavaFunctionDotApply(Symbol.MethodSymbol methodSymbol) {

@lazaroclapp lazaroclapp force-pushed the lazaro_fluent_future_function_hack_handler branch from 96621fd to ad67028 Compare June 21, 2023 22:26
@lazaroclapp lazaroclapp merged commit 37f4bb8 into master Jun 21, 2023
@lazaroclapp lazaroclapp deleted the lazaro_fluent_future_function_hack_handler branch June 21, 2023 22:33
msridhar added a commit to msridhar/NullAway that referenced this pull request Jul 18, 2023
msridhar added a commit to msridhar/NullAway that referenced this pull request Jul 18, 2023
msridhar added a commit to msridhar/NullAway that referenced this pull request Jul 19, 2023
msridhar added a commit to msridhar/NullAway that referenced this pull request Jul 19, 2023
msridhar added a commit to msridhar/NullAway that referenced this pull request Jul 19, 2023
msridhar added a commit that referenced this pull request Feb 15, 2024
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants