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

[@bs] at application side treats f () as 0-arity application #674

Open
andreypopp opened this issue Aug 4, 2023 · 17 comments · May be fixed by #949
Open

[@bs] at application side treats f () as 0-arity application #674

andreypopp opened this issue Aug 4, 2023 · 17 comments · May be fixed by #949
Milestone

Comments

@andreypopp
Copy link
Contributor

Consider this gist:

let sleep ms =
  Js.Promise.make @@ fun ~resolve ~reject:_ ->
  ignore (Js.Global.setInterval (fun () -> (resolve () [@bs])) ms)

which errors with

Line 3, 44: Error This expression has type ('a -> unit)   
  Js__Js_internal.Fn.arity1 but an expression was expected of type 'b  
  Js__Js_internal.Fn.arity0

The issue is that Melange treats f ()[@bs] syntax as 0-arity application.
If we do

let unit = () in f unit [@bs]

then it compiles.

I think 0-arity applications should have another syntax.

@anmonteiro
Copy link
Member

I took a look at this. Here are some thoughts:

  • f () [@u] (previoulsy [@bs]) is a syntactical transformation
    • if the only argument is (), melange considers it a 0-arity application
  • We're making a few breaking changes for Melange 2.0. If desirable and agreed upon, changing the 0-arity application syntax is possible

Here are some suggestions for a new 0-arity application syntax:

  1. introduce a new attribute:
let () = f () [@u0] (* or any other attribute different than [@u] *)
  1. introduce a new module:
let () = Uncurried.apply0 f
  1. introduce a new special operator:
let () = f $$u
  1. introduce a new let operator:
let>$ () = f ()
  1. introduce a new extension point:
let%u0 () = f ()

@jchavarri
Copy link
Member

Sorry for the probably stupid questions, but I never understood this:

  • Why is it necessary to mark functions with arity 1 with @u?
  • What does "arity 0" mean? If a function has no arguments, then it's not a function, right?

@anmonteiro
Copy link
Member

  • f () [@u] becomes Js.Internal.run f, compiles to f() in JS
  • f arg1 .. argN [@u] becomes Js.Internal.Fn.arityN, compiles to f(arg1, .., argN)

so 0-arity is different than n-arity for uncurried because:

  • in OCaml it still has 1 argument
  • we want to compile to f() and not f(undefined)
  • this needs to be decided at PPX level

@jchavarri
Copy link
Member

Thanks for the explanation. About the options available, as a user, option 1 seems more "natural" to me, I am not sure about technical implications and how confusing it might be to other users.

I am also surprised this didn't come up earlier? Sounds like a really strong limitation. 🤔

@andreypopp
Copy link
Contributor Author

Is it possible to use f [@u0]?

Otherwise my vote is for option 1.

@anmonteiro
Copy link
Member

I am also surprised this didn't come up earlier?

It did: rescript-lang/rescript#3429

@anmonteiro
Copy link
Member

anmonteiro commented Aug 10, 2023

Is it possible to use f [@u0]?

It's technically possible, but that doesn't look like function application anymore. Something to ponder.

@anmonteiro
Copy link
Member

This seems to come up especially for Promise.make, which is defined as:

external make :
  ((resolve:(('a -> unit)[@u]) -> reject:((exn -> unit)[@u]) -> unit)
  [@mel.uncurry]) ->
  'a t = "Promise"
[@@mel.new]

However, both resolve and reject only take 1 argument each, and they probably don't need to be curried at all.

So we could fix the immediate case with

external make :
  ((resolve:('a -> unit) -> reject:(exn -> unit) -> unit)[@mel.uncurry]) -> 'a t
  = "Promise"
[@@mel.new]

This would be a breaking change, but so would changing the 0-arity application syntax.

@anmonteiro
Copy link
Member

anmonteiro commented Aug 10, 2023

OK, I suppose the issue occurs when you pass resolve as an argument. Consider the following program:

type 'a t

external make :
  ((resolve:('a -> unit) -> reject:(exn -> unit) -> unit)[@mel.uncurry]) -> 'a t
  = "Promise"
[@@mel.new]

external makeU :
  ((resolve:(('a -> unit)[@u]) -> reject:((exn -> unit)[@u]) -> unit)
  [@mel.uncurry]) ->
  'a t = "Promise"
[@@mel.new]

let r resolve =
  Js.log "resolve normal";
  resolve "normal"

let rU resolve =
  Js.log "resolve uncurried";
  resolve "uncurried" [@u]

let x = make (fun ~resolve ~reject -> r resolve)
let xU = makeU (fun ~resolve ~reject -> rU resolve)

and the generated JS:

$ melc -ppx melppx x.ml

// Generated by Melange
'use strict';

var Curry = require("melange.js/curry.js");

function r(resolve) {
  console.log("resolve normal");
  return Curry._1(resolve, "normal");
}

function rU(resolve) {
  console.log("resolve uncurried");
  return resolve("uncurried");
}

var x = new Promise((function (resolve, param) {
        r(resolve);
      }));

var xU = new Promise((function (resolve, param) {
        rU(resolve);
      }));

exports.r = r;
exports.rU = rU;
exports.x = x;
exports.xU = xU;
/* x Not a pure module */

@jchavarri
Copy link
Member

jchavarri commented Aug 11, 2023

Hm, and it seems that mel.uncurry only works with the "outer" type of the external, but not to annotate "internal" types like the labelled resolve: example.

Input:

type 'a t

external make :
  ((resolve:(('a -> unit)[@bs.uncurry]) -> reject:(exn -> unit) -> unit)[@bs.uncurry]) -> 'a t
  = "Promise"
[@@bs.new]

let r resolve =
  Js.log "resolve normal";
  resolve "normal"

let x = make (fun ~resolve ~reject -> r resolve)

Output:

// Generated by Melange

import * as Curry from "melange.js/curry.js";

function r(resolve) {
  console.log("resolve normal");
  return Curry._1(resolve, "normal");
}

var x = new Promise((function (resolve, reject) {
        r(resolve);
      }));

export {
  r ,
  x ,
}
/* x Not a pure module */

@anmonteiro
Copy link
Member

I propose we postpone this for v3 to give us some more room for brainstorming.

In the meantime, we should probably document this in the website as an edge case.

@anmonteiro anmonteiro added this to the Melange v3 milestone Oct 19, 2023
@anmonteiro
Copy link
Member

anmonteiro commented Jan 1, 2024

#949 is progressing nicely, though a bit more invasive than I'd like. I didn't expect this would touch so much code.

There are currently 2 open questions:

  • what to do about [@mel.meth]?
    • it currently uses the same detection as [@u] and a single unit argument. A few options:
      1. [mel.meth0] doesn't have the same ring to it as [@u0]
      2. [mel.meth] and [@u0] together -- this becomes a bit harder to explain since [@mel.meth] is assumed to be uncurried and doesn't need [@u] otherwise
      3. keep the previous behavior of unit meaning 0-arity -- the downside here is that there are 2 behaviors for uncurried now, and that is certainly confusing.
        • since [@mel.meth] is way less used than [@u] I'm going to keep the previous behavior for now to unblock the PR.
  • this is quite the breaking change for code using [@u] and unit arguments. If we merge feat: uncurry 0 #949, every function that was inferred to be 0-arity will now become 1-arity with an undefined argument (because that's what unit compiles to). I think this one can be easily detected and add an upgrade path:
    1. add a transitory alert when detecting unit -> 'a and [@u], warning that users probably want [@u0] instead

@anmonteiro anmonteiro linked a pull request Jan 9, 2024 that will close this issue
@jchavarri
Copy link
Member

  • [mel.meth0] doesn't have the same ring to it as [@u0]

What about mel.meth_no_params? I think we should stay consistent, as you mention in the PR.

  1. add a transitory alert when detecting unit -> 'a and [@u], warning that users probably want [@u0] instead

This sounds great.

@anmonteiro
Copy link
Member

  1. add a transitory alert when detecting unit -> 'a and [@u], warning that users probably want [@u0] instead

I had forgotten to add a test showing what the alert looks like. Added a commit to #949: 66a247a

@anmonteiro
Copy link
Member

What about mel.meth_no_params?

probably worth it to be consistent indeed. I don't like mel.meth_no_params either.

Another option I thought about was @mel.meth_void, but I'd probably still prefer @mel.meth0 in that case.

@jchavarri
Copy link
Member

I also prefer meth0 to meth_void fwiw.

@anmonteiro
Copy link
Member

In our meeting today we decided to:

  • drop the work for @mel.meth0
  • drop @mel.meth entirely from the melange ffi
    • perhaps deprecate first, then remove for v4

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

Successfully merging a pull request may close this issue.

3 participants