Feature/#4876 nonnull annotations by jschneider · Pull Request #5051 · ReactiveX/RxJava

jschneider · 2017-02-02T20:14:04Z

Starting to add @nonnull and @nullable annotations.

I decided to add a compileOnly dependency to findbugs:jsr305 artifact.

We could also use "io.reactivex.annotations.NonNull" which already exists. But there is no "io.reactivex.annotations.Nullable" annotation which is at least as important in my experience.

Since these are only annotations, it is no problem when the class files are missing at compile time (JLS 9.6.1.2 Retention).

I have started to add annotations in the Scheduler and RxJavaPlugins.
The test RxJavaPlugins contains some invalid checks using null (lines 1353 and following).

This pull request is work in progress and should be discussed.

Questions so far:

use jsr305 (compileOnly) or existing annotation in io.reactivex?
(if not using jsr305): Use own @nullable annotation or try to get one added to "io.reactivex"? Or skip these completely (which misses the point of the static code analysis)
Why has there been tests calling the RxJavaPlugins.on*Scheduler with null arguments? Can these be removed securely?

akarnokd · 2017-02-02T20:49:16Z

Maybe it wasn't well articulated, but based on #5023 I thought we try with just the functional interfaces and come back later if it was not enough.

I decided to add a compileOnly dependency to findbugs:jsr305 artifact.

We rather use our own annotations as most tools allow customizing what annotations to consider.

Expect futher comments inline.

akarnokd

All annotations should be RxJava's own.

akarnokd · 2017-02-02T20:52:23Z

src/main/java/io/reactivex/Scheduler.java

     */
-    public Disposable scheduleDirect(Runnable run, long delay, TimeUnit unit) {
+    @Nonnull
+    public Disposable scheduleDirect(@Nonnull Runnable run, long delay, TimeUnit unit) {


TimeUnits should also receive our own non-null annotation.

akarnokd · 2017-02-02T20:55:00Z

src/main/java/io/reactivex/plugins/RxJavaPlugins.java

    @SuppressWarnings("rawtypes")
+    @Nullable
    static volatile Function<Single, Single> onSingleAssembly;
-


Please don't remove these empty lines.

akarnokd · 2017-02-02T20:56:58Z

src/main/java/io/reactivex/plugins/RxJavaPlugins.java

     */
    public static void setOnCompletableSubscribe(
-            BiFunction<Completable, CompletableObserver, CompletableObserver> onCompletableSubscribe) {
+      @Nullable BiFunction<Completable, CompletableObserver, CompletableObserver> onCompletableSubscribe) {


Indentation

akarnokd · 2017-02-02T20:57:18Z

src/main/java/io/reactivex/plugins/RxJavaPlugins.java

    @SuppressWarnings("rawtypes")
    public static void setOnObservableSubscribe(
-            BiFunction<Observable, Observer, Observer> onObservableSubscribe) {
+      @Nullable BiFunction<Observable, Observer, Observer> onObservableSubscribe) {


Please adjust indentation.

akarnokd · 2017-02-02T21:01:35Z

src/test/java/io/reactivex/internal/operators/flowable/FlowableReplayTest.java

            this.mockDisposable = mockDisposable;
        }

+        @javax.annotation.Nonnull


I don't think it is worth updating the unit tests.

Idea did it on itself ;-)
If it is ok, I'd just leave it.

akarnokd · 2017-02-02T21:03:23Z

src/test/java/io/reactivex/plugins/RxJavaPluginsTest.java

-            assertNull(RxJavaPlugins.onSingleScheduler(null));
+            //this is not valid, because io.reactivex.functions.Function.apply() does not allow null parameters
+            if (false) {
+                assertNull(RxJavaPlugins.onComputationScheduler(null));


Instead of "disabling" this, change it to use the thin scheduler below and assertSame instead of null.

I removed the null checks completely. As you said, below are the assertSame checks. This should be good enough.

jschneider · 2017-02-03T07:20:31Z

Thanks for the feedback. I will improve the pull request further.

akarnokd · 2017-02-03T09:04:29Z

src/main/java/io/reactivex/annotations/Nullable.java

+import static java.lang.annotation.ElementType.PARAMETER;
+import static java.lang.annotation.RetentionPolicy.CLASS;
+
+@Documented


Could you add a short javadoc to this and @NonNull while you are at it?

jschneider mentioned this pull request Feb 2, 2017

2.x: Add @NonNull annotation for Function/Predicate/etc. #4876

Closed

akarnokd requested changes Feb 2, 2017

View reviewed changes

akarnokd added 2.x Cleanup labels Feb 3, 2017

akarnokd added this to the 2.1 milestone Feb 3, 2017

akarnokd added the Enhancement label Feb 3, 2017

akarnokd approved these changes Feb 3, 2017

View reviewed changes

jschneider force-pushed the feature/#4876-nonnull-annotations branch 4 times, most recently from 6717518 to 8e8978c Compare February 3, 2017 10:06

jschneider added 3 commits February 3, 2017 11:10

add @nullable annotations to RxJavaPlugins

eba7ff8

add @nonnull annotations to schedulers

6dff800

javadoc for NonNull/Nullable annotations

ab7e287

jschneider force-pushed the feature/#4876-nonnull-annotations branch from 8e8978c to ab7e287 Compare February 3, 2017 10:10

akarnokd merged commit 8720828 into ReactiveX:2.x Feb 3, 2017

jschneider deleted the feature/#4876-nonnull-annotations branch February 3, 2017 11:21

akarnokd mentioned this pull request Feb 8, 2017

2.0.6 release preparations #5083

Closed

akarnokd mentioned this pull request Mar 3, 2017

Nullability Annotations #5151

Closed

Conversation

jschneider commented Feb 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

akarnokd commented Feb 2, 2017

Uh oh!

akarnokd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jschneider commented Feb 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jschneider commented Feb 2, 2017 •

edited

Loading