Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Contravariant method parameter types declarations for Future #5314

Merged
merged 1 commit into from
Sep 12, 2024

Conversation

vietj
Copy link
Member

@vietj vietj commented Sep 11, 2024

Motivation

io.vertx.core.Future does not use variance in its declarations preventing reuse or forcing to introduce adapter functions, e.g.

public void flatMap(Future<String> fut, Function<CharSequence, Future<Integer>> fn) {
  fut.flatMap(fn::apply); // produce a new function adapting "fn", it could be "fn" instead
}

public void onComplete(Future<String> fut, Promise<CharSequence> promise) {
  // That
  fut.onComplete(event -> promise.handle(event.map(s -> s))); // we would like to use "promise" instead

  // Or
  fut.map(s -> (CharSequence)s).onComplete(promise);
}

Changes

Three changes are necessary to solve this problem at the expense of small breaking changes

Use variance on existing methods

// After
<U> flatMap(Function<T, Future<U>> fn);
// After
<U> flatMap(Function<? super T, Future<U>> fn);

This changes remains source compatible (for user), there are breaking changes but those are for implementation of the Future type, which are acceptable.

It does not apply to Handler<AsyncResult<T>> arguments. In practice Handler<AsyncResult<T>> is solvable but not realistic, it should be Handler<? extends AsyncResult<? super T>> but this one exhibit issues with lambdas, e.g. future.onComplete(ar -> ar.result() /* Object and not T */).

Overloading handler of async results methods

There are two possible solutions

  • Introduce AsyncResultHandler<T> which extends Handler<AsyncResult<T>>
  • Use types accepting two arguments so a lambda will get the value and the error instead of the combined async result, pretty much like CompletionStage#whenComplete(BiConsumer<? super T, ? super Throwable>).

The former does not seem possible to use as overload because onComplete(ar -> ...) will be ambiguous. It could be solved by adding new methods (aliases) though.

The later does not introduce issues since we are adding method overloads

Future<T> onComplete(Handler<AsyncResult<T>> handler);
Future<T> onComplete(Completable<? super T> handler); // Overload

Likewise

<U> Future<U> transform(Function<AsyncResult<T>, Future<U>> fn);
<U> Future<U> transform(BiFunction<? super T, ? super Throwable, Future<U>> fn); // Overload

with

@FunctionalInterface
public interface Completable<T> {
  default void succeed(T value) {
    complete(value, null);
  }
  default void fail(Throwable cause) {
    complete(null, cause);
  }
  void complete(T value, Throwable err);
}

There are a few breaking changes though, when null is an argument, the compiler cannot decide with overload to use. We consider those are marginal, we found some of these in the vertx test suite to check that null arguments produces errors, e.g. future.onComplete(null).

Retrofit Promise as Completable instead of async result handler

Since now we have variant part with a Completable, the need Promise to extend Completable instead of Handler<AsyncResult<T>>

Promise<T> extends Completable<T> {
  ...
}

This creates breaking changes when Promise<T> was used at the place of Handler<AsyncResult<T>>, e.g. internally in vertx we still have a few of those, and promise::handle should be used to fix the issues. Since Vert.x 5 does not anymore exhibit Handler<AsyncResult<T>> methods, this should not be an issue for users.

This will require adaptation in Vert.x code base because there are many remaining usage of Handler<AsyncResult<T>> idiom: here is a recap of the necessary changes https://gist.github.com/vietj/02ce9dc89c8a1c11dabe8828f760f973 . This list is actually quite long, however it mostly solves internal issues of the vertx codebase still using Vert.x 3 idioms and was never migrated to use futures, e.g. recent vertx components like the new vertx-grpc or vertx-service-resolver do not need any changes.

