Skip to content

Fix JSON switch to correctly handle null and arrays without explicit … #8254

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.

Sign up for GitHub

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

Closed
jagguji wants to merge 1 commit into from
+506 −63
Closed

Fix JSON switch to correctly handle null and arrays without explicit … #8254

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
63 changes: 51 additions & 12 deletions compiler/core/lam_compile.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -814,9 +814,23 @@ let compile output_prefix =
| _ -> false
in
let clause_is_not_typeof (tag, _) = tag_is_not_typeof tag in
let clause_is_object_typeof = function
| Ast_untagged_variants.Untagged ObjectType, _ -> true
| _ -> false
let has_null_clause clauses =
Ext_list.exists clauses (fun (tag, _) ->
match (tag : Ast_untagged_variants.tag_type) with
| Null -> true
| _ -> false)
in
let has_object_clause clauses =
Ext_list.exists clauses (fun (tag, _) ->
match (tag : Ast_untagged_variants.tag_type) with
| Untagged ObjectType -> true
| _ -> false)
in
let has_array_clause clauses =
Ext_list.exists clauses (fun (tag, _) ->
match (tag : Ast_untagged_variants.tag_type) with
| Untagged (InstanceType Array) -> true
| _ -> false)
in
let switch ?default ?declaration e clauses =
let not_typeof_clauses, typeof_clauses =
Expand All @@ -831,17 +845,42 @@ let compile output_prefix =
(E.emit_check (IsInstanceOf (instance_type, Expr e)))
switch_body
~else_:[build_if_chain rest]
| _ ->
let typeof_switch () =
| _ -> (
let switch_stmt =
S.string_switch ?default ?declaration (E.typeof e) typeof_clauses
in
if has_null_case && List.exists clause_is_object_typeof typeof_clauses
then
match default with
| Some default_body ->
S.if_ (E.is_null e) default_body ~else_:[typeof_switch ()]
| None -> typeof_switch ()
else typeof_switch ()
(* If there's an Object clause but no Null clause and null is a possible value,
we need to exclude null from matching the Object case.
In JavaScript, typeof null === "object", so null would incorrectly match Object. *)
let needs_null_check =
has_null_case
&& (not (has_null_clause typeof_clauses))
&& has_object_clause typeof_clauses
in
(* If Array is a possible block type but there's no Array clause and there is an Object clause,
we need to exclude arrays from matching the Object case.
In JavaScript, typeof [] === "object", so arrays would incorrectly match Object. *)
let needs_array_check =
List.mem Ast_untagged_variants.(InstanceType Array) block_cases
&& (not (has_array_clause typeof_clauses))
&& has_object_clause typeof_clauses
in
let cond =
match (needs_null_check, needs_array_check) with
| true, true ->
Some (E.and_ (E.not (E.is_null e)) (E.not (E.is_array e)))
| true, false -> Some (E.not (E.is_null e))
| false, true -> Some (E.not (E.is_array e))
| false, false -> None
in
match cond with
| Some c ->
S.if_ c [switch_stmt]
~else_:
(match default with
| Some block -> block
| None -> [])
| None -> switch_stmt)
in
build_if_chain not_typeof_clauses
in
Expand Down
199 changes: 199 additions & 0 deletions tests/tests/src/JSONSwitchNullCheck_test.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,199 @@
// Generated by ReScript, PLEASE EDIT WITH CARE

import * as Mocha from "mocha";
import * as Test_utils from "./test_utils.mjs";

let jsonNull = null;

let jsonArray = ["a", "b"];

let jsonObject = {"key": "value"};

let jsonString = "hello";

let jsonNumber = 42;

let jsonBool = true;

