Add callback forwarding

[crai0]

This PR implements a new callback forwarding syntax that reduces boilerplate code for callback connections that only call another callback/function as requested in #6373.

[ogoffart]

Thanks a lot for this great PR!

For the other reviewers, this adds support for callback forwarding, which is a syntactic sugar allowing foo => elem.bar; to be equivalent to foo(x, y, z) => { elem.bar(x, y, z); }.

I think this is a great feature to have. The only thing I’m unsure about is whether reusing => for both callback connections and forwarding is the best choice. But I can't think of a better alternative. @tronical @NigelBreslaw @FloVanGH @hunger, what do you think?

On the tooling site, this change the callback completion from foo => { | } to foo => |, only if the callback don't have named arguments, otherwise, it stays foo(x, y, z) => { | }

I'd like to see a bit more tests. The runtime should be tested with an extra file in tests/cases/callbacks. It should test that the feature works including with arguments or return value conversion (eg, int->string and such), as well as forwarding to macros such as Math.max and builtin functions.
Also the syntax tests can be a bit extended. Tests that the following produce an error: foo => 3+3;. Forwarding to a non-pure callback/functions from a pure callback should also be an error. Forwarding to private functions and so on.

[ogoffart]

could you add a test in this file that tests completion for callback forwarding:
foo => 🔺

[crai0]

there's actually no completion for this right now except the generic completion for identifiers/qualified names (i.e. users would have to start typing to get completions and they wouldn't complete the semicolon).

i think a user friendly completion would look like this:

component Foo {
  callback foo1;
  callback foo2(string, int);
}
component Bar {
  callback bar1;
  callback bar2(x: string, y: int);
  callback bar3(string, x: int);
  
  component Foo {
    // callback completion without args
    // foo1 => 🔺
    
    // callback completion with args
    // foo2🔺
    // foo3🔺
    
    // callback connection completion with named args
    // foo2(🔺 completes to foo2(x, y) => {🔺}
    
    // callback connection completion with partially named args
    // foo3(🔺 does not complete 
    
    // callback forwarding completion
    // foo1 => 🔺
    // foo2 => 🔺
    // foo3 => 🔺 
    // complete with methods from parent element scope that are compatible.
    // for example:
    // foo1 => bar1;
    // foo2 => bar2; or foo2 => bar3;
    // foo3 => bar3; or foo3 => bar2;
  }
}

this way users aren't forced to delete the completed arguments if they actually wanted to forward a callback with named arguments but still get completion for the callback connection when they type the open parenthesis.

[ogoffart]

Thanks a lot.

I'm happy with the implementation.

I'm just waiting with the other from the team if that is really the syntax we want with =>.

One drawback is that this prevent us, in the future, to allow to have callback connection without {}

For example, this is not allowed currently, and cannot be allowed in the future with this change, as it would then be ambiguous.

clicked => debug("foo clicked");
double-clicked => root.activate(index);
something(foo) => root.something-else(foo + 1);

The original report in #6373 suggests to use <=> but this is also misleading since it only goes one way. (calling the callback on the right hand side wouldn't cause the callback on the left hand side to be called) Although since callback can only have one handler, this might not be critical.
Other options include:

• : as in clicked: root.clicked; (but that looks like a binding)
• = as in clicked = root.clicked;
• -> as in clicked -> root.clicked;
• :> as in clicked :> root.clicked;
• use a (context sensitive) keyword such as forward clicked to root.clicked;
• ... imagination is the limit.

In my opinion, repurposing => looks good but I'm a bit worried about the ambiguity of having the same symbol means two things.

[tronical]

Thanks a lot.

I'm happy with the implementation.

I'm just waiting with the other from the team if that is really the syntax we want with =>.

👍 from me, but would like to also hear more opinions before merging this.

One drawback is that this prevent us, in the future, to allow to have callback connection without {}

For example, this is not allowed currently, and cannot be allowed in the future with this change, as it would then be ambiguous.

clicked => debug("foo clicked");
double-clicked => root.activate(index);
something(foo) => root.something-else(foo + 1);

Could you elaborate why this is ambiguous?

Just from reading the code this looks different to me from the syntax introduced in this patch. This patch doesn't use parentheses, right?

The original report in #6373 suggests to use <=> but this is also misleading since it only goes one way. (calling the callback on the right hand side wouldn't cause the callback on the left hand side to be called) Although since callback can only have one handler, this might not be critical. Other options include:

• : as in clicked: root.clicked; (but that looks like a binding)
• = as in clicked = root.clicked;
• -> as in clicked -> root.clicked;
• :> as in clicked :> root.clicked;
• use a (context sensitive) keyword such as forward clicked to root.clicked;
• ... imagination is the limit.

In my opinion, repurposing => looks good but I'm a bit worried about the ambiguity of having the same symbol means two things.

I think reusing => is better than introducing a new "symbol".

