Add CheckReturnValue annotation

[vanniktech]

First draft on this.

So far I have only annotated subscribeWith() and test() methods.

I checked this against Error Prone and their check is able of picking up the RxJava annotation.

How do you feel if I extend BaseTypeAnnotations to test for @CheckReturnValue too?

Related #4878

[codecov-io]

Current coverage is 95.74% (diff: 100%)

Merging #4881 into 2.x will decrease coverage by 0.04%

@@                2.x      #4881   diff @@
==========================================
  Files           581        581          
  Lines         37212      37212          
  Methods           0          0          
  Messages          0          0          
  Branches       5601       5601          
==========================================
- Hits          35643      35628    -15   
- Misses          647        654     +7   
- Partials        922        930     +8   

Powered by Codecov. Last update 442c6ce...94571e5

[artem-zinnatullin]

Gooooooood. Afaik Findbugs also able to process annotation with this name correctly.

[artem-zinnatullin]

nit: static import makes it a little bit better

[vanniktech]

Yes I know however for RxJava it's common to not static import them (look at BackpressureSupport & SchedulerSupport) so I left it that way. I leave the final decision up to David.

[akarnokd]

It's fine as is.

[artem-zinnatullin]

And here

[vanniktech]

same as above

[vanniktech]

I quickly checked findbugs and it does not seem like they pick up our annotation since they have their own

[akarnokd]

It's fine as is.

[akarnokd]

Would you manually ignore methods like Disposable subscribe()?

[artem-zinnatullin]

@vanniktech see https://github.com/findbugsproject/findbugs/blob/264ae7baf890d2b347d91805c90057062b5dcb1e/findbugs/src/java/edu/umd/cs/findbugs/detect/BuildCheckReturnAnnotationDatabase.java#L120

[vanniktech]

@artem-zinnatullin weird somehow findbugs didn't pick it up though. Could be a configuration error on my side.

I'd argue that not annotating Diposable subscribe() is fine however I feel like Disposable subscribe(Consumer<T>) and the others should be annotated. I personally almost never use subscribeWith and prefer the variations with lambdas.

We're on the same point that static factory methods and operators such as flatMap, first(), last() etc should be annotated right? If so I'd apply those and then we can see which might need an annotation too.

[vanniktech]

@akarnokd added the @CheckReturnValue annotation to all public API methods of Flowable, Maybe, Single, Observable and Completable except for private methods, void methods and the Disposable subscribe() method. Is that okay now?

[akarnokd]

Yes.

/cc @JakeWharton