[foreign] RFC simplify callback API

Thu Nov 29 12:21:54 UTC 2018

On 29/11/2018 12:01, Jorn Vernee wrote:
> Hi,
>
> I'm not so sure about this. Some comments:
>
>> 1) it allows a subclass of Callback to still act as a functional
>> interface (only one abstract method)
>> 2) it gives an error whenever the user is attempting to pass an
>> unwrapped lambda to some native function
>
> So, to use it, you still need to call allocateCallback:
>
>     try(var scope = Scope.newNativeScope()) {
>         QsortComparator comp = (a, b) -> Integer.compare(a.get(), 
> b.get());
>         QsortComparator realComp = scope.allocateCallback(comp);
>
>         ...
>
>         qsort(arr.elementPointer(), 6, 4, realComp);
>     }
>
> If you pass `comp` or the lambda expression directly you end up with 
> an exception. The method signature makes it seem like you can pass a 
> lambda or method reference directly, but in reality you can't. This 
> seems like a source for bugs to me.
Yeah - this is the crux of the issue, and why we currently are settled 
on Callback<XYZ>
>
>> * callbacks now have a common supertypes, as structs do - which means
>> the various methods Scope::allocateCallback and LayoutType::ofFunction
>> can be expressed with much stronger static guarantees
>
> I'm not sure I see what you mean here? AFAICT the only hole in the 
> static guarantees given by those methods is the fact that a given 
> 'callback' type might or might not be annotated, which is still there.

Well, it's true that lack of annotation cannot be verified statically, 
but, if you think about it, we need an annotation simply because we need 
a way to extract a layout. You could imagine variants of the 
LayoutType::ofStruct method which also take a Struct layout and in that 
case no annotation would be needed - at which point you'd be 100% safe 
(for structs). But callback functional interface are not verifiable in 
this way: their only property is that they must be... functional 
interface, which is something that requires a very expensive check at 
runtime, and cannot be captured statically. I guess that is the 'hole' 
I'm referring to.

>
>> * APIs feel less cluttered - e.g. the signature of qsort just says
>> 'QsortComparator' rather than 'Callback<QsortComparator>'
>
> From a different perspective you could say that the signature is 
> lacking essential information; that you need to pass something 
> returned by Scope::allocateCallback, i.e. an object that actually 
> points to an upcall stub, and not just any implementation of 
> 'QsortComparator'.
Fair point
>
>> Is this a simplification worth exploring? Or is lumping callback and
>> functional interfaces a step too far?
>
> I think I'm in the "step too far" camp on this one for now.

This is all fair play - I was well aware of the hacks - and I expected 
this reaction; but I do think that the callback API, as it is now, 
doesn't feel 100% 'finished'.

Maurizio

>
> Maybe the civilization layer could handle some of the conversions 
> between functional interface and Callback in the future to make the 
> API simpler?
>
> Jorn
>
> Maurizio Cimadamore schreef op 2018-11-28 18:48:
>> Hi,
>> We currently have a Callback<T> type which exposes two functionalities:
>>
>> * returns the entry point (a Pointer) of the underlying native stub
>> * allows Java code to view it as a functional interface (of type T)
>>
>> While this is ok, I've always been a bit puzzled that we needed to
>> separate the Callback interface from the functional interface which
>> represents it in the Java world. Let's consider a concrete case (from
>> StdLIbTest):
>>
>> void qsort(Pointer<Integer> base, int nitems, int size,
>> Callback<QsortComparator> comparator);
>>
>> @NativeCallback("(u64:i32u64:i32)i32")
>> interface QsortComparator {
>>     int compare(Pointer<Integer> u1, Pointer<Integer> u2);
>> }
>>
>>
>> The fact that the qsort API has to say Callback<QsortComparator> seems
>> a bit cumbersome; of course there's a good reason why it's this way:
>> we wanted to force an explicit callback instantiation (via
>> Scope::allocateCallback) and move the responsibility of allocating
>> callback objects onto clients. So, having a different type in the
>> qsort signature force the client to think about the fact that a
>> callback has to be allocated here.
>>
>>
>> Thinking more generally about this problem, and how to improve over
>> the status quo, there are two issues here: (i) on the one hand we'd
>> like some functional interface to be available somewhere, so that we
>> can express the callback code in terms of a lambda expression; (ii) on
>> the other hand we want to protect the binder code from running with
>> random functional interface implementations. The status quo achieves
>> these objectives by moving the functional interface 'away' from the
>> user - wrapping it in a Callback<F> type (where F is the functional
>> interface).
>>
>>
>> Another solution to achieve this would be to have a Callback interface
>> defined as follows:
>>
>> interface Callback<T extends Callback<T>> extends Resource {
>>
>>     default Pointer<?> ptr() {
>>         throw new UnsupportedOperationException("Invalid callback!");
>>     }
>>
>>     @Override
>>     default Scope scope() {
>>         return ptr().scope();
>>     }
>> }
>>
>>
>> That is, a Callback still has a method to obtain a pointer (the entry
>> point of the callback) - but this time we just reuse the ptr() method
>> declared on the Resource interface. Also note that the ptr() method
>> has been defaulted with some throwing code. This achieves two effects:
>>
>> 1) it allows a subclass of Callback to still act as a functional
>> interface (only one abstract method)
>> 2) it gives an error whenever the user is attempting to pass an
>> unwrapped lambda to some native function
>>
>>
>> This would allow us to rewrite the above example as follows:
>>
>> void qsort(Pointer<Integer> base, int nitems, int size,
>> QsortComparator comparator);
>>
>> @NativeCallback("(u64:i32u64:i32)i32")
>> interface QsortComparator extends Callback<QsortComparator> {
>>     int compare(Pointer<Integer> u1, Pointer<Integer> u2);
>> }
>>
>>
>> Now, I'm well aware that this is not a 100% clean approach; but there
>> seem to be some advantages to it that, I think, are worth discussing:
>>
>> * callbacks now have a common supertypes, as structs do - which means
>> the various methods Scope::allocateCallback and LayoutType::ofFunction
>> can be expressed with much stronger static guarantees
>>
>> * clients no longer have to call 'asFunction' on the callback object
>> to be able to call the underlying Java method - they can just invoke
>> that on the Callback
>>
>> * APIs feel less cluttered - e.g. the signature of qsort just says
>> 'QsortComparator' rather than 'Callback<QsortComparator>'
>>
>> * some of the irregularities surrounding callbacks in LayoutType are
>> gone (e.g. we no longer need the ad-hoc getFunctionalIntf() impl
>> method, we can just rely on the carrier)
>>
>> * we no longer need CallbackImpl
>>
>> * allocating a callback is much more similar to allocate a struct: we
>> fetch a pointer and we allocate an instance of the synthetic class
>> (generated by the binder for us)
>>
>>
>> The patch below is an example of how this can be made to work:
>>
>> http://cr.openjdk.java.net/~mcimadamore/panama/simpler_callback_intf_v2/
>>
>>
>> Is this a simplification worth exploring? Or is lumping callback and
>> functional interfaces a step too far?
>>
>> Maurizio