From 516bd06261949a4d02b85daf0113d449415a588b Mon Sep 17 00:00:00 2001 From: Cristiano Calcagno Date: Wed, 25 Feb 2026 14:19:20 +0100 Subject: [PATCH] Also guard against arrays falling into object branch in untagged variant switch (#8251) Arrays also have typeof "object", so when there's an ObjectType typeof case but no Array instanceof case, arrays would incorrectly match the object branch. Add an Array.isArray guard alongside the null guard. Also clean up the guard logic to use direct pattern matching instead of intermediate list building. Signed-Off-By: Cristiano Calcagno Co-Authored-By: Claude Opus 4.6 --- compiler/core/lam_compile.ml | 43 +++++++++++++++++++++--------- tests/tests/src/js_json_test.mjs | 45 ++++++++++++++++++++++---------- tests/tests/src/js_json_test.res | 15 ++++++++++- 3 files changed, 76 insertions(+), 27 deletions(-) diff --git a/compiler/core/lam_compile.ml b/compiler/core/lam_compile.ml index 680bce50a7..87a25bfe2a 100644 --- a/compiler/core/lam_compile.ml +++ b/compiler/core/lam_compile.ml @@ -814,14 +814,29 @@ 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 - in let switch ?default ?declaration e clauses = let not_typeof_clauses, typeof_clauses = List.partition clause_is_not_typeof clauses in + let has_object_typeof = + List.exists + (function + | Ast_untagged_variants.Untagged ObjectType, _ -> true + | _ -> false) + typeof_clauses + in + let has_array_case = + List.exists + (function + | Ast_untagged_variants.Untagged (InstanceType Array), _ -> true + | _ -> false) + not_typeof_clauses + in + (* When there's an ObjectType typeof case, null and arrays can + incorrectly match it (typeof null === typeof [] === "object"). + Guard against them when they should fall through to default. *) + let needs_null_guard = has_object_typeof && has_null_case in + let needs_array_guard = has_object_typeof && not has_array_case in let rec build_if_chain remaining_clauses = match remaining_clauses with | ( Ast_untagged_variants.Untagged (InstanceType instance_type), @@ -831,17 +846,21 @@ let compile output_prefix = (E.emit_check (IsInstanceOf (instance_type, Expr e))) switch_body ~else_:[build_if_chain rest] - | _ -> + | _ -> ( let typeof_switch () = 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 () + let guard = + match (needs_null_guard, needs_array_guard) with + | true, true -> Some (E.or_ (E.is_null e) (E.is_array e)) + | true, false -> Some (E.is_null e) + | false, true -> Some (E.is_array e) + | false, false -> None + in + match (guard, default) with + | Some guard, Some default_body -> + S.if_ guard default_body ~else_:[typeof_switch ()] + | _ -> typeof_switch ()) in build_if_chain not_typeof_clauses in diff --git a/tests/tests/src/js_json_test.mjs b/tests/tests/src/js_json_test.mjs index c69a9fd6f3..8ff5ed4fa0 100644 --- a/tests/tests/src/js_json_test.mjs +++ b/tests/tests/src/js_json_test.mjs @@ -367,7 +367,7 @@ Mocha.describe("Js_json_test", () => { Test_utils.eq("File \"js_json_test.res\", line 314, characters 7-14", Js_json.decodeArray({}), undefined); Test_utils.eq("File \"js_json_test.res\", line 315, characters 7-14", Js_json.decodeArray(1.23), undefined); }); - Mocha.test("JSON Array/Object switch falls through to wildcard on null", () => { + Mocha.test("JSON Array/Object switch falls through to wildcard on null and array", () => { let classifyArrayOrObject = json => { if (Array.isArray(json)) { return json.length; @@ -386,25 +386,42 @@ Mocha.describe("Js_json_test", () => { Test_utils.eq("File \"js_json_test.res\", line 328, characters 7-14", classifyArrayOrObject(null), undefined); Test_utils.eq("File \"js_json_test.res\", line 329, characters 7-14", classifyArrayOrObject([1]), 1); Test_utils.eq("File \"js_json_test.res\", line 330, characters 7-14", classifyArrayOrObject({}), 0); + let classifyObjectOnly = json => { + if (json === null || Array.isArray(json)) { + return "default"; + } + switch (typeof json) { + case "string" : + return "String"; + case "object" : + return "Object"; + default: + return "default"; + } + }; + Test_utils.eq("File \"js_json_test.res\", line 340, characters 7-14", classifyObjectOnly(null), "default"); + Test_utils.eq("File \"js_json_test.res\", line 341, characters 7-14", classifyObjectOnly([]), "default"); + Test_utils.eq("File \"js_json_test.res\", line 342, characters 7-14", classifyObjectOnly({}), "Object"); + Test_utils.eq("File \"js_json_test.res\", line 343, characters 7-14", classifyObjectOnly("hi"), "String"); }); Mocha.test("JSON decodeBoolean", () => { - Test_utils.eq("File \"js_json_test.res\", line 334, characters 7-14", Js_json.decodeBoolean("test"), undefined); - Test_utils.eq("File \"js_json_test.res\", line 335, characters 7-14", Js_json.decodeBoolean(true), true); - Test_utils.eq("File \"js_json_test.res\", line 336, characters 7-14", Js_json.decodeBoolean([]), undefined); - Test_utils.eq("File \"js_json_test.res\", line 337, characters 7-14", Js_json.decodeBoolean(null), undefined); - Test_utils.eq("File \"js_json_test.res\", line 338, characters 7-14", Js_json.decodeBoolean({}), undefined); - Test_utils.eq("File \"js_json_test.res\", line 339, characters 7-14", Js_json.decodeBoolean(1.23), undefined); + Test_utils.eq("File \"js_json_test.res\", line 347, characters 7-14", Js_json.decodeBoolean("test"), undefined); + Test_utils.eq("File \"js_json_test.res\", line 348, characters 7-14", Js_json.decodeBoolean(true), true); + Test_utils.eq("File \"js_json_test.res\", line 349, characters 7-14", Js_json.decodeBoolean([]), undefined); + Test_utils.eq("File \"js_json_test.res\", line 350, characters 7-14", Js_json.decodeBoolean(null), undefined); + Test_utils.eq("File \"js_json_test.res\", line 351, characters 7-14", Js_json.decodeBoolean({}), undefined); + Test_utils.eq("File \"js_json_test.res\", line 352, characters 7-14", Js_json.decodeBoolean(1.23), undefined); }); Mocha.test("JSON decodeNull", () => { - Test_utils.eq("File \"js_json_test.res\", line 343, characters 7-14", Js_json.decodeNull("test"), undefined); - Test_utils.eq("File \"js_json_test.res\", line 344, characters 7-14", Js_json.decodeNull(true), undefined); - Test_utils.eq("File \"js_json_test.res\", line 345, characters 7-14", Js_json.decodeNull([]), undefined); - Test_utils.eq("File \"js_json_test.res\", line 346, characters 7-14", Js_json.decodeNull(null), null); - Test_utils.eq("File \"js_json_test.res\", line 347, characters 7-14", Js_json.decodeNull({}), undefined); - Test_utils.eq("File \"js_json_test.res\", line 348, characters 7-14", Js_json.decodeNull(1.23), undefined); + Test_utils.eq("File \"js_json_test.res\", line 356, characters 7-14", Js_json.decodeNull("test"), undefined); + Test_utils.eq("File \"js_json_test.res\", line 357, characters 7-14", Js_json.decodeNull(true), undefined); + Test_utils.eq("File \"js_json_test.res\", line 358, characters 7-14", Js_json.decodeNull([]), undefined); + Test_utils.eq("File \"js_json_test.res\", line 359, characters 7-14", Js_json.decodeNull(null), null); + Test_utils.eq("File \"js_json_test.res\", line 360, characters 7-14", Js_json.decodeNull({}), undefined); + Test_utils.eq("File \"js_json_test.res\", line 361, characters 7-14", Js_json.decodeNull(1.23), undefined); }); Mocha.test("JSON serialize/deserialize identity", () => { - let idtest = obj => Test_utils.eq("File \"js_json_test.res\", line 354, characters 27-34", obj, Js_json.deserializeUnsafe(Js_json.serializeExn(obj))); + let idtest = obj => Test_utils.eq("File \"js_json_test.res\", line 367, characters 27-34", obj, Js_json.deserializeUnsafe(Js_json.serializeExn(obj))); idtest(undefined); idtest({ hd: [ diff --git a/tests/tests/src/js_json_test.res b/tests/tests/src/js_json_test.res index 26fa6320d0..0e1bad5add 100644 --- a/tests/tests/src/js_json_test.res +++ b/tests/tests/src/js_json_test.res @@ -315,7 +315,7 @@ describe(__MODULE__, () => { eq(__LOC__, J.decodeArray(J.number(1.23)), None) }) - test("JSON Array/Object switch falls through to wildcard on null", () => { + test("JSON Array/Object switch falls through to wildcard on null and array", () => { let classifyArrayOrObject = (json: J.t) => switch json { | J.Array(items) => Some(items->Js.Array2.length) @@ -328,6 +328,19 @@ describe(__MODULE__, () => { eq(__LOC__, classifyArrayOrObject(J.null), None) eq(__LOC__, classifyArrayOrObject(J.array([J.number(1.)])), Some(1)) eq(__LOC__, classifyArrayOrObject(J.object_(Js.Dict.empty())), Some(0)) + + // When there's no Array case, arrays should fall to wildcard + let classifyObjectOnly = (json: J.t) => + switch json { + | J.Object(_) => "Object" + | J.String(_) => "String" + | _ => "default" + } + + eq(__LOC__, classifyObjectOnly(J.null), "default") + eq(__LOC__, classifyObjectOnly(J.array([])), "default") + eq(__LOC__, classifyObjectOnly(J.object_(Js.Dict.empty())), "Object") + eq(__LOC__, classifyObjectOnly(J.string("hi")), "String") }) test("JSON decodeBoolean", () => {