Skip to content

Fix issue with untagged variants and object when null is one case. #7303

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

Merged
merged 2 commits into from
Feb 17, 2025
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 @@ -32,6 +32,7 @@
- Fix issue in functors with more than one argument (which are curried): emit nested function always. https://github.com/rescript-lang/rescript/pull/7273
- Fix dot completion issue with React primitives. https://github.com/rescript-lang/rescript/pull/7292
- Stdlib namespace for Core modules (fixes name clashes with user modules). https://github.com/rescript-lang/rescript/pull/7285
- Fix runtime type check for Object in untagged variants when one variant case is `null`. https://github.com/rescript-lang/rescript/pull/7303

#### :house: Internal

Expand Down
27 changes: 16 additions & 11 deletions compiler/core/lam_compile.ml
Original file line number Diff line number Diff line change
Expand Up @@ -638,14 +638,14 @@ let compile output_prefix =
Some ((String name, lam) :: string_table)
| _, _ -> None)
table (Some [])
and compile_cases ?(untagged = false) ~cxt ~(switch_exp : E.t)
?(default = NonComplete) ?(get_tag = fun _ -> None) ?(block_cases = [])
cases : initialization =
and compile_cases ?(untagged = false) ?(has_null_case = false) ~cxt
~(switch_exp : E.t) ?(default = NonComplete) ?(get_tag = fun _ -> None)
?(block_cases = []) cases : initialization =
match use_compile_literal_cases cases ~get_tag with
| Some string_cases ->
if untagged then
compile_untagged_cases ~cxt ~switch_exp ~block_cases ~default
string_cases
~has_null_case string_cases
else compile_string_cases ~cxt ~switch_exp ~default string_cases
| None ->
cases
Expand Down Expand Up @@ -718,7 +718,7 @@ let compile output_prefix =
else
(* [e] will be used twice *)
let dispatch e =
let is_a_literal_case =
let is_a_literal_case () =
if untagged then
E.is_a_literal_case
~literal_cases:(get_literal_cases sw_names)
Expand All @@ -740,13 +740,17 @@ let compile output_prefix =
&& List.length sw_consts = 0
&& eq_default sw_num_default sw_blocks_default
then
let literal_cases = get_literal_cases sw_names in
let has_null_case =
List.mem Ast_untagged_variants.Null literal_cases
in
compile_cases ~untagged ~cxt
~switch_exp:(if untagged then e else E.tag ~name:tag_name e)
~block_cases ~default:sw_blocks_default ~get_tag:get_block_tag
sw_blocks
~block_cases ~has_null_case ~default:sw_blocks_default
~get_tag:get_block_tag sw_blocks
else
[
S.if_ is_a_literal_case
S.if_ (is_a_literal_case ())
(compile_cases ~cxt ~switch_exp:e ~block_cases
~default:sw_num_default ~get_tag:get_const_tag sw_consts)
~else_:
Expand Down Expand Up @@ -789,16 +793,17 @@ let compile output_prefix =
~switch:(fun ?default ?declaration e clauses ->
S.string_switch ?default ?declaration e clauses)
~switch_exp ~default
and compile_untagged_cases ~cxt ~switch_exp ~default ~block_cases cases =
and compile_untagged_cases ~cxt ~switch_exp ~default ~block_cases
~has_null_case cases =
let mk_eq (i : Ast_untagged_variants.tag_type option) x j y =
let check =
match (i, j) with
| Some tag_type, _ ->
Ast_untagged_variants.DynamicChecks.add_runtime_type_check ~tag_type
~block_cases (Expr x) (Expr y)
~has_null_case ~block_cases (Expr x) (Expr y)
| _, Some tag_type ->
Ast_untagged_variants.DynamicChecks.add_runtime_type_check ~tag_type
~block_cases (Expr y) (Expr x)
~has_null_case ~block_cases (Expr y) (Expr x)
| _ -> Ast_untagged_variants.DynamicChecks.( == ) (Expr x) (Expr y)
in
E.emit_check check
Expand Down
13 changes: 8 additions & 5 deletions compiler/ml/ast_untagged_variants.ml
Original file line number Diff line number Diff line change
Expand Up @@ -535,7 +535,8 @@ module DynamicChecks = struct
else (* (undefiled + other) || other *)
typeof e != object_

let add_runtime_type_check ~tag_type ~(block_cases : block_type list) x y =
let add_runtime_type_check ~tag_type ~has_null_case
~(block_cases : block_type list) x y =
let instances =
Ext_list.filter_map block_cases (function
| InstanceType i -> Some i
Expand All @@ -547,14 +548,16 @@ module DynamicChecks = struct
| FunctionType ) ->
typeof y == x
| Untagged ObjectType ->
let object_case =
if has_null_case then typeof y == x &&& (y != nil) else typeof y == x
in
if instances <> [] then
let not_one_of_the_instances =
Ext_list.fold_right instances
(typeof y == x)
(fun i x -> x &&& not (is_instance i y))
Ext_list.fold_right instances object_case (fun i x ->
x &&& not (is_instance i y))
in
not_one_of_the_instances
else typeof y == x
else object_case
| Untagged (InstanceType i) -> is_instance i y
| Untagged UnknownType ->
(* This should not happen because unknown must be the only non-literal case *)
Expand Down
12 changes: 12 additions & 0 deletions tests/tests/src/UntaggedVariants.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -589,6 +589,17 @@ let MergeCases = {
can_merge: can_merge
};

function printLength(json) {
if (typeof json !== "object" || json === null || Array.isArray(json)) {
return;
}
console.log("Length: ", Object.values(json).length);
}

let ObjectAndNull = {
printLength: printLength
};

let $$Array;

let i = 42;
Expand Down Expand Up @@ -641,5 +652,6 @@ export {
Aliased,
OnlyOne,
MergeCases,
ObjectAndNull,
}
/* l2 Not a pure module */
8 changes: 8 additions & 0 deletions tests/tests/src/UntaggedVariants.res
Original file line number Diff line number Diff line change
Expand Up @@ -461,3 +461,11 @@ module MergeCases = {
| Boolean(_) => "merge"
}
}

