Add variadic callback generators

[crocket]

My variadic callback generators are efficient because they directly iterate over arguments object.

This pull request adds

• h$makeCallbackVar in jsbits/callback.js
• Variadic callback generators in GHCJS/Foreign/Callback/Variadic.hs
• Better explanation of OnBlocked and syncCallback' in README
• A detailed explanation of variadic callbacks in README

Also, it bumps the version to 0.2.1.0

This relates to https://github.com/ghcjs/ghcjs-base/issues/76

[luite]

Thanks for the code and detailed readme, and apologies for taking so long to review it.

A downside I see with this approach is that it always has to convert arguments to an array, which is relatively expensive in JS engines. Ideally the implementation would pick the most efficient implementation available. I think it's possible to do this by using somewhat different approach on the Haskell side, which also makes it possible to tell the JS code how many arguments are expected. I'll make a small proof of concept implementation so we can more easily compare.

Also you use PFromJSVal here, while the normal non-variadic callbacks have simple monomorphic types like JSVal -> JSVal -> IO (). Is there a reason for having pure marshalling built-in for variadic callbacks? Personally I think it's better to separate creation of callbacks from data conversion, unless there's a strong reason to bundle them together.

My last point is about handling the situation where the number of arguments is different from what we expect. JavaScript uses the undefined value for arguments that are not supplied. It would be more consistent with the non-variadic callbacks to have a JSVal with value undefined, instead of throwing an exception. However, I do think that we should have a way to catch callbacks being called with a different number of arguments than expected. I'll include a way of doing this in the proof of concept code.

Anyway, variadic callbacks are long ovedue, so thanks for building a working implementation; we'll definitely merge some variant of this in ghcjs-base! I'd just like to make sure that the API we choose is as good as possible, and I think we may be able to improve efficiency and safety a little bit, so I'll soon follow your example, and reply with a bit of code.

[crocket]

A downside I see with this approach is that it always has to convert arguments to an array, which is relatively expensive in JS engines.

It doesn't convert arguments object to an actual proper array. My implementatin just treats arguments as JSArray. Thus, there is no conversion overhead. Perhaps, do you mean that obtaining arguments object is expensive?

Also you use PFromJSVal here, while the normal non-variadic callbacks have simple monomorphic types like JSVal -> JSVal -> IO (). Is there a reason for having pure marshalling built-in for variadic callbacks?

With my implementation, JSVal -> JSVal -> IO () is possible since JSVal has a PFromJSVal instance. Also, my implementation greatly reduces boilerplate when conversion from JSVal is needed. With mine, people don't have to call pFromJSVal in their haskell callbacks or create a separate function that hides pFromJSVal.

Personally I think it's better to separate creation of callbacks from data conversion, unless there's a strong reason to bundle them together.

I want to hear your rationale. The benefit of removing the noise of pFromJSVal is valid. In many or most cases, JSVal is not useful unless you're going to pass it back to javascript directly. For example, without converting JSVal to JSString, Double, or other primitive types via pFromJSVal, it is cumbersome to manipulate values in GHCJS.

JavaScript uses the undefined value for arguments that are not supplied. It would be more consistent with the non-variadic callbacks to have a JSVal with value undefined, instead of throwing an exception.

If undefined was passed to a callback, it would be captured by arguments object as below.

% node
> function abc() { console.log(arguments); }
undefined
> abc(1, undefined);
{ '0': 1, '1': undefined }

Unless a callback itself is variadic, when a callback is called with wrong numbers of arguments, the callback or the caller should be modified.

If a caller could pass undefined to an expected parameter, the type of the parameter can be Maybe Something. PFromJSVal can marshal Maybe a.

