ocsigen / js_of_ocaml

Compiler from OCaml to Javascript.
http://ocsigen.org/js_of_ocaml/
Other
943 stars 185 forks source link

[BUG] `--enable=effects` may cause incorrect compilation results (TypeError: cont is not a function) #1589

Closed kxc-wraikny closed 3 months ago

kxc-wraikny commented 3 months ago

Describe the bug When building the following code with js_of_ocaml, if --enable=effects is specified, execution fails with the error TypeError: cont is not a function. If --enable=effects is not specified, it runs fine.

https://github.com/kxc-wraikny/jsoo_effects_bug_reproduction/blob/1a387c59e7b78df5986cadcd243d40fb5542edb9/test/jsoo_effects_bug_reproduction.ml

$ dune runtest
File "test/dune", line 8, characters 0-105:
 8 | (rule
 9 |  (alias runtest)
10 |  (deps jsoo_effects_bug_reproduction.bc.js)
11 |  (action
12 |   (run %{bin:node} %{deps})))
[07:44:43.085] [SUCCESS] (1/3) plus - should return 2 by 1, 1
[07:44:43.085] [SUCCESS] (2/3) plus - should return 3 by 1, 2
/Users/wk/Projects/jsoo_effects_bug_reproduction/_build/default/test/jsoo_effects_bug_reproduction.bc.js:5730
     throw err;
     ^

TypeError: cont is not a function
    at Timeout._onTimeout (/Users/wk/Projects/jsoo_effects_bug_reproduction/_build/default/test/jsoo_effects_bug_reproduction.bc.js:44956:43)
    at listOnTimeout (node:internal/timers:573:17)
    at process.processTimers (node:internal/timers:514:7)

Node.js v20.10.0

repo: https://github.com/kxc-wraikny/jsoo_effects_bug_reproduction/

Expected behavior The built js code executed correctly without errors.

Versions

vouillon commented 3 months ago

There is a bug in prr. In file fut.ml, the definition of tick should be:

let tick ~ms =
  fut @@ Jv.Promise.create @@ fun res _rej ->
  ignore (Jv.apply (Jv.get Jv.global "setTimeout") Jv.[| callback ~arity:1 res; of_int ms |])
haochenx commented 3 months ago

@vouillon many thanks for taking time to look into it. I just applied your suggested fix and now the test passes!

As prr is merely a fork (and out-of-date version) of brr and it seems that brr still has the same code, we will open a bug report on brr as well.

Do you mind to share how you traced the root cause down? We found it quite difficult to trace such bugs as stack trace isn't very helpful (even if source map is on..)

vouillon commented 3 months ago

This looked like a calling convention mismatch (an OCaml function called from JavaScript as if it was a JavaScript function). The error was in some prr code related to promises (function Jv.Promise.create), and indeed in a callback. And the stack trace mentions Timeout._onTimeout, so I searched for a setTimeout.

haochenx commented 3 months ago

Wow that's impressive and thanks for the hint!

Fixing fut.ml made our codebase all worked again. I think this issue can be closed as it's now apparent that it's due to a misuse instead of a bug in jsoo.

(@kxc-wraikny opened this issue on behalf of our team but @kxc-wraikny is taking a break until Mar 28; so I will be handing the communication until then.)

dbuenzli commented 3 months ago

Thanks, this has been fixed in https://github.com/dbuenzli/brr/commit/5b1dbedc1fb98ef553dcacffe2695bd403a66cd1. It seems I missed that in the PR by Jérôme and subsequent commit of mine that was supposed to sanitize all that.

vouillon commented 3 months ago

Thanks, this has been fixed in dbuenzli/brr@5b1dbed. It seems I missed that in the PR by Jérôme and subsequent commit of mine that was supposed to sanitize all that.

@dbuenzli Thanks! I think I missed it in my PR as well.

haochenx commented 3 months ago

Thank you all again!