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

[akarnokd]

Closing via #5051 & #5055. Let us know if further annotations are necessary.

[tevjef]

I recently had an issue where a null List was accidentally passed into Observable.fromIterable(). Unfortunately that bug made it to production, but it would have been caught if the parameters are annotated.

@akarnokd @dlew I believe it would be worth it add nullability annotations every where ObjectHelper.requireNonNull(object, message) is used to guard execution.

This would be a massive change and it will definitely break compilation in some Kotlin code bases, mine included.

[akarnokd]

Recent IntelliJ infers most nullability annotations for me in Java:

and in Kotlin but it doesn't post a warning (see Kotlin):

[tevjef]

Thanks for the quick reply.

I haven't been able to get IntelliJ to infer nullability annotations. I've put together a quick example of my problem.

The post you linked seems to address the problem of consuming Java types in Kotlin, rather than here where Kotlin types are consumed by Java. RxJava's io.reactivex.annotations.NonNull is compatible with the Kotlin's compiler as such will cause a compiler error if a nullable Kotlin type is passed into a @NonNull Java parameter.

I apologize if this level of Kotlin support is outside of the scope of this project.

[akarnokd]

rather than here where Kotlin types are consumed by Java

I thought writing in Kotlin requires one to explicitly forfeit non-nullness. Have you been using this particular nullable List source elsewhere where it being null conveys additional information?

I apologize if this level of Kotlin support is outside of the scope of this project.

We could add the @NonNull annotations, but for the sake of any other library out there, I'd also prefer IntelliJ had the same nullness warning in a Kotlin file (officially) that is already available for Java.

[tevjef]

Have you been using this particular nullable List source elsewhere where it being null conveys additional information?

The project was recently converted from RxJava 1.x -> 2.x and from Java to Kotlin in a short time span and this bug went unnoticed. As far as I can tell, null does not convey additional information in the stream.

I thought writing in Kotlin requires one to explicitly forfeit non-nullness.

It's not necessarily the case if Java types are correctly annotated.

[lukaville]

@akarnokd Is there any reason why return type of Function interface is not annotated with @NonNull? BiFunction and Function3 - Function9 return types are annotated with @NonNull. Does it break some contract?

[akarnokd]

Nulls that can't end up in a sequence are allowed, for example, in groupBy key selector and distinct selector.

[lukaville]

Nulls that can't end up in a sequence are allowed, for example, in groupBy key selector and distinct selector.

What do you think about creating annotation @ReturnsNonNull and annotate all Function interface usages where the nullable value is not allowed? Unfortunately, it will require changes in static analysis tools. Another option is to create NonNullFunction interface with @NonNull return type but imho breaking API to fix this isn't worth it.

[akarnokd]

Java 6 doesn't allow annotating type arguments that way. You'd need to break binary compatibility to introduce all combinations of possible nullness to the interfaces and use sites.