-
Notifications
You must be signed in to change notification settings - Fork 299
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
Introduce FluentFutureHandler as a workaround for Guava Futures/FluentFuture #771
Conversation
Pull Request Test Coverage Report for Build #1105
💛 - Coveralls |
There was a problem hiding this 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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private static boolean isGuavaFunctionDotAppy(Symbol.MethodSymbol methodSymbol) { | |
private static boolean isGuavaFunctionDotApply(Symbol.MethodSymbol methodSymbol) { |
…ullable T> without generics support
96621fd
to
ad67028
Compare
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.
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'sFunction
to be ignoredwhen determining the correct overriding of the functional interface by a lambda.
After the fix, however, code such as:
will fail with an error about
Function::apply
having a@NonNull
result. Where nullability shouldactually depend on the parameter
T
of theListenableFuture<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
andcom.google.common.util.concurrent.AsyncFunction
to be e.g.Function<@Nullable T>
wheneverthese functional interfaces are implemented as a lambda expression passed to a list of specific
methods of
com.google.common.util.concurrent.FluentFuture
orcom.google.common.util.concurrent.Futures
. This is unsound, but permits the code example above topass NullAway checking, while strictly reducing the safety hole of NullAway versions prior to PR #765.