Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issue of incorrect switch cases with identical bodies when mixing… #6792

Merged
merged 1 commit into from
May 30, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
22 changes: 14 additions & 8 deletions jscomp/core/lam_compile.ml
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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 =
Expand Down Expand Up @@ -766,25 +768,29 @@ 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)))
(switch_body)
~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
~cxt
~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
Expand Down
37 changes: 37 additions & 0 deletions jscomp/test/UntaggedVariants.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

29 changes: 28 additions & 1 deletion jscomp/test/UntaggedVariants.res
Original file line number Diff line number Diff line change
Expand Up @@ -431,4 +431,31 @@ module Aliased = {
module OnlyOne = {
@unboxed type onlyOne = OnlyOne
let onlyOne = OnlyOne
}
}

module MergeCases = {
type obj = {name: string}

@unboxed
type t =
| Boolean(bool)
| Object(obj)
| Array(array<int>)
| 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"
}
}
Loading