From 8064fa9d612832da9199b5ae378e464cbc233335 Mon Sep 17 00:00:00 2001 From: "roshan.chourasiya" Date: Wed, 25 Feb 2026 15:44:37 +0530 Subject: [PATCH] Fix JSON switch to correctly handle null and arrays without explicit cases When switching on JSON.t values without explicit Null or Array cases, both null and arrays were incorrectly matching the Object case because in JavaScript: - typeof null === "object" - typeof [] === "object" Changes: - Modified compiler/core/lam_compile.ml to add null/array guards when the switch has an Object clause but no explicit Null/Array clauses - Added tests/tests/src/JSONSwitchNullCheck_test.res with comprehensive test cases covering all scenarios Fixes scenarios like: switch jsonNull { | Object(_) => "Object" | Array(_) => "Array" | _ => "default" // null was incorrectly matching Object } The fix generates guards like: if (x !== null && !Array.isArray(x)) { switch (typeof x) { case "object": ... } } Signed-Off-By: Claude Opus 4.6 --- compiler/core/lam_compile.ml | 63 ++++-- tests/tests/src/JSONSwitchNullCheck_test.mjs | 199 +++++++++++++++++++ tests/tests/src/JSONSwitchNullCheck_test.res | 191 ++++++++++++++++++ tests/tests/src/UntaggedVariants.mjs | 116 ++++++----- 4 files changed, 506 insertions(+), 63 deletions(-) create mode 100644 tests/tests/src/JSONSwitchNullCheck_test.mjs create mode 100644 tests/tests/src/JSONSwitchNullCheck_test.res diff --git a/compiler/core/lam_compile.ml b/compiler/core/lam_compile.ml index 680bce50a7..5de8127194 100644 --- a/compiler/core/lam_compile.ml +++ b/compiler/core/lam_compile.ml @@ -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 = @@ -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 diff --git a/tests/tests/src/JSONSwitchNullCheck_test.mjs b/tests/tests/src/JSONSwitchNullCheck_test.mjs new file mode 100644 index 0000000000..d1152f8bce --- /dev/null +++ b/tests/tests/src/JSONSwitchNullCheck_test.mjs @@ -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 */ diff --git a/tests/tests/src/JSONSwitchNullCheck_test.res b/tests/tests/src/JSONSwitchNullCheck_test.res new file mode 100644 index 0000000000..612492fc67 --- /dev/null +++ b/tests/tests/src/JSONSwitchNullCheck_test.res @@ -0,0 +1,191 @@ +open Mocha +open Test_utils + +// Test cases for JSON switch with null and various block types +// These test the fix for: null incorrectly matching Object case in switch + +// Helper to create test JSON values +let jsonNull: JSON.t = %raw(`null`) +let jsonArray: JSON.t = %raw(`["a", "b"]`) +let jsonObject: JSON.t = %raw(`{"key": "value"}`) +let jsonString: JSON.t = %raw(`"hello"`) +let jsonNumber: JSON.t = %raw(`42`) +let jsonBool: JSON.t = %raw(`true`) + +describe(__MODULE__, () => { + describe("JSON direct switch with Null - should NOT match Object", () => { + // Test: Object | Array | _ with null - null should go to default + test( + "switch null { | Object(_) => Object | Array(_) => Array | _ => default }", + () => { + let result = switch jsonNull { + | Object(_) => "Object" + | Array(_) => "Array" + | _ => "default" + } + eq(__LOC__, result, "default") + }, + ) + + // Test: Object | String | _ with null - null should go to default + test( + "switch null { | Object(_) => Object | String(_) => String | _ => default }", + () => { + let result = switch jsonNull { + | Object(_) => "Object" + | String(_) => "String" + | _ => "default" + } + eq(__LOC__, result, "default") + }, + ) + + // Test: Object | Number | _ with null - null should go to default + test( + "switch null { | Object(_) => Object | Number(_) => Number | _ => default }", + () => { + let result = switch jsonNull { + | Object(_) => "Object" + | Number(_) => "Number" + | _ => "default" + } + eq(__LOC__, result, "default") + }, + ) + + // Test: Multiple block cases with null - null should go to default + test( + "switch null { | Object(_) => Object | Array(_) => Array | String(_) => String | _ => default }", + () => { + let result = switch jsonNull { + | Object(_) => "Object" + | Array(_) => "Array" + | String(_) => "String" + | _ => "default" + } + eq(__LOC__, result, "default") + }, + ) + }) + + describe("JSON direct switch with Array - should NOT match Object", () => { + // Test: Array being matched as Object - Object | String | _ with array + test( + "switch array { | Object(_) => Object | String(_) => String | _ => default }", + () => { + let result = switch jsonArray { + | Object(_) => "Object" + | String(_) => "String" + | _ => "default" + } + eq(__LOC__, result, "default") + }, + ) + + // Test: Array being matched as Object - Object | Number | _ with array + test( + "switch array { | Object(_) => Object | Number(_) => Number | _ => default }", + () => { + let result = switch jsonArray { + | Object(_) => "Object" + | Number(_) => "Number" + | _ => "default" + } + eq(__LOC__, result, "default") + }, + ) + + // Test: Array being matched as Object - Object | Boolean | _ with array + test( + "switch array { | Object(_) => Object | Boolean(_) => Boolean | _ => default }", + () => { + let result = switch jsonArray { + | Object(_) => "Object" + | Boolean(_) => "Boolean" + | _ => "default" + } + eq(__LOC__, result, "default") + }, + ) + }) + + describe("JSON switch - correct positive cases", () => { + test( + "switch object { | Object(_) => Object | _ => default }", + () => { + let result = switch jsonObject { + | Object(_) => "Object" + | _ => "default" + } + eq(__LOC__, result, "Object") + }, + ) + + test( + "switch array { | Array(_) => Array | _ => default }", + () => { + let result = switch jsonArray { + | Array(_) => "Array" + | _ => "default" + } + eq(__LOC__, result, "Array") + }, + ) + + test( + "switch string { | String(_) => String | _ => default }", + () => { + let result = switch jsonString { + | String(_) => "String" + | _ => "default" + } + eq(__LOC__, result, "String") + }, + ) + + test( + "switch number { | Number(_) => Number | _ => default }", + () => { + let result = switch jsonNumber { + | Number(_) => "Number" + | _ => "default" + } + eq(__LOC__, result, "Number") + }, + ) + + test( + "switch bool { | Boolean(_) => Boolean | _ => default }", + () => { + let result = switch jsonBool { + | Boolean(_) => "Boolean" + | _ => "default" + } + eq(__LOC__, result, "Boolean") + }, + ) + + test( + "switch null { | Null => Null | _ => default }", + () => { + let result = switch jsonNull { + | Null => "Null" + | _ => "default" + } + eq(__LOC__, result, "Null") + }, + ) + + test( + "switch null { | Null => Null | Object(_) => Object | _ => default }", + () => { + let result = switch jsonNull { + | Null => "Null" + | Object(_) => "Object" + | _ => "default" + } + eq(__LOC__, result, "Null") + }, + ) + }) +}) diff --git a/tests/tests/src/UntaggedVariants.mjs b/tests/tests/src/UntaggedVariants.mjs index b57405ea26..5c1dc4383f 100644 --- a/tests/tests/src/UntaggedVariants.mjs +++ b/tests/tests/src/UntaggedVariants.mjs @@ -157,13 +157,15 @@ function classify$5(x) { if (Array.isArray(x)) { return "array"; } - switch (typeof x) { - case "string" : - return "string"; - case "number" : - return "int"; - case "object" : - return "Object" + x.name; + if (!Array.isArray(x)) { + switch (typeof x) { + case "string" : + return "string"; + case "number" : + return "int"; + case "object" : + return "Object" + x.name; + } } } @@ -188,22 +190,24 @@ function classify$6(x) { _0: x }; } - switch (typeof x) { - case "string" : - return { - TAG: "JSONString", - _0: x - }; - case "number" : - return { - TAG: "JSONNumber", - _0: x - }; - case "object" : - return { - TAG: "JSONObject", - _0: x - }; + if (!Array.isArray(x)) { + switch (typeof x) { + case "string" : + return { + TAG: "JSONString", + _0: x + }; + case "number" : + return { + TAG: "JSONNumber", + _0: x + }; + case "object" : + return { + TAG: "JSONObject", + _0: x + }; + } } } } @@ -357,11 +361,13 @@ function classify$9(v) { if (Array.isArray(v)) { return v[0]; } - switch (typeof v) { - case "object" : - return v.x; - case "function" : - return v(3); + if (!Array.isArray(v)) { + switch (typeof v) { + case "object" : + return v.x; + case "function" : + return v(3); + } } } @@ -481,13 +487,15 @@ async function classify$10(a) { console.log(await a); return; } - switch (typeof a) { - case "string" : - console.log(a); - return; - case "object" : - console.log(a.userName); - return; + if (!Array.isArray(a)) { + switch (typeof a) { + case "string" : + console.log(a); + return; + case "object" : + console.log(a.userName); + return; + } } } } @@ -589,13 +597,15 @@ async function classifyAll(t) { console.log("WeakMap"); return; } - switch (typeof t) { - case "string" : - console.log(t); - return; - case "object" : - console.log(t.userName); - return; + if (!Array.isArray(t)) { + switch (typeof t) { + case "string" : + console.log(t); + return; + case "object" : + console.log(t.userName); + return; + } } } @@ -629,11 +639,13 @@ function should_not_merge(x) { if (x instanceof Date) { return "do not merge"; } - switch (typeof x) { - case "boolean" : - return "boolean"; - case "object" : - return "do not merge"; + if (!Array.isArray(x)) { + switch (typeof x) { + case "boolean" : + return "boolean"; + case "object" : + return "do not merge"; + } } } @@ -644,10 +656,12 @@ function can_merge(x) { if (x instanceof Date) { return "do not merge"; } - switch (typeof x) { - case "boolean" : - case "object" : - return "merge"; + if (!Array.isArray(x)) { + switch (typeof x) { + case "boolean" : + case "object" : + return "merge"; + } } }