Skip to content

Also guard against arrays falling into object branch in untagged vari… #8255

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

Merged
cristianoc merged 1 commit into from
Feb 25, 2026
Merged

Also guard against arrays falling into object branch in untagged vari… #8255

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
43 changes: 31 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,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),
Expand All @@ -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
Expand Down
45 changes: 31 additions & 14 deletions tests/tests/src/js_json_test.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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: [
Expand Down
15 changes: 14 additions & 1 deletion tests/tests/src/js_json_test.res
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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", () => {
Expand Down
Loading