NullAway
NullAway copied to clipboard
Add support for containing class fields to `RxNullabilityPropagator`
This is to track a known limitation in handling fields from the containing context when dealing with nullability propagation across RxJava filter and map operations.
Here is an illustrative test case that currently produces nullness issues but should be safe:
private static class FieldFromContext {
@Nullable public NullableContainer foo;
private Observable<String> filterThenMapTwoGets(Observable<Void> observable) {
return observable
.filter(unit -> foo != null && foo.get() != null)
.map(t -> foo.get().toString());
}
}
(add to RxSupportNegativeCases.java
)
This is way trickier than it seems at first, see #305 :
One potential issue I see with this change, is that, while it is likely to work for
filter(...).map(...)
it's aggressively thread unsafe and dangerous in a more general case.
Consider:
filter(s -> ...).delay(1000).map(s -> ...)
It's safe to assume the values of fields rooted at
s
haven't changed in between the end offilter
and the start ofmap
, since they are "internal" to the stream that got delayed. However, if those lambdas access fields of the containing class, those can easily change between those two program points.
We do something similar for captured locals from the enclosing context (see here), but, in that case, it's actually safe since only effectively final locals from enclosing contexts can be accessed within a lambda. With class fields, this is not true at all, and mutable fields can be accessed, leading to the problem above.
NullAway doesn't attempt to be fully sound with respect to concurrency, but this seems like the kind of optimistic treatment that can easily cause us to miss bugs in the future, so I'd want to make sure there is enough value in supporting this case.