Remove @NonNull annotation in Consumer method parameter

[ansman]

This fixes #5442

[ansman]

This should be safe to merge, see the discussion in #5216

[codecov]

Codecov Report

Merging #5447 into 2.x will decrease coverage by 0.08%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##                2.x    #5447      +/-   ##
============================================
- Coverage     96.07%   95.99%   -0.09%     
+ Complexity     5798     5792       -6     
============================================
  Files           631      631              
  Lines         41297    41297              
  Branches       5744     5744              
============================================
- Hits          39675    39641      -34     
- Misses          637      648      +11     
- Partials        985     1008      +23

Impacted Files	Coverage Δ	Complexity Δ
...l/operators/observable/ObservableFlatMapMaybe.java	84.96% <0%> (-11.12%)	2% <0%> (ø)
...nternal/operators/observable/ObservableCreate.java	88.88% <0%> (-11.12%)	2% <0%> (ø)
.../operators/observable/ObservableFlatMapSingle.java	88.8% <0%> (-5.98%)	2% <0%> (ø)
...ernal/operators/flowable/FlowableFlatMapMaybe.java	90.33% <0%> (-5.8%)	2% <0%> (ø)
.../operators/flowable/FlowableBlockingSubscribe.java	91.66% <0%> (-5.56%)	9% <0%> (-1%)
...al/operators/observable/ObservableSampleTimed.java	91.66% <0%> (-5%)	3% <0%> (ø)
...internal/operators/completable/CompletableAmb.java	94.91% <0%> (-3.39%)	10% <0%> (-1%)
...ternal/operators/completable/CompletableCache.java	96.96% <0%> (-3.04%)	23% <0%> (-1%)
...ex/internal/operators/maybe/MaybeTimeoutMaybe.java	94.11% <0%> (-2.95%)	2% <0%> (ø)
...main/java/io/reactivex/subjects/SingleSubject.java	95.23% <0%> (-2.39%)	38% <0%> (-1%)
... and 39 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3656b9b...f7394e8. Read the comment docs.

[ansman]

I'm curious, why did the code coverage decrease because of this?

[akarnokd]

There are unit tests that race for a certain code path inside the operators. Travis-CI lately has become 2-3x slower and often these code paths are no longer exercised. It is sometimes +/- 0.2%. If there is a generic drop unrelated to your changes, there is nothing to worry about.

[akarnokd]

Looks like this nullability thing is hitting many Kotlin users now (see #5448). What do you think about removing all nullability annotations from the functional types in an additional PR?

[svenjacobs]

What do you think about removing all nullability annotations from the functional types

@akarnokd To add my two cents ;) I think this is a good idea. There is nothing wrong with nullability annotations as long as the code adheres to this contract - meaning null should never be passed where @NonNull is declared.

[akarnokd]

Java 6 doesn't enforce this and only Java 8+ allows annotating the type arguments themselves:

public Observable<T> doOnNext(Consumer<@NonNull T> onNext);

public Completable doOnEvent(Consumer<@Nullable Throwable> onEvent);

[ansman]

@akarnokd I'd be happy to add a PR for removing them if you wish.
Should all annotations in the io.reactivex.functions package be removed then?

[akarnokd]

Looks like Function is the next to be adjusted: #5448 indicates it's return type may be null with groupBy. I'd delay removing all annotations in case Kotlin comes to senses because I doubt RxJava is the only library that has inconsistent nullability annotation usage.

[svenjacobs]

Looks like Function is the next to be adjusted: #5448 indicates it's return type may be null with groupBy. I'd delay removing all annotations in case Kotlin comes to senses because I doubt RxJava is the only library that has inconsistent nullability annotation usage.

@akarnokd What do you mean by "Kotlin comes to senses"? This is by language design and I doubt it will change. I don't understand why we have to discuss this and why it's not clear that as a library developer you should not put null into an argument that is annotated by @NonNull. As I said before this is not a Kotlin-only problem and certainly will affect Java developers too as here clearly the contract is broken.

