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

Port ghcjs-base.GHCJS.Foreign.Callback as base.GHC.JS.Foreign.Callback for the GHC JavaScript Backend #150

Closed
JoshMeredith opened this issue Mar 21, 2023 · 25 comments
Labels
approved Approved by CLC vote

Comments

@JoshMeredith
Copy link

Code available at https://gitlab.haskell.org/ghc/ghc/-/merge_requests/10128

Introduction

GHCJS previously implemented a Callback a data type, which is currently missing from GHC's JavaScript backend. This type represents functions passable in the FFI that use a more standard JavaScript calling convention - rather than the calling conventions used in the JS generated code.

GHCJS supported functions up to 3 arity, and it supported various synchronicities of callbacks. Arguments are passed as an untyped JSVal - essentially a representation of a plain JavaScript value.

Usage

To construct callbacks, the following functions are used depending on synchronisation and the existence (or lack thereof) of a return value:

data OnBlocked = ContinueAsync | ThrowWouldBlock

asyncCallback1 :: (JSVal -> IO ()) -> IO (Callback (JSVal -> IO ()))
syncCallback1 :: OnBlocked -> (JSVal -> IO ()) -> IO (Callback (JSVal -> IO ()))
syncCallback1' :: (JSVal -> IO JSVal) -> IO (Callback (JSVal -> IO JSVal))

Only the 1-arity signatures are demonstrated here, but sizes up to 3 - including 0 - are available.

These are expected to be used as a way to call into Haskell-generated code from regular JavaScript, via FFI imports. A simple example demonstrating this is as follows:

foreign import javascript "((f,x) -> { f(x); })"
  js_apply :: Callback (JSVal -> IO ()) -> JSVal -> IO ()

fn x = if isNull x then putStrLn "isNull" else fromJSString x

main = do
  f <- syncCallback1 ThrowWouldBlock fn
  js_apply f (toJSString "example!")
  releaseCallback f -- Memory can't be automatically freed because we don't know if the function is being held on the JavaScript side

Callbacks as FFI "exports"

Callbacks enable a form of FFI "exports", through setting global variables:

foreign import javascript "((f) => { globalF = f; })"
  setF :: Callback (JSVal -> IO ()) -> IO ()

fn x = if isNull x then putStrLn "isNull" else fromJSString x

main = setF =<< syncCallback1 ThrowWouldBlock fn

Then, if our generated Haskell-main is called in the HTML head, globalF will be available as a callback for e.g. HTML buttons.

Implementation

The GHCJS.Foreign.Callback module from the ghcjs-base package ports with minimal changes to the module and without changes to the current JavaScript backend (except for a few bugfixes that have already been merged).

@Kleidukos
Copy link
Member

I'm not CLC and this is a fairly naive question so feel free to ignore it, but shouldn't this live under the Foreign.* module hierarchy?

@JoshMeredith
Copy link
Author

I think you're right that that's one correct place for it to live, but IMO since it's more about defining functions as a JavaScript "primitive" type, it's good for it to live near GHC.JS.Prim.

@chshersh
Copy link
Member

@JoshMeredith Could you provide motivation for this change? Currently the proposal doesn't mention why you need to move this module to base from ghcjs-base.


In the context of the current discussion about the base split, it looks like the current mood of the crowd is to move GHC internal stuff from base somewhere else. And therefore, ghcjs-base sounds like the best place to have a module with name like GHC.JS.Foreign.Callback.

If there're any technical challenges that require to move this module to base, I believe @Ericson2314 would love to hear about them.

@JoshMeredith
Copy link
Author

ghcjs-base as a package existed for GHCJS (forked GHC), and doesn't support the upstreamed JavaScript backend for GHC - so rather than being a "move" of the module, it's part of the upstreaming effort of GHCJS into GHC.

While I personally agree with such split-base proposals, the upsteaming began previous to these, and as such we already have a significant proportion of ghcjs-base upsteamed - so this proposal seeks to continue the upstreaming and keep the code together.

@hasufell
Copy link
Member

I agree that the base split shouldn't be a blocker for this. This should be voted on as per the status quo.

@david-christiansen
Copy link

This also seems orthogonal to me, based on my understanding of the other proposal.

@Ericson2314
Copy link
Contributor

Yeah this is just an example of the issue that a ghc-base is trying to fix: for now this stuff falls under CLC purview for little good reason, but with a split base it could be internal only (at least initially).

Agreed for now just carry on ignoring splitting base, but remember this thing as motivation for split base thereafter.

ghcjs-base having a similar name is completely coincidental; it depended on base not the other way around.

@JoshMeredith
Copy link
Author

Yeah this is just an example of the issue that a ghc-base is trying to fix: for now this stuff falls under CLC purview for little good reason, but with a split base it could be internal only (at least initially).

Yes, I think that the JavaScript modules being conditionally compiled in base's cabal file are somewhat indicative of this being the case.

@Bodigrim
Copy link
Collaborator

AFAIU we are upstreaming a stable, battle-tested module from ghcjs-base to base as a part of efforts for JavaScript backend. The module is supposed to used by clients and forms a very base functionality for anyone intercting with JS FFI. I think that's fine and happy to +1, but would like to hear from other CLC members.

@mixphix
Copy link
Collaborator

mixphix commented Mar 27, 2023

This is the kind of thing that's on the edge for me. Should it go in base because it's critical for Haskell programs, or because it's critical for the Haskell compiler? I think the latter is a slightly worse reason in general, but in this case it's a tough call because it's functionality people trying out the new GHC backends will want to try.

Is there no other place to put this code in the GHC source tree?

@JoshMeredith
Copy link
Author

Should it go in base because it's critical for Haskell programs, or because it's critical for the Haskell compiler?

It's critical for Haskell programs.

GHC's JavaScript backend doesn't require any of the functionality implemented by this module. Rather, it is an entirely user-facing module that provides users with a way to interact with the browser (and JavaScript libraries in general - given the prevalence of callbacks). To not supply this functionality in base would be less ergonomic to most users of the JavaScript backend.

The examples provided in the description are demonstrations of usage in user code.

@chshersh
Copy link
Member

@JoshMeredith Thanks for clarifying the motivation behind this change and confirming that this module is indeed for base users and not GHC devs 😌

I'm +1 on this.


In general, it's an interesting design challenge. base is a standard Haskell library but GHC has multiple backends nowadays (native, JS, WASM), and different programs targetting different backends need different modules.

I would love to have base (or maybe some new core) to be something that is portable and works across all backends. So I would love to hear more why having a separate package like ghcjs-base with JS only modules is not the way forward.

But since the transition was already made with base having it all, let's not allow perfect be the enemy of good. I'm happy to proceed with the existing approach until we figure out something better.

@Bodigrim
Copy link
Collaborator

I would love to have base (or maybe some new core) to be something that is portable and works across all backends. So I would love to hear more why having a separate package like ghcjs-base with JS only modules is not the way forward.

I assume the new module is to be guarded by if arch(javascript) similar to existing modules in GHC.JS hierarchy. Thus it is invisible for non-JS users, and JS users can reasonably argue that when arch(javascript) this is a very base module.

@Bodigrim
Copy link
Collaborator

Bodigrim commented Apr 1, 2023

Dear CLC members, any more non-binding opinions on this?
@hasufell @parsonsmatt @angerman @tomjaguarpaw

@angerman
Copy link

angerman commented Apr 2, 2023

+1

1 similar comment
@hasufell
Copy link
Member

hasufell commented Apr 2, 2023

+1

@mixphix
Copy link
Collaborator

mixphix commented Apr 3, 2023

The arch(javascript) guard would make this much more palatable for me.

@JoshMeredith
Copy link
Author

The arch(javascript) guard would make this much more palatable for me.

This is indeed the case in the linked merge request.

@Bodigrim
Copy link
Collaborator

Bodigrim commented Apr 3, 2023

Dear CLC members, let's vote on the proposal to port ghcjs-base:GHCJS.Foreign.Callback as base:GHC.JS.Foreign.Callback guarded by if arch(javascript) as detailed in https://gitlab.haskell.org/ghc/ghc/-/merge_requests/10128/diffs.

@tomjaguarpaw @chshersh @mixphix @hasufell @angerman @parsonsmatt


+1 from me.

@chshersh
Copy link
Member

chshersh commented Apr 3, 2023

+1

4 similar comments
@parsonsmatt
Copy link

+1

@hasufell
Copy link
Member

hasufell commented Apr 4, 2023

+1

@angerman
Copy link

angerman commented Apr 4, 2023

+1

@mixphix
Copy link
Collaborator

mixphix commented Apr 4, 2023

+1

@Bodigrim
Copy link
Collaborator

Bodigrim commented Apr 7, 2023

Thanks all, 6 votes in favour out of 7 are enough, approved.

@Bodigrim Bodigrim closed this as completed Apr 7, 2023
@Bodigrim Bodigrim added the approved Approved by CLC vote label Apr 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Approved by CLC vote
Projects
None yet
Development

No branches or pull requests

10 participants