[jnigen] Enable "listener" callbacks

[HosseinYousefi]

Continuing in the path to get "interface implementation" feature out of experimental in JNIgen.

JNIgen currently generates an implement factory on interface types like so:

abstract interface class $RunnableImpl {
  factory $RunnableImpl({
    required void Function() run,
  }) = _$RunnableImpl;

  void run();
}

class _$RunnableImpl implements $RunnableImpl {
  _$RunnableImpl({
    required void Function() run,
  }) : _run = run;

  final void Function() _run;

  void run() {
    return _run();
  }
}

class Runnable extends JObject {
  // Runnable has a factory named implement:
  factory Runnable.implement(
    $RunnableImpl $impl
  ) {
    // ...
  }
  // ...
}

If we pass Runnable.implement(...) to Java and it tries to call .run method, it will block until Dart actually runs it. However sometimes we don't have to block when the callback is intended to be a listener.

Of course, making a method into a "listener" is only possible to do if the return type of the said method is void (or maybe handling other cases like Java's Future).

We could add an enum parameter per (viable) method to communicate which methods have to be blocking vs. listener: enum JCallMode { blocking, listener }.

// Changed to mixin so `$mode`s can have a default value
abstract mixin class $RunnableImpl {
  factory $RunnableImpl({
    required void Function() run,
  }) = _$RunnableImpl;

  void run();
  JCallMode get run$mode => JCallMode.blocking;
}

class _$RunnableImpl implements $RunnableImpl {
  _$RunnableImpl({
    required void Function() run,
    JCallMode run$mode = JCallMode.blocking,
  })  : _run = run,
        _run$mode = run$mode;

  final JCallMode _run$mode;
  final void Function() _run;

  void run() {
    return _run();
  }

  JCallMode get run$mode => _run$mode;
}

Alternatively, a la NativeCallable, we could create a JCallable<T extends Function> and instead of requiring a simple function, require a JCallable. Then JCallable can have two factories: .listener, and .blocking.

The issue here is that a simple callback implementation will be quite verbose:

Runnable.implement($RunnableImpl(run: JCallable.blocking(() => print('hello'))));

And non-void returning methods cannot be JCallable.listener anyways so it's unnecessary in those cases.

@liamappelbe @dcharkes @mkustermann @lrhn Which syntax do you prefer? Or maybe something different.

[lrhn]

Tricky.
My immediate comment is to drop the enum and use a bool.

Then a flag would be run$block: false,
and the member would be bool get run$block;.
My only quip is that a default of true is weird. But run$noBlock: true doesn't read well either.
Is there a positive word for not blocking?
run$lazy: true? Or late or async?

The flag would only exist for methods that are void, so I do prefer that over the wrapper.

[liamappelbe]

My immediate comment is to drop the enum and use a bool.

Only issue with a bool is that we're adding more NativeCallable variants. Temporary isolate callbacks are in development.

Fwiw, the way ffigen handles this is by having 2 implementation methods, one which uses blocking callbacks for everything, and the other which uses listener callbacks for all the void methods. If the user wants more granularity than that, they can implement individual methods either as listeners or as blocking callbacks using the ObjCProtocolMethod fields on the protocol bindings. The implement methods themselves show how that's done:

/// MyProtocol
abstract final class MyProtocol {
  /// Builds an object that implements the MyProtocol protocol. To implement
  /// multiple protocols, use [addToBuilder] or [objc.ObjCProtocolBuilder] directly.
  static objc.ObjCObjectBase implement(
      {int Function(SomeStruct)? optionalMethod_,
      void Function(int)? voidMethod_,
      required objc.NSString Function(objc.NSString, double)
          instanceMethod_withDouble_}) {
    final builder = objc.ObjCProtocolBuilder();
    MyProtocol.optionalMethod_.implement(builder, optionalMethod_);
    MyProtocol.voidMethod_.implement(builder, voidMethod_);
    MyProtocol.instanceMethod_withDouble_
        .implement(builder, instanceMethod_withDouble_);
    return builder.build();
  }

  /// Builds an object that implements the MyProtocol protocol. To implement
  /// multiple protocols, use [addToBuilder] or [objc.ObjCProtocolBuilder] directly. All
  /// methods that can be implemented as listeners will be.
  static objc.ObjCObjectBase implementAsListener(
      {int Function(SomeStruct)? optionalMethod_,
      void Function(int)? voidMethod_,
      required objc.NSString Function(objc.NSString, double)
          instanceMethod_withDouble_}) {
    final builder = objc.ObjCProtocolBuilder();
    MyProtocol.optionalMethod_.implement(builder, optionalMethod_);
    MyProtocol.voidMethod_.implementAsListener(builder, voidMethod_);
    MyProtocol.instanceMethod_withDouble_
        .implement(builder, instanceMethod_withDouble_);
    return builder.build();
  }

  static void addToBuilder(...
  static void addToBuilderAsListener(...

  /// optionalMethod:
  static final optionalMethod_ =
      objc.ObjCProtocolMethod<int Function(SomeStruct)>(...);

  /// voidMethod:
  static final voidMethod_ =
      objc.ObjCProtocolListenableMethod<void Function(int)>(...);

  /// instanceMethod:withDouble:
  static final instanceMethod_withDouble_ =
      objc.ObjCProtocolMethod<objc.NSString Function(objc.NSString, double)>(...);
}

[HosseinYousefi]

Only issue with a bool is that we're adding more NativeCallable variants. Temporary isolate callbacks are in development.

If we add "blocking" and "temporaryIsolate" modes to NativeCallable, then we probably should just use them instead of normal Dart functions. I assume for example, temporaryIsolate is going to have certain limitations, we want our users to get compiler errors/warnings if they ignore the lints.

implementAsListener

This is convenient. The slight issue I have with it is the fact that it implies that everything is a listener. Of course we know that the functions returning a value cannot be listeners but the user might not.

---

@lrhn What do you think about using FutureOr<void> as the return type of all the void methods instead? In the code we can check if run is Future<void> Function() or void Function() and decide if it's async/sync based on that. The nice thing is that there is no need for an extra argument per method.

[liamappelbe]

If we add "blocking" and "temporaryIsolate" modes to NativeCallable, then we probably should just use them instead of normal Dart functions. I assume for example, temporaryIsolate is going to have certain limitations, we want our users to get compiler errors/warnings if they ignore the lints.

I considered doing this for ObjC, but there are some details that make this not quite work (eg the block's invoke function pointer has a different signature to the user's function). Not sure if you'd have similar issues in jnigen.

[HosseinYousefi]

Not sure if you'd have similar issues in jnigen.

Yes, for starters every java object is a void pointer instead. Still, not sure if the enum value will be the best. Maybe it's best if we generate wrappers with multiple constructors like NativeCallable. And maybe factor out any potential compiler special cases for things like temporary isolate callables to be available anywhere with some pragma so we can also use them in the generated wrappers.

[HosseinYousefi]

I've already implemented the async method way: #1596.

This is the check it uses to see if the run method is blocking or not:

           (($impl is _$MyRunnable &&
                ($impl as _$MyRunnable)._run is _$core.Future<void>
                    Function() &&
                ($impl as _$MyRunnable)._run is! _$core.Never Function()) ||
            ($impl.run is _$core.Future<void> Function() &&
                $impl.run is! _$core.Never Function()))

    // If the implementation is using the callback passing style, look at the
    // actual passed callback instead of the wrapper function. The wrapper is
    // always going to return `FutureOr<void>`.
    //
    // If the callback simply throws its return type will be `Never`. As any
    // function `R <F>` is a subtype of `Never <F>`, we should have a special
    // case for it. It's not possible to throw from a listener function, so
    // functions that only throw and hence have a `Never` return type, will be
    // considered to be sync.

---

Also I could have used void instead of FutureOr<void>. Maybe I'll even do that to make the autocompletion of implementing $Foo look nicer.

[HosseinYousefi]

Actually void seems to be a better choice overall as this is allowed:

FutureOr<void> Function() f = () { return Future<void>.delayed(Duration(seconds: 1)); };

but this isn't:

void Function() f = () { return Future<void>.delayed(Duration(seconds: 1)); };

While this is:

void Function() f = () async { return Future<void>.delayed(Duration(seconds: 1)); };

So it really makes it dependent on the existence of async keyword.

[lrhn]

The problems here come from inference, but inference is hard to avoid with function expressions, since you cannot write a return type.
The reason

void Function() f = () { return Future<void>.delayed(Duration(seconds: 1)); };

is an error is that the function return type is inferred to be void during downwards inference, and a return expr; is only (grudginly) accepted in a void-returning function if expr has a type of void, dynamic or Null. Which is two (or three) types too many IMO.
All for historical reasons, obviously.

I generally try to avoid deducing anything from a function return type because it is so hard to control the return type.
(The main reasons Future.catchError(Function onError, ...) doesn't type-check its argument function more precisely is that it provides no useful context type, so a user function literal can't control the return type. It usually ends up as dynamic, so we can't check that the onError function is one of Future<T> Function(Object) or Future<T> Function(Object, StackTrace), because it'll be neither.)

And you do need to consider a function type like FutureOr<void> Function(), which can also exist, and are likely if that's the context type provided.
(If trying to do return type switching, a FutureOr should probably be treated as "worst-case" for anything. It can be either synchronous or asynchronous, and it can even decide for each call.)

If I understand this correctly, after trying to swap it back in, the problem is:

• Creating Java values that run Dart code (Java objects with Dart-implemented methods, or similar).
• If the Java code calls a Dart method, the Java thread must block until the call returns, because the Dart isolate isn't ready to run code "right now", it has to return to the event loop.
• Unless the Dart code knows that it's OK to return as void immediately, and start doing something in Dart later, in which case the Dart code is more of a message passing to the Dart isolate than a Dart method invocation. So fundamentally different.

We can't know if a Dart void Function() function is intended to run for its side effects before continuing, or if it's fine to just run later. We do need the author to say that. I wouldn't use the return type for that.

For a Future<void> Function() function, it's possibly intended to be something you wait for, but it would be so easy to use an async function an asynchronous message handler/"listener" callback, and it's equally easy for that to end up with a Future<void> Function() runtime type, even if nobody needs to wait for the future.

If the context type is void or FutureOr<void>, a function literal is likely to just have that type, no matter what it intends to do.

(If we want to save space, use shorthands and shorter names.

var javaComputeRunnable = Runnable.of(run: block((x) => compute(x)));
var javaAsyncComputeeRunnable = Runnable.of(run: blockAwait((x) async => await computeMore(x)));
var javaCallbackRunnable = Runnable.of(run: schedule((x) => laterAction(x)));

where block is JCallMode.block which expects a synchronous function, blockAwait is a JCallMode.block which expects an asynchronous function, and will await its returned future before returning and unblocking, and schedule is JCallMode.listener, a Java function that messages Dart to run the callback when it's ready, and the returns immediately.

(If the behavior is chosen at runtime, based on the JCallMode/JCallable object that is used as argument to run, then making helper functions should be fine. If it tries to deduce things at compile time, it easily becomes annoying.)

So

JRunnable<T Function()> block<T>(T Function() dartFunction) => ...
JRunnable<T Function()> blockAwait<T>(Future<T> Function() dartFunction) => ...
JRunnable<void Function()> schedule(void Function() dartFunction) => ...

and $Runnable(run: ...) has a type of JRunnable<void Function()> because of the Java signature of void run();, so you can use any of the variants.
(Or call them sync, async and schedule. The words sync and async are not reserved in Dart.)

[HosseinYousefi]

JRunnable<T Function()> block<T>(T Function() dartFunction) => ...
JRunnable<T Function()> blockAwait<T>(Future<T> Function() dartFunction) => ...
JRunnable<void Function()> schedule(void Function() dartFunction) => ...

The functions can have arguments as well, so they should look more like:

JCallable<T> block<T extends Function>(T dartFunction) => ...

Which doesn't allow us to limit the return types.

I generally try to avoid deducing anything from a function return type because it is so hard to control the return type.

I think I'll use the initial idea to use an extra enum value per void function then. Shorter and no magic.