Rescript-compiler: Change in how bs raw ([%raw]) behaves

[Risto-Stevcev]

Steps to replicate

• Initialize a new project using bsb -init [some name]
• Edit src/demo.ml to be:

let foo: unit -> unit = [%raw {|
  function() {
    console.log('foobar');
  }
|}]

let _ = foo ()

let () = Js.log "Hello, BuckleScript"

• Compile and run src/demo.bs.js

Expected

I'm expecting that the output should be the same for bs-platform versions 4.0.3 and 4.0.7

Actual

If you do those steps for 4.0.3, it runs as expected and outputs:

foobar
Hello, BuckleScript

But if you do those steps for 4.0.7, it will throw an error:

foobar
/home/risto/.config/yarn/global/node_modules/bs-platform/lib/js/curry.js:9
    var arity = f.length;
                  ^

TypeError: Cannot read property 'length' of undefined
    at app (/home/risto/.config/yarn/global/node_modules/bs-platform/lib/js/curry.js:9:19)
    at curry_1 (/home/risto/.config/yarn/global/node_modules/bs-platform/lib/js/curry.js:57:14)
    at Object._1 (/home/risto/.config/yarn/global/node_modules/bs-platform/lib/js/curry.js:66:12)
    at Object.<anonymous> (/home/risto/git/bucklescript/tmp/after/src/demo.bs.js:12:7)
    at Module._compile (internal/modules/cjs/loader.js:688:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:699:10)
    at Module.load (internal/modules/cjs/loader.js:598:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:537:12)
    at Function.Module._load (internal/modules/cjs/loader.js:529:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:741:12)

[anmonteiro]

I think this is related to the breaking change introduced in 4.0.7. Most likely, uncurrying explicitly will make it work.

[Risto-Stevcev]

What was the motivation for doing that? I can't find any ticket about it
Also would be nice to have a "Breaking changes" section in the changelog

[anmonteiro]

@Risto-Stevcev https://github.com/BuckleScript/bucklescript/pull/3073 is the PR that broke this. I've also been trying to understand why a breaking change would be released as a patch version.

[bobzhang]

hi @Risto-Stevcev technically it is not a breaking change since it is an undefined behavior no arity guarantee when a curried type signature is enforced, we can only make such guarantee for uncurried type signature, we introduced such change to make the generated code more predictable.
I do have an announcement a couple of weeks before this release on discord, but it seems to be mostly ignored, I will take this lesson and make an announcement in reasonml.chat in the forum next time

[Risto-Stevcev]

I don't understand, why is there no arity guarantee for curried type signatures?
IMO it's a breaking change because it breaks existing code

[bobzhang]

hi @Risto-Stevcev It is not because we don't want, it is because it is impossible. Take this for example

let f1 (x : unit ) = Js.log x 
let f2 : unit -> unit = fun _ -> Js.log "hello"

In theory, it is impossible to infer the arity just by a curried type signature

[Risto-Stevcev]

What's the issue with that example, aren't both of those unary functions (arity of 1)?

[bobzhang]

The example I gave shows that you can not infer the arity by just reading the signature.
Here is another common example:

let f1 : int -> int -> int = fun x  y -> x + y
let f2 : int -> int -> int = fun x -> let z = x * x in z + x 

In general, it is strongly recommended to annotate as uncurried function when dealing with bs.raw which gets guaranteed semantics. It will work in most cases, but may suddenly get broken (undefined behavior)

[Risto-Stevcev]

I see what you're saying, but why does this change the behavior of %raw? there's nothing the compiler can guarantee for FFI boundaries. What did this "fix"?

[bobzhang]

Previously we try to remove unused arguments for a function which makes the generated code less predictable, as a consequence we introduced a hack in the runtime which treat the function with arity = 0 and arity = 1 the same. Since the hack is remove so we treat function with arity = 1 and arity = 0 differently.
In your case, mark it as unit -> unit [@bs] will ensure it is always successful

[Risto-Stevcev]

Would you accept a PR (or add one) to describe the breaking changes in Changes.md in a bit more detail?
Right now there's nothing in the changelog that explains any of this in there. That and because it's only a patch version bump and not caught by normal semver ranges in package.json is why I thought this was a bug

[bobzhang]

@Risto-Stevcev it is listed https://github.com/BuckleScript/bucklescript/blob/master/Changes.md#407 here, yes, a PR is welcome. I also plan to write an article about it this week, thanks for asking

[bobzhang]

@Risto-Stevcev an article added https://github.com/BuckleScript/bucklescript.github.io/pull/82/files