[ogoffart]

Could you elaborate why this is ambiguous?
Just from reading the code this looks different to me from the syntax introduced in this patch. This patch doesn't use parentheses, right?

Because from a parsing point of view both are expressions.
All my examples had parentheses but they don't need to.
If you have foobar => 43; which would then return 43.
Or could you have foobar => condition ? func1 : func2; where func1 and func2 are two callbacks and then forward to the right callback, meaning that depending on the return value of the expression, we'd do something different? (we don't have function as expression type, but we could in the future if we introduce lambda)

[crai0]

My two cents are that at this point the syntax should be unified. So a callback forwarding is just a callback connection where the right hand side is a compatible callback/function. The other cases are then handled separately.

From a user perspective, I'd agree that it probably makes sense to allow the right hand side of a callback connection to be every expression that evaluates to the callback's return type (or a convertible one) or a compatible callback/function. This could be realized by treating every callback connection that doesn't have a code block for the right hand side as syntactic sugar like this PR does. This leaves us with the following cases:

1. foobar => expr;
   1. expr type is compatible with foobar return type: foobar(args...) => {return expr;}
   2. expr type is a compatible callback/function: foobar(args...) => {return expr(args...);}
2. foobar(args...) => expr;
   1. expr type is compatible with foobar return type: foobar(args...) => {return expr;}
   2. expr type is a compatible callback/function: foobar(args...) => {return expr(args...);}
3. foobar => {...} and foobar(args...) => {...} don't need special treatment

Note that case 1.i only works for callbacks with named arguments, although I'd rather always treat it as an error because it is ambiguous where the arguments come from and it breaks when renaming the arguments.

I'm interested to know what all of you think. Maybe it would've made more sense to already extend the callback connection syntax instead of introducing a new one.

[NigelBreslaw]

My 2 cents. 1. What a delightful improvement! 2. I'm not sure I followed everything, but to me the simplest thing to understand is clicked => root.whatever() would be the nice sugar. You intentionally list no arguments at either side of the expression and don't use curly braces to indicate 'do the right thing'. But then if you do start listing arguments on either side or use curly braces you have to fully specify the whole thing and no magic/sugar is applied?

[ogoffart]

In the future, if we were to support functions as values, we might run into ambiguity like this:

callback get-the-callback() -> fn();
SomeComponent {
    // get_callback is a callback that returns a callback
    get_callback => root.get-the-callback
}

In this case, it would be unclear whether we should call get-the-callback or return it. This means we can't support higher-order functions, or we'd have to restrict the forwarding syntax for them. That's why I'd prefer if we had a different grammar for it.

Having two similar way may also be an issue in error recovery.
For example in clicked => debug("hello"); the current error is telling you that it's expecting a { before the debug. But the error after this patch would be The expression in a callback forwarding must be a callback/function reference which is not as clear.

But if you all think I'm overthinking this, then I can change my mind.

[tronical]

I know understand the problem with higher order types and share the concern.

[crai0]

But if you all think I'm overthinking this, then I can change my mind.

I share your concerns. The more I think about it, the less I feel like this PR is an elegant or future proof solution.

I still think that this can be a unified syntax that has an unambiguous logic for when the right hand side is returned/called though (the approach I outlined above).

1. foobar => expr;

   1. expr type is compatible with foobar return type: foobar(args...) => {return expr;}
   2. expr type is a compatible callback/function: foobar(args...) => {return expr(args...);}

2. foobar(args...) => expr;

   1. expr type is compatible with foobar return type: foobar(args...) => {return expr;}
   2. expr type is a compatible callback/function: foobar(args...) => {return expr(args...);}

3. foobar => {...} and foobar(args...) => {...} don't need special treatment

Higher order functions wouldn't be a problem as long as there is no type erasure for the returned function type. Let's say that fn(Ti...) -> Tr is the type of a callback/function with 0 <= i < n arguments and the return type Tr. A higher order function would then have the type fn(Ti...) -> (fn(Tj...) -> Tr).

The example posited by @ogoffart falls under case 1.ii because even though both root.get-the-callback and get_callback have the type fn() -> fn(), it isn't compatible with get_callback's return type which is fn() (i.e. it doesn't fall under case 1.i).

callback get-the-callback() -> fn();
SomeComponent {
    // get_callback is a callback that returns a callback
    get_callback => root.get-the-callback
}

In order for get_callback to return root.get-the-callback, get_callback's type would have to be fn() -> (fn() -> fn()) to fall under case 1.i.

The error reporting seems straightforward as well:

• For case 1 and 2 expr either has a compatible type or not and the mismatch can be reported accordingly
• For case 1.ii and 2.ii evaluating expr will report if it references an argument not specified in args...
• For case 3 everything works as usual

It's up to you whether this should get merged in this state. After all, it is an improvement in terms of usability but I'm also fine with going back to the drawing board and thinking about a better solution.