Wrong compilation of closures

[hhugo]

The following program raises but should not.
Fixed in js_of_ocaml by ocsigen/js_of_ocaml@90456a3

let () =
  let delayed = ref (fun () -> ()) in
  for i = 1 to 2 do
    let rec f n = function
      | 0 -> assert (i = n)
      | j -> delayed :=
         let prev = !delayed in
         (fun () -> prev (); f (succ n + i - i) (pred j))
    in f 0 i
  done;
  !delayed ();;

[bobzhang]

thanks for sharing

[hhugo]

Here is a slightly different example:

let direct = ref []
let indirect = ref []

let () =
  for i = 0 to 3 do
    let rec f = function
      | 0 -> i
      | -1 -> g (-2) (* to prevent g from been inlined *)
      | n -> g (pred n)
    and g = function
      | 0 -> i
      | -1 -> f (-2)  (* to prevent f from been inlined *)
      | n -> f (pred n)
    in
    direct   := f i :: !direct;
    indirect := (fun () -> f i) :: !indirect
  done;
  let indirect = List.map (fun f -> f ()) !indirect in
  let direct = !direct in
  assert (indirect = direct)

[bobzhang]

Hi, I was aware of the issue, will fix it once I got a working build tool, thanks for sharing.
Btw, if we compile to ES6 using let, this compilation would be much simplified..

reply@reply.github.com At: 10/31/16 19:42:31" data-digest="From: reply@reply.github.com At: 10/31/16 19:42:31" style="">
From: reply@reply.github.com At: 10/31/16 19:42:31
To: bucklescript@noreply.github.com
Cc: HONGBO ZHANG (BLOOMBERG/ 731 LEX), comment@noreply.github.com
Subject: Re: [bloomberg/bucklescript] Wrong compilation of closures (#856)

Here is a slightly different example: let direct = ref []
let indirect = ref []

let () =
for i = 0 to 3 do
let rec f = function
| 0 -> i
| -1 -> g (-2) (* to prevent g from been inlined )
| n -> g (pred n)
and g = function
| 0 -> i
| -1 -> f (-2) ( to prevent f from been inlined *)
| n -> f (pred n)
in
direct := f i :: !direct;
indirect := (fun () -> f i) :: !indirect
done;
let indirect = List.map (fun f -> f ()) !indirect in
let direct = !direct in
assert (indirect = direct)

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.

[OvermindDL1]

Huh, this explains an issue I had a while back...

I am entirely for ES6 compilation, most browsers already support it, and if someone wants to support it on something that does not then it is simple to transpile it using brunch (as my setup already does anyway) or webpack or whatever. ES6 code with ES6 modules and all are best.

[malthe]

I can't reproduce this.

However, this issue might be the same as what happens with this program:

let () =
  for i = 0 to 2 do
    let go j =
      ignore j;
      Js.log(i)
    in
    go i;
    Js.Global.setTimeout (fun () -> go i) 500 |. ignore
  done

Which produces the output: 0, 1, 2, 2, 2, 2.

The problem is that in Javascript, a for-loop does not have its own scope, but it obviously does in OCaml.

In the generated code below, it's clear how the go function is not part of the closure because there is an incorrect assumption about the scope of the for-loop.

  for(var i$1$1 = 0; i$1$1 <= 2; ++i$1$1){
    var go = (function(i$1){
    return function go(j) {
      console.log(i$1);
      return /* () */0;
    }
    }(i$1$1));
    go(i$1$1);
    setTimeout((function(i$1){
        return function (param) {
          return go(i$1);
        }
        }(i$1$1)), 500);
  }

Below is a different version of the same program that also triggers the issue:

let () =
  let rec go i =
    if i = 3 then
      ()
    else
      let log j =
        ignore j;
        Js.log(i)
      in
      log i;
      Js.Global.setTimeout (fun () -> log i) 500 |. ignore;
      go (i + 1)
  in
  go 0

[kevinji]

I think this issue is resolved if you consistently use let bindings; e.g.

for(let i$1 = 0; i$1 <= 2; ++i$1){
  let go = function go(j) {
    console.log(i$1);
    return /* () */0;
  };
  go(i$1);
  setTimeout(function (param) {
    return go(i$1);
  }, 500);
}

has the expected behavior.

[hhugo]

I'm surprised this issue has not been solved after 4 years. Should the high priority label be removed ?

[bobzhang]

this should be an easy fix given that let performance is quite good now. Will have a look when we finish the build issue. There are lots of issues, sorry it skipped