From 7856979efbeb8f8cee6ae09557f5aeed2ac70b8a Mon Sep 17 00:00:00 2001 From: Cristiano Calcagno Date: Wed, 29 May 2024 18:20:06 +0200 Subject: [PATCH] Fix issue of incorrect switch cases with identical bodies when mixing object and array. Fixes https://github.com/rescript-lang/rescript-compiler/issues/6789 The issue happens when 2 cases, here `Object` and `Array`, have identical body (here `Console.log(v)`). The switch-generation code, which was not designed with untagged unions in mind, merges the two cases into one (and makes one of the two empty). However, for `Object` and `Array`, what's generated is not a straight switch, but a mix of `if-then-else` and `switch`. This means that the `Object` and `Array` cases are apart in the generated code, and merging them (making one empty) is wrong. --- CHANGELOG.md | 1 + jscomp/core/lam_compile.ml | 22 ++++++++++++------- jscomp/test/UntaggedVariants.js | 37 ++++++++++++++++++++++++++++++++ jscomp/test/UntaggedVariants.res | 29 ++++++++++++++++++++++++- 4 files changed, 80 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3cd660afc0..62c439b2ed 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,7 @@ #### :bug: Bug Fix - Allow to use exotic ParscalCased identifiers for types. https://github.com/rescript-lang/rescript-compiler/pull/6777 +- Fix issue of incorrect switch cases with identical bodies when mixing object and array. https://github.com/rescript-lang/rescript-compiler/pull/6792 #### :house: Internal diff --git a/jscomp/core/lam_compile.ml b/jscomp/core/lam_compile.ml index 6f192c1bbc..8161894bf9 100644 --- a/jscomp/core/lam_compile.ml +++ b/jscomp/core/lam_compile.ml @@ -121,10 +121,10 @@ let morph_declare_to_assign (cxt : Lam_compile_context.t) k = k { cxt with continuation = Assign did } (Some (kind, did)) | _ -> k cxt None -let group_apply cases callback = +let group_apply ~merge_cases cases callback = Ext_list.flat_map - (Ext_list.stable_group cases (fun (_, lam) (_, lam1) -> - Lam.eq_approx lam lam1)) + (Ext_list.stable_group cases (fun (tag1, lam) (tag2, lam1) -> + merge_cases tag1 tag2 && Lam.eq_approx lam lam1)) (fun group -> Ext_list.map_last group callback) (* TODO: for expression generation, @@ -509,6 +509,7 @@ and compile_general_cases : _ -> ('a * J.case_clause) list -> J.statement) -> switch_exp: J.expression -> default: default_case -> + ?merge_cases: ('a -> 'a -> bool) -> ('a * Lam.t) list -> J.block = fun (type a) @@ -522,6 +523,7 @@ and compile_general_cases : ) ~(switch_exp : J.expression) ~(default : default_case) + ?(merge_cases = fun _ _ -> true) (cases : (a * Lam.t) list) -> match (cases, default) with | [], Default lam -> Js_output.output_as_block (compile_lambda cxt lam) @@ -584,7 +586,7 @@ and compile_general_cases : Some (Js_output.output_as_block (compile_lambda cxt lam)) in let body = - group_apply cases (fun last (switch_case, lam) -> + group_apply ~merge_cases cases (fun last (switch_case, lam) -> if last then (* merge and shared *) let switch_body, should_break = @@ -766,11 +768,12 @@ and compile_untagged_cases ~cxt ~switch_exp ~default ~block_cases cases = in E.emit_check check in - let is_not_typeof (l, _) = match l with - | Ast_untagged_variants.Untagged (InstanceType _) -> true - | _ -> false in + let tag_is_not_typeof = function + | Ast_untagged_variants.Untagged (InstanceType _) -> true + | _ -> false in + let clause_is_not_typeof (tag, _) = tag_is_not_typeof tag in let switch ?default ?declaration e clauses = - let (not_typeof_clauses, typeof_clauses) = List.partition is_not_typeof clauses in + let (not_typeof_clauses, typeof_clauses) = List.partition clause_is_not_typeof clauses in let rec build_if_chain remaining_clauses = (match remaining_clauses with | (Ast_untagged_variants.Untagged (InstanceType instance_type), {J.switch_body}) :: rest -> S.if_ (E.emit_check (IsInstanceOf (instance_type, Expr e))) @@ -778,6 +781,8 @@ and compile_untagged_cases ~cxt ~switch_exp ~default ~block_cases cases = ~else_:([build_if_chain rest]) | _ -> S.string_switch ?default ?declaration (E.typeof e) typeof_clauses) in build_if_chain not_typeof_clauses in + let merge_cases tag1 tag2 = (* only merge typeof cases, as instanceof cases are pulled out into if-then-else *) + not (tag_is_not_typeof tag1 || tag_is_not_typeof tag2) in cases |> compile_general_cases ~make_exp: E.tag_type ~eq_exp: mk_eq @@ -785,6 +790,7 @@ and compile_untagged_cases ~cxt ~switch_exp ~default ~block_cases cases = ~switch ~switch_exp ~default + ~merge_cases and compile_stringswitch l cases default (lambda_cxt : Lam_compile_context.t) = (* TODO might better optimization according to the number of cases diff --git a/jscomp/test/UntaggedVariants.js b/jscomp/test/UntaggedVariants.js index 739535cd41..3f3380e7f7 100644 --- a/jscomp/test/UntaggedVariants.js +++ b/jscomp/test/UntaggedVariants.js @@ -587,6 +587,42 @@ let OnlyOne = { onlyOne: "OnlyOne" }; +function should_not_merge(x) { + if (Array.isArray(x)) { + return "do not merge"; + } + if (x instanceof Date) { + return "do not merge"; + } + switch (typeof x) { + case "boolean" : + return "boolean"; + case "object" : + return "do not merge"; + + } +} + +function can_merge(x) { + if (Array.isArray(x)) { + return "do not merge"; + } + if (x instanceof Date) { + return "do not merge"; + } + switch (typeof x) { + case "boolean" : + case "object" : + return "merge"; + + } +} + +let MergeCases = { + should_not_merge: should_not_merge, + can_merge: can_merge +}; + let i = 42; let i2 = 42.5; @@ -634,4 +670,5 @@ exports.Arr = Arr; exports.AllInstanceofTypes = AllInstanceofTypes; exports.Aliased = Aliased; exports.OnlyOne = OnlyOne; +exports.MergeCases = MergeCases; /* l2 Not a pure module */ diff --git a/jscomp/test/UntaggedVariants.res b/jscomp/test/UntaggedVariants.res index 0bcfe81208..d49722195f 100644 --- a/jscomp/test/UntaggedVariants.res +++ b/jscomp/test/UntaggedVariants.res @@ -431,4 +431,31 @@ module Aliased = { module OnlyOne = { @unboxed type onlyOne = OnlyOne let onlyOne = OnlyOne -} \ No newline at end of file +} + +module MergeCases = { + type obj = {name: string} + + @unboxed + type t = + | Boolean(bool) + | Object(obj) + | Array(array) + | Date(Js.Date.t) + + let should_not_merge = x => + switch x { + | Object(_) => "do not merge" + | Array(_) => "do not merge" + | Date(_) => "do not merge" + | Boolean(_) => "boolean" + } + + let can_merge = x => + switch x { + | Object(_) => "merge" + | Array(_) => "do not merge" + | Date(_) => "do not merge" + | Boolean(_) => "merge" + } +}