instance PFromJSVal a => PFromJSVal (Maybe a) where
    pFromJSVal x | isUndefined x || isNull x = Nothing
    pFromJSVal x = Just (pFromJSVal x)
    {-# INLINE pFromJSVal #-}

Since javascript doesn't have compile time error checking, the callback argument mismatch should be reported as a runtime error.

Recommendation

In README, I attached (Draft) to the section about variadic callback generator. Draft means it is an experimental API that will have breaking changes anytime. Thus, it is ok for you to merge this and modify it as much as you like until you can remove (Draft) from the README section.

[luite]

Ah you're right, you're not completely converting arguments to an Array, but indeed passing the arguments object around like this is still not free. The arguments object still has to be constructed, since optimized (JIT compiled) JavaScript code usually has a more efficient calling convention, with arguments on the call stack (not directly accessible from JS). The arguments object is then only allocated if it's actually used, and filled by copying the values or references from the implementation-dependent call stack.

I do agree that the type signature for your variant is strictly more general, it's still possible to use JSVal after all, but I don't think it's necessarily better because of that. Ideally I'd like to structure the ghcjs-base code in a way that maintains a reasonably clean separation between lower and higher level bits. The Marshal code is convenient, but makes some choices that might not be right for every use case, and at the very least adds complexity. It could well end up in a separate package at some point (giving users the choice to stay with an older API after breaking changes for example, rather than forcing them to use whatever comes with the GHCJS they use).

So from that perspective, I think that unless we have a strong reason to keep both parts together, it'd be best to implement the core functionality with purely JSVal. Depending on how much code it'd take to add marshalling, we could then either add a small example in the documentation, or add a convenience function somwhere in the Marshall hierarchy.

Having undefined inside a JSVal would not be the same as the error call, since it'd be the JavaScript undefined, not the Haskell one. From a Haskell perspective, it's a normal, non-bottom value: You can force it safely. This would make the behaviour consistent with the non-variadic case.

I still think it'd be better to first review the alternative I have in mind. Perhaps we'll find that my idea has some other downsides that I hadn't thought about, or figure out an even better approach. But I don't think it'd be fair to let you wait much longer, so I'll do the code for it this weekend. If I fail to come up with a reasonable working (not necessarily finished) approach then I agree that merging your PR with an experimental status is better than letting this sit for much longer. Is that ok?

[crocket]

Yes, it's ok.

[luite]

{-# LANGUAGE ScopedTypeVariables #-}
{-# LANGUAGE TypeFamilies #-}
{-# LANGUAGE TypeOperators #-}
{-# LANGUAGE DataKinds #-}
{-# LANGUAGE UndecidableInstances #-}
{-# LANGUAGE FlexibleContexts #-}
{-# LANGUAGE ConstraintKinds #-}

module Main where

import GHC.TypeLits

import Data.Proxy
import Data.Type.Equality
import Data.Type.Bool

-- uh fake, use the real Any in the implementation
data Any = Any
unsafeCoerce :: a -> Any
unsafeCoerce _ = Any

data Callback = Callback { callbackArity  :: Integer
                         , callbackAction :: Any
                         }

-- | The end result of a function
type family Result x where
  Result (a -> b) = Result b
  Result a        = a

-- | The arity of a function
type family Arity n :: Nat where
  Arity (a -> b) = 1 + Arity b
  Arity a        = 0

type family AllArgsEq t f :: Bool where
  AllArgsEq t (a -> b) = a == t && AllArgsEq t b
  AllArgsEq t a        = 'True

type IsValidCallback a =
  ( KnownNat (Arity a)
  , 'True ~ (AllArgsEq Int a && Result a == IO ()))

mkCallback :: forall a. IsValidCallback a => a -> IO Callback
mkCallback f = mkCallbackInternal (natVal (Proxy :: Proxy (Arity a)))
                                  (unsafeCoerce f)

mkCallbackInternal :: Integer -> Any -> IO Callback
mkCallbackInternal n x = do
  putStrLn ("creating callback with arity " ++ show n)
  -- do some foreign call to some RTS function here
  pure (Callback n x)

-------------------------------------------------------------------------------
cb1 :: Int -> Int -> IO ()
cb1 = undefined

cb2 :: IO ()
cb2 = undefined

cb3 :: Int -> String -> IO ()
cb3 = undefined

cb4 :: Int -> a -> IO ()
cb4 = undefined

main :: IO ()
main = do
  mkCallback cb1
  mkCallback cb2
  -- mkCallback cb3
  -- mkCallback cb4
  pure ()

This was what I had in mind. Only the general case is implemented here, where an IO action is passed to JS with its arity. We don't need the pass the arguments around in an array-like object, but we would need to use some RTS macros to construct the application.

Since we know the expected arity at creation time, we can easily add optimized cases for small arities.

An actual implementation could avoid this runtime test by making more use of the typelevel arity nat to dispatch to specialized callback makers in JS for some arities (say 0..2). But I'm not sure if that'd be worth the trouble, since I expect callback creation to be relatively rare compared to callback use.

[crocket]

As long as there is user documentation for mkCallback and your solution doesn't involve manually written macros such as MK_AP1, MK_AP2, and so on which cannot deal with arbitrary numbers of arguments, it'd be fine.

On electron, callbacks of 6~11 arguments are not unusual. The current iteration of ghcjs-base cannot be used to make a GHCJS binding for electron.

[crocket]

I'm closing this due to inactivity.