Mocha.describe("JSONSwitchNullCheck_test", () => {
Mocha.describe("JSON direct switch with Null - should NOT match Object", () => {
Mocha.test("switch null { | Object(_) => Object | Array(_) => Array | _ => default }", () => {
let result;
if (Array.isArray(jsonNull)) {
result = "Array";
} else if (jsonNull !== null && !Array.isArray(jsonNull)) {
switch (typeof jsonNull) {
case "object" :
result = "Object";
break;
default:
result = "default";
}
} else {
result = "default";
}
Test_utils.eq("File \"JSONSwitchNullCheck_test.res\", line 26, characters 11-18", result, "default");
});
Mocha.test("switch null { | Object(_) => Object | String(_) => String | _ => default }", () => {
let result;
if (jsonNull !== null && !Array.isArray(jsonNull)) {
switch (typeof jsonNull) {
case "string" :
result = "String";
break;
case "object" :
result = "Object";
break;
default:
result = "default";
}
} else {
result = "default";
}
Test_utils.eq("File \"JSONSwitchNullCheck_test.res\", line 39, characters 11-18", result, "default");
});
Mocha.test("switch null { | Object(_) => Object | Number(_) => Number | _ => default }", () => {
let result;
if (jsonNull !== null && !Array.isArray(jsonNull)) {
switch (typeof jsonNull) {
case "number" :
result = "Number";
break;
case "object" :
result = "Object";
break;
default:
result = "default";
}
} else {
result = "default";
}
Test_utils.eq("File \"JSONSwitchNullCheck_test.res\", line 52, characters 11-18", result, "default");
});
Mocha.test("switch null { | Object(_) => Object | Array(_) => Array | String(_) => String | _ => default }", () => {
let result;
if (Array.isArray(jsonNull)) {
result = "Array";
} else if (jsonNull !== null && !Array.isArray(jsonNull)) {
switch (typeof jsonNull) {
case "string" :
result = "String";
break;
case "object" :
result = "Object";
break;
default:
result = "default";
}
} else {
result = "default";
}
Test_utils.eq("File \"JSONSwitchNullCheck_test.res\", line 66, characters 11-18", result, "default");
});
});
Mocha.describe("JSON direct switch with Array - should NOT match Object", () => {
Mocha.test("switch array { | Object(_) => Object | String(_) => String | _ => default }", () => {
let result;
if (jsonArray !== null && !Array.isArray(jsonArray)) {
switch (typeof jsonArray) {
case "string" :
result = "String";
break;
case "object" :
result = "Object";
break;
default:
result = "default";
}
} else {
result = "default";
}
Test_utils.eq("File \"JSONSwitchNullCheck_test.res\", line 81, characters 11-18", result, "default");
});
Mocha.test("switch array { | Object(_) => Object | Number(_) => Number | _ => default }", () => {
let result;
if (jsonArray !== null && !Array.isArray(jsonArray)) {
switch (typeof jsonArray) {
case "number" :
result = "Number";
break;
case "object" :
result = "Object";
break;
default:
result = "default";
}
} else {
result = "default";
}
Test_utils.eq("File \"JSONSwitchNullCheck_test.res\", line 94, characters 11-18", result, "default");
});
Mocha.test("switch array { | Object(_) => Object | Boolean(_) => Boolean | _ => default }", () => {
let result;
if (jsonArray !== null && !Array.isArray(jsonArray)) {
switch (typeof jsonArray) {
case "boolean" :
result = "Boolean";
break;
case "object" :
result = "Object";
break;
default:
result = "default";
}
} else {
result = "default";
}
Test_utils.eq("File \"JSONSwitchNullCheck_test.res\", line 107, characters 11-18", result, "default");
});
});
Mocha.describe("JSON switch - correct positive cases", () => {
Mocha.test("switch object { | Object(_) => Object | _ => default }", () => {
let result;
result = typeof jsonObject === "object" && jsonObject !== null && !Array.isArray(jsonObject) ? "Object" : "default";
Test_utils.eq("File \"JSONSwitchNullCheck_test.res\", line 120, characters 11-18", result, "Object");
});
Mocha.test("switch array { | Array(_) => Array | _ => default }", () => {
let result;
result = Array.isArray(jsonArray) ? "Array" : "default";
Test_utils.eq("File \"JSONSwitchNullCheck_test.res\", line 131, characters 11-18", result, "Array");
});
Mocha.test("switch string { | String(_) => String | _ => default }", () => {
let result;
result = typeof jsonString === "string" ? "String" : "default";
Test_utils.eq("File \"JSONSwitchNullCheck_test.res\", line 142, characters 11-18", result, "String");
});
Mocha.test("switch number { | Number(_) => Number | _ => default }", () => {
let result;
result = typeof jsonNumber === "number" ? "Number" : "default";
Test_utils.eq("File \"JSONSwitchNullCheck_test.res\", line 153, characters 11-18", result, "Number");
});
Mocha.test("switch bool { | Boolean(_) => Boolean | _ => default }", () => {
let result;
result = typeof jsonBool === "boolean" ? "Boolean" : "default";
Test_utils.eq("File \"JSONSwitchNullCheck_test.res\", line 164, characters 11-18", result, "Boolean");
});
Mocha.test("switch null { | Null => Null | _ => default }", () => {
let result;
result = jsonNull === null ? "Null" : "default";
Test_utils.eq("File \"JSONSwitchNullCheck_test.res\", line 175, characters 11-18", result, "Null");
});
Mocha.test("switch null { | Null => Null | Object(_) => Object | _ => default }", () => {
let result;
result = jsonNull === null ? "Null" : (
typeof jsonNull === "object" && !Array.isArray(jsonNull) ? "Object" : "default"
);
Test_utils.eq("File \"JSONSwitchNullCheck_test.res\", line 187, characters 11-18", result, "Null");
});
});
});

export {
jsonNull,
jsonArray,
jsonObject,
jsonString,
jsonNumber,
jsonBool,
}
/* Not a pure module */
Loading
Loading