From 0b8048674d8feda7c0d103c1fdaf2a2e05703b66 Mon Sep 17 00:00:00 2001 From: Cristiano Calcagno Date: Mon, 25 Nov 2024 13:20:02 +0100 Subject: [PATCH 1/3] Fix bug where a ref assignment is moved ouside a conditional Fixes https://github.com/rescript-lang/rescript/issues/7170 Assignment of a ref, or other record with a single mutable field, is compiled to simple variable assignment. This triggers an incorrect optimisation that moves identical assignments in the true and false branches of a conditional above if they are identical and the guard has no side effects. Added a check that if the assigments to be moved are to variable, that variable must not occur free in the guard of the conditional. --- CHANGELOG.md | 3 +++ compiler/core/js_analyzer.mli | 2 +- compiler/core/js_stmt_make.ml | 10 +++++++++ tests/tests/src/move_ref_assignment.mjs | 29 +++++++++++++++++++++++++ tests/tests/src/move_ref_assignment.res | 21 ++++++++++++++++++ 5 files changed, 64 insertions(+), 1 deletion(-) create mode 100644 tests/tests/src/move_ref_assignment.mjs create mode 100644 tests/tests/src/move_ref_assignment.res diff --git a/CHANGELOG.md b/CHANGELOG.md index bb20b8affc..c53d07088c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,9 @@ # 12.0.0-alpha.6 (Unreleased) +#### :bug: Bug fix +- Fix bug where a ref assignment is moved ouside a conditional. https://github.com/rescript-lang/rescript/pull/7176 + # 12.0.0-alpha.5 #### :rocket: New Feature diff --git a/compiler/core/js_analyzer.mli b/compiler/core/js_analyzer.mli index 7a1f281663..786c29fece 100644 --- a/compiler/core/js_analyzer.mli +++ b/compiler/core/js_analyzer.mli @@ -29,7 +29,7 @@ val free_variables_of_statement : J.statement -> Set_ident.t -val free_variables_of_expression : J.finish_ident_expression -> Set_ident.t +val free_variables_of_expression : J.expression -> Set_ident.t (* val no_side_effect_expression_desc : J.expression_desc -> bool *) diff --git a/compiler/core/js_stmt_make.ml b/compiler/core/js_stmt_make.ml index 0067e89ccc..a60b6976e6 100644 --- a/compiler/core/js_stmt_make.ml +++ b/compiler/core/js_stmt_make.ml @@ -303,6 +303,16 @@ let if_ ?comment ?declaration ?else_ (e : J.expression) (then_ : J.block) : t = aux ?comment (E.or_ e (E.not pred1)) ifso ifso1 | ifso1 :: ifso_rest, ifnot1 :: ifnot_rest when Js_analyzer.eq_statement ifnot1 ifso1 + && (match ifso1.statement_desc with + | Exp + { + expression_desc = + Bin (Eq, {expression_desc = Var (Id v)}, _); + _; + } -> + let guard_vars = Js_analyzer.free_variables_of_expression e in + not (Set_ident.mem guard_vars v) + | _ -> true) && Js_analyzer.no_side_effect_expression e -> (* here we do agressive optimization, because it can help optimization later, move code outside of branch is generally helpful later diff --git a/tests/tests/src/move_ref_assignment.mjs b/tests/tests/src/move_ref_assignment.mjs new file mode 100644 index 0000000000..d74e228436 --- /dev/null +++ b/tests/tests/src/move_ref_assignment.mjs @@ -0,0 +1,29 @@ +// Generated by ReScript, PLEASE EDIT WITH CARE + + +let j = 0; + +let k = 1; + +if (j === 0) { + j = j + 2 | 0; + console.log("j.c"); +} else { + j = j + 2 | 0; +} + +j = j + 2 | 0; + +if (k === 0) { + console.log("k.c"); +} + +let j$1 = 0; + +let k$1 = 0; + +export { + j$1 as j, + k$1 as k, +} +/* Not a pure module */ diff --git a/tests/tests/src/move_ref_assignment.res b/tests/tests/src/move_ref_assignment.res new file mode 100644 index 0000000000..1b2454249b --- /dev/null +++ b/tests/tests/src/move_ref_assignment.res @@ -0,0 +1,21 @@ +type t = {mutable c: int} + +let j = {c: 0} +let k = {c: 1} + +if j.c == 0 { + j.c = j.c + 2 + Console.log("j.c") +} else { + j.c = j.c + 2 +} + +if k.c == 0 { + j.c = j.c + 2 + Console.log("k.c") +} else { + j.c = j.c + 2 +} + +let j = 0 +let k = 0 From b911eee9fbd2985d83fe577542b533eae6090c1e Mon Sep 17 00:00:00 2001 From: Cristiano Calcagno Date: Tue, 26 Nov 2024 10:23:48 +0100 Subject: [PATCH 2/3] Change the example so the optimisation is still incorrect. --- tests/tests/src/move_ref_assignment.mjs | 22 +++++++++++++--------- tests/tests/src/move_ref_assignment.res | 16 +++++++--------- 2 files changed, 20 insertions(+), 18 deletions(-) diff --git a/tests/tests/src/move_ref_assignment.mjs b/tests/tests/src/move_ref_assignment.mjs index d74e228436..4bba748010 100644 --- a/tests/tests/src/move_ref_assignment.mjs +++ b/tests/tests/src/move_ref_assignment.mjs @@ -1,21 +1,24 @@ // Generated by ReScript, PLEASE EDIT WITH CARE -let j = 0; +let j = 1; -let k = 1; +let k = { + c: 1 +}; -if (j === 0) { - j = j + 2 | 0; - console.log("j.c"); -} else { - j = j + 2 | 0; +function upd() { + k.c = 3; } +upd(); + j = j + 2 | 0; -if (k === 0) { - console.log("k.c"); +if (k.c === 1) { + console.log("correct"); +} else { + console.log("incorrect"); } let j$1 = 0; @@ -23,6 +26,7 @@ let j$1 = 0; let k$1 = 0; export { + upd, j$1 as j, k$1 as k, } diff --git a/tests/tests/src/move_ref_assignment.res b/tests/tests/src/move_ref_assignment.res index 1b2454249b..3e6af9b5ca 100644 --- a/tests/tests/src/move_ref_assignment.res +++ b/tests/tests/src/move_ref_assignment.res @@ -1,20 +1,18 @@ type t = {mutable c: int} -let j = {c: 0} +let j = {c: 1} let k = {c: 1} -if j.c == 0 { - j.c = j.c + 2 - Console.log("j.c") -} else { - j.c = j.c + 2 -} +let upd = () => k.c = 3 -if k.c == 0 { +if k.c == 1 { + upd() j.c = j.c + 2 - Console.log("k.c") + Console.log("correct") } else { + upd() j.c = j.c + 2 + Console.log("incorrect") } let j = 0 From bf6406f218493bd5f5ddd8cb2ce41d80797ac853 Mon Sep 17 00:00:00 2001 From: Cristiano Calcagno Date: Tue, 26 Nov 2024 10:35:10 +0100 Subject: [PATCH 3/3] Remove problematic optimisation entirely. --- compiler/core/js_stmt_make.ml | 27 ++----------------------- tests/tests/src/move_ref_assignment.mjs | 8 ++++---- tests/tests/src/test_ramification.mjs | 12 ++++++++--- 3 files changed, 15 insertions(+), 32 deletions(-) diff --git a/compiler/core/js_stmt_make.ml b/compiler/core/js_stmt_make.ml index a60b6976e6..e4d87302e9 100644 --- a/compiler/core/js_stmt_make.ml +++ b/compiler/core/js_stmt_make.ml @@ -232,8 +232,6 @@ let rec block_last_is_return_throw_or_continue (x : J.block) = *) let if_ ?comment ?declaration ?else_ (e : J.expression) (then_ : J.block) : t = let declared = ref false in - let common_prefix_blocks = ref [] in - let add_prefix b = common_prefix_blocks := b :: !common_prefix_blocks in let rec aux ?comment (e : J.expression) (ifso : J.block) (ifnot : J.block) : t = match (e.expression_desc, ifnot) with @@ -301,24 +299,6 @@ let if_ ?comment ?declaration ?else_ (e : J.expression) (then_ : J.block) : t = | _, [{statement_desc = If (pred1, ifso1, ifnot1)}] when Js_analyzer.eq_block ifso ifnot1 -> aux ?comment (E.or_ e (E.not pred1)) ifso ifso1 - | ifso1 :: ifso_rest, ifnot1 :: ifnot_rest - when Js_analyzer.eq_statement ifnot1 ifso1 - && (match ifso1.statement_desc with - | Exp - { - expression_desc = - Bin (Eq, {expression_desc = Var (Id v)}, _); - _; - } -> - let guard_vars = Js_analyzer.free_variables_of_expression e in - not (Set_ident.mem guard_vars v) - | _ -> true) - && Js_analyzer.no_side_effect_expression e -> - (* here we do agressive optimization, because it can help optimization later, - move code outside of branch is generally helpful later - *) - add_prefix ifso1; - aux ?comment e ifso_rest ifnot_rest | _ -> {statement_desc = If (e, ifso, ifnot); comment}) in let if_block = @@ -327,12 +307,9 @@ let if_ ?comment ?declaration ?else_ (e : J.expression) (then_ : J.block) : t = | None -> [] | Some v -> v) in - let prefix = !common_prefix_blocks in match (!declared, declaration) with - | true, _ | _, None -> - if prefix = [] then if_block else block (List.rev_append prefix [if_block]) - | false, Some (kind, id) -> - block (declare_variable ~kind id :: List.rev_append prefix [if_block]) + | true, _ | _, None -> if_block + | false, Some (kind, id) -> block (declare_variable ~kind id :: [if_block]) let assign ?comment id e : t = {statement_desc = J.Exp (E.assign (E.var id) e); comment} diff --git a/tests/tests/src/move_ref_assignment.mjs b/tests/tests/src/move_ref_assignment.mjs index 4bba748010..acd9050656 100644 --- a/tests/tests/src/move_ref_assignment.mjs +++ b/tests/tests/src/move_ref_assignment.mjs @@ -11,13 +11,13 @@ function upd() { k.c = 3; } -upd(); - -j = j + 2 | 0; - if (k.c === 1) { + upd(); + j = j + 2 | 0; console.log("correct"); } else { + upd(); + j = j + 2 | 0; console.log("incorrect"); } diff --git a/tests/tests/src/test_ramification.mjs b/tests/tests/src/test_ramification.mjs index 18fb48f4f4..bcde916376 100644 --- a/tests/tests/src/test_ramification.mjs +++ b/tests/tests/src/test_ramification.mjs @@ -33,11 +33,12 @@ function f(x) { function f2(x) { let v = 0; let y; - v = 1; if (x.TAG === "A") { + v = 1; let z = 33; y = z + 3 | 0; } else { + v = 1; let z$1 = 33; y = z$1 + 4 | 0; } @@ -47,8 +48,13 @@ function f2(x) { function f3(x) { let v = 0; let y; - v = 1; - y = x.TAG === "A" ? 3 : 4; + if (x.TAG === "A") { + v = 1; + y = 3; + } else { + v = 1; + y = 4; + } return y + 32 | 0; }