[BUG] regression causing some definitions in recursive functions to be out of scope

[rickyvetter]

Describe the bug
I'm seeing an issue where a very specific set of side effects and recursion used together causes a runtime "value is not defined" error in strict mode and can't be executed in most environments.

A file I've been able to reproduce this with looks like:

let my_ref = ref 1

module _ : sig end = struct
  type 'a thing =
    | Thing of 'a
    | No

  let f2 t =
    match t with
    | Thing 1 -> true
    | Thing _ | No -> false
  ;;

  let length = function
    | Thing i -> i
    | No -> -1
  ;;

  let () =
    let init = Thing 1 in
    let nesting = 1 in
    let rec handle_state t =
      let this_will_be_undefined () = if f2 t then 1 else 2 in
      match length t with
      | 0 -> this_will_be_undefined ()
      | 1 -> if Stdlib.Int.equal nesting 0 then nesting else this_will_be_undefined ()
      | _ -> handle_state (Thing 0)
    in
    print_endline (Int.to_string (handle_state init))
  ;;

  let _ : _ thing = No
end

let () = my_ref := 2

Full expect test with output

let%expect_test "recursive function not defined" =
  with_temp_dir ~f:(fun () ->
    let name = "test.ml" in
    Filetype.write_file
      name
      {|
      let my_ref = ref 1

      module _ : sig end = struct
        type 'a thing =
          | Thing of 'a
          | No

        let f2 t =
          match t with
          | Thing 1 -> true
          | Thing _ | No -> false
        ;;

        let length = function
          | Thing i -> i
          | No -> -1
        ;;

        let () =
          let init = Thing 1 in
          let nesting = 1 in
          let rec handle_state t =
            let this_will_be_undefined () = if f2 t then 1 else 2 in
            match length t with
            | 0 -> this_will_be_undefined ()
            | 1 -> if Stdlib.Int.equal nesting 0 then nesting else this_will_be_undefined ()
            | _ -> handle_state (Thing 0)
          in
          print_endline (Int.to_string (handle_state init))
        ;;

        let _ : _ thing = No
      end

      let () = my_ref := 2
    |};
    let file = Filetype.ocaml_file_of_path name in
    let cmo = compile_ocaml_to_cmo file in
    let lib = compile_lib [ cmo ] "test.cmo" in
    let js = compile_cmo_to_javascript lib in
    let js_source = Filetype.read_js js in
    (* In this output this_will_be_undefined will not be in scope in one of it's uses. *)
    print_endline (Filetype.string_of_js_text js_source);
    [%expect
      {|
      //# unitInfo: Provides: Test
      //# unitInfo: Requires: Stdlib, Stdlib__Int
      (function
        (globalThis){
         "use strict";
         var runtime = globalThis.jsoo_runtime;
         function caml_call1(f, a0){
          return (f.l >= 0 ? f.l : f.l = f.length) == 1
                  ? f(a0)
                  : runtime.caml_call_gen(f, [a0]);
         }
         function caml_call2(f, a0, a1){
          return (f.l >= 0 ? f.l : f.l = f.length) == 2
                  ? f(a0, a1)
                  : runtime.caml_call_gen(f, [a0, a1]);
         }
         var
          global_data = runtime.caml_get_global_data(),
          t$0 = [0, 0],
          init = [0, 1],
          Stdlib_Int = global_data.Stdlib__Int,
          Stdlib = global_data.Stdlib,
          my_ref = [0, 1];
         a:
         {
          var t = init, nesting = 1;
          for(;;){
           let t$1 = t;
           function this_will_be_undefined(param){
            var _c_ = 1 === t$1[1] ? 1 : 0;
            return _c_ ? 1 : 2;
           }
           var i = t[1];
           if(0 === i){var _a_ = this_will_be_undefined(0); break a;}
           if(1 === i) break;
           var t = t$0;
          }
          var
           _a_ =
             caml_call2(Stdlib_Int[8], nesting, 0)
              ? nesting
              : this_will_be_undefined(0);
         }
         var _b_ = caml_call1(Stdlib_Int[12], _a_);
         caml_call1(Stdlib[46], _b_);
         my_ref[1] = 2;
         var Test = [0, my_ref];
         runtime.caml_register_global(4, Test, "Test");
         return;
        }
        (globalThis));

      //# sourceMappingURL=test.map
    |}];
 )
;;

Expected behavior
If you expand the full expect test, or compile and examine the JS, you'll see that this_will_be_undefined is created as a block-level function declaration and then an attempt to access it outside of that block fails.

Versions
This issue is present in afdb5f3 and is a regression. I know it's not present in 77ad64f (a very broad range, apologies).

[hhugo]

Thanks for the report, would you be able to test #1561 and report if it works for you ? (The fix is probably not in its final form)

[hhugo]

Bug most probably introduced by #1505

[hhugo]

fixed by #1561