[akarnokd]

RxJava is a Java 6 library and thus we are bound by the capabilities of Java language level 6. We are also trying to be nice with other languages but that has limits. The best we can do is to remove nullability annotations from controversial functional types that come up.

What I mean by "senses" is it would be Kotlin's responsibility to work with native Java libraries and provide workaround since Java's type system doesn't have a primary notion of nullability; I just leave off/replace the annotations to satisfy some static analysis tools if necessary. The fact that just by upgrading to 1.1.3 compatiblity broke indicates there has to be work done on Kotlin's part.

[svenjacobs]

I'm aware that Java 6 cannot check nullability constraints during compile time. However this is not a language issue. IDEs like Intellij IDEA will analyze these annotations and give hints. So as a developer you might think that the argument can never be null but in fact it can. So what is the point of this annotation then?

[akarnokd]

See #5151. If we could support callsite nullability, we'd do it but we can't. So either annotate for the majority uses or not annotate at all through the common functional interfaces. Adding/removing annotations is considered binary compatible thus any other means, such as alternate interfaces has to be added and accompanied by new methods which also need to be named differently to avoid lambda ambiguity.

IDEs like Intellij IDEA will analyze these annotations and give hints

The keyword is "hints", the annotations were mostly working for Java/Android but Kotlin no longer considers them as hints. If there was a "I take my chances" type/hint or you could disable the RxJava annotations being considered, that would make things simpler.

[JakeWharton]

If they're just hints I'd prefer them entirely removed. They should either be 100% accurate or entirely absent. We have Java tools which take these annotations as being 100% accurate and having anything less than that in actuality will cause bugs.

…

On Tue, Jun 27, 2017, 9:52 AM David Karnok ***@***.***> wrote: See #5151 <#5151>. If we could support callsite nullability, we'd do it but we can't. So either annotate for the majority uses or not annotate at all through the common functional interfaces. Adding/removing annotations is considered binary compatible thus any other means, such as alternate interfaces has to be added and accompanied by new methods which also need to be named differently to avoid lambda ambiguity. IDEs like Intellij IDEA will analyze these annotations and give hints The keyword is "hints", the annotations were mostly working for Java/Android but Kotlin no longer considers them as hints. If there was a "I take my chances" type/hint or you could disable the RxJava annotations being considered, that would make things simpler. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#5447 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEEEQnyUxJd1NV_GGpQ0A-QfcRtdZ41ks5sIRcRgaJpZM4OGSgK> .

[akarnokd]

AFAIK, the problem with the non-annotated interfaces was that IntelliJ/Kotlin inferred all of them as nullable T and caused inconveniences.

[JakeWharton]

Non-annotated types will be inferred as a "platform type" which has ambiguous nullability. This allows the user to specify it as nullable or non-nullable (with a runtime check in the latter case) based on what they know of the data and operator.

…

On Tue, Jun 27, 2017, 10:32 AM David Karnok ***@***.***> wrote: AFAIK, the problem with the non-annotated interfaces was that IntelliJ/Kotlin inferred all of them as nullable T and caused inconveniences. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#5447 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEEEQKPnSkVzGKmakiDTs2nfpFjtxyXks5sISBvgaJpZM4OGSgK> .

[akarnokd]

So far, these were the cases that require the removal of annotations:

1. Consumer may be called with null (i.e.,doOnEvent)
2. BiConsumer may be called with null (i.e., subscribe)
3. Function may return null in groupBy's key selector

The other functional types are never called with null and are not allowed to return null. Option 3 has no PR yet.

[ansman]

@akarnokd I added a PR for 3 in #5449

[akarnokd]

Thanks @ansman !

[svenjacobs]

The fact that just by upgrading to 1.1.3 compatiblity broke indicates there has to be work done on Kotlin's part.

It just indicates that there were "holes" in the type system which have been fixed ;) Anyway thanks to everybody that this issue has been resolved!