From 137367c0a572f984e0b75174dbae09eab13149bd Mon Sep 17 00:00:00 2001 From: Christoph Knittel Date: Sat, 24 Oct 2026 18:32:05 +0000 Subject: [PATCH 1/3] Keep the operator of a negated float comparison MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Js_exp_make.not negated a comparison by swapping its operator, turning !(x < 0.0) into x >= 0.0, which is false for NaN. Float comparisons, and comparisons of operands of unknown type, now use their own operators, printed as <, <=, >, >=, which not leaves as !(…). Fixes #9026. JS modified (negated float comparison): Example: let k = (x: float) => !(x < 0.0) Before (js): function k(x) { return x >= 0.0; } After (js): function k(x) { return !(x < 0.0); } Review: Do int, bigint and string comparisons change? No, they keep the old operators, so negating them still swaps the operator. The only changed output is in caml_compare_bigint_test, whose bigint values are typed as float. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_015yE4gCmTmJKVecrVt1PU8C Signed-off-by: Christoph Knittel --- changes/negated-float-comparison-nan.fix.md | 1 + compiler/core/js_exp_make.ml | 9 ++- compiler/core/js_exp_make.mli | 2 + compiler/core/js_op.ml | 7 ++ compiler/core/js_op_util.ml | 12 ++-- compiler/core/lam_compile_primitive.ml | 6 +- compiler/core/lam_compile_util.ml | 10 +++ compiler/core/lam_compile_util.mli | 2 + tests/tests/src/caml_compare_bigint_test.mjs | 8 +-- .../src/negated_float_comparison_test.mjs | 71 +++++++++++++++++++ .../src/negated_float_comparison_test.res | 41 +++++++++++ 11 files changed, 158 insertions(+), 11 deletions(-) create mode 100644 changes/negated-float-comparison-nan.fix.md create mode 100644 tests/tests/src/negated_float_comparison_test.mjs create mode 100644 tests/tests/src/negated_float_comparison_test.res diff --git a/changes/negated-float-comparison-nan.fix.md b/changes/negated-float-comparison-nan.fix.md new file mode 100644 index 00000000000..fb0c1064d68 --- /dev/null +++ b/changes/negated-float-comparison-nan.fix.md @@ -0,0 +1 @@ +- Fix negated float comparisons with NaN: `!(x < 0.0)` compiled to `x >= 0.0`, which is `false` for NaN. diff --git a/compiler/core/js_exp_make.ml b/compiler/core/js_exp_make.ml index 0f42b366495..31b6a1dd784 100644 --- a/compiler/core/js_exp_make.ml +++ b/compiler/core/js_exp_make.ml @@ -1555,8 +1555,15 @@ let bool_comp (cmp : Lambda.comparison) ?comment (e0 : t) (e1 : t) = bin ?comment (Lam_compile_util.jsop_of_comp cmp) e0 e1) | _, _ -> bin ?comment (Lam_compile_util.jsop_of_comp cmp) e0 e1 +let float_comp (cmp : Lambda.comparison) ?comment (e0 : t) (e1 : t) = + match cmp with + | Ceq | Cneq -> int_comp cmp ?comment e0 e1 + | Clt | Cgt | Cle | Cge -> + bin ?comment (Lam_compile_util.jsop_of_float_comp cmp) e0 e1 + +(* The operands may be of any type, so they may be NaN *) let js_comp cmp ?comment e0 e1 = - bin ?comment (Lam_compile_util.jsop_of_comp cmp) e0 e1 + bin ?comment (Lam_compile_util.jsop_of_float_comp cmp) e0 e1 let rec int32_lsr ?comment (e1 : J.expression) (e2 : J.expression) : J.expression = diff --git a/compiler/core/js_exp_make.mli b/compiler/core/js_exp_make.mli index 9be5dd431e6..70f6e7326f8 100644 --- a/compiler/core/js_exp_make.mli +++ b/compiler/core/js_exp_make.mli @@ -244,6 +244,8 @@ val bigint_div : checked:bool -> ?comment:string -> t -> t -> t val bigint_mod : checked:bool -> ?comment:string -> t -> t -> t +val float_comp : Lambda.comparison -> ?comment:string -> t -> t -> t + val js_comp : Lambda.comparison -> ?comment:string -> t -> t -> t val not : t -> t diff --git a/compiler/core/js_op.ml b/compiler/core/js_op.ml index c610fc7d78d..8aec9529350 100644 --- a/compiler/core/js_op.ml +++ b/compiler/core/js_op.ml @@ -38,6 +38,13 @@ type binop = | Le | Gt | Ge + | Float_lt + | Float_le + | Float_gt + | Float_ge + (* Printed as [<], [<=], [>], [>=], for operands that may be NaN, so + that negating them can't swap the operator: [!(a < b)] is not + [a >= b] when [a] or [b] is NaN *) | Bor | Bxor | Band diff --git a/compiler/core/js_op_util.ml b/compiler/core/js_op_util.ml index 9d4da5cddb5..bc8191342fa 100644 --- a/compiler/core/js_op_util.ml +++ b/compiler/core/js_op_util.ml @@ -34,7 +34,9 @@ let op_prec (op : Js_op.binop) = | Or -> (3, 3, 3) | And -> (4, 4, 4) | EqEqEq | NotEqEq -> (8, 8, 9) - | Gt | Ge | Lt | Le | InstanceOf -> (9, 9, 10) + | Gt | Ge | Lt | Le | Float_lt | Float_le | Float_gt | Float_ge | InstanceOf + -> + (9, 9, 10) | Bor -> (5, 5, 5) | Bxor -> (6, 6, 6) | Band -> (7, 7, 7) @@ -62,10 +64,10 @@ let op_str (op : Js_op.binop) = | And -> "&&" | EqEqEq -> "===" | NotEqEq -> "!==" - | Lt -> "<" - | Le -> "<=" - | Gt -> ">" - | Ge -> ">=" + | Lt | Float_lt -> "<" + | Le | Float_le -> "<=" + | Gt | Float_gt -> ">" + | Ge | Float_ge -> ">=" | InstanceOf -> "instanceof" let update_used_stats (ident_info : J.ident_info) used_stats = diff --git a/compiler/core/lam_compile_primitive.ml b/compiler/core/lam_compile_primitive.ml index e0b081d3d66..e36c74a927b 100644 --- a/compiler/core/lam_compile_primitive.ml +++ b/compiler/core/lam_compile_primitive.ml @@ -337,13 +337,17 @@ let translate output_prefix loc (cxt : Lam_compile_context.t) match args with | [e1; e2] -> E.bool_comp cmp e1 e2 | _ -> assert false) - | Pfloatcomp cmp | Pintcomp cmp -> ( + | Pintcomp cmp -> ( (* Global Builtin Exception is an int, like [Not_found] or [Invalid_argument] ? *) match args with | [e1; e2] -> E.int_comp cmp e1 e2 | _ -> assert false) + | Pfloatcomp cmp -> ( + match args with + | [e1; e2] -> E.float_comp cmp e1 e2 + | _ -> assert false) | Pbigintcomp cmp -> ( match args with | [e1; e2] -> E.bigint_comp cmp e1 e2 diff --git a/compiler/core/lam_compile_util.ml b/compiler/core/lam_compile_util.ml index 4b23e7715e2..1cfa5ebd6da 100644 --- a/compiler/core/lam_compile_util.ml +++ b/compiler/core/lam_compile_util.ml @@ -31,6 +31,16 @@ let jsop_of_comp (cmp : Lambda.comparison) : Js_op.binop = | Cle -> Le | Cge -> Ge +(* For operands that may be NaN, see [Js_op.Float_lt] *) +let jsop_of_float_comp (cmp : Lambda.comparison) : Js_op.binop = + match cmp with + | Ceq -> EqEqEq + | Cneq -> NotEqEq + | Clt -> Float_lt + | Cgt -> Float_gt + | Cle -> Float_le + | Cge -> Float_ge + let runtime_of_comp (cmp : Lambda.comparison) : string = match cmp with | Ceq -> "equal" diff --git a/compiler/core/lam_compile_util.mli b/compiler/core/lam_compile_util.mli index 031bcbd748c..55660b1eaad 100644 --- a/compiler/core/lam_compile_util.mli +++ b/compiler/core/lam_compile_util.mli @@ -26,4 +26,6 @@ val jsop_of_comp : Lambda.comparison -> Js_op.binop +val jsop_of_float_comp : Lambda.comparison -> Js_op.binop + val runtime_of_comp : Lambda.comparison -> string diff --git a/tests/tests/src/caml_compare_bigint_test.mjs b/tests/tests/src/caml_compare_bigint_test.mjs index 49fa77b2558..87b8da60d42 100644 --- a/tests/tests/src/caml_compare_bigint_test.mjs +++ b/tests/tests/src/caml_compare_bigint_test.mjs @@ -28,9 +28,9 @@ Mocha.describe("Caml_compare_bigint_test", () => { Test_utils.ok("File \"caml_compare_bigint_test.res\", line 27, characters 7-14", BigInt("123") !== BigInt("555555")); }); Mocha.test("compare: 98765 and 98765", () => { - Test_utils.ok("File \"caml_compare_bigint_test.res\", line 31, characters 7-14", BigInt("98765") >= BigInt("98765")); + Test_utils.ok("File \"caml_compare_bigint_test.res\", line 31, characters 7-14", !(BigInt("98765") < BigInt("98765"))); Test_utils.ok("File \"caml_compare_bigint_test.res\", line 32, characters 7-14", BigInt("98765") <= BigInt("98765")); - Test_utils.ok("File \"caml_compare_bigint_test.res\", line 33, characters 7-14", BigInt("98765") <= BigInt("98765")); + Test_utils.ok("File \"caml_compare_bigint_test.res\", line 33, characters 7-14", !(BigInt("98765") > BigInt("98765"))); Test_utils.ok("File \"caml_compare_bigint_test.res\", line 34, characters 7-14", BigInt("98765") >= BigInt("98765")); Test_utils.eq("File \"caml_compare_bigint_test.res\", line 35, characters 7-14", Primitive_float.min(BigInt("98765"), BigInt("98765")), BigInt("98765")); Test_utils.eq("File \"caml_compare_bigint_test.res\", line 36, characters 7-14", Primitive_float.max(BigInt("98765"), BigInt("98765")), BigInt("98765")); @@ -46,9 +46,9 @@ Mocha.describe("Caml_compare_bigint_test", () => { Test_utils.ok("File \"caml_compare_bigint_test.res\", line 46, characters 7-14", BigInt("98765") === BigInt("98765")); }); Mocha.test("same instance", () => { - Test_utils.ok("File \"caml_compare_bigint_test.res\", line 50, characters 7-14", five >= five); + Test_utils.ok("File \"caml_compare_bigint_test.res\", line 50, characters 7-14", !(five < five)); Test_utils.ok("File \"caml_compare_bigint_test.res\", line 51, characters 7-14", five <= five); - Test_utils.ok("File \"caml_compare_bigint_test.res\", line 52, characters 7-14", five <= five); + Test_utils.ok("File \"caml_compare_bigint_test.res\", line 52, characters 7-14", !(five > five)); Test_utils.ok("File \"caml_compare_bigint_test.res\", line 53, characters 7-14", five >= five); Test_utils.eq("File \"caml_compare_bigint_test.res\", line 54, characters 7-14", Primitive_float.min(five, five), five); Test_utils.eq("File \"caml_compare_bigint_test.res\", line 55, characters 7-14", Primitive_float.max(five, five), five); diff --git a/tests/tests/src/negated_float_comparison_test.mjs b/tests/tests/src/negated_float_comparison_test.mjs new file mode 100644 index 00000000000..9a5eb83b443 --- /dev/null +++ b/tests/tests/src/negated_float_comparison_test.mjs @@ -0,0 +1,71 @@ +// Generated by ReScript, PLEASE EDIT WITH CARE + +import * as Mocha from "mocha"; +import * as Test_utils from "./test_utils.mjs"; + +function notLt(x, y) { + return !(x < y); +} + +function notLe(x, y) { + return !(x <= y); +} + +function notGt(x, y) { + return !(x > y); +} + +function notGe(x, y) { + return !(x >= y); +} + +let log = { + contents: /* [] */0 +}; + +function ifNotLt(x) { + if (!(x < 0.0)) { + log.contents = {hd: "then", tl: log.contents}; + } + return log.contents; +} + +function condNotLt(x) { + if (x < 0.0) { + return "else"; + } else { + return "then"; + } +} + +Mocha.describe("Negated_float_comparison_test", () => { + Mocha.test("negated comparisons with NaN", () => { + let nan = NaN; + Test_utils.eq("File \"negated_float_comparison_test.res\", line 25, characters 7-14", true, !(nan < 0.0)); + Test_utils.eq("File \"negated_float_comparison_test.res\", line 26, characters 7-14", true, !(nan <= 0.0)); + Test_utils.eq("File \"negated_float_comparison_test.res\", line 27, characters 7-14", true, !(nan > 0.0)); + Test_utils.eq("File \"negated_float_comparison_test.res\", line 28, characters 7-14", true, !(nan >= 0.0)); + Test_utils.eq("File \"negated_float_comparison_test.res\", line 29, characters 7-14", true, !(0.0 < nan)); + }); + Mocha.test("negated comparisons with numbers", () => { + Test_utils.eq("File \"negated_float_comparison_test.res\", line 32, characters 7-14", false, false); + Test_utils.eq("File \"negated_float_comparison_test.res\", line 33, characters 7-14", true, true); + Test_utils.eq("File \"negated_float_comparison_test.res\", line 34, characters 7-14", false, false); + }); + Mocha.test("negated comparison as a condition", () => { + Test_utils.eq("File \"negated_float_comparison_test.res\", line 37, characters 7-14", {hd: "then", tl: /* [] */0}, ifNotLt(NaN)); + Test_utils.eq("File \"negated_float_comparison_test.res\", line 38, characters 7-14", "then", condNotLt(NaN)); + Test_utils.eq("File \"negated_float_comparison_test.res\", line 39, characters 7-14", "else", "else"); + }); +}); + +export { + notLt, + notLe, + notGt, + notGe, + log, + ifNotLt, + condNotLt, +} +/* Not a pure module */ diff --git a/tests/tests/src/negated_float_comparison_test.res b/tests/tests/src/negated_float_comparison_test.res new file mode 100644 index 00000000000..9d0b68cc558 --- /dev/null +++ b/tests/tests/src/negated_float_comparison_test.res @@ -0,0 +1,41 @@ +open Mocha +open Test_utils + +// Negating a float comparison must not flip its operator: for NaN, +// !(x < y) is true while x >= y is false. +let notLt = (x: float, y: float) => !(x < y) +let notLe = (x: float, y: float) => !(x <= y) +let notGt = (x: float, y: float) => !(x > y) +let notGe = (x: float, y: float) => !(x >= y) + +let log = ref(list{}) + +let ifNotLt = (x: float) => { + if !(x < 0.0) { + log := list{"then", ...log.contents} + } + log.contents +} + +let condNotLt = (x: float) => !(x < 0.0) ? "then" : "else" + +describe(__MODULE__, () => { + test("negated comparisons with NaN", () => { + let nan = Float.Constants.nan + eq(__LOC__, true, notLt(nan, 0.0)) + eq(__LOC__, true, notLe(nan, 0.0)) + eq(__LOC__, true, notGt(nan, 0.0)) + eq(__LOC__, true, notGe(nan, 0.0)) + eq(__LOC__, true, notLt(0.0, nan)) + }) + test("negated comparisons with numbers", () => { + eq(__LOC__, false, notLt(-1.0, 0.0)) + eq(__LOC__, true, notLt(1.0, 0.0)) + eq(__LOC__, false, notGe(1.0, 0.0)) + }) + test("negated comparison as a condition", () => { + eq(__LOC__, list{"then"}, ifNotLt(Float.Constants.nan)) + eq(__LOC__, "then", condNotLt(Float.Constants.nan)) + eq(__LOC__, "else", condNotLt(-1.0)) + }) +}) From adde988da7470c544ce6c75e8613dafa5b2a04fe Mon Sep 17 00:00:00 2001 From: Christoph Knittel Date: Sat, 24 Oct 2026 16:35:00 +0000 Subject: [PATCH 2/3] Drop a pure condition when both branches of an if are empty A switch whose remaining cases all fall through to the same code left their test behind as a bare statement, such as `x === null;`, and the empty block it left in an enclosing if blocked merging the conditions. JS modified (switch with cases that fall through): Example: type rank = Uninitialized | Visited | Ranked(int) let f3 = (lhs: rank, rhs: rank) => switch (lhs, rhs) { | (Ranked(x), Ranked(y)) => Pervasives.compare(x, y) | _ => assert(false) } Before (js): tests/tests/src/fun_pattern_match.res if (typeof lhs !== "object") { lhs === "Uninitialized"; } else { if (typeof rhs === "object") { return Primitive_int.compare(lhs._0, rhs._0); } rhs === "Uninitialized"; } throw { After (js): tests/tests/src/fun_pattern_match.res if (typeof lhs === "object" && typeof rhs === "object") { return Primitive_int.compare(lhs._0, rhs._0); } throw { Review: Is a condition with side effects still evaluated? Yes, only conditions that no_side_effect_expression accepts are dropped. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_015yE4gCmTmJKVecrVt1PU8C Signed-off-by: Christoph Knittel --- changes/drop-pure-empty-if.polish.md | 1 + compiler/core/js_stmt_make.ml | 14 +++- tests/tests/src/fun_pattern_match.mjs | 18 +---- tests/tests/src/option_optimisation.mjs | 1 - .../tests/src/stdlib/Stdlib_IteratorTests.mjs | 76 +++++++++---------- 5 files changed, 56 insertions(+), 54 deletions(-) create mode 100644 changes/drop-pure-empty-if.polish.md diff --git a/changes/drop-pure-empty-if.polish.md b/changes/drop-pure-empty-if.polish.md new file mode 100644 index 00000000000..7ba447d0374 --- /dev/null +++ b/changes/drop-pure-empty-if.polish.md @@ -0,0 +1 @@ +- Stop emitting a bare condition like `x === null;` for a switch whose cases all fall through. diff --git a/compiler/core/js_stmt_make.ml b/compiler/core/js_stmt_make.ml index 60e1b7d6aa7..cbf82a980e3 100644 --- a/compiler/core/js_stmt_make.ml +++ b/compiler/core/js_stmt_make.ml @@ -246,12 +246,24 @@ let if_ ?comment ?declaration ?else_ (e : J.expression) (then_ : J.block) : t = let declared = ref false in let rec aux ?comment (e : J.expression) (ifso : J.block) (ifnot : J.block) : t = + (* An empty block, such as a dropped condition of a nested [if], leaves + its branch empty *) + let is_not_empty (s : t) = + match s.statement_desc with + | Block [] -> false + | _ -> true + in + let ifso = List.filter is_not_empty ifso in + let ifnot = List.filter is_not_empty ifnot in match (e.expression_desc, ifnot) with | Bool boolean, _ -> block (if boolean then ifso else ifnot) | Js_not pred_not, _ :: _ -> aux ?comment pred_not ifnot ifso | _ -> ( match (ifso, ifnot) with - | [], [] -> exp e + | [], [] -> + (* Both branches are empty: only a condition with side effects + needs to be kept *) + if Js_analyzer.no_side_effect_expression e then empty_stmt else exp e | [], _ -> aux ?comment (E.not e) ifnot [] (*Make sure no infinite loop*) | ( [{statement_desc = Return ret_ifso; _}], [{statement_desc = Return ret_ifnot; _}] ) -> diff --git a/tests/tests/src/fun_pattern_match.mjs b/tests/tests/src/fun_pattern_match.mjs index 275aa52e96e..d72036200e6 100644 --- a/tests/tests/src/fun_pattern_match.mjs +++ b/tests/tests/src/fun_pattern_match.mjs @@ -13,13 +13,8 @@ function f2(param, param$1) { function f3(param, param$1) { let lhs = param.rank; let rhs = param$1.rank; - if (typeof lhs !== "object") { - lhs === "Uninitialized"; - } else { - if (typeof rhs === "object") { - return Primitive_int.compare(lhs._0, rhs._0); - } - rhs === "Uninitialized"; + if (typeof lhs === "object" && typeof rhs === "object") { + return Primitive_int.compare(lhs._0, rhs._0); } throw { RE_EXN_ID: "Assert_failure", @@ -35,13 +30,8 @@ function f3(param, param$1) { function f4(param, param$1) { let lhs = param.rank; let rhs = param$1.rank; - if (typeof lhs !== "object") { - lhs === "Uninitialized"; - } else { - if (typeof rhs === "object") { - return Primitive_int.compare(lhs._0, rhs._0); - } - rhs === "Uninitialized"; + if (typeof lhs === "object" && typeof rhs === "object") { + return Primitive_int.compare(lhs._0, rhs._0); } throw { RE_EXN_ID: "Assert_failure", diff --git a/tests/tests/src/option_optimisation.mjs b/tests/tests/src/option_optimisation.mjs index 4e00b4595d8..a6bd76738fd 100644 --- a/tests/tests/src/option_optimisation.mjs +++ b/tests/tests/src/option_optimisation.mjs @@ -13,7 +13,6 @@ function $$null(val1, val2) { if (val2 !== null && val2 !== undefined) { return "a"; } - val2 === null; if (val1 == null) { return "b"; } else { diff --git a/tests/tests/src/stdlib/Stdlib_IteratorTests.mjs b/tests/tests/src/stdlib/Stdlib_IteratorTests.mjs index 4bab6002b76..ced38b3bad3 100644 --- a/tests/tests/src/stdlib/Stdlib_IteratorTests.mjs +++ b/tests/tests/src/stdlib/Stdlib_IteratorTests.mjs @@ -184,11 +184,11 @@ let syncResult = { contents: undefined }; -let match = Stdlib_IterableIterator.next(iterator); +Stdlib_IterableIterator.next(iterator); -let match$1 = Stdlib_IterableIterator.next(iterator); +let match = Stdlib_IterableIterator.next(iterator); -if (match$1.done === false && match$1.value === "b") { +if (match.done === false && match.value === "b") { syncResult.contents = "b"; } @@ -212,9 +212,9 @@ let protocolResult = { contents: undefined }; -let match$2 = Stdlib_Iterator.next(iteratorProtocol); +let match$1 = Stdlib_Iterator.next(iteratorProtocol); -if (match$2.done === false && match$2.value === "x") { +if (match$1.done === false && match$1.value === "x") { protocolResult.contents = "x"; } @@ -243,10 +243,10 @@ let omittedDoneResult = { contents: undefined }; -let match$3 = Stdlib_Iterator.next(omittedDoneIterator); +let match$2 = Stdlib_Iterator.next(omittedDoneIterator); -if (match$3.done === false) { - if (match$3.value !== 1) { +if (match$2.done === false) { + if (match$2.value !== 1) { } else { omittedDoneResult.contents = "yield"; @@ -279,10 +279,10 @@ let createdProtocolResult = { contents: undefined }; -let match$4 = Stdlib_Iterator.next(createdIterator); +let match$3 = Stdlib_Iterator.next(createdIterator); -if (match$4.done === false) { - if (match$4.value !== 0) { +if (match$3.done === false) { + if (match$3.value !== 0) { } else { createdProtocolResult.contents = "protocol"; @@ -315,12 +315,12 @@ let createdIterableResult = { contents: undefined }; -let match$5 = Stdlib_IterableIterator.next(createdIterableIterator); +Stdlib_IterableIterator.next(createdIterableIterator); -let match$6 = Stdlib_IterableIterator.next(createdIterableIterator); +let match$4 = Stdlib_IterableIterator.next(createdIterableIterator); -if (match$6.done === false) { - if (match$6.value !== 1) { +if (match$4.done === false) { + if (match$4.value !== 1) { } else { createdIterableResult.contents = "iterable"; @@ -346,11 +346,11 @@ let iteratorNextValueResult = { contents: undefined }; -let match$7 = Stdlib_Iterator.nextValue(generatorViaIterator, undefined); +Stdlib_Iterator.nextValue(generatorViaIterator, undefined); -let match$8 = Stdlib_Iterator.nextValue(generatorViaIterator, 4); +let match$5 = Stdlib_Iterator.nextValue(generatorViaIterator, 4); -if (match$8.done !== false && match$8.value === "5") { +if (match$5.done !== false && match$5.value === "5") { iteratorNextValueResult.contents = "nextValue"; } @@ -401,13 +401,13 @@ let generatorWithNext = ((function* () { yield injected + 1 })()); -let match$9 = Stdlib_Generator.next(generatorWithNext); +let match$6 = Stdlib_Generator.next(generatorWithNext); let tmp; -if (match$9.done === false && match$9.value === 1) { - let match$10 = Stdlib_IteratorObject.nextValue(generatorWithNext, 41); - tmp = match$10.done === false && match$10.value === 42 ? "nextValue" : undefined; +if (match$6.done === false && match$6.value === 1) { + let match$7 = Stdlib_IteratorObject.nextValue(generatorWithNext, 41); + tmp = match$7.done === false && match$7.value === 42 ? "nextValue" : undefined; } else { tmp = undefined; } @@ -431,11 +431,11 @@ let generatorReturnValue = ((function* () { return "done" })()); -let match$11 = Stdlib_Generator.next(generatorReturnValue); +Stdlib_Generator.next(generatorReturnValue); -let match$12 = Stdlib_Generator.returnValue(generatorReturnValue, "stopped"); +let match$8 = Stdlib_Generator.returnValue(generatorReturnValue, "stopped"); -if (match$12.done !== false && match$12.value === "stopped") { +if (match$8.done !== false && match$8.value === "stopped") { generatorReturnValueResult.contents = "returnValue"; } @@ -462,11 +462,11 @@ let generatorThrowError = ((function* () { } })()); -let match$13 = Stdlib_Generator.next(generatorThrowError); +Stdlib_Generator.next(generatorThrowError); -let match$14 = Stdlib_Generator.throwError(generatorThrowError, Stdlib_JsExn.anyToExnInternal(new Error("boom"))); +let match$9 = Stdlib_Generator.throwError(generatorThrowError, Stdlib_JsExn.anyToExnInternal(new Error("boom"))); -if (match$14.done !== false) { +if (match$9.done !== false) { generatorThrowErrorResult.contents = "throwError"; } @@ -489,7 +489,7 @@ let asyncResult = { contents: undefined }; -let match$15 = await Stdlib_AsyncIterableIterator.next(asyncIterableIterator); +await Stdlib_AsyncIterableIterator.next(asyncIterableIterator); await Stdlib_AsyncIterableIterator.forEach(asyncIterableIterator, param => { if (param[0] === "second") { @@ -584,11 +584,11 @@ let asyncGeneratorNextValueResult = { contents: undefined }; -let match$16 = await Stdlib_AsyncIterator.nextValue(asyncGeneratorNextValue, undefined); +await Stdlib_AsyncIterator.nextValue(asyncGeneratorNextValue, undefined); -let match$17 = await Stdlib_AsyncGenerator.nextValue(asyncGeneratorNextValue, 4); +let match$10 = await Stdlib_AsyncGenerator.nextValue(asyncGeneratorNextValue, 4); -if (match$17.done !== false && match$17.value === "5") { +if (match$10.done !== false && match$10.value === "5") { asyncGeneratorNextValueResult.contents = "nextValue"; } @@ -750,11 +750,11 @@ let asyncGeneratorReturnValue = ((async function* () { return "done" })()); -let match$18 = await Stdlib_AsyncGenerator.next(asyncGeneratorReturnValue); +await Stdlib_AsyncGenerator.next(asyncGeneratorReturnValue); -let match$19 = await Stdlib_AsyncGenerator.returnValue(asyncGeneratorReturnValue, "stopped"); +let match$11 = await Stdlib_AsyncGenerator.returnValue(asyncGeneratorReturnValue, "stopped"); -if (match$19.done !== false && match$19.value === "stopped") { +if (match$11.done !== false && match$11.value === "stopped") { asyncGeneratorReturnValueResult.contents = "returnValue"; } @@ -781,11 +781,11 @@ let asyncGeneratorThrowError = ((async function* () { } })()); -let match$20 = await Stdlib_AsyncGenerator.next(asyncGeneratorThrowError); +await Stdlib_AsyncGenerator.next(asyncGeneratorThrowError); -let match$21 = await Stdlib_AsyncGenerator.throwError(asyncGeneratorThrowError, Stdlib_JsExn.anyToExnInternal(new Error("boom"))); +let match$12 = await Stdlib_AsyncGenerator.throwError(asyncGeneratorThrowError, Stdlib_JsExn.anyToExnInternal(new Error("boom"))); -if (match$21.done !== false) { +if (match$12.done !== false) { asyncGeneratorThrowErrorResult.contents = "throwError"; } From 6e9058762376012c3102fea585a55795ace6dfed Mon Sep 17 00:00:00 2001 From: Christoph Knittel Date: Sat, 24 Oct 2026 17:09:43 +0000 Subject: [PATCH 3/3] Leave no empty branch when flattening a conditional statement The flatten pass turned a statement `c ? b : e` into an if with `b;` and `e;` as branches; a pure branch value such as `undefined` was dropped later and left an empty branch behind. JS modified (switch with an empty case at statement level): Example: let r = ref(0) let f = (x: int) => { switch x { | 1 => r := 2 | _ => () } Console.log(r) } Before (js): if (x !== 1) { } else { r.contents = 2; } After (js): if (x === 1) { r.contents = 2; } Review: Why not build the if with S.if_? It folds an if/else of two expression statements back into a conditional expression, which is what this pass undoes. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_015yE4gCmTmJKVecrVt1PU8C Signed-off-by: Christoph Knittel --- changes/drop-pure-empty-if.polish.md | 2 +- compiler/core/js_pass_flatten.ml | 23 +++++++++++-------- compiler/core/lam_compile.ml | 2 +- tests/tests/src/gpr_4491_test.mjs | 2 -- tests/tests/src/guard_action_test.mjs | 4 +--- tests/tests/src/mario_game.mjs | 4 +--- .../tests/src/stdlib/Stdlib_IteratorTests.mjs | 12 +++------- 7 files changed, 21 insertions(+), 28 deletions(-) diff --git a/changes/drop-pure-empty-if.polish.md b/changes/drop-pure-empty-if.polish.md index 7ba447d0374..e5a2d6b5026 100644 --- a/changes/drop-pure-empty-if.polish.md +++ b/changes/drop-pure-empty-if.polish.md @@ -1 +1 @@ -- Stop emitting a bare condition like `x === null;` for a switch whose cases all fall through. +- Stop emitting bare conditions like `x === null;` and empty `if`/`else` branches. diff --git a/compiler/core/js_pass_flatten.ml b/compiler/core/js_pass_flatten.ml index c22b4139d67..f3325ed4cce 100644 --- a/compiler/core/js_pass_flatten.ml +++ b/compiler/core/js_pass_flatten.ml @@ -49,15 +49,20 @@ let flatten_map = S.block (Ext_list.map args (fun arg -> self.statement self (S.exp arg))) | Exp {expression_desc = Cond (a, b, c); comment; source_loc} -> - { - statement_desc = - If - ( a, - [self.statement self (S.exp b)], - [self.statement self (S.exp c)] ); - comment; - source_loc; - } + (* A branch whose value is pure leaves nothing to run *) + let branch e = + if Js_analyzer.no_side_effect_expression e then [] + else [self.statement self (S.exp e)] + in + let statement_desc : J.statement_desc = + match (branch b, branch c) with + | [], [] -> + if Js_analyzer.no_side_effect_expression a then Block [] + else Exp a + | [], ifnot -> If (E.not a, ifnot, []) + | ifso, ifnot -> If (a, ifso, ifnot) + in + {statement_desc; comment; source_loc} | Exp { expression_desc = Bin (Eq, a, ({expression_desc = Seq _; _} as v)); diff --git a/compiler/core/lam_compile.ml b/compiler/core/lam_compile.ml index b48f993b1fd..3428b9a40f6 100644 --- a/compiler/core/lam_compile.ml +++ b/compiler/core/lam_compile.ml @@ -1512,7 +1512,7 @@ let compile output_prefix = ( Js_exp_make.remove_pure_sub_exp out1, Js_exp_make.remove_pure_sub_exp out2 ) with - | None, None -> Js_output.make (Ext_list.append_one b (S.exp e)) + | None, None -> Js_output.make (Ext_list.append_one b (S.if_ e [])) (* FIX #1762 *) | Some out1, Some out2 -> Js_output.make b ~value:(E.econd e out1 out2) diff --git a/tests/tests/src/gpr_4491_test.mjs b/tests/tests/src/gpr_4491_test.mjs index 44a26abb854..7d8d13cf606 100644 --- a/tests/tests/src/gpr_4491_test.mjs +++ b/tests/tests/src/gpr_4491_test.mjs @@ -5,8 +5,6 @@ function f(xs) { if (xs !== undefined) { console.log("side effect"); - } else { - } console.log("nothing to see here", xs); } diff --git a/tests/tests/src/guard_action_test.mjs b/tests/tests/src/guard_action_test.mjs index c2aea1ab938..cff9482a859 100644 --- a/tests/tests/src/guard_action_test.mjs +++ b/tests/tests/src/guard_action_test.mjs @@ -12,9 +12,7 @@ function guard() { function both_guards_run() { calls.contents = 0; - if (guard()) { - - } else { + if (!guard()) { guard(); } return calls.contents; diff --git a/tests/tests/src/mario_game.mjs b/tests/tests/src/mario_game.mjs index eca037f2378..d8da28954c6 100644 --- a/tests/tests/src/mario_game.mjs +++ b/tests/tests/src/mario_game.mjs @@ -1991,9 +1991,7 @@ function process_collision(dir, c1, c2, state) { switch (t2$1) { case "Mushroom" : dec_health(o2$2); - if (o1$2.health === 2) { - - } else { + if (o1$2.health !== 2) { o1$2.health = o1$2.health + 1 | 0; } o1$2.vel.x = 0; diff --git a/tests/tests/src/stdlib/Stdlib_IteratorTests.mjs b/tests/tests/src/stdlib/Stdlib_IteratorTests.mjs index ced38b3bad3..d2e2201a9e8 100644 --- a/tests/tests/src/stdlib/Stdlib_IteratorTests.mjs +++ b/tests/tests/src/stdlib/Stdlib_IteratorTests.mjs @@ -246,9 +246,7 @@ let omittedDoneResult = { let match$2 = Stdlib_Iterator.next(omittedDoneIterator); if (match$2.done === false) { - if (match$2.value !== 1) { - - } else { + if (match$2.value === 1) { omittedDoneResult.contents = "yield"; } } @@ -282,9 +280,7 @@ let createdProtocolResult = { let match$3 = Stdlib_Iterator.next(createdIterator); if (match$3.done === false) { - if (match$3.value !== 0) { - - } else { + if (match$3.value === 0) { createdProtocolResult.contents = "protocol"; } } @@ -320,9 +316,7 @@ Stdlib_IterableIterator.next(createdIterableIterator); let match$4 = Stdlib_IterableIterator.next(createdIterableIterator); if (match$4.done === false) { - if (match$4.value !== 1) { - - } else { + if (match$4.value === 1) { createdIterableResult.contents = "iterable"; } }