module ObjectAndNull = {
let printLength = (json: JSON.t) =>
switch json {
| Object(o) => Console.log2("Length: ", o->Dict.valuesToArray->Array.length)
| _ => ()
}
}
4 changes: 2 additions & 2 deletions tests/tests/src/core/Core_JsonTests.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@ import * as Test from "./Test.mjs";
function decodeJsonTest() {
let json = {"someProp":{"otherProp": null, "thirdProp": [true, false]}};
let decodedCorrectly;
if (typeof json === "object" && !Array.isArray(json)) {
if (typeof json === "object" && json !== null && !Array.isArray(json)) {
let match = json["someProp"];
if (typeof match === "object" && !Array.isArray(match)) {
if (typeof match === "object" && match !== null && !Array.isArray(match)) {
let match$1 = match["thirdProp"];
if (Array.isArray(match$1) && match$1.length === 2) {
let match$2 = match$1[0];
Expand Down
4 changes: 2 additions & 2 deletions tests/tests/src/json_decorders.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
import * as Stdlib_Array from "rescript/lib/es6/Stdlib_Array.js";

function decodeUser(json) {
if (typeof json !== "object" || Array.isArray(json)) {
if (typeof json !== "object" || json === null || Array.isArray(json)) {
return;
}
let id = json.id;
Expand All @@ -30,7 +30,7 @@ function decodeUser(json) {
}

function decodeGroup(json) {
if (typeof json !== "object" || Array.isArray(json)) {
if (typeof json !== "object" || json === null || Array.isArray(json)) {
return;
}
let id = json.id;
Expand Down