Simplif.simplify_lets generates incorrect code if fed its own output

[lthls]

This is not a real bug, and more a lack of documentation and checks, but Simplif.simplify_lets will generate incorrect code on some seemingly correct lambda inputs. In particular, running the function on its own output can result in incorrect code being produced.

I've found two examples so far (sample ml programs and their lambda output after an extra round of simplify_lets).
Example 1:

let f x =
  let r = ref x in
  r := !r + 1;
  !r

compiles to:

(let (f/82 = (function x/84[int] : int (seq (assign r/85 (+ x/84 1)) x/84)))

The cause is this optimisation, which replaces let x = y in expr by expr[x <- y] even for Variable bindings.

Example 2:

let g () =
  let r = ref 0 in
  let x = !r in
  (fun _ -> x)

compiles to:

(let g/86 =
     (function param/91
       (let (r/88 =v[int] 0) (function param/90 : int r/88)))))

which breaks the invariant that variables bound to a Variable let must not occur in nested function bodies (they can't be passed in the function's closure like regular variables, in general).

This was discovered while trying to force a different type for mutable variables in the lambda representation (and dedicated Llet and Lvar variants). I wasn't sure how to patch simplify_lets in these cases, and it turns out the current code is wrong for those cases where there are already some Variable bindings. Fortunately this cannot occur with the current code paths, as the only place where Variable bindings are generated is in simplify_lets itself, and the function doesn't call itself recursively on the result.

I'd still like to know if other maintainers are bothered by this behaviour and want to discuss how to fix it, or if it's just how it works and it's not worth spending too much time on it.

[gasche]

This indeed seems concerning to me, I would expect simplifications to be applicable at any point (potentially several times). At least it should be documented, and it would be nice to fix it.

[lthls]

I'll probably propose some changes to the representation of mutable variables in lambda at some point, which would include a fix for this, but that may not happen soon. I can review related PRs by other people though.

[github-actions]

This issue has been open one year with no activity. Consequently, it is being marked with the "stale" label. What this means is that the issue will be automatically closed in 30 days unless more comments are added or the "stale" label is removed. Comments that provide new information on the issue are especially welcome: is it still reproducible? did it appear in other contexts? how critical is it? etc.

[gasche]

If I understand correctly, this bug has been fixed by #10090.