@vietj vietj force-pushed the introduce-future-promise-variance branch 2 times, most recently from df923fd to 18dc8f5 Compare September 11, 2024 16:17
@vietj vietj added this to the 5.0.0 milestone Sep 11, 2024
@vietj vietj self-assigned this Sep 11, 2024
@vietj vietj force-pushed the introduce-future-promise-variance branch 2 times, most recently from e2d81ec to 0200ec6 Compare September 12, 2024 08:47
@vietj vietj marked this pull request as ready for review September 12, 2024 08:47
@vietj vietj force-pushed the introduce-future-promise-variance branch 4 times, most recently from ab8c965 to 4bf9ee1 Compare September 12, 2024 13:28
@vietj vietj changed the title Introduce future promise variance Introduce future promise contra variance Sep 12, 2024
@vietj vietj changed the title Introduce future promise contra variance Introduce future promise contravariance Sep 12, 2024
@vietj vietj changed the title Introduce future promise contravariance Contravariant method parameter types declarations for Future Sep 12, 2024
io.vertx.core.Future does not use variance in its declarations preventing reuse or forcing to introduce adapter functions, e.g.

public void flatMap(Future<String> fut, Function<CharSequence, Future<Integer>> fn) {
  fut.flatMap(fn::apply); // produce a new function adapting "fn", it could be "fn" instead
}

public void onComplete(Future<String> fut, Promise<CharSequence> promise) {
  // That
  fut.onComplete(event -> promise.handle(event.map(s -> s))); // we would like to use "promise" instead

  // Or
  fut.map(s -> (CharSequence)s).onComplete(promise);
}

Changes:

Three changes are necessary to solve this problem at the expense of small breaking changes

1. Use variance on existing methods

// After
<U> flatMap(Function<T, Future<U>> fn);
// After
<U> flatMap(Function<? super T, Future<U>> fn);

This changes remains source compatible (for user), there are breaking changes but those are for implementation of the `Future` type, which are acceptable.

It does not apply to Handler<AsyncResult<T>> arguments. In practice Handler<AsyncResult<T>> is solvable but not realistic, it should be Handler<? extends AsyncResult<? super T>> but this one exhibit issues with lambdas, e.g. future.onComplete(ar -> ar.result() /* Object and not T */).

2. Overloading handler of async results methods

Use a functionnal interface accepting two arguments so a lambda will get the value and the error instead of the combined async result, pretty much like CompletionStage#whenComplete(BiConsumer<? super T, ? super Throwable>).

Future<T> onComplete(Handler<AsyncResult<T>> handler);
Future<T> onComplete(Completable<? super T> handler); // Overload

Likewise

<U> Future<U> transform(Function<AsyncResult<T>, Future<U>> fn);
<U> Future<U> transform(BiFunction<? super T, ? super Throwable, Future<U>> fn); // Overload

with

@FunctionalInterface
public interface Completable<T> {
  default void succeed(T value) {
    complete(value, null);
  }
  default void fail(Throwable cause) {
    complete(null, cause);
  }
  void complete(T value, Throwable err);
}

There are a few breaking changes though, when null is an argument, the compiler cannot decide with overload to use. We consider those are marginal, we found some of these in the vertx test suite to check that null arguments produces errors, e.g. `future.onComplete(null)`.

3. Retrofit Promise as Completable instead of async result handler

Since now we have variant part with a Completable, we need Promise to extend Completable instead of Handler<AsyncResult<T>>

Promise<T> extends Completable<T> {
  ...
}

This creates breaking changes when Promise<T> was used at the place of Handler<AsyncResult<T>>, e.g. internally in vertx we still have a few of those, and promise::handle should be used to fix the issues. Since Vert.x 5 does not anymore exhibit Handler<AsyncResult<T>> methods, this should not be an issue for users.

This will require adaptation in Vert.x code base because there are many remaining usage of `Handler<AsyncResult<T>>` idiom: here is a recap of the necessary changes https://gist.github.com/vietj/02ce9dc89c8a1c11dabe8828f760f973 . This list is actually quite long, however it mostly solves internal issues of the vertx codebase still using Vert.x 3 idioms and was never migrated to use futures, e.g. recent vertx components like the new _vertx-grpc_ or _vertx-service-resolver_ do not need any changes.
@vietj vietj force-pushed the introduce-future-promise-variance branch from 4bf9ee1 to 7c42984 Compare September 12, 2024 15:56
@vietj vietj merged commit 79edb86 into master Sep 12, 2024
7 checks passed
@vietj vietj deleted the introduce-future-promise-variance branch September 12, 2024 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant