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 nullability annotations to io.reactivex.annotations interfaces. #5023

Merged
merged 1 commit into from
Jan 27, 2017

Conversation

tonycosentini
Copy link
Contributor

@tonycosentini tonycosentini commented Jan 27, 2017

(Addresses #4876)

This pull request annotates everything in the io.reactivex.annotations package with a new @NonNull annotation defined in RxJava. This doesn’t annotate everything, but I think it’s a good start.

In particular, when migrating a large codebase of RxJava 1.x code to 2.x, catching null returns in these interfaces was very difficult. At best, it’s noticed while migrating, otherwise it’s up to unit tests or a production crash to catch these.

With these new annotations, you get much better IDE support (once you tell IntelliJ about the new annotation):
screen shot 2017-01-27 at 2 30 18 pm
^ in this example, getPhoneNumber() is marked as @Nullable

In addition, support for this annotation can easily be added to static analysis tools like Infer, checker, or any other popular tool.

Open Questions

  • Does this even make sense to add? There seem to be some concerns in the issue.
  • The @NonNull annotation needs JavaDoc - I’m planning on following up with whatever is in existing NonNull annotations floating around unless anyone thinks otherwise.
  • There are no tests since this is essentially just metadata. Should there be something that enforces this? I'm not sure if there is any kind of lint tool that runs on this project, but one approach would be to write a check that enforces everything in specific packages in annotated. This would also help keep future changes annotated.
  • Is it worth adding this elsewhere? I find these function interfaces to be the easiest place to run into nullability problems, but I'm curious what others think. A simple, but kind of crazy brute force approach would be to require annotations on every public API.

@codecov-io
Copy link

codecov-io commented Jan 27, 2017

Current coverage is 95.63% (diff: 100%)

Merging #5023 into 2.x will increase coverage by 0.10%

@@                2.x      #5023   diff @@
==========================================
  Files           609        609          
  Lines         39379      39379          
  Methods           0          0          
  Messages          0          0          
  Branches       6025       6025          
==========================================
+ Hits          37616      37659    +43   
+ Misses          764        747    -17   
+ Partials        999        973    -26   

Powered by Codecov. Last update f53e029...929f0fb

@akarnokd akarnokd added this to the 2.0 backlog milestone Jan 27, 2017
@akarnokd
Copy link
Member

/cc @dlew, @vanniktech

Copy link
Collaborator

@vanniktech vanniktech left a comment

Choose a reason for hiding this comment

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

Looks good. I'd say this is for a first try just enough. Would not annotate more.

@dlew
Copy link

dlew commented Jan 27, 2017

This looks fine to me as a start.

@akarnokd
Copy link
Member

Great!

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

Successfully merging this pull request may close these issues.

5 participants