Revisit nullability annotations #864

sdeleuze · 2017-09-15T18:29:39Z

WIP please do not merge yet, I have to review it with JetBrains, see pending remark and questions.

This commit introduces following changes:

It adds @NonNull and @Nullable annotations which allows to apply
null-safety semantics on a specific type like JSR 305 but allows to
target generic type arguments.
@NonNullApi does not apply to ElementType.TYPE anymore
Nullability annotations are now also applied to generic type arguments,
varargs and array elements.
Package is now reactor.util.annotation
Add null-safety reference documentation

sdeleuze · 2017-09-19T14:11:25Z

@smaldini @simonbasle This pull request is now ready to be reviewed.

Setting default T nullability at class level as proposed by Stéphane is not supported yet, so better to use the more verbose form on each method for now. If supported later we should be able to define something like public abstract class Mono<T extends @NonNull Object>.

simonbasle · 2017-09-19T15:44:35Z

I'm currently reviewing, seeing a few things to change so don't merge it yet

simonbasle

first pass, didn't review whole Flux and Mono but all the rest

simonbasle · 2017-09-19T15:31:45Z

reactor-core/src/main/java/reactor/core/publisher/ConnectableFluxOnAssembly.java

@@ -33,7 +34,7 @@
 * <p>
 * The operator sanitizes the stacktrace and removes noisy entries such as:
 * <ul>
- * <li>java.lang.Thread entries</li>
+ * <li>java.annotation.Thread entries</li>


bad search-and-replace (IntelliJ tends to be too eager when refactoring, I always check the "occurrences in strings and comments" and most of the time reject that category from the refactor altogether)

simonbasle · 2017-09-19T15:33:33Z

reactor-core/src/main/java/reactor/core/Scannable.java

