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

[dlew]

One of the larger tasks when transitioning from 1 to 2 is removing the use of null. Right now there's no automated way to do it, but if we had @NonNull then we'd be able to use static analysis to find where we're returning null when we shouldn't be.

I'm not sure the best way to go about it - if we should include jsr305, use Java 8's NonNull, create our own, or something else.

[akarnokd]

We can't do external dependencies like jsr305 and I can't find @NonNull or @NotNull in Java 8's public API. We could introduce our own if your tools support custom annotations for this purpose.

[dlew]

It looks like Java 8 doesn't actually include it - it's part of a checker framework. More details here.

Regardless, I'm specifically targeting IntelliJ's "Constant Conditions & Expressions" inspection and it looks like you can tell IntelliJ about which annotations should be considered non-null / nullable. So it would work if RxJava had its own @NonNull annotation included with the library (as long as developers are willing to do a little configuration on their end to accept it).

The only downside to not using jsr305 is that we can't use something like @ParametersAreNonnullByDefault which would be less verbose.

If people are on board with this, I could start implementing this (but it's a lot of busywork so I don't want to bother unless people want it).

[akarnokd]

Looks like there is an interest for new annotations, #4878, so it this has to be coordinated with that. I'm not sure about adding @NonNull to the functional interfaces though as it may add internal noise to RxJava. We could, however, annotate parameters and return types of the base reactive classes.

[dlew]

Hmm, I'm most concerned about the functional interfaces, actually. Those are the ones where the app may accidentally crash during execution unexpectedly.

Here's a contrived example:

void main() {
    Observable.just("Dan")
        .map(new Function<String, String>() {
            @Override
            public String apply(String s) throws Exception {
                return usuallyDependableMethod(s);
            }
        })
        .subscribe();
}

String usuallyDependableMethod(String name) {
    return (Math.random() < .95) ? "Hello " + name : null;
}

I can imagine a lot of circumstances where coders don't even realize that usuallyDependableMethod() could sometimes return null (when it's not such an obvious problem). That's where static analysis would really help out.

[Jawnnypoo]

Another +1 reason for this would be that Kotlin automatically changes variables to Not null as a language feature if they are annotated with @NonNull from JSR 305 along with the IntelliJ and Android equivalents.

https://kotlinlang.org/docs/reference/java-interop.html#nullability-annotations

Therefore, Kotlin users using RxJava would have the benefit of knowing that their values in something like onNext() are not null.

@akarnokd Can you give a reason for not being able to pull in JSR 305 or some equivalent? Just curious.

[akarnokd]

Historically, RxJava is kept to a minimum dependency: 0 in v1 and 1 in v2

[tonycosentini]

I agree @dlew - in a larger code base, it's pretty easy to run into issues like this and there's no way to catch them until runtime.

If it's annotated, a handful of static analysis tools and IDEs can leverage this at compile time (even if it's a custom annotation within the library).

[jschneider]

I have created a pull request with some changes:
#5051