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

Add support for containing class fields to RxNullabilityPropagator #282

Open
lazaroclapp opened this issue Feb 26, 2019 · 1 comment
Open
Assignees

Comments

@lazaroclapp
Copy link
Collaborator

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)

@lazaroclapp
Copy link
Collaborator Author

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 of filter and the start of map, 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant