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

rfc: bindings #777

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

rfc: bindings #777

wants to merge 4 commits into from

Conversation

jchavarri
Copy link
Member

A proposal to improve the bindings layer in Melange.

Preview markdown.

@texastoland
Copy link

texastoland commented Oct 15, 2023

I have some ad hoc bindings:

module Performance = struct
  external now : unit -> float = "performance.now"
end

module BigInt = struct
  type t
  type op = t -> t -> t

  external toString : t -> string = "toString" [@@mel.send]

  let zero : t = [%mel.raw "0n"]
  let one : t = [%mel.raw "1n"]
  let ( + ) : op = [%mel.raw "(x, y) => x + y"]
end

They'd become:

module Performance = struct
  let%js now : unit -> float = "performance.now()"
end

module BigInt = struct
  type t

  let%js zero : t = "0n"
  let%js one : t = "1n"

  let%js toString : this_:t -> string = "$this_.toString()"
  let%js ( + ) : x_:t -> y_:t -> t = "$x_ + $y_"
end

Correct? I like:

  • my operator doesn't require a closure
  • annotations don't feel like recalling physics equations
  • JS function invocation is explicit

Could you support positional arguments inspired by Swift?

let%js toString : t -> string = "$0.toString()"
let%js ( + ) : op = "$0 + $1"

@jchavarri
Copy link
Member Author

@texastoland Your snippet looks good, but I think you're missing let%js in definitions of toString and +.

Could you support positional arguments inspired by Swift?

I thought about this, but it introduces some complexity if we mix positional and labeled arguments in the same expression. If I have let%js foo : x:t -> u -> v and I want to refer to the positional argument, would it be $0 (the first positional argument) or $1 (the second argument)? I would like users to not have to think about this kind of stuff, so maybe we could establish a rule that either one labels all arguments or leaves them all positional. Wdyt?

@texastoland
Copy link

texastoland commented Oct 16, 2023

I think you're missing let%js in definitions

Oops fixed 🙈

it introduces some complexity if we mix positional and labeled arguments in the same expression

My intuition is positional terms refer to unlabeled arguments otherwise you'd use $label.

maybe we could establish a rule that either one labels all arguments or leaves them all positional

That's a reasonable compromise if you think it'd be confusing. 1 labeled argument with many positional ones sounds undesirable anyway. I prefer vice versa.

I was just experimenting more and love that it generalizes arbitrary JS including ..., new, throw, or await! It reminds me of Fable's FFI but simpler.

PureScript originally exposed a similar feature but removed it in purescript/purescript#887. Their reasoning was it tied libraries to specific compiler back ends. I assume Melange's solution is selective compilation with Dune?

PS this is 🔥

@jchavarri jchavarri mentioned this pull request Oct 17, 2023
@texastoland
Copy link

1 additional note I like and prefer the js. convention compared to mel. (or bs.). I assume Melange won't be competing for that namespace.

@johnhaley81
Copy link

This is a really great improvement towards a binding API that doesn't make me need to look up the docs each time I need to do it (even after like 6 years 😵‍💫).

So after reading through this I have 2 thoughts:

Mixing arg types

we could establish a rule that either one labels all arguments or leaves them all positional.

That would prevent things like t-middle bindings without some workarounds. Ex:

let%js slice : indexEnd:int -> string -> indexStart:int -> string = "$0.slice($indexStart, $indexEnd)"

Would have to become something like:

let%js slice : indexStart : int -> indexEnd : int option -> str : string -> string = "$str.slice($indexStart, $indexEnd)"
(* using shadowing to override the API *)
let slice ?indexEnd str ~indexStart => slice(indexStart, indexEnd, str)

Which might be ok but I wanted to present a valid use case for mixing labeled, optional labeled, and positional args.

Importing class modules

Before:

type t
external book : unit -> t = "Book" [@@mel.new] [@@mel.module]
let myBook = book ()

After:

type t
let%js.import book: t = "Book"
let%js create_book : ~t_:t -> unit -> t = "new $t_()"
let myBook = book |> create_book ()

Maybe the After was typo-ed but it is resulting in a more confusing API IMO. The module that is imported is of type t and you need that type to create the type t after calling create_book? Do I have to create a second type to pass into the first one?

Maybe we could have an import_constructor?

type t
let%js.import_constructor book: unit -> t = "Book" (* converts to "new Book(...args)" *)
let myBook = book()

wdyt?

@johnhaley81
Copy link

Speaking of needing to reference the docs every time I make a binding...

Relevant XKCD

@texastoland
Copy link

texastoland commented Oct 21, 2023

I think your slice example would just be:

let%js slice : ?end_:int -> this_:string -> start:int -> string = "$this_.slice($start, $end_)"

Underscore is supposed to allow it to be used positionally (not sure why though). I'm not familiar with "t-middle" style?

@texastoland
Copy link

texastoland commented Oct 21, 2023

@johnhaley81 I think your book example should be:

(* Book module *)
type t

open struct (* private? *)
  let%js.import construct = "@book/lib"
end

let%js make : unit -> t = "new construct()"

(* in another file *)
let book = make ()

The issue I see with a special constructor annotation is it doesn't add labels, spread arguments in JS, etc.

@jchavarri
Copy link
Member Author

Thanks for the feedback @texastoland @johnhaley81.

Mixing arg types

As Texas mentioned above, you can use the underscored labelled args to place positional arguments wherever in the signature. Using a shorter name like t_ would require to type a few extra characters (t_ :) more than the introduction of $0, $1 for positional args 🤔

let%js slice : indexStart : int ->  t_ : string -> indexEnd : int option -> string = "$t_.slice($indexStart, $indexEnd)"
(* no need for shadowing to override the API *)

Importing class modules

The module that is imported is of type t and you need that type to create the type t after calling create_book? Do I have to create a second type to pass into the first one?

I think the way I see it is: there is a type to represent the default value exported by the JavaScript module itself (the class), and another type to represent the values / instances created when the class is invoked:

(* the type of the js class "Book" *)
type c 
(* the type of instances created  *)
type i 
let%js.import book: c = "Book"
let%js create_book : ~c_:c -> unit -> i = "new $c_()"
let myBook = book |> create_book ()

I am not sure about adding import_constructor because it wouldn't solve the problem if there are multiple values being destructured from the JS module, e.g. let%js.import {foo = (hey : t); bar: u} = "./bar". What if foo is a class but bar isn't?

Maybe we could allow decorating types in the destructuring using new or some other attribute:

type t
let%js.import (book : (unit -> c[@new])) = "Book"
let myBook = book()

This would play well with more complex destructuring:

let%js.import {foo = (hey : (string -> int -> c[@new])); bar: u} = "./bar"

wdyt?

@texastoland
Copy link

texastoland commented Oct 21, 2023

@jchavarri Did you see my solution? Another way of using import is an imperative statement that introduces identifiers in the current JS environment. Its type can be inferred or unit.

type t
let%js.import construct = "@book/lib"
let%js make : unit -> t = "new construct()"

IFL the @new annotation:

  1. Adds cognitive burden
  2. Is less explicit (more magical)
  3. Doesn't cover cases like spreading array arguments
  4. Doesn't save that much typing

@jchavarri
Copy link
Member Author

@texastoland yeah, I agree that @new somehow feels like going back.

Your solution could work, but it would need some changes to prevent breakages. Notice that Melange identifiers can compile to different JavaScript identifiers for various reasons. For example, this code:

let t = Js.Date.make()
let u b = t
let t = 3

Will produce:

var t = new Date();
function u(b) {
  return t;
}
var t$1 = 3;

The second t identifier is compiled into t$1.

So we can't directly reference Melange identifiers safely from the JavaScript code in the let%js string payload.

To work around this, maybe we can rely in the recently added mel.as functionality:

type t
let%js.import [@mel.as "Book"] book = "@book/lib"
let%js make : unit -> t = "new Book()"

Maybe there could be some other way to directly reference Melange identifiers in scope from the JavaScript strings, using $ as well as a way to tell the compiler, e.g.:

type t
let%js.import book = "@book/lib"
let%js make : unit -> t = "new $book()"

This solution looks more complex technically, as we would need to "resolve" the Melange identifiers into the JS ones, @anmonteiro must know if this could be done directly from a ppx, but I believe it would need a deep integration with the compiler.

@texastoland
Copy link

texastoland commented Oct 21, 2023

Thanks that makes sense. I guess I assumed @as (using the same name as OCaml) was implied. It reminds me of the Disord discussion about callbacks having @called_from_js by default. But I like the $ notation for consistency! In (my) perfect world no other annotations would be necessary.

Comment on lines +48 to +49
The preprocessor of `let%js` will check that type definitions to have at least
an arrow type. So in cases where before one would just have some abstract type
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't this contradictory with the pi example below? where's the arrow type then?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is. I think I started from a point where removing single typed externals was a goal, but as the proposal evolved, I got distracted by migrating existing bindings to the proposal approach.


## `let%js.obj`

Would be similar to `mel.obj` but as an extension applied to `let` bindings:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so it's never possible to construct a JS object without using a let binding?

how would you express Js.log([%mel.obj { foo= "bar"}])

- Another source of friction comes from the optimizations done by the compiler
in `external` statements. Melange compilation model is module-oriented, unlike
js_of_ocaml, which has an executable-oriented compilation model. In Melange,
users expect that the code referenced by an external function will be compiled
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this an actual expectation of users?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is, either consciously or unconsciously, considering how confused we all are when bugs related to this happen.

itself

Sometimes only the first situation happens. For example, if we write bindings to
`React.useCallback` we do not need to care about uncurrying, because the `react`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is simply incorrect. You can pass the result of React.useCallback to a JavaScript library that calls the function.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you pass the result of React.useCallback to a JavaScript library that calls the function, there will be some binding to that function. There should be mel.uncurry at the boundary layer of that function, not at the React.useCallback definition.

If we apply your point more generically, every function in a Melange program should be uncurried because potentially it can be passed to a JavaScript library as callback.

cb:('a -> 'b -> 'c) ->
'c array =
[%mel.raw
"function (arr1, arr2, cb) { return map(arr1, arr2, Js.wrap_callback(cb)) }"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the definition of Js.wrap_callback(cb)? I don't understand this example.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@anmonteiro
Copy link
Member

anmonteiro commented Nov 6, 2023

OK so I took a first look at this. Here are some unrefined thoughts (I need to analyze the proposal a bit more carefully):

  • I'm not the target audience for this proposal, and I personally find it harder to read compared to the current external-based FFI
  • I find it weird to use let for the FFI when the JavaScript is out-of-band (external fits very well here)
    • I can't help but wonder that the errors will be even worse if you forget to apply the PPX, e.g. you get a type mismatch on let%js foo: foo:int -> unit = {| some string |}
  • I think this proposal is not that easy to implement and get right, especially to get all the validation right
  • The let%js.obj proposal doesn't make sense to me. It feels like there is some confusion between [%mel.obj ...] and external ... [@@mel.obj]
    • I don't know why the first one needs to be changed, since it's an extension point, and not part of the FFI, but maybe you meant the latter. In that case you'd need to include the labeled arguments anyway and a type annotation, right?

Those are all the downsides I found with this approach.

That said, I think this is a great proposal, and absolutely worth it to implement and put in front of users. If it turns out it works, and people like it better, then we should refine it and move forward with it.

As for moving forward, I wonder:

  • can this be implemented as
    1. a PPX either in userland, or even in the melange repo?
    2. can the first version compile to external declarations + mel.raw? I know that probably wouldn't satisfy all the constraints, but I think it could give us a starting point that's a lot easier to implement than the entire proposal.

As a final note: let me reiterate that I've only read this once and need to think more carefully about it holistically, so please take my (subjective) comments with a grain of salt.

@jchavarri
Copy link
Member Author

jchavarri commented Nov 6, 2023

Thanks for the feedback @anmonteiro

  • I'm not the target audience for this proposal, and I personally find it harder to read compared to the current external-based FFI

Is this for everything in the proposal? I find surprising that examples like mel.send or importing with mel.module are easier to read than the proposal alternatives.

  • I find it weird to use let for the FFI when the JavaScript is out-of-band (external fits very well here)

I would argue that using external is misleading because it wouldn't behave like OCaml externals, both in terms of semantics and optimizations (the let transformations wouldn't be inlined at callpoints).

I think this proposal is not that easy to implement and get right, especially to get all the validation right

I don't think the current approach to externals in Melange is simple, nor easy to maintain. The amount of combinations that exist between the different attributes makes it (imo) much more complex to validate. But of course it's already implemented, while this proposal is not.

  • The let%js.obj proposal doesn't make sense to me. It feels like there is some confusion between [%mel.obj ...] and external ... [@@mel.obj]

Yes, I think this and the import limitations we discussed above are the 2 weakest points of the proposal. I would still have to think more about it. Worst case, mel.obj could stay like it is now.

As for moving forward, I wonder:

can this be implemented as
  
  1. a PPX either in userland, or even in the melange repo?
  2. can the first version compile to external declarations + `mel.raw`? I know that probably wouldn't satisfy all the constraints, but I think it could give us a starting point that's a lot easier to implement than the entire proposal.

This is precisely what I had in mind, so I think I might proceed with it (if/when time allows).

@andreypopp
Copy link
Contributor

I really like the proposal, few comments:

  • big 👍 on defining bindings purely with JS syntax
    • I hope it's possible to implement so it can just inline the JS fragment into the output w/o any function overhead
  • I agree about external n : t = "" syntax being more natural instead of introducing let%js extension nodes:
    • external is made for FFI and we are doing FFI here
    • as for ocamlformat reformatting into "" - probably not a big deal as strings in JS can be single quoted ''
    • don't understand how compilation to native should drive JS FFI here (my understanding is that shared code shouldn't contain neither JS nor C FFI)
  • similar feeling about let%js.import, maybe something like this instead:
    (* var {name} = require('./module.js') *)
    (* import {name} from './module.js' *)
    external%import name : t = "./module.js"
    
    (* var name = require('./module.js') *)
    (* import * as name from './module.js' *)
    external%import_namespace name : <..> Js.t = "./module.js"
    
    
    yes, that won't have the little DSL to automatically destructure the module, but I feel this is for the better (one less thing to learn)
  • as for [@@mel.obj], maybe replace it with something like (inline with native backend intrinsics like %identity):
    external name : t = "%obj"
    

So to summarise — if all FFI uses could be covered with (hope I didn't forget anything):

  1. external name : t = "some(js.with($names))"
  2. external%import name : t = "path.js"
  3. external%import_namespace name : t = "path.js"
  4. external name : t = "%obj" to replace [@@mel.obj]
  5. external%raw name : t = "function(..) { ... }" to replace [@@mel.raw]
  6. [@mel.uncurried]

I'd love that!

P.S. commenting on current state of JS FFI, I agree that it is a bit too complex. I feel this is because of over reliance on attributes — need to think which attribute and to what place to attach.

@dmmulroy
Copy link

dmmulroy commented Nov 8, 2023

I don't have much of a horse in this race, but I largely agree with everything @andreypopp has said. I personally think that the external keyword makes a lot of sense for bindings and I think I much prefer syntax like:

external name : t = "some(js.with($names))"
external%import name : t = "path.js"
external%import_namespace name : t = "path.js"
external name : t = "%obj" to replace [@@mel.obj]
external%raw name : t = "function(..) { ... }" to replace [@@mel.raw]

That being said, I also don't know or understand all of the implications of that wrt the compiler and or PPXs, but regardless I'm stoked to see thought, experimentation, and iteration going into this - your rfc was well written @jchavarri 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants