2.x BiConsumer nullability annotations violated

[davidschreiber]

RxJava 2.0.7

The BiConsumer interface has both parameters of its accept() method marked as @NonNull. However, when using the BiConsumer interface as result callback for a Single, the received parameter values are mutually exclusive being a non-null value and the other one being null. Although it seems to make sense in use, it violates the interface annotations and makes usage, at least, confusing.

Single.just("Some string")
    .subscribe(new BiConsumer<String, Throwable>() {
        @Override
        public void accept(@io.reactivex.annotations.NonNull String s, @io.reactivex.annotations.NonNull Throwable throwable) throws Exception {
            assert s != null;
            assert throwable == null;
        }
    });

Single.<String>error(new RuntimeException())
    .subscribe(new BiConsumer<String, Throwable>() {
        @Override
        public void accept(@io.reactivex.annotations.NonNull String s, @io.reactivex.annotations.NonNull Throwable throwable) throws Exception {
            assert s == null;
            assert throwable != null;
        }
    });

[akarnokd]

Indeed, those use cases should override the default annotation with @Nullable. Would you like to submit a PR?

[bloderxd]

@akarnokd I can submit a PR if @davidschreiber can't, but what do you suggest? An exclusive BiConsumer for Single ?

[akarnokd]

That would be a breaking change. I see 3 options:

• remove annotations from BiConsumer,
• change annotations on BiConsumer to @Nullable - may introduce a lot of warnings,
• Do nothing and have your IDE suppress/ignore that warning.

[bloderxd]

Well I don't think it's a breaking change, it's just an interface extending BiConsumer example:

  public void biconsumerWithCorrectNullableAnnotationInThrowable() {
        Single.just(1).subscribe(new SingleBiConsumer<Integer, Throwable>() {
            @Override
            public void accept(@Nullable Integer integer, @Nullable Throwable throwable) throws Exception {
                assertNull(throwable);
            }
        });
    }

What do you think?

[akarnokd]

Yes, you can simply delete the annotations or ignore them in your code.

[bloderxd]

Yes I know, but sometimes it's important to let developers know about property nullability, but ok thank you about your suggestions.

[PaulWoitaschek]

With kotlin it's actually more of an issue because if you create a biConsumer subclass it will take both parameters NonNull and throw an exception at runtime.

I think the correct solution here is to remove the annotation (makes sense, sometimes it's nullable, sometimes it it's nonNull, )

[akarnokd]

Closing via #5257