@@ -361,7 +361,7 @@ default String operatorName() {
 	 *
 	 */
 	@Nullable
-	default <T> T scan(Attr<T> key) {
+	default <T> T scan(Attr<@NonNull T> key) {


I think Scannable.Attr generic's shouldn't be marked as @NonNull, as some attributes have a null default.

simonbasle · 2017-09-19T15:36:30Z

reactor-core/src/main/java/reactor/core/publisher/DrainUtils.java

@@ -172,7 +173,7 @@
 	 * @param instance the parent instance of the requested field
 	 * @param isCancelled callback to detect cancellation
 	 */
-	public static <T, F> void postComplete(CoreSubscriber<? super T> actual,
+	public static <T, F> void postComplete(CoreSubscriber<? super @NonNull T> actual,
 			Queue<T> queue,


unlike other methods, queue and field here are not annotated. That said, I don't think AtomicLongFieldUpdater's T should be annotated (similar to Class<T>)

simonbasle · 2017-09-19T15:37:51Z

reactor-core/src/main/java/reactor/core/publisher/FluxCallableOnAssembly.java

@@ -29,7 +29,7 @@
 * Note that getting a stacktrace is a costly operation.
 * <p>
 * The operator sanitizes the stacktrace and removes noisy entries such as: <ul>
- * <li>java.lang.Thread entries</li> <li>method references with source line of 1 (bridge
+ * <li>java.annotation.Thread entries</li> <li>method references with source line of 1 (bridge


overeager search and replace

simonbasle · 2017-09-19T15:38:31Z

reactor-core/src/main/java/reactor/core/publisher/FluxOnAssembly.java

@@ -37,7 +37,7 @@
 * <p>
 * The operator sanitizes the stacktrace and removes noisy entries such as:
 * <ul>
- * <li>java.lang.Thread entries</li>
+ * <li>java.annotation.Thread entries</li>


overeager search and replace

simonbasle · 2017-09-19T16:07:41Z

reactor-test/src/main/java/reactor/test/publisher/TestPublisher.java


 	/**
 	 * Send 1 {@link Subscriber#onNext(Object) onNext} signal to the subscribers.
 	 *
 	 * @param value the item to emit (can be null if the relevant {@link Violation} is push)
 	 * @return this {@link TestPublisher} for chaining.
 	 */
-	public abstract TestPublisher<T> next(@Nullable T value);
+	public abstract TestPublisher<@NonNull T> next(@Nullable T value);


as sometimes value can be null, should probably not be annotated @NonNull

simonbasle · 2017-09-19T16:08:03Z

reactor-test/src/main/java/reactor/test/publisher/TestPublisher.java

@@ -176,7 +177,7 @@
 	 * @see #next(Object) next
 	 */
 	@SafeVarargs
-	public final TestPublisher<T> next(@Nullable T first, T... rest) {
+	public final TestPublisher<@NonNull T> next(@Nullable T first, T @Nullable ... rest) {


same as for next, can be containing nulls

simonbasle · 2017-09-19T16:08:36Z

reactor-test/src/main/java/reactor/test/publisher/TestPublisher.java

@@ -194,7 +195,7 @@
 	 * @see #complete() complete
 	 */
 	@SafeVarargs
-	public final TestPublisher<T> emit(T... values) {
+	public final TestPublisher<@NonNull T> emit(T @NonNull ... values) {


same as for next can be containing nulls, values are nullable

simonbasle · 2017-09-19T16:09:04Z

reactor-test/src/test/java/reactor/test/StepVerifierAssertionsTests.java

@@ -158,7 +158,7 @@ public void assertNotDroppedErrorsFailureOneDrop() {
 			fail("expected an AssertionError");
 		}
 		catch (AssertionError ae) {


overeager search and replace all over the class (revert all changes)

simonbasle · 2017-09-19T16:09:25Z

reactor-test/src/test/java/reactor/test/StepVerifierTests.java

@@ -371,7 +371,7 @@ public void errorMatchesInvalid() {
 		            .expectNext("foo")
 		            .expectErrorMatches(t -> t instanceof IllegalStateException)
 		            .verify())
-	            .withMessage("expectation \"expectErrorMatches\" failed (predicate failed on exception: java.lang.IllegalArgumentException)");
+	            .withMessage("expectation \"expectErrorMatches\" failed (predicate failed on exception: java.annotation.IllegalArgumentException)");


overeager search and replace (revert all changes)

simonbasle

finished review with Flux and Mono

simonbasle · 2017-09-19T16:44:23Z

reactor-core/src/main/java/reactor/core/publisher/Flux.java

@@ -5459,7 +5460,7 @@ public int getPrefetch() {
 	 *
 	 * @return a reduced {@link Flux}
 	 */
-	public final Mono<T> reduce(BiFunction<T, T, T> aggregator) {
+	public final Mono<@NonNull T> reduce(BiFunction<T, T, T> aggregator) {


T, T, T not annotated NonNull

simonbasle · 2017-09-19T16:46:48Z

reactor-core/src/main/java/reactor/core/publisher/Flux.java

@@ -6841,7 +6842,7 @@ public final void subscribe(Subscriber<? super T> actual) {
 	 *
 	 * @return a new {@link Flux} that wait for source completion then emits from the supplied {@link Mono}
 	 */
-	public final <V> Mono<V> then(Mono<V> other) {
+	public final <V> Mono<@NonNull V> then(Mono<V> other) {


parameter Mono<@NonNull T> other ?

simonbasle · 2017-09-19T16:50:43Z

reactor-core/src/main/java/reactor/core/publisher/Flux.java

@@ -7814,7 +7815,7 @@ public String toString() {
 	 *
 	 * @return the mono representing that Flux
 	 */
-	static <T> Mono<T> convertToMono(Callable<T> supplier) {
+	static <T> Mono<@NonNull T> convertToMono(Callable<@NonNull T> supplier) {


here the Callable can return null

simonbasle · 2017-09-19T16:51:13Z

reactor-core/src/main/java/reactor/core/publisher/Flux.java

@@ -7834,7 +7835,7 @@ public String toString() {
 	}

 	@SafeVarargs
-	static <I> Flux<I> merge(int prefetch, boolean delayError, Publisher<? extends I>... sources) {
+	static <I> Flux<@NonNull I> merge(int prefetch, boolean delayError, Publisher<? extends I> @NonNull ... sources) {


missing nonnull on ? extends I

simonbasle · 2017-09-19T16:52:52Z

reactor-core/src/main/java/reactor/core/publisher/Mono.java

@@ -320,7 +321,7 @@
 	 *
 	 * @return A {@link Mono}.
 	 */
-	public static <T> Mono<T> fromCallable(Callable<? extends T> supplier) {
+	public static <T> Mono<@NonNull T> fromCallable(Callable<? extends @NonNull T> supplier) {


fromCallable now accepts callable returning null

simonbasle · 2017-09-19T16:56:49Z

reactor-core/src/main/java/reactor/core/publisher/Mono.java

@@ -464,7 +465,7 @@
 	 *
 	 * @return a {@link Mono}.
 	 */
-	public static <T> Mono<T> justOrEmpty(@Nullable Optional<? extends T> data) {
+	public static <T> Mono<@NonNull T> justOrEmpty(@Nullable Optional<? extends T> data) {


would Optional's <T> need a Nullable? (but maybe Optional is not concerned as a core java type?)

I have indeed chosen to keep Optional generic type argument as it is because it is a system type.

simonbasle · 2017-09-19T17:00:26Z

reactor-core/src/main/java/reactor/core/publisher/Mono.java

@@ -1624,7 +1625,7 @@ public T block(Duration timeout) {
 	 * @see #materialize()
 	 * @see Signal
 	 */
-	public final Mono<T> doOnEach(Consumer<? super Signal<T>> signalConsumer) {
+	public final Mono<@NonNull T> doOnEach(Consumer<? super @NonNull Signal<@NonNull T>> signalConsumer) {


doubt on Signal's <T> non-nullability (consider the case of a Signal.complete(), its T get() method would return null)

simonbasle · 2017-09-19T17:02:07Z

reactor-core/src/main/java/reactor/core/publisher/Mono.java

@@ -1717,7 +1718,7 @@ public T block(Duration timeout) {
 	 *
 	 * @return a new {@link Mono}
 	 */
-	public final Mono<T> doOnSubscribe(Consumer<? super Subscription> onSubscribe) {
+	public final Mono<@NonNull T> doOnSubscribe(Consumer<? super @NonNull Subscription> onSubscribe) {
 		Objects.requireNonNull(onSubscribe, "onSubscribe");
 		return doOnSignal(this, onSubscribe, null, null,  null, null, null);
 	}


the doOnSuccessOrError below should have its BiConsumer generic types marked as Nullable both, like doAfterSuccessOrError

Wrong line I guess, I have done the update on the BiConsumer mentioned.

yes, couldn't comment on the line as it was not part of the diff, 👍

simonbasle · 2017-09-19T17:03:04Z

reactor-core/src/main/java/reactor/core/publisher/Mono.java

@@ -2268,7 +2269,7 @@ public T block(Duration timeout) {
 	 * @return a {@link Mono} of materialized {@link Signal}
 	 * @see #dematerialize()
 	 */
-	public final Mono<Signal<T>> materialize() {
+	public final Mono<@NonNull Signal<@NonNull T>> materialize() {


once again same question about Signal's <T> non-nullability

simonbasle · 2017-09-19T17:06:06Z

reactor-core/src/main/java/reactor/core/publisher/Mono.java

@@ -3323,7 +3324,7 @@ public final void subscribe(Subscriber<? super T> actual) {
 	 *
 	 * @return a {@link CompletableFuture}
 	 */
-	public final CompletableFuture<T> toFuture() {
+	public final CompletableFuture<@NonNull T> toFuture() {


Future can contain null if the mono is empty

sdeleuze · 2017-09-19T21:07:24Z

@simonbasle Thanks for these great feedbacks, I have pushed an updated version with fixes for all of your proposals.

@nonnull

This commit introduces following changes: - It adds @nonnull and @nullable annotations which allows to apply null-safety semantics on a specific type like JSR 305 but allows to target generic type arguments. - @NonNullApi does not apply to ElementType.TYPE anymore - Nullability annotations are now also applied to generic type arguments, varargs and array elements. - Package is now reactor.util.annotation - Add null-safety reference documentation

sdeleuze · 2017-09-19T21:53:24Z

I did an additional update including fixing broken test (GuideTests#debuggingActivated) and including correctly the documentation.

codecov-io · 2017-09-19T21:59:36Z

Codecov Report

Merging #864 into master will decrease coverage by 0.05%.
The diff coverage is 73.33%.

@@             Coverage Diff              @@
##             master     #864      +/-   ##
============================================
- Coverage     83.55%   83.49%   -0.06%     
+ Complexity     3278     3275       -3     
============================================
  Files           319      319              
  Lines         26052    26052              
  Branches       4840     4840              
============================================
- Hits          21767    21753      -14     
- Misses         2825     2832       +7     
- Partials       1460     1467       +7

Impacted Files	Coverage Δ	Complexity Δ
...n/java/reactor/core/publisher/FluxMapFuseable.java	`85.93% <ø> (ø)`	`3 <0> (ø)`	⬇️
.../java/reactor/core/publisher/EmitterProcessor.java	`81.25% <ø> (ø)`	`80 <0> (ø)`	⬇️
...re/src/main/java/reactor/util/function/Tuple2.java	`100% <ø> (ø)`	`18 <0> (ø)`	⬇️
...va/reactor/core/publisher/MonoStreamCollector.java	`69.64% <ø> (ø)`	`2 <0> (ø)`	⬇️
.../java/reactor/core/publisher/MonoPeekTerminal.java	`60.46% <ø> (ø)`	`3 <0> (ø)`	⬇️
...a/reactor/core/publisher/MonoSubscribeOnValue.java	`76.47% <ø> (ø)`	`3 <0> (ø)`	⬇️
...a/reactor/core/publisher/LambdaMonoSubscriber.java	`89.39% <ø> (ø)`	`24 <0> (ø)`	⬇️
.../main/java/reactor/core/publisher/FluxPublish.java	`80.41% <ø> (ø)`	`12 <0> (ø)`	⬇️
...ain/java/reactor/core/publisher/FluxRetryWhen.java	`92.1% <ø> (ø)`	`4 <0> (ø)`	⬇️
...reactor/core/publisher/ParallelFluxOnAssembly.java	`76.47% <ø> (ø)`	`11 <0> (ø)`	⬇️
... and 215 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 f3e13bf...4c363f0. Read the comment docs.

simonbasle · 2017-09-20T08:00:38Z

reactor-core/src/main/java/reactor/core/publisher/Hooks.java

@@ -423,7 +424,7 @@ public static void resetOnNextDropped() {
 	}

 	@Nullable
-	static BiFunction<? super Throwable, Object, ? extends Throwable> createOrUpdateOpErrorHook(Collection<BiFunction<? super Throwable, Object, ? extends Throwable>> hooks) {
+	static BiFunction<? super @NonNull Throwable, @Nullable Object, ? extends @NonNull Throwable> createOrUpdateOpErrorHook(Collection<@NonNull BiFunction<? super @NonNull Throwable, @NonNull Object, ? extends @NonNull Throwable>> hooks) {


the right-hand side Object in the parameter BiFunction is also @Nullable ;)

simonbasle · 2017-09-20T08:13:53Z

reactor-core/src/main/java/reactor/core/publisher/Mono.java

@@ -335,7 +336,7 @@
 	 * @param <T> type of the expected value
 	 * @return A {@link Mono}.
 	 */
-	public static <T> Mono<T> fromCompletionStage(CompletionStage<? extends T> completionStage) {
+	public static <T> Mono<@NonNull T> fromCompletionStage(@Nullable CompletionStage<? extends @Nullable T> completionStage) {


remove the @Nullable in fromCompletionStage(@Nullable

simonbasle · 2017-09-20T08:15:20Z

reactor-core/src/main/java/reactor/core/publisher/Mono.java

@@ -1717,7 +1718,7 @@ public T block(Duration timeout) {
 	 *
 	 * @return a new {@link Mono}
 	 */
-	public final Mono<T> doOnSubscribe(Consumer<? super Subscription> onSubscribe) {
+	public final Mono<@NonNull T> doOnSubscribe(Consumer<? super @NonNull Subscription> onSubscribe) {
 		Objects.requireNonNull(onSubscribe, "onSubscribe");
 		return doOnSignal(this, onSubscribe, null, null,  null, null, null);
 	}


yes, couldn't comment on the line as it was not part of the diff, 👍

simonbasle

a couple things left and it'll be good to go ;)

This reverts commit b597815.

- Revert "Remove remaining javax nullability annotations" - Revert "Revisit nullability annotations (#864)" - Revisit nullability annotations

sdeleuze added this to the 3.1.0.RELEASE milestone Sep 15, 2017

sdeleuze self-assigned this Sep 15, 2017

sdeleuze requested review from simonbasle and smaldini September 15, 2017 18:32

sdeleuze force-pushed the nullable-v2 branch 4 times, most recently from 999726e to c7b5e43 Compare September 19, 2017 14:04

sdeleuze changed the title ~~[WIP] Revisit nullability annotations~~ Revisit nullability annotations Sep 19, 2017

sdeleuze force-pushed the nullable-v2 branch from c7b5e43 to ce679f7 Compare September 19, 2017 15:00

simonbasle requested changes Sep 19, 2017

View reviewed changes

sdeleuze force-pushed the nullable-v2 branch from ce679f7 to de4b975 Compare September 19, 2017 21:04

sdeleuze force-pushed the nullable-v2 branch from de4b975 to 894be08 Compare September 19, 2017 21:37

sdeleuze force-pushed the nullable-v2 branch from 894be08 to acf8565 Compare September 19, 2017 21:51

simonbasle reviewed Sep 20, 2017

View reviewed changes

simonbasle requested changes Sep 20, 2017

View reviewed changes

Polishing after Simon latest feedback

3b08c18

simonbasle approved these changes Sep 20, 2017

View reviewed changes

simonbasle added the wide-change label Sep 20, 2017

Merge branch 'master' into nullable-v2

4c363f0

smaldini merged commit b597815 into reactor:master Sep 21, 2017

smaldini mentioned this pull request Sep 21, 2017

Add TYPE_USE nullability annotation on generic parameters in the API #714

Closed

sdeleuze deleted the nullable-v2 branch September 21, 2017 06:48

sdeleuze added a commit to sdeleuze/reactor-core that referenced this pull request Sep 21, 2017

Revert "Revisit nullability annotations (reactor#864)"

671f07a

This reverts commit b597815.

sdeleuze added a commit to sdeleuze/reactor-core that referenced this pull request Sep 21, 2017

Revert "Revisit nullability annotations (reactor#864)"

430580d

This reverts commit b597815.

smaldini pushed a commit that referenced this pull request Sep 21, 2017

Revert generic type arguments nullability (#875)

1798dd9

- Revert "Remove remaining javax nullability annotations" - Revert "Revisit nullability annotations (#864)" - Revisit nullability annotations

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revisit nullability annotations #864

Revisit nullability annotations #864

sdeleuze commented Sep 15, 2017 •

edited

Loading

sdeleuze commented Sep 19, 2017

simonbasle commented Sep 19, 2017

simonbasle left a comment

simonbasle Sep 19, 2017

simonbasle Sep 19, 2017

simonbasle Sep 19, 2017

simonbasle Sep 19, 2017

simonbasle Sep 19, 2017

simonbasle Sep 19, 2017

simonbasle Sep 19, 2017

simonbasle Sep 19, 2017

simonbasle Sep 19, 2017

simonbasle Sep 19, 2017

simonbasle left a comment

simonbasle Sep 19, 2017

simonbasle Sep 19, 2017

simonbasle Sep 19, 2017

simonbasle Sep 19, 2017

simonbasle Sep 19, 2017

simonbasle Sep 19, 2017

sdeleuze Sep 19, 2017

simonbasle Sep 19, 2017

simonbasle Sep 19, 2017

sdeleuze Sep 19, 2017

simonbasle Sep 20, 2017

simonbasle Sep 19, 2017

simonbasle Sep 19, 2017

sdeleuze commented Sep 19, 2017

sdeleuze commented Sep 19, 2017

codecov-io commented Sep 19, 2017 •

edited

Loading

simonbasle Sep 20, 2017

simonbasle Sep 20, 2017

simonbasle Sep 20, 2017

simonbasle left a comment

Revisit nullability annotations #864

Revisit nullability annotations #864

Conversation

sdeleuze commented Sep 15, 2017 • edited Loading

sdeleuze commented Sep 19, 2017

simonbasle commented Sep 19, 2017

simonbasle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonbasle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdeleuze commented Sep 19, 2017

sdeleuze commented Sep 19, 2017

codecov-io commented Sep 19, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonbasle left a comment

Choose a reason for hiding this comment

sdeleuze commented Sep 15, 2017 •

edited

Loading

codecov-io commented Sep 19, 2017 •

edited

Loading